Fix pass-through transforms for legacy color space names by jstone-lucasfilm · Pull Request #3062 · AcademySoftwareFoundation/MaterialX

jstone-lucasfilm · 2026-09-05T23:40:02Z

This changelist follows up on the color interop alignment in #3042, resolving a regression for documents that use legacy color space names such as lin_rec709.

With shader generation now targeting lin_rec709_scene, every color input in a legacy document was seen as requiring a transform, and the default color management system expressed the equivalence of these names as a pass-through dot node rather than omitting the transform. This added a redundant node and renamed uniform for each color input, and in MDL it dropped the uniform qualifier from inputs such as glTF PBR attenuation_color, producing shaders that fail to compile.

The following specific changes are included:

Add a virtual ColorManagementSystem::isNoOpTransform method, with a DefaultColorManagementSystem override that recognizes legacy and color interop names for the same color space, and consult it in ShaderGraph so that equivalent color spaces introduce no transform nodes.
Preserve the uniform flag of an input when inserting a color or unit transform node, so that MDL declares the published value correctly.
Update TextureBaker and the Metal render test to the lin_rec709_scene working space, with baked documents now written using color interop names.
Expose isNoOpTransform in Python, with unit tests covering the alias logic and the generated GLSL and MDL shaders.

This changelist follows up on the color interop alignment in AcademySoftwareFoundation#3042, resolving a regression for documents that use legacy color space names such as `lin_rec709`. With shader generation now targeting `lin_rec709_scene`, every color input in a legacy document was seen as requiring a transform, and the default color management system expressed the equivalence of these names as a pass-through `dot` node rather than omitting the transform. This added a redundant node and renamed uniform for each color input, and in MDL it dropped the uniform qualifier from inputs such as glTF PBR `attenuation_color`, producing shaders that fail to compile. The following specific changes are included: - Add a virtual `ColorManagementSystem::isNoOpTransform` method, with a `DefaultColorManagementSystem` override that recognizes legacy and color interop names for the same color space, and consult it in `ShaderGraph` so that equivalent color spaces introduce no transform nodes. - Preserve the uniform flag of an input when inserting a color or unit transform node, so that MDL declares the published value correctly. - Update `TextureBaker` and the Metal render test to the `lin_rec709_scene` working space, with baked documents now written using color interop names. - Expose `isNoOpTransform` in Python, with unit tests covering the alias logic and the generated GLSL and MDL shaders.

doug-walker

Good catch, Jonathan! Please see my questions below.

I'd like to add the new function to OcioColorManagementSystem too. Would you prefer I make a separate PR, or provide code you could include in your PR?

doug-walker · 2026-09-06T18:45:25Z

        return sourceSpace == other.sourceSpace &&
               targetSpace == other.targetSpace &&
               type == other.type;
    }


Should this use isNoOpTransform?

Alas, I can't access our color management system here, since it's the equality operator on the ColorSpaceTransform struct!

doug-walker · 2026-09-06T18:54:57Z

    if (sourceSpace == targetSpace)
    {
        return _document->getNodeDef("ND_dot_" + transform.type.getName());
    }


Do we still want to return a dot node? This was added recently in my PR #2954, thinking that some kind of nodedef must be returned, but maybe there is a better approach?

I think we still want the dot node as a fallback. With this changelist, ShaderGraph consults isNoOpTransform before requesting a nodedef, so the graph itself never receives a pass-through node for these cases. The remaining consumers are direct callers of supportsTransform and createNode, and for those I'd rather report a trivially supported transform than throw "No nodedef found".

doug-walker · 2026-09-06T19:48:55Z

+    virtual bool isNoOpTransform(const string& sourceColorSpace, const string& targetColorSpace) const
+    {
+        return sourceColorSpace == targetColorSpace;
+    }


This seems to me more like an areColorSpacesEqual function rather than an isNoOpTransform. If it were the latter, I think it would need to return true if isNoOpColorSpace were true for either source or target.

The only place isNoOpColorSpace is currently used is in that same if-statement in populateColorTransformMap in shaderGraph.cpp. I'm thinking that we should build the isNoOpColorSpace functionality into isNoOpTransform and then we could simplify the API by removing isNoOpColorSpace (since it has not been included in an official release yet)?

I'm not sure there is a reason for wanting to know if a color space is a no-op, other than in order to decide if a transform involving it is a no-op, so isNoOpTransform might be a better level of abstraction. But I have no objection if you prefer to keep things more granular and instead have isNoOpColorSpace and areColorSpacesEqual.

Unlike for the DefaultColorManagementSystem, in the OcioColorManagementSystem, it's possible for isNoOpColorSpace to be false and areColorSpacesEqual to be false (if based on alias name resolution) and yet still return a dot node as the result because when OCIO optimizes the mathematical operators it's possible that the result is an identity. But it seems isNoOpColorSpace is probably intended to be a more lightweight function and so perhaps we should just implement it on the OCIO side by just doing alias name resolution and isData on the color space. I think it would depend on how undesirable it is to return a dot node.

Agreed on both points, and I've taken your first suggestion in the latest update to this PR. Since neither method has shipped in a release, isNoOpColorSpace is removed from the base class and Python bindings, and isNoOpTransform now returns true for identical names or when either name is a reserved no-op space.

meshula

I don't have anything to add beyond deferring to Doug's comments where he's left them.

Overall it looks good to me, although I'd appreciate slight tweaks to two of the comments which (huge apologies if I'm reading it wrong) appear to be LLM book keeping comments.

@doug-walker

Following up on review notes from @doug-walker and @meshula, this changelist consolidates the `isNoOpColorSpace` and `isNoOpTransform` methods of `ColorManagementSystem` into a single `isNoOpTransform` query, and clarifies the comments and string literals of the new unit test. Both methods were introduced in the color interop alignment of AcademySoftwareFoundation#3042 and have not yet shipped in a MaterialX release, so a single query covering both equivalent color space names and no-op color spaces is the simpler abstraction for both `ShaderGraph` and color management system implementations. The following specific changes are included: - Fold the no-op color space check into `ColorManagementSystem::isNoOpTransform`, which now returns true for identical names or when either name is a reserved no-op color space, and remove `isNoOpColorSpace` from the base class and Python bindings. - Update the `DefaultColorManagementSystem` and `OcioColorManagementSystem` overrides to call their base implementations, with the OCIO override additionally recognizing color spaces flagged as data in the active config. - Consult `isNoOpTransform` in `DefaultColorManagementSystem::getNodeDef`, so that the pass-through node returned to direct callers of `supportsTransform` and `createNode` follows the same rule as `ShaderGraph`, which omits these transforms entirely. - Collapse the color space checks in `ShaderGraph::populateColorTransformMap` into a single `isNoOpTransform` query. - Return by value from the `remapColorSpace` helper, removing a reference that would dangle if a caller passed a temporary.

@meshula

Following up on review notes from @meshula, this changelist expresses the test documents of the no-op color space test as raw string literals, matching the color space test updated in the previous commit, so that both tests share a single style for inline MaterialX documents.

doug-walker

Your updates look good Jonathan! I have one suggestion for the OCIO part.

doug-walker · 2026-09-08T22:27:53Z


    OCIO::ConstColorSpaceRcPtr colorSpaceObj = _config->getColorSpace(supportedColorSpace);
    return colorSpaceObj && colorSpaceObj->isData();
 }


In order to handle aliased names, I suggest replacing this with the following, and then calling it below.

bool OcioColorManagementSystemImpl::isNoOpTransform(const string& source, const string& target) const { const char* supportedSource = getSupportedColorSpaceName(source.c_str()); if (!supportedSource) { return false; } const char* supportedTarget = getSupportedColorSpaceName(target.c_str()); if (!supportedTarget) { return false; } OCIO::ConstColorSpaceRcPtr sourceObj = _config->getColorSpace(supportedSource); OCIO::ConstColorSpaceRcPtr targetObj = _config->getColorSpace(supportedTarget); if (!sourceObj || !targetObj) { return false; } return sourceObj->isData() || targetObj->isData() // The getName call converts aliases into the canonical names. || (sourceObj->getName() == targetObj->getName()); }

Thanks, @doug-walker, and I've included your suggestion in the latest update to the PR. One small adjustment that I made is to compare the canonical names as strings rather than pointers, so that we don't depend on OCIO returning identical pointers in this situation, although it's possible that either approach would be safe in the context of OCIO.

Since our GitHub CI doesn't yet build with OCIO enabled, a validation pass on your side may be warranted before we merge.

@doug-walker

Following up on review notes from @doug-walker, this changelist extends the `OcioColorManagementSystem` override of `isNoOpTransform` to recognize names that resolve to the same color space in the active OCIO config, such as an alias and its canonical name.

doug-walker

That looks perfect Jonathan! I proposed a new test below to validate the OCIO-based alias resolution is working. I cloned your branch and built with OCIO enabled and all tests pass.

doug-walker · 2026-09-09T04:52:31Z

Suggest "Color Spaces" --> "Color Transforms"

doug-walker · 2026-09-09T04:53:33Z

+        CHECK(ocioColorManagementSystem->isNoOpTransform("none", "lin_rec709_scene"));
+        CHECK(ocioColorManagementSystem->isNoOpTransform("data", "lin_rec709_scene"));
+        CHECK(ocioColorManagementSystem->isNoOpTransform("Raw", "lin_rec709_scene"));
+        CHECK(ocioColorManagementSystem->isNoOpTransform("lin_rec709", "lin_rec709_scene"));


Please add the following to test that OCIO-based alias resolution is working:
CHECK(ocioColorManagementSystem->isNoOpTransform("ACES - ACES2065-1", "lin_ap0"));

@doug-walker

Following up on review notes from @doug-walker, this changelist adds a unit test for the OCIO-based alias resolution introduced in the previous commit, and aligns surrounding names and comments with the consolidated `isNoOpTransform` query.

jstone-lucasfilm · 2026-09-09T16:53:01Z

@doug-walker @meshula Let me know if this latest version looks good to you, and ideally I'd like to merge this work soon, as it addresses a failure in our nightly CI builds that was introduced by #3042. (Multi-hour nightly CI builds FTW!)

doug-walker

LGTM!

jstone-lucasfilm requested a review from doug-walker September 5, 2026 23:40

doug-walker reviewed Sep 6, 2026

View reviewed changes

meshula suggested changes Sep 7, 2026

View reviewed changes

Comment thread source/MaterialXTest/MaterialXGenShader/GenShader.cpp Outdated

Comment thread source/MaterialXTest/MaterialXGenShader/GenShader.cpp Outdated

jstone-lucasfilm added 2 commits September 7, 2026 17:05

doug-walker reviewed Sep 8, 2026

View reviewed changes

doug-walker reviewed Sep 9, 2026

View reviewed changes

doug-walker approved these changes Sep 9, 2026

View reviewed changes

jstone-lucasfilm merged commit 7274d34 into AcademySoftwareFoundation:main Sep 9, 2026
36 checks passed

jstone-lucasfilm deleted the dev_color_space_fix branch September 9, 2026 18:04

Conversation

jstone-lucasfilm commented Sep 5, 2026

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstone-lucasfilm commented Sep 9, 2026

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants