Fix downstream traversal in graph editor

[kwokcb]

Issue

Update #1348
Fixes #1460
Fixes #1457
Fixes #1459

Fixes

• Includes nodegraph references in port connection cache
• Adds NodeGraph::getDownstreamPorts() so downstream traversal is possible from graphs
• Update GraphEditor logic to use downstream traversal which will now propery traverse through nodegraphs.

Test Document

• Has 2 sets of triplets: { nodegraph, surfacesshader, material }

<?xml version="1.0"?>
<materialx version="1.38">
  <nodegraph name="nodegraph1">
    <output name="output_color3" type="color3" xpos="6.615942" ypos="-2.181035" nodename="multiply_color3" />
    <input name="input_color3" type="color3" value="0.0115494, 0.337408, 0.119903" xpos="2.217391" ypos="-1.922414" />
    <multiply name="multiply_color3" type="color3" xpos="4.166667" ypos="-2.629310">
      <input name="in1" type="color3" interfacename="input_color3" />
    </multiply>
  </nodegraph>
  <surface_unlit name="surface_unlit" type="surfaceshader" xpos="5.652174" ypos="-4.793103">
    <input name="emission_color" type="color3" output="output_color3" nodegraph="nodegraph1" />
  </surface_unlit>
  <surfacematerial name="green_material" type="material" xpos="8.086957" ypos="-4.258621">
    <input name="surfaceshader" type="surfaceshader" nodename="surface_unlit" />
  </surfacematerial>
  <surface_unlit name="surface_unlit2" type="surfaceshader" xpos="5.884058" ypos="-2.724138">
    <input name="emission_color" type="color3" output="output_color3" nodegraph="nodegraph2" />
  </surface_unlit>
  <nodegraph name="nodegraph2">
    <output name="output_color3" type="color3" xpos="6.615942" ypos="-2.181035" nodename="multiply_color3" />
    <input name="input_color3" type="color3" value="0.94132, 0.346111, 0.0989652" xpos="2.217391" ypos="-1.922414" />
    <multiply name="multiply_color3" type="color3" xpos="4.166667" ypos="-2.629310">
      <input name="in1" type="color3" interfacename="input_color3" value="0.94132, 0.346111, 0.0989652" />
    </multiply>
  </nodegraph>
  <surfacematerial name="orange_material" type="material" xpos="8.115942" ypos="-2.594828">
    <input name="surfaceshader" type="surfaceshader" nodename="surface_unlit2" />
  </surfacematerial>
</materialx>

Test Workflow

• Each of the triplet's elements is selected alternately to reflect new logic of finding surface shaders or materials directly or via downstream node / nodegraph.

downstream_traversal.mp4

[kwokcb]

Hi @fl-eholthouser, @jstone-lucasfilm
Just curious if you had any feedback on this one? It should give much more predictable results when finding nodes for preview rendering. Thanks.

[jstone-lucasfilm]

This looks like a good proposal, thanks @kwokcb, and I've written up a couple of suggestions for edits.

[kwokcb]

For demonstration, I've added a test file and test vid + updated the minor src changes.

[jstone-lucasfilm]

This looks very promising, thanks @kwokcb!

I'm running through initial tests with the pull request, though, and I'm encountering a regression where clicking on any node within the standard_surface graph of the initial marble example will trigger a shader compilation error:

I have a build from the main branch in a separate folder, and I don't see the same issue when running the graph editor from that build.

[kwokcb]

Thanks for finding this. I'll check out what's going on.

[kwokcb]

Hmm. Seems like code generation is creating invalid code when trying to render the surface shader inside of the std surface definition graph. As, this was never supported, I'll just ignore trying to do this (same as before).

There is no logic which supports traversing from inside a functional nodegraph to where it's used as a node instance to downstream nodes. If this is desirable then new functionality needs to be added which is beyond the scope of this change.

@niklasharrysson, leaving a ping as the mx_luminance_color3 has a a signature of

void mx_luminance_color3(vec3 _in, vec3 lumacoeffs, out vec3 result)

and is being called with this call:

mx_luminance_color3(opacity, opacity_luminance_lumacoeffs, opacity_luminance_out);

where opacity is float, opacity_luminance_lumacoeffs is vec3, and opacity_luminance_out is float.
Hence the implicit cast error.

[kwokcb]

Actually if you copy the std surface graph to the top level it exhibits the same behaviour. This just seems to be a pre-existing issue which was ignored before.

[kwokcb]

Update logic to avoid trying to traverse downstream through a functional nodegraph which was not supported before.

[niklasharrysson]

Hmm. Seems like code generation is creating invalid code when trying to render the surface shader inside of the std surface definition graph. As, this was never supported, I'll just ignore trying to do this (same as before).

There is no logic which supports traversing from inside a functional nodegraph to where it's used as a node instance to downstream nodes. If this is desirable then new functionality needs to be added which is beyond the scope of this change.

@niklasharrysson, leaving a ping as the mx_luminance_color3 has a a signature of

void mx_luminance_color3(vec3 _in, vec3 lumacoeffs, out vec3 result)

and is being called with this call:

mx_luminance_color3(opacity, opacity_luminance_lumacoeffs, opacity_luminance_out);

where opacity is float, opacity_luminance_lumacoeffs is vec3, and opacity_luminance_out is float. Hence the implicit cast error.

Hey @bernardkwok, I'm not able to reproduce this problem. In the generated code I get opacity as a vec3 so it compiles successfully. The test I did was to copy the graph content to root level, and then connect the surface node to a surfacematerial in order for it to be picked up by our test suite rendering. Perhaps there is a difference in how we run the test?

[kwokcb]

Hi @niklasharrysson,
Yes, the situation you mentioned does work as I tried this in the node editor.

Seems to be either when it's in a functional nodegraph or trying to evaluate the root surface node shader_constructor. If you grab the branch of code for this PR and revert my last change you can see it, but I'm not sure how much time and effort this is worth as it just seems like a edge case.

[kwokcb]

Hi @jstone-lucasfilm, Not sure if you have to to re-look at this. There are some issues being raised by Marcel about missing evaluation / updates, and they could be fixed since this should do proper nodegraph downstream search now.

[jstone-lucasfilm]

One question about the logic in the new NodeGraph::getDownstreamPorts method:

[jstone-lucasfilm]

This looks good to me, thanks @kwokcb!