Fix crash in Shadergen for constant nodes with zero inputs

[bowald]

Closes #2745

Please read Issue #2745 for more background details.

This PR skips optimization "bypassing" values from the input of a node, if the node don't contain any inputs.
This fixes a crash for documents where a custom node was defined which was classified as constant but had zero inputs.

[ld-kerley]

If we stick with this approach I think it makes sense to atleast move this inside the check for CONSTANT on line 982 - so we we don't accidentally skip other optimizations that may be added later.

But I wonder if the better thing to do here is to look at why you're custom node is being identified as a CONSTANT node. That classification is specifically to capture the standard library constant node that has the expected value input. From what I can tell of the classification code in ShaderNode.cpp - it's just matching the node string from the node definition. And in your example they appear different, so I'm a little confused as to why your custom node gets classified this way.

Another option might be to improve the CONSTANT classification logic in ShaderNode.cpp to check for the existence of the expected value input on the node definition, as that is really the assumption being made by the optimization.

[kwokcb]

Some thoughts if it helps:

I think all constant nodes assume they follow by the existing low level <constant> definition. Thus there must be an input.

The definition provided has a constant node inside but does not expose an input so you could rationalize it is not a constant node. i.e. there is no such thing as a constant node which has no inputs.

Also there is no way to "bypass" this node since there is no way to tell what it evaluates. You could set the <output> to have a pass-through value but that is not the same meaning as a constant bypass. It means to disable the node.

[kwokcb]

Some tracing info:

• The constant color node in the nodegraph implementation is classified as CONSTNT so triggers a bypass. This is set when reading and build the shader graph.
• Then the embbed_constant_instance however is also tagged as CONSTANT but there is no explicit code to do so perhaps so it's copied over ? After flattening this node should no longer be even considered but it is.

So it appears the bypass occurs and this check makes sense as a firewall check (see comments later on in the code which say a uniform input is required to be on the nodedef).

[bowald]

@ld-kerley

If we stick with this approach I think it makes sense to at least move this inside the check for CONSTANT on line 982 - so we we don't accidentally skip other optimizations that may be added later.

Good point. We could go with the check inside each if-cases dependent on classification. As DOT also expects an input (and I think the same issue can occur for custom nodes congaing a dot-node)

I'm leaning to remove the check completely and exactly match the classification against the CONSTANT/DOT number. As suggested here

[kwokcb]

Did some more digging and is the cause of the situation:

• The constant color node can be elided since it is created as a shader node with a CONSTANT classification
• The graph is created and it is not initially classified as CONSTANT (has classification == 1).
• When the graph is finalized there is this code:

void ShaderGraph::finalize(GenContext& context)
{
    // Allow node implementations to update the classification
    // on its node instances
    for (ShaderNode* node : getNodes())
    {
        node->getImplementation().addClassification(*node);
    }

which basically promotes all child node classifications to the compound parent which is created for embedded_constant_instance. Thus it's all of the sudden a CONSTANT node. As the node has no inputs it cannot be bypassed.

I thus think the constant folding code is too "lax" in checking for constants since the CONSTANT classification bit is set but it could very well not be a constant since many classification bits could be promoted in the above code. The CONSTANT bits == 64 + original bit of 1 gives you 65 for embedded_constant_instance. The constant color node inside the graph is only 64.

I think if you check explicit for only the constant classification then you can still check if there are inputs but this is a better check. Same for DOT. i.e. the logic is not does it have a classification of CONSTANT or DOT, but that it only has a classification of CONSTANT or DOT.

BTW, I'm not sure why classifications are promoted upwards. Guess it makes sense for some cases like PBR classifications or texture classifications but make constant tagging iffy.

[bowald]

Nice find @kwokcb

I think if you check explicit for only the constant classification then you can still check if there are inputs but this is a better check. Same for DOT. i.e. the logic is not does it have a classification of CONSTANT or DOT, but that it only has a classification of CONSTANT or DOT.

I think it makes sense to remove my numInputs() > 0-check and instead change the if-case tests for Constant and DOT to follow this check instead. From:

node->hasClassification(ShaderNode::Classification::CONSTANT)

to:

node->getClassification() == ShaderNode::Classification::CONSTANT

or add a helper function to the node class: like node->matchClassification(uint32_t c).
If you agree, I will update the PR.

BTW, I'm not sure why classifications are promoted upwards. Guess it makes sense for some cases like PBR classifications or texture classifications but make constant tagging iffy.

I didn't expect this behavior. I didn't expect our custom node to keep the constant classification.

[kwokcb]

It would be nice for code clarity and maintenance to have a new matching method. Any future nodes added to the elide logic could use this as well.

Also, would it be too much trouble to also make bypass throw an exception if there are no inputs as the method assumes 1 or more inputs? Better than outright crashing I think.

Thanks.

[bowald]

Thank you for the feedback. I have updated the PR with all suggestions.

I also updated the comment description for the hasClassification(), as by adding the matchClassifcation() helper function, made the "matching" part of that comment a bit confusing. I hope I made it less confusing with this update.

[bowald]

Got a failing test, it crashed caused by not bypassing the "seperate alpha" dot node in the gltf_pbr colortexture node, line 427-L429. Its not considered for bypassing as it does not explicitly matching the DOT classification. That node is classified as both "DOT" and "TEXTURE", _classification = 0x00100001. This is similar to the CLASSIFICATION=64 + 1 = 65, which caused the original issue ).

