Document inputs don't work when connected to NodeGraph inputs

[frohnej-adsk]

I don't think this is mentioned explicitly in the specification but judging by PR #1453, documents are allowed to contain <input> elements.

• Document inputs work if they feed into an input on a Node ✅
• Document inputs don't work if they feed into an input of a NodeGraph ❌

✅ Node input referencing a document input

This renders correctly:

<?xml version="1.0"?>
<materialx version="1.39">
  <!-- Document input -->
  <input name="A" type="color3" value="0, 1, 0" />

  <add name="add1" type="color3" nodedef="ND_add_color3">
    <!-- Node input connected to document input is resolved correctly. -->
    <input name="in1" type="color3" interfacename="A" />
  </add>

  <convert name="convert1" type="surfaceshader" nodedef="ND_convert_color3_surfaceshader">
    <input name="in" type="color3" nodename="add1" />
  </convert>
  <surfacematerial name="surfacematerial1" type="material" nodedef="ND_surfacematerial">
    <input name="surfaceshader" type="surfaceshader" nodename="convert1" />
  </surfacematerial>
</materialx>

❌ NodeGraph input referencing a document input

This renders black:

<?xml version="1.0"?>
<materialx version="1.39">
  <!-- Document input -->
  <input name="A" type="color3" value="0, 1, 0" />

  <nodegraph name="compound1">
    <!-- NodeGraph input connected to document input. -->
    <!-- This isn't resolved correctly by `Input::getInterfaceInput()` -->
    <input name="B" type="color3" interfacename="A" />

    <add name="add1" type="color3" nodedef="ND_add_color3">
      <!-- Calling `Input::getInterfaceInput()` on this input returns input B. -->
      <!-- This isn't wrong, but it might be desirable to follow interfacenames -->
      <!-- recursively and return input A. -->
      <input name="in1" type="color3" interfacename="B" />
    </add>
    <output name="out" type="color3" nodename="add1" />
  </nodegraph>

  <convert name="convert1" type="surfaceshader" nodedef="ND_convert_color3_surfaceshader">
    <input name="in" type="color3" nodegraph="compound1" output="out" />
  </convert>
  <surfacematerial name="surfacematerial1" type="material" nodedef="ND_surfacematerial">
    <input name="surfaceshader" type="surfaceshader" nodename="convert1" />
  </surfacematerial>
</materialx>

It also prints a validation warning:

*** Validation warnings for B.mtlx ***
Interface name not found in referenced declaration: <input name="B" type="color3" interfacename="A">

Issue

The issue is mainly in Input::getInterfaceInput(), which calls getAncestorOfType<GraphElement>().

In the broken example, calling it on /compound1/B will return the first ancestor of type GraphElement: the parent /compound1. But we actually want to go to the grandparent /, so that we can find the input /A.

What would be the desired behavior? I haven't contributed before but I'd be happy to contribute a fix.

Suggestion A:

A simple fix could look like this:

    InputPtr Input::getInterfaceInput() const
    {
-       if (hasInterfaceName())
+       if (hasInterfaceName() && getParent()->getParent())
        {
-           ConstGraphElementPtr graph = getAncestorOfType<GraphElement>();
+           ConstGraphElementPtr graph = getParent()->getParent()->asA<GraphElement>();
            if (graph)
            {
                return graph->getInput(getInterfaceName());
            }
        }
        return nullptr;
    }

This makes the method work correctly, but it's not enough to fix the render: Calling it on /compound1/add1/in1 returns /compound1/B which has an interfacename attribute as well. The caller would be responsible to call getInterfaceInput() again on the returned input until the end of the chain is reached. Thus, it would require additional changes in other places.

Suggestion B:

Instead, the method could traverse the whole chain of interfacename connections. E.g. recursively:

InputPtr Input::getInterfaceInput() const
{
    if (!hasInterfaceName() || !getParent()->getParent())
    {
        return nullptr;
    }

    const auto graph = getParent()->getParent()->asA<GraphElement>();
    if (!graph)
    {
        return nullptr;
    }

    const auto interfaceInput = graph->getInput(getInterfaceName());
    if (!interfaceInput)
    {
        return nullptr;
    }
    const auto recursiveInput = interfaceInput->getInterfaceInput();

    return recursiveInput ? recursiveInput : interfaceInput;
}

This is enough to fix the render.

Other places that need fixing

I found two other places that'll need similar fixes:

• Input::setConnectedInterfaceName()
• ValueElement::validate()

[kwokcb]

I think the spec does not specify clearly what happens here:

A compound nodegraph provides a set of named input and output connection ports which may be referenced by its contained nodes using interfacename attributes, and interface token names whose values may be substituted into filenames used within the nodegraph; nodes within this nodegraph> adopt the context of that nodegraph.

The <input>s and <token>s of a compound nodegraph may also be connected to other nodes outside the nodegraph at the same scope as the nodegraph itself using nodename attributes;

inputs of nodes within a compound nodegraph may only be connected to the outputs of other nodes within the same compound nodegraph, or to the input connection ports using interfacename

Second line says to use nodename. This does not currently work for parent inputs but works for other node types (sounds like a bug?).

Here are 2 possible options I can see:

1. only for input nodes should it always start to look at nodes outside the compound iff interfacename is use. input to input connections in the same scope are disallowed (already). Update the spec to include this logic.

2. Same as 1. except only support nodename which would be covered by the above spec description.

Searching within a compound and outside a compound at the same time could get confusing. e.g. if you add A inside your example you have A input inside and A outside. You will currently find A inside and pass validation but silently fail connection logic :(.

[frohnej-adsk]

Second line says to use nodename. This does not currently work for parent inputs but works for other node types (sounds like a bug?).

The nodename attribute is usually used to connect to node-elements. <input>s are not node-elements but port-elements, so I'd argue it shouldn't be possible to connect to <inputs>s using nodename. I wouldn't say that's a bug.

I'd say option 1 is most logical. The spec might need to be updated to capture it more clearly though, I agree.

In general, there are three ways to connect an input to something.

1. nodename connects to an output of a node. That node has to be a sibling of the input's parent.
2. nodegraph connects to an output of a node graph. That node graph has to be a sibling of the input's parent.
3. interfacename connects to another input on the parent interface.

So using interfacename to connect to document inputs seems consistent to me. It'd even be consistent for nested node graphs:

<?xml version="1.0"?>
<materialx version="1.39">
  <input name="A" type="color3" value="0, 1, 0" />
  <nodegraph name="compound1">
    <input name="A" type="color3" interfacename="A" />        <!-- connects to /A -->
    <nodegraph name="compound2">
      <input name="A" type="color3" interfacename="A" />      <!-- connects to /compound1/A -->
      <add name="add1" type="color3" nodedef="ND_add_color3">
        <input name="in1" type="color3" interfacename="A" />  <!-- connects to /compound1/compound2/A -->
      </add>
    </nodegraph>
  </nodegraph>
</materialx>

Searching within a compound and outside a compound at the same time could get confusing. e.g. if you add A inside your example you have A input inside and A outside.

It shouldn't be necessary to search both inside and outside of the compound at the same time:

• If the input is the child of a node, search inside of the compound.
• If the input is the child of a compound, search outside of the compound.

In fact, this simplifies to: Search the siblings of the input's parent.

Examples:

• For /compound1/compound2/add1/in1, search the siblings of /compound1/compound2/add1 (i.e. the children of /compound1/compound2)
  • Result: /compound1/compound2/A
• For /compound1/compound2/A, search the siblings of /compound1/compound2 (i.e. the children of /compound1)
  • Result: /compound1/A
• For /compound1/A, search the siblings of /compound1 (i.e. the children of /)
  • Result: /A

[jstone-lucasfilm]

I'll link recent PR #2785, which is designed to address the validation portion of the issue described here.

Feel free to give this a try, @frohnej-adsk, to see if it improves the handling of the examples you discuss above.

[frohnej-adsk]

Thank you @jstone-lucasfilm! Yes, that fixes the validation issue I was observing. Making it render correctly will still require a change in Input::getInterfaceInput() though.