Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra Convert nodes #1905

Merged

Conversation

ld-kerley
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks @ld-kerley, and I'm CC'ing @dbsmythe for his thoughts on the documentation approach.

@ld-kerley
Copy link
Contributor Author

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.

@jstone-lucasfilm jstone-lucasfilm merged commit 9b4b66e into AcademySoftwareFoundation:main Jun 27, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants