Incorrect generation of color transforms in ShaderGraph::createNode

[dcourtois]

Hi,

I've been working on better support for color space recently, and I think there is a bug in how color transforms are created in the ShaderGraph::createNode method.

In this method, the inputs of the node are iterated over, and when needed, color transforms are created. But there is a special case when the input is of type FILENAME_TYPE_STRING: https://github.com/AcademySoftwareFoundation/MaterialX/blob/c34a1066bb194d36174454b3b763a6d68bc8480d/source/MaterialXGenShader/ShaderGraph.cpp#L856-L861

Here, the color transform is generated based on the node's output, but the problem is that if the node has multiple outputs, only the first output will have color transforms correctly applied. See for instance the UsdUVTexture: it has multiple output, the first one being r.

Additionally, I'm not entirely sure about this piece of code in ShaderGraph::populateColorTransformMap: https://github.com/AcademySoftwareFoundation/MaterialX/blob/c34a1066bb194d36174454b3b763a6d68bc8480d/source/MaterialXGenShader/ShaderGraph.cpp#L1281

For instance, again in the case of the UsdUVTexture, this test means that the scalar outputs will never be color corrected. If feels wrong: even if we're only connecting the red channel, it should still be color corrected, right?

So to sum up:

• In ShaderGraph::createNode, I think all outputs should be iterated to generate color correction on all of them, not just the first one.
• In ShaderGraph::populateColorTransformMap, I think at least float types should also be considered.

I'm going to patch my local repo to see if the first point fixes color correction on UsdUVTexture nodes, at least, and I'll post my results back here.

[dcourtois]

Concerning the multiple outputs, here is the patch I applied, and it fixes color correction for multi output nodes. It's a bit hard to read, but I just replaced the ShaderOutput* shaderOutput = newNode->getOutput(); line by a for loop which iterates on all outputs.

diff --git a/source/MaterialXGenShader/ShaderGraph.cpp b/source/MaterialXGenShader/ShaderGraph.cpp
index b8a002f4..90829767 100644
--- a/source/MaterialXGenShader/ShaderGraph.cpp
+++ b/source/MaterialXGenShader/ShaderGraph.cpp
@@ -855,16 +855,19 @@ ShaderNode* ShaderGraph::createNode(const Node& node, GenContext& context)
     {
         if (input->getType() == FILENAME_TYPE_STRING)
         {
-            ShaderOutput* shaderOutput = newNode->getOutput();
-            if (shaderOutput)
+            // FIX: https://github.com/AcademySoftwareFoundation/MaterialX/issues/1436
+            for (ShaderOutput* shaderOutput : newNode->getOutputs())
             {
-                string colorSpace = populateColorTransformMap(colorManagementSystem, shaderOutput, input, targetColorSpace, false);
-                ShaderInput* shaderInput = newNode->getInput(input->getName());
-                if (shaderInput && !colorSpace.empty())
+                if (shaderOutput)
                 {
-                    shaderInput->setColorSpace(colorSpace);
+                    string colorSpace = populateColorTransformMap(colorManagementSystem, shaderOutput, input, targetColorSpace, false);
+                    ShaderInput* shaderInput = newNode->getInput(input->getName());
+                    if (shaderInput && !colorSpace.empty())
+                    {
+                        shaderInput->setColorSpace(colorSpace);
+                    }
+                    populateUnitTransformMap(unitSystem, shaderOutput, input, targetDistanceUnit, false);
                 }
-                populateUnitTransformMap(unitSystem, shaderOutput, input, targetDistanceUnit, false);
             }
         }
         else

[kwokcb]

Thanks for looking into this. Would need to take a closer look at the code, but could be color space was added before mulitouputs in this case.

Strictly speaking there is no color management on floats, and there is no way to “guess” that it is a color channel. UsdUVTexture is interesting case and perhaps something to bring up with Usd folks as well as color mgmt is being looked at within Usd currently.