Unlit shader fails code generation when used with `<mix>` node.

[ld-kerley]

Given the following material:

<materialx version="1.38">
  <mix name="mix_surfaceshader" type="surfaceshader" nodedef="ND_mix_surfaceshader">
    <input name="mix" type="float" value="0.5" />
    <input name="fg" type="surfaceshader" nodename="surface_unlit" />
    <input name="bg" type="surfaceshader" nodename="convert_color3_surfaceshader" />
  </mix>
  <surface_unlit name="surface_unlit" type="surfaceshader" nodedef="ND_surface_unlit">
    <input name="emission_color" type="color3" value="1, 0, 0" />
  </surface_unlit>
  <convert name="convert_color3_surfaceshader" type="surfaceshader" nodedef="ND_convert_color3_surfaceshader">
    <input name="in" type="color3" value="0, 0, 1" />
  </convert>
  <surfacematerial name="material" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="mix_surfaceshader" />
  </surfacematerial>
</materialx>

Generating the OSL shader code yields this the following abbreviated code.

    closure color null_closure = 0;
    surfaceshader convert_color3_surfaceshader_out = surfaceshader(null_closure, null_closure, 1.0);
    NG_convert_color3_surfaceshader(convert_color3_surfaceshader_in, convert_color3_surfaceshader_out);
    surfaceshader mix_surfaceshader_out = surfaceshader(null_closure, null_closure, 1.0);
    mx_mix_surfaceshader(surface_unlit_out, convert_color3_surfaceshader_out, mix1, mix_surfaceshader_out);
    out = mix_surfaceshader_out;

The surface_unlit_out variable is missing. This is due to the code generation for the mix surfaceshader node emitting calls for its inputs with the CLOSURE classification (see here) which unlit shaders don't have.

On initial investigations this could be resolved by changing the code in ShaderNode.cpp.

diff --git a/source/MaterialXGenShader/ShaderNode.cpp b/source/MaterialXGenShader/ShaderNode.cpp
index ff3c8fdb..91396053 100644
--- a/source/MaterialXGenShader/ShaderNode.cpp
+++ b/source/MaterialXGenShader/ShaderNode.cpp
@@ -235,13 +235,10 @@ ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name,
     }
     else if (*primaryOutput->getType() == *Type::SURFACESHADER)
     {
+        newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::CLOSURE;
         if (nodeDefName == "ND_surface_unlit")
         {
-            newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::UNLIT;
-        }
-        else
-        {
-            newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::CLOSURE;
+            newNode->_classification |= Classification::UNLIT;
         }
     }
     else if (*primaryOutput->getType() == *Type::LIGHTSHADER)

With this patch then we generate valid OSL code.

    closure color null_closure = 0;
    surfaceshsader surface_unlit_out = surfaceshader(null_closure, null_closure, 1.0);
    ix_surface_unlit(surface_unlit_emission, surface_unlit_emission_color, surface_unlit_transmission, surface_unlit_transmission_color, surface_unlit_opacity, surface_unlit_out);
    surfaceshsader convert_color3_surfaceshader_out = surfaceshader(null_closure, null_closure, 1.0);
    NG_convert_color3_surfaceshader(convert_color3_surfaceshader_in, convert_color3_surfaceshader_out);
    surfaceshsader mix_surfaceshader_out = surfaceshader(null_closure, null_closure, 1.0);
    mx_mix_surfaceshader(surface_unlit_out, convert_color3_surfaceshader_out, mix1, mix_surfaceshader_out);
    out = mix_surfaceshader_out;

But then I found Issue #839 - which then made me wonder if this incompatibility is intentional? The issue appears to describe the <unlit> material as a container for non-pbr style materials, that perhaps is intentionally incompatible with the pbrlib nodes. If this is the case then I think we should update the specification to disallow the combination of <unlit> with other pbrlib nodes, and then also introduce some sort of validation to report to the user if they unintentionally combine these two incompatible concepts.

[jstone-lucasfilm]

This is a great observation, @ld-kerley, and I would expect the mix node to work with any two surfaceshader streams, regardless of how they were constructed.

I would guess that this is just an untested edge case for OSL shader generation, though I'm CC'ing @niklasharrysson in case he has additional insights.

[niklasharrysson]

I would also expect a mix of any two surfaceshaders to work, including unlit ones. For example just to mask out difference parts of a surface horizontally. So I agree, this looks like an edge case we just haven't tested properly.

The proposed fix looks good to me. At least by a quick look I can't see a good reason for not classifying surface_unlit as a closure. In the OSL and MDL implementations it's implemented using closures.

[ld-kerley]

Sounds great - I'll put together a quick PR with the proposed change.

[ld-kerley]

Digging deeper here When @niklasharrysson added the unlit shader support this function was explicitly updated to return false if any node is classified as UNLIT.

