Normalize shader codegen to produce identical source across materials

[ashwinbhat]

Ensure materials using the same shading model generate structurally identical shader code, differing only in uniform values.

• Use short input names (e.g. base_color) instead of node-prefixed names (e.g. SR_brass_base_color) for shader/closure node uniforms in finalize().
• Replace std::deque with std::set using a name-based comparator in topologicalSort() to guarantee deterministic node ordering at each topological level.
• Assign generic output variable names (material_out, surfaceshader_out) in setVariableNames() instead of deriving them from per-material node names.

Attaching shader diffs of standard_surface_wood_tiled_ps.glsl vs standard_surface_brass_tiled_ps.glsl (before and after)
shaderdiff.txt
shaderdiff_after.txt

Attaching shader diffs of standard_surface_defauilt_ps.glsl vs standard_surface_plastic_ps.glsl (before and after)
shaderdiff_b4.txt
shaderdiff_after.txt

[Copilot]

Pull request overview

This pull request aims to normalize shader code generation so that materials using the same shading model produce structurally identical shader code, differing only in uniform values. This improves shader caching and reduces compilation overhead when switching between materials with the same underlying shader structure.

Changes:

• Modified input naming in finalize() to use short names (e.g., "base_color") for shader/closure node uniforms instead of node-prefixed names (e.g., "SR_brass_base_color")
• Replaced std::deque with std::set using a name-based comparator in topologicalSort() to ensure deterministic node ordering at each topological level
• Changed output variable naming in setVariableNames() to use generic names ("material_out", "surfaceshader_out") for material and surfaceshader outputs

Comments suppressed due to low confidence (1)

source/MaterialXGenShader/ShaderGraph.cpp:954

• When multiple shader or closure nodes have the same input name, only the first node's input properties are used to initialize the shared inputSocket. The subsequent nodes with the same input name will reuse the existing socket but their value, path, unit, and colorSpace properties will not be applied to the socket (these are only set in the if block on lines 943-951). This could lead to incorrect behavior if different shader/closure nodes have different values, paths, units, or color spaces for inputs with the same name.

Either these properties should be validated to be identical across all shader/closure nodes with the same input name (and an error thrown if they differ), or the naming strategy should be reconsidered to avoid collisions in cases where the properties actually differ.

                        const bool isShaderNode = node->hasClassification(ShaderNode::Classification::SHADER) ||
                                                  node->hasClassification(ShaderNode::Classification::CLOSURE);
                        const string interfaceName = isShaderNode
                            ? input->getName()
                            : input->getFullName();

                        ShaderGraphInputSocket* inputSocket = getInputSocket(interfaceName);
                        if (!inputSocket)
                        {
                            inputSocket = addInputSocket(interfaceName, input->getType());
                            inputSocket->setPath(input->getPath());
                            inputSocket->setValue(input->getValue());
                            inputSocket->setUnit(input->getUnit());
                            inputSocket->setColorSpace(input->getColorSpace());
                            if (input->isUniform())
                            {
                                inputSocket->setUniform();
                            }
                        }
                        inputSocket->makeConnection(input);
                        inputSocket->setMetadata(input->getMetadata());

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jstone-lucasfilm]

Thanks for putting this proposal together, @ashwinbhat.

I'm still reading through the proposal to gain a better understanding, but there's one point brought up by the Copilot review that seems worthwhile to consider.

Now that finalize uses short names for shader/closure/material nodes, two different nodes with inputs of the same name (e.g. two BSDF closures that both have tint) will map to the same interface socket. The first node to be visited will create the socket and set its value, path, unit, and colorSpace; every subsequent node will silently reuse that socket without checking whether its own properties match. Under the old <nodename>_<inputname> scheme this was nearly impossible to hit, but with short input names it becomes a much more likely scenario.

What are your thoughts?

[jstone-lucasfilm]

@ashwinbhat If anything, we may need to go in the reverse direction, making shader uniform names more unique, in order to address reported issues such as #2784.

Given the tension between these two goals, I wonder if the premise of the PR might be misguided, and if we might need to use a completely different approach to improve the effectiveness of shader caching across generated source files.

As always, I'm very interested in thoughts from the community on this, and this seems like an important area to fully understand before we choose a path forward.

[ashwinbhat]

@ashwinbhat If anything, we may need to go in the reverse direction, making shader uniform names more unique, in order to address reported issues such as #2784.

Given the tension between these two goals, I wonder if the premise of the PR might be misguided, and if we might need to use a completely different approach to improve the effectiveness of shader caching across generated source files.

As always, I'm very interested in thoughts from the community on this, and this seems like an important area to fully understand before we choose a path forward.

@jstone-lucasfilm my changes was mainly trying to generalize the uniform name for materials, and shader nodes.
The issue #2784 is not directly related but could be addressed is a similar way, so we would have:

uniform vec2 crosshatch1_uvtiling = vec2(4.000000, 4.000000);
uniform vec2 crosshatch1_uvoffset = vec2(0.000000, 0.500000);
uniform float crosshatch1_thickness = 0.511600;
uniform bool crosshatch1_staggered = false;
uniform vec2 crosshatch1_uvtiling_compound2_color_crosshatch1 = vec2(10.000000, 10.000000);
uniform vec2 crosshatch1_uvoffset_compound2_color_crosshatch1 = vec2(0.000000, 0.000000);
uniform float crosshatch1_thickness_compound2_color_crosshatch1 = 0.050000;
uniform bool crosshatch1_staggered_compound2_color_crosshatch1 = true;

[ashwinbhat]

Updated shader that handles the name collision
2x-crosshatch-sameName_ps.glsl.txt

[jstone-lucasfilm]

@ashwinbhat Just to propose an alternative that might be more robust, what would you think about the idea of computing a structural hash of the shader graph topology (typed node signatures, node connections, and so on) before code generation, and then using that hash as the key for your shader cache?

This would avoid the potential fragility of shortening uniform names and checking for collisions, especially in light of the goals of #2784, which will require us to make our uniform names longer and more unique.

[ashwinbhat]

@jstone-lucasfilm Ideally that would work, so if I understand the workflow is if the hash of two shader graphs match, we would be able to reuse the same shadercode and bind updated uniforms.
I recall discussing this before with @JGamache-autodesk and @ppenenko, one challenge is we don't know besides inputs and connections, what attributes would influence the graph and generated shader to generate a stable hash. I think it would have be done at per target level?

[jstone-lucasfilm]

Those are good questions, and I'm interested in thoughts from your colleagues, but I do feel that this approach will end up being a more robust solution.

If we make the structural hash computation part of our MaterialXGenShader system, it should have access to all of the mechanisms that affect the final shader topology, without actually needing to generate any shader code.

[ashwinbhat]

Hi @jstone-lucasfilm assuming we have some structural hash computation in MaterialXGenShader (e.g. mx:computeStructuralHash) I think there is still a usability concern unless we have some generic names for shader/material/closure nodes in place.
Consider standard_surface_defauilt_ps.glsl vs standard_surface_plastic_ps.glsl:
Though the shadergraph can have the same hash, it would be confusing to the renderer since I would prefer the uniform name to be base_color a generic name instead of SR_plastic_base_color or SR_default_base_color depending on which shader I created first and reuse.

[jstone-lucasfilm]

That's a good point, @ashwinbhat, and one option would be to name the higher-level scopes using a [category][index] convention, giving us names such as standard_surface_base_color, standard_surface2_base_color, standard_surface3_base_color, and so forth. That would maintain the uniqueness and scope of each shader uniform without baking unique node names into the shading code.

[ppenenko]

Sorry it took me a while to catch up but I think it's important to note that this work has strong parallels with @JGamache-autodesk 's topology cache OpenUSD PR: PixarAnimationStudios/OpenUSD#3073, namely, in pxr/imaging/hdSt/materialXFilter.cpp, in the _BuildEquivalentMaterialNetwork function:

1. Depth-first traversal from terminals for stable, name-independent ordering: L1413-L1423
   "we process the network in a depth-first traversal starting at the terminals. This provides a stable traversal and consistent anonymized renaming that will not be affected by the ordering of the SdfPaths in the hdNetwork."

2. Anonymized naming scheme - index + node type + input name (stripping USD node names):
   L1454-L1459 - produces names like N0_ND_standard_surface_surfaceshader: the nodeCounter ensures uniqueness, nodeTypeId preserves type semantics, and the original USD node name is dropped entirely.

3. Non-topological parameters stripped (only topology-affecting params retained):
   L1489-L1534 - _IsTopologicalShader keeps parameters for special nodes (e.g. atan2); for everything else only color-management and normal-map space params are preserved. All other uniform values are dropped.

4. Hash computation over the anonymized network:
   L1550-L1569 - hashes node names, node types, retained parameters, and connection topology using TfHash.

It does of course make a lot of sense to port that optimization to MaterialX proper, just like with Lobe Pruning.

[ppenenko]

As for hashing, I think it should ideally happen during Shader Graph construction, before code emission. Doing it earlier (on the Node Graph) would conflict with the Visitor Pattern direction. My current work on #2680 actually ended up being at the Shader Graph construction phase as well (rather than rewriting the Shader Graph after the fact) because that's a good opportunity to skip unnecessary codegen. Ideally, individual nodes should be responsible for reporting their own hash, which would avoid leaking implementation details like in the OpenUSD implementation.