Fix #132781: MaterialX node name conflict causes crash

Use BLI_uniquename_cb to check and fix naming conflicts, and keep track
of the potentially modified node names in a map.

Reserve the material name, so that later the output node can be renamed to it
without conflicts. Also make sure there are no conflicts with names auto
generated by MaterialX, by always naming nodes ourselves.

Pull Request: https://projects.blender.org/blender/blender/pulls/133234

source/blender/io/usd/hydra/material.cc

@@ -87,11 +87,11 @@ void MaterialData::init()
   pxr::UsdShadeMaterial usd_material;
 #ifdef WITH_MATERIALX
   if (scene_delegate_->use_materialx) {
-    blender::nodes::materialx::ExportParams materialx_export_params{
-        cache_or_get_image_file, "st", "UVMap"};
     std::string material_name = pxr::TfMakeValidIdentifier(id->name);
+    blender::nodes::materialx::ExportParams materialx_export_params{
+        material_name, cache_or_get_image_file, "st", "UVMap"};
     MaterialX::DocumentPtr doc = blender::nodes::materialx::export_to_materialx(
-        scene_delegate_->depsgraph, (Material *)id, material_name, materialx_export_params);
+        scene_delegate_->depsgraph, (Material *)id, materialx_export_params);
     pxr::UsdMtlxRead(doc, stage);
 
     /* Logging stage: creating lambda stage_str() to not call stage->ExportToString()

source/blender/io/usd/intern/usd_writer_material.cc

@@ -1412,6 +1412,8 @@ static void create_usd_materialx_material(const USDExporterContext &usd_export_c
                                           const pxr::UsdShadeMaterial &usd_material)
 {
   blender::nodes::materialx::ExportParams export_params = {
+      /* Output surface material node will have this name. */
+      usd_path.GetElementString(),
       /* We want to re-use the same MaterialX document generation code as used by the renderer.
        * While the graph is traversed, we also want it to export the textures out. */
       (usd_export_context.export_image_fn) ? usd_export_context.export_image_fn :
@@ -1426,9 +1428,8 @@ static void create_usd_materialx_material(const USDExporterContext &usd_export_c
       active_uvmap_name,
   };
 
