From 1fb83c551a41a252a9d6433b60fee55c4ec2bf9b Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Mon, 20 Jan 2025 11:37:08 +0100 Subject: [PATCH] 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 | 6 +- .../io/usd/intern/usd_writer_material.cc | 5 +- .../nodes/shader/materialx/group_nodes.cc | 2 +- .../nodes/shader/materialx/material.cc | 8 +- .../blender/nodes/shader/materialx/material.h | 2 +- .../nodes/shader/materialx/node_graph.cc | 99 ++++++++++++++++++- .../nodes/shader/materialx/node_graph.h | 22 +++++ .../nodes/shader/materialx/node_item.cc | 16 +-- .../nodes/shader/materialx/node_item.h | 1 - .../nodes/shader/materialx/node_parser.cc | 56 +++-------- .../nodes/shader/materialx/node_parser.h | 2 +- .../shader/nodes/node_shader_tex_image.cc | 2 +- 12 files changed, 148 insertions(+), 73 deletions(-) diff --git a/source/blender/io/usd/hydra/material.cc b/source/blender/io/usd/hydra/material.cc index aaaf3b8b55f..e0829f96dee 100644 --- a/source/blender/io/usd/hydra/material.cc +++ b/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() diff --git a/source/blender/io/usd/intern/usd_writer_material.cc b/source/blender/io/usd/intern/usd_writer_material.cc index 9f897b1ba38..b574d6a3d5d 100644 --- a/source/blender/io/usd/intern/usd_writer_material.cc +++ b/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 diff --git a/source/blender/nodes/shader/materialx/group_nodes.cc b/source/blender/nodes/shader/materialx/group_nodes.cc index bc92dda37ec..615508cfa25 100644 --- a/source/blender/nodes/shader/materialx/group_nodes.cc +++ b/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_) diff --git a/source/blender/nodes/shader/materialx/material.cc b/source/blender/nodes/shader/materialx/material.cc index 57cd61917bb..43ccbf5f5ad 100644 --- a/source/blender/nodes/shader/materialx/material.cc +++ b/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, diff --git a/source/blender/nodes/shader/materialx/material.h b/source/blender/nodes/shader/materialx/material.h index c84f4cf1775..5e55becbdc1 100644 --- a/source/blender/nodes/shader/materialx/material.h +++ b/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 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 diff --git a/source/blender/nodes/shader/materialx/node_graph.cc b/source/blender/nodes/shader/materialx/node_graph.cc index 9b8a7144851..e1b270e9b5d 100644 --- a/source/blender/nodes/shader/materialx/node_graph.cc +++ b/source/blender/nodes/shader/materialx/node_graph.cc @@ -6,10 +6,16 @@ # include #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( - child_name); + valid_child_name); if (!graph) { - CLOG_INFO(LOG_MATERIALX_SHADER, 1, "", child_name.c_str()); - graph = parent.graph_element_->addChild(child_name); + CLOG_INFO(LOG_MATERIALX_SHADER, 1, "", valid_child_name.c_str()); + graph = parent.graph_element_->addChild(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 diff --git a/source/blender/nodes/shader/materialx/node_graph.h b/source/blender/nodes/shader/materialx/node_graph.h index 404d5c34914..4f88246913c 100644 --- a/source/blender/nodes/shader/materialx/node_graph.h +++ b/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 root_key_to_name_map_; + Map &key_to_name_map_; + std::string node_name_prefix_; }; } // namespace blender::nodes::materialx diff --git a/source/blender/nodes/shader/materialx/node_item.cc b/source/blender/nodes/shader/materialx/node_item.cc index 96668cf681b..ab6696f19e6 100644 --- a/source/blender/nodes/shader/materialx/node_item.cc +++ b/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 diff --git a/source/blender/nodes/shader/materialx/node_item.h b/source/blender/nodes/shader/materialx/node_item.h index d6b6f3d8401..375a0e60d7d 100644 --- a/source/blender/nodes/shader/materialx/node_item.h +++ b/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; diff --git a/source/blender/nodes/shader/materialx/node_parser.cc b/source/blender/nodes/shader/materialx/node_parser.cc index 85338271131..318f898b2f6 100644 --- a/source/blender/nodes/shader/materialx/node_parser.cc +++ b/source/blender/nodes/shader/materialx/node_parser.cc @@ -2,10 +2,6 @@ * * SPDX-License-Identifier: GPL-2.0-or-later */ -#ifdef WITH_USD -# include -#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(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) diff --git a/source/blender/nodes/shader/materialx/node_parser.h b/source/blender/nodes/shader/materialx/node_parser.h index 3cb28761487..3a19f58d315 100644 --- a/source/blender/nodes/shader/materialx/node_parser.h +++ b/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, diff --git a/source/blender/nodes/shader/nodes/node_shader_tex_image.cc b/source/blender/nodes/shader/nodes/node_shader_tex_image.cc index 4bbfd4bbe62..84e59031344 100644 --- a/source/blender/nodes/shader/nodes/node_shader_tex_image.cc +++ b/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 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) {