Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: "glTF Settings" cleanup #1694

Closed
donmccurdy opened this issue Jul 26, 2022 · 11 comments
Closed

Suggestion: "glTF Settings" cleanup #1694

donmccurdy opened this issue Jul 26, 2022 · 11 comments

Comments

@donmccurdy
Copy link
Contributor

The addition of the "glTF Settings" node to the "Output" dropdown is helpful, but does expose a couple small consistency/naming/docs issues —

Screen Shot 2022-07-26 at 11 27 37 AM

  1. Note the missing tooltip for this item, replaced by "(undocumented operator)". All the other nodes say, "Add a node to the active tree", which is not terribly helpful but okay.
  2. Should we consider renaming this "glTF Output" or "glTF Material Output" rather than "glTF Settings" for consistency? Or supporting both, with Output as the default? The node is effectively an output for export purposes.

Screen Shot 2022-07-26 at 11 40 39 AM

I'm not feeling so confident about the "glTF Original PBR Data" node. Its name does not match the button shown in the UI, possibly it should just be part of the other glTF Settings/Output node. And Blender 3.3 is the upcoming LTS release, which means we might want to be careful about adding features in that release. Especially with the upcoming Principled BSDF v2 work (which sounds very promising!) I'm not sure we should be planning to maintain this additional node in an LTS release. I'd be inclined to remove it from the menu until a later date.

Thank you, and let me know if PRs would be welcome for any of these changes.

/cc @emackey

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Jul 26, 2022

Comment from @emackey in #1686 is also good. 👍🏻

@julienduroure
Copy link
Collaborator

undocumented operator

Will fix it after my holidays

glTF Material Output

Ok to rename. I will included it in 3.3 ( but I find a way to keep compatibility with older files, created with old name)

glTF Original PBR Data

I will fix name consistency between node and menu.
I disagree about remove it from 3.3
People need a way to export exactly what they want about specular textures/data. I already spend so much of my time explaining why some features are not 1 to 1 between Blender and glTF. This will be the case for specular too.
Basic users will not use it, but having a way to export exactly needed data for advanced users is a good feature I think.
Principled v2 will of course not be part of 3.3, so using this node is for me the way to go until v2 is ready (not planned yet I think)

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Jul 27, 2022

Thank you @julienduroure — please enjoy the holidays, my questions can certainly wait until afterward. :)

What is the difference between the "glTF Material Output" and "glTF Original PBR Data" nodes, from your perspective? To me they are both output nodes to ensure that the user's glTF-specific data is included at export — so perhaps they should be combined into "glTF Material Output"?

Similarly, I think we could allow the "glTF Material Output" node to always take precedence over Principled BSDF, so we would not require an "Export original PBR Specular" output checkbox.

I am happy to make any of these changes if you agree with the direction.

@julienduroure
Copy link
Collaborator

What is the difference between the "glTF Material Output" and "glTF Original PBR Data" nodes, from your perspective? To me they are both output nodes to ensure that the user's glTF-specific data is included at export — so perhaps they should be combined into "glTF Material Output"?

My goal was to differentiate these 2 cases:

  • Data that are not available in Blender, but can be exported, like AO or thickness. In that case, no choice, we need to use custom socket to export these data.
  • Data that are available in Blender, but not fully compatible with glTF, like specular. In that case, the user has the choice at export of using Blender data, with loosing conversion, or using glTF data (not displayed in Blender, but fully compliant exported)

Not sure if we have to have an option at export (this has the advantage to let the user decide exactly what he want at export), or take precedence of original data over Principled Shader ( has to pro to have one less export option, but the con to not let user decide exactly what he want. He will need to remove some socket links - or reset to default - if he want to test exporting both cases).

@emackey
Copy link
Member

emackey commented Aug 5, 2022

Data that are available in Blender, but not fully compatible with glTF, like specular.

It's possible that this second option may be temporary. There's an effort underway to overhaul Blender's Principled BSDF shader, and bring it more in line with glTF's own PBR material model: https://developer.blender.org/T99447

Of course there's no guarantee when this work might be done or whether it will fully replace all incompatible options. But to me this is an exciting and hopeful sign that we might not need this second option long-term.

@donmccurdy
Copy link
Contributor Author

At least in my opinion, the cases of "Blender doesn't have this" and "Blender has this, but different" do not require two different nodes. It feels more understandable to present the additional inputs the Blender exporter accepts in one place. Similarly I'd say that, if a socket is connected to this custom node, we can simply assume the user wants these inputs to be exported. But I'm happy to defer to your preference here. :)

@julienduroure
Copy link
Collaborator

I merged the 2 nodes into a single one here: #1702

@julienduroure
Copy link
Collaborator

Committed, I think we can now close this ticket

@donmccurdy
Copy link
Contributor Author

Agreed, thank you @julienduroure!

@LorenzWieseke
Copy link

LorenzWieseke commented Jul 3, 2023

How would I add a gltf Material Output node via python?

My add-on (GLB Texture Tools) doesn't work anymore since Blender 3.3.

I tried to fix it this way, but it doesn't always work:

 def add_gltf_material_output_node(self, material):
        nodes = material.node_tree.nodes
        gltf_material_output_node = nodes.get('Group')
        if gltf_material_output_node is None:
            # Get the reference to the current area
            previous_area = 'VIEW_3D'

            # Switch to the Shader Editor layout
            bpy.context.area.type = 'NODE_EDITOR'
            bpy.context.space_data.tree_type = 'ShaderNodeTree'

            # call operator
            bpy.ops.node.gltf_settings_node_operator()

            # Switch back to the previous area
            bpy.context.area.type = previous_area
        
            gltf_material_output_node = nodes.get('Group')

        return gltf_material_output_node

This is how I used to do it:

 def add_gltf_settings_node(self, material):
        nodes = material.node_tree.nodes
         # create group data
        gltf_settings = bpy.data.node_groups.get('glTF Settings')
        if gltf_settings is None:
            bpy.data.node_groups.new('glTF Settings', 'ShaderNodeTree')
        
        # add group to node tree
        gltf_settings_node = nodes.get('glTF Settings')
        if gltf_settings_node is None:
            gltf_settings_node = nodes.new('ShaderNodeGroup')
            gltf_settings_node.name = 'glTF Settings'
            gltf_settings_node.node_tree = bpy.data.node_groups['glTF Settings']

        # create group inputs
        if gltf_settings_node.inputs.get('Occlusion') is None:
            gltf_settings_node.inputs.new('NodeSocketFloat','Occlusion')

        return gltf_settings_node

@julienduroure
Copy link
Collaborator

Hello,

You can check the code used to create the custom group node:

class NODE_OT_GLTF_SETTINGS(bpy.types.Operator):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants