AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.78k stars 324 forks source link

Add extra Convert nodes #1905

Closed ld-kerley closed 3 days ago

ld-kerley commented 5 days ago

Related to #1895

Fills out more of the "missing" convert nodes.

Notably this moves a lot of the implementations to nodegraphs, to reduce the maintenance going forward and to ensure all shader generation targets are aligned.

ld-kerley commented 5 days ago

The current code in the PR is failing unit tests. I think this might be because of a bug somewhere in GraphElement::flattenSubgraphs(). Before I go too far down the rabbit hole I wanted to check to see if this is a known limitation.

What I think is going on here is as follows....

The PR changes the implementation of ND_convert_float_color3 from an <implementation> with source code,


    <nodegraph name="NG_convert_float_color3" nodedef="ND_convert_float_color3">
        <combine3 name="combine" type="color3">
            <input name="in1" type="float" interfacename="in"/>
            <input name="in2" type="float" interfacename="in"/>
            <input name="in3" type="float" interfacename="in"/>
        </combine3>
        <output name="out" type="color3" nodename="combine" />
    </nodegraph>

Notice we're now using interfacename="in" to pull the input from the node definition.

In the unit test that is failing we're loading a material (minimal failing case below), and then running the following code

        // Flatten all subgraphs.
        doc->flattenSubgraphs();
        for (mx::NodeGraphPtr graph : doc->getNodeGraphs())
        {
            if (graph->getActiveSourceUri() == doc->getSourceUri())
            {
                graph->flattenSubgraphs();
            }
        }
        REQUIRE(doc->validate());

The failure comes because we don't successfully validate the document - the validation error message is Node input binds no value or connection: <input name="in1" type="float">. Notice that the interfacename attribute is missing - this is why validation fails.

It appears that the call to flattenSubgraphs() removes the interfacename attribute.

Is this a known issue? It seems like perhaps any nested nodegraph might fail in the same way. But its particularly sad that this would mean that nodes who's implementations are nodegraphs will fail if they themselves are used in a nodegraph.

----- minimal failure case ----

<materialx version="1.39" colorspace="lin_rec709" fileprefix="../../../Images/">
  <nodegraph name="NG_BrickPattern">
    <input name="uvtiling" type="float" value="3" uisoftmin="1" uisoftmax="16" uiname="UV Tiling" uifolder="Texturing" />
    <convert name="node_convert_1" type="color3">
      <input name="in" type="float" interfacename="uvtiling" />
    </convert>
    <output name="base_color_output" type="color3" nodename="node_convert_1" />
  </nodegraph>
  <standard_surface name="N_StandardSurface" type="surfaceshader">
    <input name="base_color" type="color3" nodegraph="NG_BrickPattern" output="base_color_output" />
  </standard_surface>
  <surfacematerial name="M_BrickPattern" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="N_StandardSurface" />
  </surfacematerial>
</materialx>
jstone-lucasfilm commented 5 days ago

@ld-kerley Good catch, and this sounds like a previously-unknown bug in flattenSubgraphs, rather than any intentional or known limitation. I'm CC'ing @kwokcb and @niklasharrysson for additional visibility, and ideally we'd want to address the bug itself before merging your proposed changes.

ld-kerley commented 4 days ago

I pushed a fix that I think is correct - but it would be good if someone who is more familiar with the intention of this code that I changed, to make sure I'm applying the fix the right way.

jstone-lucasfilm commented 4 days ago

Thanks for the fix, @ld-kerley! Is the conversion table at the root of the MaterialX repository included intentionally, or is that a temporary artifact that has now been integrated into the specification?

ld-kerley commented 4 days ago

Happy for @dbsmythe to edit as he sees fit - I just wanted to take a stab in the same PR so we keep the spec inline with the code.