-  std::string material_name = usd_path.GetElementString();
   MaterialX::DocumentPtr doc = blender::nodes::materialx::export_to_materialx(
-      usd_export_context.depsgraph, material, material_name, export_params);
+      usd_export_context.depsgraph, material, export_params);
 
   /* We want to merge the MaterialX graph under the same Material as the USDPreviewSurface
    * This allows for the same material assignment to have two levels of complexity so other

source/blender/nodes/shader/materialx/group_nodes.cc

@@ -34,7 +34,7 @@ NodeItem GroupNodeParser::compute()
     return empty();
   }
 
-  NodeGraph group_graph(graph_, MaterialX::createValidName(ngroup->id.name));
+  NodeGraph group_graph(graph_, ngroup->id.name + 2);
 
   NodeItem out = GroupOutputNodeParser(
                      group_graph, node_out, socket_out_, to_type_, this, use_group_default_)

source/blender/nodes/shader/materialx/material.cc

@@ -34,7 +34,6 @@ class DefaultMaterialNodeParser : public NodeParser {
 
     NodeItem res = create_node(
         "surfacematerial", NodeItem::Type::Material, {{"surfaceshader", surface}});
-    res.node->setName("Material_Default");
     return res;
   }
 
@@ -45,14 +44,12 @@ class DefaultMaterialNodeParser : public NodeParser {
                                    {{"base_color", val(MaterialX::Color3(1.0f, 0.0f, 1.0f))}});
     NodeItem res = create_node(
         "surfacematerial", NodeItem::Type::Material, {{"surfaceshader", surface}});
-    res.node->setName("Material_Error");
     return res;
   }
 };
 
 MaterialX::DocumentPtr export_to_materialx(Depsgraph *depsgraph,
                                            Material *material,
-                                           const std::string &material_name,
                                            const ExportParams &export_params)
 {
   CLOG_INFO(LOG_MATERIALX_SHADER, 0, "Material: %s", material->id.name);
@@ -82,9 +79,8 @@ MaterialX::DocumentPtr export_to_materialx(Depsgraph *depsgraph,
                       .compute();
   }
 
-  if (output_item.node) {
-    output_item.node->setName(material_name);
-  }
+  /* This node is expected to have a specific name to link up to USD. */
+  graph.set_output_node_name(output_item);
 
   CLOG_INFO(LOG_MATERIALX_SHADER,
             1,

source/blender/nodes/shader/materialx/material.h

@@ -19,6 +19,7 @@ struct Scene;
 namespace blender::nodes::materialx {
 
 struct ExportParams {
+  std::string output_node_name;
   std::function<std::string(Main *, Scene *, Image *, ImageUser *)> image_fn;
   std::string new_active_uvmap_name;
   std::string original_active_uvmap_name;
@@ -26,7 +27,6 @@ struct ExportParams {
 
 MaterialX::DocumentPtr export_to_materialx(Depsgraph *depsgraph,
                                            Material *material,
-                                           const std::string &material_name,
                                            const ExportParams &export_params);
 
 }  // namespace blender::nodes::materialx

source/blender/nodes/shader/materialx/node_graph.cc

@@ -6,10 +6,16 @@
 #  include <pxr/base/tf/stringUtils.h>
 #endif
 
+#include "BLI_hash.hh"
+#include "BLI_string_utils.hh"
+
 #include "node_graph.h"
 
 namespace blender::nodes::materialx {
 
+/* Prefix for nodes that don't map directly to a Blender shader node. */
+static const char *ANONYMOUS_NODE_NAME_PREFIX = "node";
+
 /* Valid name for MaterialX and USD. */
 static std::string valid_name(const StringRef name)
 {
@@ -26,6 +32,19 @@ static std::string valid_name(const StringRef name)
   return res;
 }
 
+/* Node Key */
+
+uint64_t NodeGraph::NodeKey::hash() const
+{
+  return get_default_hash(node, socket_name, to_type, graph_element);
+}
+
+bool NodeGraph::NodeKey::operator==(const NodeGraph::NodeKey &other) const
+{
+  return node == other.node && socket_name == other.socket_name && to_type == other.to_type &&
+         graph_element == other.graph_element;
+}
+
 /* Node Graph */
 
 NodeGraph::NodeGraph(const Depsgraph *depsgraph,
@@ -35,23 +54,34 @@ NodeGraph::NodeGraph(const Depsgraph *depsgraph,
     : depsgraph(depsgraph),
       material(material),
       export_params(export_params),
-      graph_element_(document.get())
+      graph_element_(document.get()),
+      key_to_name_map_(root_key_to_name_map_)
 {
 }
 
 NodeGraph::NodeGraph(const NodeGraph &parent, const StringRef child_name)
-    : depsgraph(parent.depsgraph), material(parent.material), export_params(parent.export_params)
+    : depsgraph(parent.depsgraph),
+      material(parent.material),
+      export_params(parent.export_params),
+#ifdef USE_MATERIALX_NODEGRAPH
+      key_to_name_map_(root_key_to_name_map_)
+#else
+      key_to_name_map_(parent.key_to_name_map_)
+#endif
 {
+  std::string valid_child_name = valid_name(child_name);
+
 #ifdef USE_MATERIALX_NODEGRAPH
   MaterialX::NodeGraphPtr graph = parent.graph_element_->getChildOfType<MaterialX::NodeGraph>(
-      child_name);
+      valid_child_name);
   if (!graph) {
-    CLOG_INFO(LOG_MATERIALX_SHADER, 1, "<nodegraph name=%s>", child_name.c_str());
-    graph = parent.graph_element_->addChild<MaterialX::NodeGraph>(child_name);
+    CLOG_INFO(LOG_MATERIALX_SHADER, 1, "<nodegraph name=%s>", valid_child_name.c_str());
+    graph = parent.graph_element_->addChild<MaterialX::NodeGraph>(valid_child_name);
   }
   graph_element_ = graph.get();
 #else
   graph_element_ = parent.graph_element_;
+  node_name_prefix_ = node_name_prefix_ + valid_child_name + "_";
 #endif
 }
 
@@ -81,4 +111,63 @@ NodeItem NodeGraph::get_input(const StringRef name) const
   return item;
 }
 
+std::string NodeGraph::unique_node_name(const bNode *node,
+                                        const StringRef socket_out_name,
+                                        NodeItem::Type to_type)
+{
+  /* Reuse existing name, important in case it got changed due to conflicts. */
+  NodeKey key{node, socket_out_name, to_type, graph_element_};
+  const std::string *existing_name = key_to_name_map_.lookup_ptr(key);
+  if (existing_name) {
+    return *existing_name;
+  }
+
+  /* Generate name based on noode, socket, to type and node groups. */
+  std::string name = node->name;
+
+  if (!socket_out_name.is_empty() && node->output_sockets().size() > 1) {
+    name += std::string("_") + socket_out_name;
+  }
+  if (to_type != NodeItem::Type::Empty) {
+    name += "_" + NodeItem::type(to_type);
+  }
+
+  name = node_name_prefix_ + valid_name(name);
+
+  /* Avoid conflicts with anonymouse node names. */
+  if (StringRef(name).startswith(ANONYMOUS_NODE_NAME_PREFIX)) {
+    name = "b" + name;
+  }
+
+  /* Ensure the name does not conflict with other nodes in the graph, which may happen when
+   * another Blender node name happens to match the complete name here. */
+  name = BLI_uniquename_cb(
+      [this](const StringRef check_name) {
+        return check_name == export_params.output_node_name ||
+               graph_element_->getNode(check_name) != nullptr;
+      },
+      '_',
+      name);
+
+  key_to_name_map_.add_new(key, name);
+  return name;
+}
+
+void NodeGraph::set_output_node_name(const NodeItem &item) const
+{
+  if (item.node) {
+    item.node->setName(export_params.output_node_name);
+  }
+}
+
+std::string NodeGraph::unique_anonymous_node_name(MaterialX::GraphElement *graph_element)
+{
+  return BLI_uniquename_cb(
+      [graph_element](const StringRef check_name) {
+        return graph_element->getNode(check_name) != nullptr;
+      },
+      '_',
+      ANONYMOUS_NODE_NAME_PREFIX);
+}
+
 }  // namespace blender::nodes::materialx

source/blender/nodes/shader/materialx/node_graph.h

@@ -11,8 +11,10 @@
 #include "material.h"
 #include "node_item.h"
 
+#include "BLI_map.hh"
 #include "BLI_string_ref.hh"
 
+struct bNode;
 struct Material;
 struct Depsgraph;
 
@@ -38,8 +40,28 @@ struct NodeGraph {
   NodeItem get_output(const StringRef name) const;
   NodeItem get_input(const StringRef name) const;
 
+  std::string unique_node_name(const bNode *node,
+                               const StringRef socket_out_name,
+                               const NodeItem::Type to_type);
+  void set_output_node_name(const NodeItem &item) const;
+
+  static std::string unique_anonymous_node_name(MaterialX::GraphElement *graph_element);
+
  protected:
+  struct NodeKey {
+    const bNode *node;
+    std::string socket_name;
+    NodeItem::Type to_type;
+    MaterialX::GraphElement *graph_element;
+
+    uint64_t hash() const;
+    bool operator==(const NodeKey &other) const;
+  };
+
   MaterialX::GraphElement *graph_element_ = nullptr;
+  Map<NodeKey, const std::string> root_key_to_name_map_;
+  Map<NodeKey, const std::string> &key_to_name_map_;
+  std::string node_name_prefix_;
 };
 
 }  // namespace blender::nodes::materialx

source/blender/nodes/shader/materialx/node_item.cc

@@ -3,6 +3,7 @@
  * SPDX-License-Identifier: GPL-2.0-or-later */
 
 #include "node_item.h"
+#include "node_graph.h"
 #include "node_parser.h"
 
 #include "BLI_assert.h"
@@ -829,16 +830,17 @@ NodeItem::Type NodeItem::type() const
 
 NodeItem NodeItem::create_node(const std::string &category, Type type) const
 {
-  std::string type_str = this->type(type);
+  const std::string name = NodeGraph::unique_anonymous_node_name(graph_);
+  const std::string type_str = NodeItem::type(type);
   CLOG_INFO(LOG_MATERIALX_SHADER, 2, "<%s type=%s>", category.c_str(), type_str.c_str());
   NodeItem res = empty();
   /* Surface-shader nodes and materials are added directly to the document,
    * otherwise to the node-graph. */
   if (ELEM(type, Type::SurfaceShader, Type::Material)) {
-    res.node = graph_->getDocument()->addNode(category, MaterialX::EMPTY_STRING, type_str);
+    res.node = graph_->getDocument()->addNode(category, name, type_str);
   }
   else {
-    res.node = graph_->addNode(category, MaterialX::EMPTY_STRING, type_str);
+    res.node = graph_->addNode(category, name, type_str);
   }
   return res;
 }
@@ -846,7 +848,7 @@ NodeItem NodeItem::create_node(const std::string &category, Type type) const
 NodeItem NodeItem::create_node(const std::string &category, Type type, const Inputs &inputs) const
 {
   NodeItem res = create_node(category, type);
-  for (auto &it : inputs) {
+  for (const auto &it : inputs) {
     if (it.second) {
       res.set_input(it.first, it.second);
     }
@@ -977,10 +979,8 @@ NodeItem::Type NodeItem::cast_types(NodeItem &item1, NodeItem &item2)
     item1 = item1.convert(t2);
     return t2;
   }
-  else {
-    item2 = item2.convert(t1);
-    return t1;
-  }
+  item2 = item2.convert(t1);
+  return t1;
 }
 
 bool NodeItem::is_arithmetic() const

source/blender/nodes/shader/materialx/node_item.h

@@ -50,7 +50,6 @@ class NodeItem {
   };
   enum class CompareOp { Less = 0, LessEq, Eq, GreaterEq, Greater, NotEq };
 
- public:
   MaterialX::ValuePtr value;
   MaterialX::NodePtr node;
   MaterialX::InputPtr input;

source/blender/nodes/shader/materialx/node_parser.cc

@@ -2,10 +2,6 @@
  *
  * SPDX-License-Identifier: GPL-2.0-or-later */
 
-#ifdef WITH_USD
-#  include <pxr/base/tf/stringUtils.h>
-#endif
-
 #include "node_parser.h"
 
 #include "group_nodes.h"
@@ -40,7 +36,8 @@ NodeItem NodeParser::compute_full()
   }
 
   /* Checking if node was already computed */
-  res = graph_.get_node(node_name());
+  const std::string res_node_name = node_name();
+  res = graph_.get_node(res_node_name);
   if (!res.node) {
     CLOG_INFO(LOG_MATERIALX_SHADER,
               1,
@@ -51,51 +48,22 @@ NodeItem NodeParser::compute_full()
 
     res = compute();
     if (res.node) {
-      res.node->setName(node_name());
+      res.node->setName(res_node_name);
     }
   }
   return res.convert(to_type_);
 }
 
-std::string NodeParser::node_name(bool with_out_socket) const
+std::string NodeParser::node_name(const char *override_output_name) const
 {
-  auto valid_name = [](const std::string &name) {
-#ifdef WITH_USD
-    /* Node name should suite to MatX and USD valid names.
-     * It shouldn't start from '_', due to error occurred in Storm delegate. */
-    std::string res = MaterialX::createValidName(pxr::TfMakeValidIdentifier(name));
-#else
-    std::string res = MaterialX::createValidName(name);
-#endif
-    if (res[0] == '_') {
-      res = "node" + res;
-    }
-    return res;
-  };
-
-  std::string name = node_->name;
-  if (with_out_socket) {
-    if (node_->output_sockets().size() > 1) {
-      name += std::string("_") + socket_out_->name;
-    }
-    if (ELEM(to_type_, NodeItem::Type::BSDF, NodeItem::Type::EDF, NodeItem::Type::SurfaceOpacity))
-    {
-      name += "_" + NodeItem::type(to_type_);
-    }
-  }
-#ifdef USE_MATERIALX_NODEGRAPH
-  return valid_name(name);
-#else
-
-  std::string prefix;
-  GroupNodeParser *gr = group_parser_;
-  while (gr) {
-    const bNodeTree *ngroup = reinterpret_cast<const bNodeTree *>(gr->node_->id);
-    prefix = valid_name(ngroup->id.name) + "_" + prefix;
-    gr = gr->group_parser_;
-  }
-  return prefix + valid_name(name);
-#endif
+  const NodeItem::Type to_type =
+      ELEM(to_type_, NodeItem::Type::BSDF, NodeItem::Type::EDF, NodeItem::Type::SurfaceOpacity) ?
+          to_type_ :
+          NodeItem::Type::Empty;
+  const StringRef socket_out_name = (override_output_name) ? override_output_name :
+                                    (socket_out_)          ? socket_out_->name :
+                                                             "";
+  return graph_.unique_node_name(node_, socket_out_name, to_type);
 }
 
 NodeItem NodeParser::create_node(const std::string &category, NodeItem::Type type)

source/blender/nodes/shader/materialx/node_parser.h

@@ -42,7 +42,7 @@ class NodeParser {
   virtual NodeItem compute_full();
 
  protected:
-  std::string node_name(bool with_out_socket = true) const;
+  std::string node_name(const char *override_output_name = nullptr) const;
   NodeItem create_node(const std::string &category, NodeItem::Type type);
   NodeItem create_node(const std::string &category,
                        NodeItem::Type type,

source/blender/nodes/shader/nodes/node_shader_tex_image.cc

@@ -178,7 +178,7 @@ NODE_SHADER_MATERIALX_BEGIN
 #ifdef WITH_MATERIALX
 {
   /* Getting node name for Color output. This name will be used for <image> node. */
-  std::string image_node_name = node_name(false) + "_Color";
+  std::string image_node_name = node_name("Color");
 
   NodeItem res = graph_.get_node(image_node_name);
   if (!res.node) {