Fix node key collisions in ShaderGraph (#2768)

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.

Author: jstone-lucasfilm

source/MaterialXGenShader/ShaderGraph.cpp

@@ -83,8 +83,7 @@ void ShaderGraph::createConnectedNodes(const ElementPtr& downstreamElement,
         throw ExceptionShaderGenError("Upstream element to connect is not a node '" +
                                       upstreamElement->getName() + "'");
     }
-    const string& newNodeName = upstreamNode->getName();
-    ShaderNode* newNode = getNode(newNodeName);
+    ShaderNode* newNode = getNode(upstreamNode->getNamePath());
     if (!newNode)
     {
         newNode = createNode(upstreamNode, context);
@@ -101,7 +100,7 @@ void ShaderGraph::createConnectedNodes(const ElementPtr& downstreamElement,
         InputPtr graphInput = activeInput->getInterfaceInput();
         if (graphInput && graphInput->hasDefaultGeomPropString())
         {
-            ShaderInput* shaderInput = getNode(upstreamNode->getName())->getInput(activeInput->getName());
+            ShaderInput* shaderInput = newNode->getInput(activeInput->getName());
             addDefaultGeomNode(shaderInput, *graphInput->getDefaultGeomProp(), context);
         }
     }
@@ -127,12 +126,11 @@ void ShaderGraph::createConnectedNodes(const ElementPtr& downstreamElement,
                                       "' on upstream node '" + upstreamNode->getName() + "'");
     }
 
-    // Check if it was a node downstream
+    // Connect to downstream node or graph output socket.
     NodePtr downstreamNode = downstreamElement->asA<Node>();
     if (downstreamNode)
     {
-        // We have a node downstream
-        ShaderNode* downstream = getNode(downstreamNode->getName());
+        ShaderNode* downstream = getNode(downstreamNode->getNamePath());
         if (downstream)
         {
             if (downstream == newNode)
@@ -152,7 +150,7 @@ void ShaderGraph::createConnectedNodes(const ElementPtr& downstreamElement,
             }
             else
             {
-                throw ExceptionShaderGenError("Could not find downstream node ' " + downstreamNode->getName() + "'");
+                throw ExceptionShaderGenError("No connecting element for downstream node '" + downstreamNode->getName() + "'");
             }
         }
     }
@@ -542,8 +540,7 @@ ShaderGraphPtr ShaderGraph::create(const ShaderGraph* parent, const string& name
         graph->addOutputSockets(*nodeDef, context);
 
         // Create this shader node in the graph.
-        ShaderNodePtr newNode = ShaderNode::create(graph.get(), node->getName(), *nodeDef, context);
-        graph->addNode(newNode);
+        ShaderNode* newNode = graph->createNode(node->getName(), node->getNamePath(), nodeDef, context);
 
         // Share metadata.
         graph->setMetadata(newNode->getMetadata());
@@ -624,7 +621,7 @@ ShaderGraphPtr ShaderGraph::create(const ShaderGraph* parent, const string& name
         }
 
         // Apply color and unit transforms to each input.
-        graph->applyInputTransforms(node, newNode.get(), context);
+        graph->applyInputTransforms(node, newNode, context);
 
         // Set root for upstream dependency traversal
         root = node;
@@ -696,7 +693,7 @@ void ShaderGraph::applyInputTransforms(ConstNodePtr node, ShaderNode* shaderNode
     }
 }
 
-ShaderNode* ShaderGraph::createNode(const string& name, ConstNodeDefPtr nodeDef, GenContext& context)
+ShaderNode* ShaderGraph::createNode(const string& name, const string& uniqueId, ConstNodeDefPtr nodeDef, GenContext& context)
 {
     if (!nodeDef)
     {
@@ -705,7 +702,8 @@ ShaderNode* ShaderGraph::createNode(const string& name, ConstNodeDefPtr nodeDef,
 
     // Create this node in the graph.
     ShaderNodePtr newNode = ShaderNode::create(this, name, *nodeDef, context);
-    _nodeMap[name] = newNode;
+    newNode->_uniqueId = uniqueId;
+    _nodeMap[uniqueId] = newNode;
     _nodeOrder.push_back(newNode.get());
 
     return newNode.get();
@@ -717,7 +715,7 @@ ShaderNode* ShaderGraph::createNode(ConstNodePtr node, GenContext& context)
 
     // Create this node in the graph.
     context.pushParentNode(node);
-    ShaderNode* newNode = createNode(node->getName(), nodeDef, context);
+    ShaderNode* newNode = createNode(node->getName(), node->getNamePath(), nodeDef, context);
     newNode->initialize(*node, *nodeDef, context);
     context.popParentNode();
 
@@ -807,7 +805,7 @@ ShaderNode* ShaderGraph::inlineNodeBeforeOutput(ShaderGraphOutputSocket* output,
     }
 
     // create the new node, and connect its output to the provided graph output
-    auto newNode = createNode(newNodeName, nodeDef, context);
+    auto newNode = createNode(newNodeName, newNodeName, nodeDef, context);
     if (!newNode)
     {
         throw ExceptionShaderGenError("Error while creating node '"+newNodeName+"' of type '"+nodeDefName+"'");
@@ -852,19 +850,19 @@ ShaderGraphEdgeIterator ShaderGraph::traverseUpstream(ShaderOutput* output)
 
 void ShaderGraph::addNode(ShaderNodePtr node)
 {
-    _nodeMap[node->getName()] = node;
+    _nodeMap[node->getUniqueId()] = node;
     _nodeOrder.push_back(node.get());
 }
 
-ShaderNode* ShaderGraph::getNode(const string& name)
+ShaderNode* ShaderGraph::getNode(const string& uniqueId)
 {
-    auto it = _nodeMap.find(name);
+    auto it = _nodeMap.find(uniqueId);
     return it != _nodeMap.end() ? it->second.get() : nullptr;
 }
 
-const ShaderNode* ShaderGraph::getNode(const string& name) const
+const ShaderNode* ShaderGraph::getNode(const string& uniqueId) const
 {
-    return const_cast<ShaderGraph*>(this)->getNode(name);
+    return const_cast<ShaderGraph*>(this)->getNode(uniqueId);
 }
 
 void ShaderGraph::finalize(GenContext& context)
@@ -1045,15 +1043,19 @@ void ShaderGraph::optimize(GenContext& context)
         }
 
         // Remove any unused nodes
-        for (ShaderNode* node : _nodeOrder)
+        for (auto it = _nodeMap.begin(); it != _nodeMap.end();)
         {
-            if (usedNodesSet.count(node) == 0)
+            if (usedNodesSet.count(it->second.get()) == 0)
             {
                 // Break all connections
-                disconnect(node);
+                disconnect(it->second.get());
 
                 // Erase from storage
-                _nodeMap.erase(node->getName());
+                it = _nodeMap.erase(it);
+            }
+            else
+            {
+                ++it;
             }
         }
 

source/MaterialXGenShader/ShaderGraph.h

@@ -61,11 +61,11 @@ class MX_GENSHADER_API ShaderGraph : public ShaderNode
     /// Return true if this node is a graph.
     bool isAGraph() const override { return true; }
 
-    /// Get an internal node by name
-    ShaderNode* getNode(const string& name);
+    /// Get an internal node by its unique identifier.
+    ShaderNode* getNode(const string& uniqueId);
 
-    /// Get an internal node by name
-    const ShaderNode* getNode(const string& name) const;
+    /// Get an internal node by its unique identifier.
+    const ShaderNode* getNode(const string& uniqueId) const;
 
     /// Get a vector of all nodes in order
     const vector<ShaderNode*>& getNodes() const { return _nodeOrder; }
@@ -137,11 +137,12 @@ class MX_GENSHADER_API ShaderGraph : public ShaderNode
                               GenContext& context);
 
     /// Create a new node in a graph from a node definition.
+    /// The uniqueId argument is used as the node's key in the graph's node map.
     /// Note - this does not initialize the node instance with any concrete values, but
     /// instead creates an empty instance of the provided node definition
-    ShaderNode* createNode(const string& name, ConstNodeDefPtr nodeDef, GenContext& context);
+    ShaderNode* createNode(const string& name, const string& uniqueId, ConstNodeDefPtr nodeDef, GenContext& context);
 
-    /// Add a node to the graph
+    /// Add a node to the graph, keyed by the node's unique identifier.
     void addNode(ShaderNodePtr node);
 
     /// Add input sockets from an interface element (nodedef, nodegraph or node)

source/MaterialXGenShader/ShaderNode.cpp

@@ -164,6 +164,7 @@ const string ShaderNode::GEOMETRIC_GROUPNAME = "geometric";
 ShaderNode::ShaderNode(const ShaderGraph* parent, const string& name) :
     _parent(parent),
     _name(name),
+    _uniqueId(name),
     _classification(0),
     _impl(nullptr)
 {

source/MaterialXGenShader/ShaderNode.h

@@ -430,6 +430,13 @@ class MX_GENSHADER_API ShaderNode
         return _name;
     }
 
+    /// Return the unique identifier for this node, used as its key
+    /// in the parent graph's node map.
+    const string& getUniqueId() const
+    {
+        return _uniqueId;
+    }
+
     /// Return the implementation used for this node.
     const ShaderNodeImpl& getImplementation() const
     {
@@ -495,6 +502,7 @@ class MX_GENSHADER_API ShaderNode
 
     const ShaderGraph* _parent;
     string _name;
+    string _uniqueId;
     uint32_t _classification;
 
     std::unordered_map<string, ShaderInputPtr> _inputMap;

source/MaterialXTest/MaterialXGenShader/GenShaderUtil.cpp

@@ -313,7 +313,7 @@ void testUniqueNames(mx::GenContext& context, const std::string& stage)
     // Make sure the output and internal node output has their variable names set
     const mx::ShaderGraphOutputSocket* sgOutputSocket = shader->getGraph().getOutputSocket();
     REQUIRE(sgOutputSocket->getVariable() != outputQualifier);
-    const mx::ShaderNode* sgNode1 = shader->getGraph().getNode(node1->getName());
+    const mx::ShaderNode* sgNode1 = shader->getGraph().getNode(node1->getNamePath());
     REQUIRE(sgNode1->getOutput()->getVariable() == "unique_names_out");
 }