Graph fails on name conflicts across namespaces

[dgovil]

I was hitting a bug where some code was passing EMPTY_STRING to the node creation function. This is meant to generate a name that is unique to this node.

However if I create most of my nodes within a node graph, and then create a node outside the graph, the names will conflict. I'll get two node1 nodes.

While my assumption is that nodes are namespaced by their container, this appears to still cause issues if the output material tries to connect to the output of node1.

The graph then fails because it is unsure which of the two node1's it should connect to.

I propose that the auto name algorithm should make the node names conflict free across the full topology OR that the connection algorithm pick the node at the correct namespace level instead.

Here's an example graph that is generated using purely the auto-naming code

<?xml version="1.0"?>
<materialx version="1.38">
  <nodegraph name="NG_Material_001">
    <texcoord name="Texture_Coordinate_UV" type="vector2" />
    <multiply name="node1" type="vector2">
      <input name="in1" type="vector2" nodename="Texture_Coordinate_UV" />
      <input name="in2" type="vector2" value="32, 32" />
    </multiply>
    <modulo name="node2" type="vector2">
      <input name="in1" type="vector2" nodename="node1" />
      <input name="in2" type="vector2" value="2, 2" />
    </modulo>
    <extract name="node3" type="float">
      <input name="in" type="vector2" nodename="node2" />
      <input name="index" type="integer" value="0" />
    </extract>
    <floor name="node4" type="float">
      <input name="in" type="float" nodename="node3" />
    </floor>
    <extract name="node5" type="float">
      <input name="in" type="vector2" nodename="node2" />
      <input name="index" type="integer" value="1" />
    </extract>
    <floor name="node6" type="float">
      <input name="in" type="float" nodename="node5" />
    </floor>
    <add name="node7" type="float">
      <input name="in1" type="float" nodename="node4" />
      <input name="in2" type="float" nodename="node6" />
    </add>
    <ifequal name="Checker_Texture_Color" type="color4">
      <input name="value1" type="float" nodename="node7" />
      <input name="value2" type="float" value="1" />
      <input name="in1" type="color4" value="0.8, 0.8, 0.8, 1" />
      <input name="in2" type="color4" value="0.2, 0.2, 0.2, 1" />
    </ifequal>
    <convert name="node8" type="color3">
      <input name="in" type="color4" nodename="Checker_Texture_Color" />
    </convert>
    <oren_nayar_diffuse_bsdf name="Diffuse_BSDF_BSDF" type="BSDF">
      <input name="color" type="color3" nodename="node8" />
      <input name="roughness" type="float" value="0" />
    </oren_nayar_diffuse_bsdf>
    <output name="Diffuse_BSDF_BSDF_out" type="BSDF" nodename="Diffuse_BSDF_BSDF" />
  </nodegraph>
  <surface name="node1" type="surfaceshader">
    <input name="bsdf" type="BSDF" output="Diffuse_BSDF_BSDF_out" nodegraph="NG_Material_001" />
  </surface>
  <surfacematerial name="Material_001" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="node1" />
  </surfacematerial>
</materialx>

Notice that both the surface node and the multiply node share a name. When I pull this mtlx file into MaterialXView, it errors because the surface material finds the first node with that name, not the first node within the same namespace.

[kwokcb]

Hi @dgovil ,
This is considered a valid graph (i.e. unique naming within the "namespace" / graph level) so it seems it's more a problem with logic for code generation and vote to fix that. There are existing unique name matching issues before so adding @niklasharrysson in.

[dgovil]

That's good to know, thanks. I edited the issue title accordingly.

[kwokcb]

Hey @niklasharrysson ,

I looked at the code a bit and it's the same issue we've discussed before -- that of lack of appropriate ShaderGraph context when dealing with nodegraphs . i.e. all ShaderNodes are in the same ShaderGraph. So the node name map will encounter the same node name more than once -- resulting in the symptom encountered here

 // We have a node downstream
        ShaderNode* downstream = getNode(downstreamNode->getName());
        if (downstream && connectingElement)
        {
            ShaderInput* input = downstream->getInput(connectingElement->getName());
            if (!input)
            {
                throw ExceptionShaderGenError("Could not find an input named '" + connectingElement->getName() +
                                              "' on downstream node '" + downstream->getName() + "'");
            }
            input->makeConnection(output);
        }

getNode() looks up by name which is not unique.

[niklasharrysson]

Hey @kwokcb , thanks for investigating. And yes, this will be resolved when we get to updating the ShaderGraph to keep all graph hierarchies.