Repository navigation
Fix color space handling for filenames in nodegraph implementations - #3122
jstone-lucasfilm wants to merge 2 commits into
Conversation
This changelist addresses color space handling for the filename inputs of nodes implemented by functional nodegraphs. A color space declared on a filename input should be applied once, at the `image` node that reads the file, but shader generation previously applied it to the outputs of the enclosing node, transforming textures twice in nested nodegraphs, and applying a single transform to a blend of files in different color spaces. The following specific changes are included: - Restrict output color transforms to source code implementations, leaving the filename inputs of nodegraph implementations to the `image` nodes within them. - Resolve the color space of a filename input by following its interface name upstream through the enclosing node instances, and then through the interface of a compound nodegraph, taking the first explicit declaration or otherwise the color space active where the file is named. - Cache nodegraph implementations by the target color space and the color transforms that they require, so that instances requiring different transforms each emit their own function, while instances that agree share one. - Resolve the target color space from the document being generated rather than from the library containing a nodegraph implementation, so that transforms within library nodegraphs no longer depend on a target color space override. - Add test suite documents and unit tests to validate the cases above. Fixes AcademySoftwareFoundation#2181, AcademySoftwareFoundation#2182 and AcademySoftwareFoundation#2353
|
|
||
| <nodegraph name="ng1"> | ||
| <nested_image_outer name="texture" type="multioutput"> | ||
| <input name="file" type="filename" value="resources/Images/grid.png" colorspace="srgb_rec709_scene" /> |
There was a problem hiding this comment.
in both test cases, is srgb_rec709_scene different from the default color space? If it's the default, I wonder if the unit test is sufficient to test the colorspace was actually applied or not?
There was a problem hiding this comment.
Good question, @meshula, and I think the answer is yes -- it's different from the default. Both documents declare a working space of lin_rec709_scene, and our shader generation and render tests target lin_rec709_scene as well. So srgb_rec709_scene on the file should be a real sRGB-to-linear decode, not a no-op.
You're right that the test-suite documents alone don't prove where, or how many times, the transform is applied, since they only validate generation and rendering. My thinking is that this is covered, though, by the new Compound Filename Color Spaces unit tests in GenShader.cpp.
Let me know your thoughts, and we can definitely improve the tests if needed!
There was a problem hiding this comment.
Sounds good! I am traveling at the moment, but I will dig in more as time permits!
This changelist addresses color space handling for the filename inputs of nodes implemented by functional nodegraphs. A color space declared on a filename input should be applied once, at the
imagenode that reads the file, but shader generation previously applied it to the outputs of the enclosing node, transforming textures twice in nested nodegraphs, and applying a single transform to a blend of files in different color spaces.The following specific changes are included:
imagenodes within them.Fixes #2181, #2182 and #2353