The failing test is Test #54: MaterialXTest_GenShader_Track_Application_Variables with the error: No 'geomprop' parameter found on geompropvalue node 'geompropvalue'. Don't know what property to bind. When the dot-node is left in the graph.

I'm not sure that we can explicit match, as the classification bits are describing more then category. I'm now leaning towards if its correct that some custom nodes (such as the one in the issue) should be classified as "CONSTANT".

Not sure how to proceed. If reverting to only throwing in the bypass function, the graph editor will not crash, but other applications with custom nodes may. Maybe we should add a new type of classification corresponding to category, or not loft the "CONSTANT" classification if there is constant nodes in the implementation.

[kwokcb]

I believe the issue you are seeing is because a constant and dot nodes are constant which can be sampled as a texture as set on creation of the shader node:

 // Second, check for specific nodes types
    else if (nodeDef.getNodeString() == CONSTANT)
    {
        newNode->_classification = Classification::TEXTURE | Classification::CONSTANT;
    }
    else if (nodeDef.getNodeString() == DOT)
    {
        newNode->_classification = Classification::TEXTURE | Classification::DOT;
    }

Thus

1. the elide code needs to be classification==TEXTURE|DOT or classification==TEXTURE|CONSTANT (or strip out the TEXTURE BIT then compare).

2. I also had to do this -- which basically tests that you are getting only constant/dot bits if you want to promote constant / dot.

void CompoundNode::addClassification(ShaderNode& node) const
{
    // Add classification from the graph implementation.
    uint32_t  rootClassification = _rootGraph->getClassification();
    // If constant or dot do not promote if there is an existing classication
    bool elide = _rootGraph->hasClassification(ShaderNode::Classification::CONSTANT) || 
                 _rootGraph->hasClassification(ShaderNode::Classification::DOT);

    uint32_t existingClassification = node.getClassification();
    uint32_t new_classification = existingClassification | rootClassification;
    if (elide)
    {
        // Test if only TEXTURE and constant or dot are being added
        uint32_t test_classification = new_classification & ~(ShaderNode::Classification::TEXTURE);
        if (test_classification == ShaderNode::Classification::CONSTANT ||
            test_classification == ShaderNode::Classification::DOT)
        {
            std::cout << "Disallow promoting constant and dot if there is already an existing classification !!!" << existingClassification << " Node: " << node.getName() << " COmpound:" << getName() << std::endl;
            return;
        }
    }
    node.addClassification(rootClassification);

As maybe this is getting too complicated for a crash fix, just adding in the (inputs == 0) check like you had before is good enough. Then leave any curretnt findings as issue discussion points.

Sorry for wasting any time here. The classification / const logic is more convoluted than I thought.

[bowald]

As maybe this is getting too complicated for a crash fix, just adding in the (inputs == 0) check like you had before is good enough. Then leave any current findings as issue discussion points.

Thanks for the feedback, no worries, I'm glad you helot out figuring out what the issue was :)

I reverted the explicit match to the hasClassification check in the optimization func, fixed so the inputs are checked in each case as suggested by @ld-kerley , allowing future opt updates not needing to move this safety check.

I kept the code throwing errors in the bypass-function, as they may be relevant in the future. If those checks is deemed unnecessary, I can remove them from this PR.

[jstone-lucasfilm]

Thanks for working through these different ideas, @bowald, and I like this latest, simplified proposal for the fix.