Skip to content

Fix node key collisions in ShaderGraph#2768

Merged
jstone-lucasfilm merged 2 commits intoAcademySoftwareFoundation:mainfrom
jstone-lucasfilm:dev_shader_graph_fix
Feb 11, 2026
Merged

Fix node key collisions in ShaderGraph#2768
jstone-lucasfilm merged 2 commits intoAcademySoftwareFoundation:mainfrom
jstone-lucasfilm:dev_shader_graph_fix

Conversation

@jstone-lucasfilm
Copy link
Member

This changelist fixes a name collision error in the ShaderGraph node map.

Previously, nodes were keyed by their base name (i.e. getName), which is only unique within the scope of the parent graph. In complex documents where multiple nodes shared the same base name, these map keys would collide, leading to shader generation errors and crashes.

In order to address this, the map key now uses the full name path of the node (i.e. getNamePath), which is guaranteed to be unique across the entire document.

One specific issue addressed by this change is the Graph Editor crash reported in #1932.

This changelist fixes a name collision error in the ShaderGraph node map.

Previously, nodes were keyed by their base name (i.e. getName), which is only unique within the scope of the parent graph.  In complex documents where multiple nodes shared the same base name, these map keys would collide, leading to shader generation errors and crashes.

In order to address this, the map key now uses the full name path of the node (i.e. getNamePath), which is guaranteed to be unique across the entire document.

One specific issue addressed by this change is the Graph Editor crash reported in AcademySoftwareFoundation#1932.
@niklasharrysson
Copy link
Contributor

This looks like an excellent change =) Nice way of solving the issues we've seen with node names not being unique as graphs are flattened.

I only have one small note: The path being used is the node's path in the source MaterialX document, and it may be different from the path the ShaderNode ends up with in the ShaderGraph, since shader graph topology may change during optimization when finalizing the graph. That's ok here, the path is only being used as a unique name key. But maybe this should be clarified in the API? If someone uses ShaderNode::getNamePath() for another purpose in the future they may think it returns the ShaderNode path in the ShaderGraph.

So perhaps the wording "path" here may cause some confusion in the future. An alternative to ShaderNode::getNamePath() would be to instead use ShaderNode::getUniqueId() or ShaderNode::getIdentifier(), to focus on the usage as a unique identifier. In addition we could then have a method for generating this ID from an Element: static ShaderNode::getIdentifier(const Element& elem). This method would then hide the fact that the name path is being used currently as the identifier, and this could change later if needed.

@jstone-lucasfilm
Copy link
Member Author

That's a great point, @niklasharrysson, and I've updated the naming convention from ShaderNode::getNamePath to ShaderNode::getUniqueId, with corresponding updates to the private member variable and the interfaces of ShaderNode::createNode and ShaderNode::getNode.

For now, I'm just directly passing Element::getNamePath to the ShaderNode::createNode and ShaderNode::getNode methods, as this provides some clarity as to what string is used as the unique ID in these instances, but I'm open to abstracting this further as needed.

Copy link
Contributor

@niklasharrysson niklasharrysson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! This looks good to me.

@jstone-lucasfilm jstone-lucasfilm merged commit 6d0e937 into AcademySoftwareFoundation:main Feb 11, 2026
33 checks passed
@jstone-lucasfilm jstone-lucasfilm deleted the dev_shader_graph_fix branch February 11, 2026 23:52
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

Successfully merging this pull request may close these issues.

2 participants