bool GlslShaderGenerator::requiresLighting(const ShaderGraph& graph) const
{
    const bool isBsdf = graph.hasClassification(ShaderNode::Classification::BSDF);
    const bool isLitSurfaceShader = graph.hasClassification(ShaderNode::Classification::SHADER) &&
                                    graph.hasClassification(ShaderNode::Classification::SURFACE) &&
                                    !graph.hasClassification(ShaderNode::Classification::UNLIT);
    return isBsdf || isLitSurfaceShader;
}

The result of this call is used here

    // Determine whether lighting is required
    bool lighting = requiresLighting(graph);

    // Define directional albedo approach
    if (lighting || context.getOptions().hwWriteAlbedoTable || context.getOptions().hwWriteEnvPrefilter)
    {
        emitLine("#define DIRECTIONAL_ALBEDO_METHOD " + std::to_string(int(context.getOptions().hwDirectionalAlbedoMethod)), stage, false);
        emitLineBreak(stage);
    }

And without DIRECTIONAL_ALBEDO_METHOD being defined the shader compilation fails. So I'm wondering again if there was an explicit intention for <unlit> to differ from the rest of the surface shaders.

I can obviously remove the lighting boolean from the guard here, but I'm not sure what else might fail, in the case where a regular material, and an unlit material are combined, and now the requiresLighting() function will return false.

One approach might be to remove the UNLIT distinction completely - but again I'm clued in enough on the backstory here to know why it was added in the first place.

[niklasharrysson]

The intent of the UNLIT classification is to optimize the shader and avoid adding a light loop and all other code for lighting, when this is not needed. So it serves a good purpose.

But perhaps the tracking of the classification is not working as expected then. The classification of the graph should not be set to UNLIT for any inclusion of a surface_unlit node in the graph. If there are other surface shaders in the graph as well, that requires lighting, the classification should be set accordingly.

Will need to investigate further how this tracking behaves in the case of mix surfaceshader nodes.

[niklasharrysson]

I can't reproduce the second issue you got where compilation fails because of a missing DIRECTIONAL_ALBEDO_METHOD.

If I change the code to your proposed change, the test case you have above works for me. No compilation errors and the rendered result looks correct. I also tried mixing a standard_surface node with an surface_unlit node, that that works as well here. Maybe I'm missing something else you changed?

[ld-kerley]

@niklasharrysson Thanks for clarifying for me what the intention of UNLIT is.

Sorry I wasn't clear - I get the compile errors because DIRECTIONAL_ALBEDO_METHOD is not defined when applying the patch above, and then trying to run the unit tests - namely trying to load a material that is using standard surface, or any other non-unlit material.

I think what's happening here, is that with the patch all the materials are being tagged as UNLIT, and so the standard surface material it is not getting the light loop created.

I think perhaps the more correct patch would be...

diff --git a/source/MaterialXGenShader/ShaderNode.cpp b/source/MaterialXGenShader/ShaderNode.cpp
index 4e90fcbb..da2dded6 100644
--- a/source/MaterialXGenShader/ShaderNode.cpp
+++ b/source/MaterialXGenShader/ShaderNode.cpp
@@ -237,7 +237,7 @@ ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name,
     {
         if (nodeDefName == "ND_surface_unlit")
         {
-            newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::UNLIT;
+            newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::CLOSURE | Classification::UNLIT;
         }
         else
         {

To just add the CLOSURE classification to the unlit node, so that the mix correctly creates the necessary variables.

I did notice that with this patch then if I <mix> two unlit surfaces together, then the requiresLighting() check

bool MslShaderGenerator::requiresLighting(const ShaderGraph& graph) const
{
    const bool isBsdf = graph.hasClassification(ShaderNode::Classification::BSDF);
    const bool isLitSurfaceShader = graph.hasClassification(ShaderNode::Classification::SHADER) &&
                                    graph.hasClassification(ShaderNode::Classification::SURFACE) &&
                                    !graph.hasClassification(ShaderNode::Classification::UNLIT);
    return isBsdf || isLitSurfaceShader;
}

returns true. When I might expect it to return false, but that seems like perhaps a separate issue to resolve?

[niklasharrysson]

I think what's happening here, is that with the patch all the materials are being tagged as UNLIT, and so the standard surface material it is not getting the light loop created.

Hi @ld-kerley , I'm no seeing this on my end. This is my code with the patch.

ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name,
...
     }
     else if (primaryOutput->getType() == Type::SURFACESHADER)
     {
         newNode->_classification = Classification::SHADER | Classification::SURFACE | Classification::CLOSURE;
         if (nodeDefName == "ND_surface_unlit")
         {
             newNode->_classification |= Classification::UNLIT;
         }
     }
     else if (primaryOutput->getType() == Type::VOLUMESHADER)
...

Only surface_unlit nodes will be tagged as UNLIT. And with this change I can run the whole render test suite in both GLSL and OSL without any compile errors.

But I think you are right that classification tracking is not correct for the mix node. If both nodes connected upstream are UNLIT it will still just classify as a normal surface shader, since we only match the nodedef here. For a mix node we need to check the upstream nodes. So currently it will add the lighting code unnecessarily in that case. But I think this can be resolve as a separate issue.