Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HdMtlxCreateMtlxDocumentFromHdNetwork should copy colorSpace metadata back to MaterialX #1523

Closed
JGamache-autodesk opened this issue May 21, 2021 · 5 comments

Comments

@JGamache-autodesk
Copy link
Contributor

Description of Issue

Trying to update the MayaUsdPlugin Hydra render delegate to support MaterialX via hdMtlx. Everything works very well, but final render does not have the right color space for input textures.

Steps to Reproduce

  1. Open MaterialX/resources/Materials/Examples/StandardSurface/standard_surface_wood_tiled.mtlx in a text editor. Notice that the tiledimage node has colorspace="srgb_texture" declarations.
  2. Run the file thru UsdMtlx._TestFile( ) and save the resulting stage. Notice the file texture nodes have the correct colorSpace = "srgb_texture" metadata.
  3. Create a test scene that uses the material
    MaterialXSampler.zip
    , and dump the MaterialX document generated by HdMtlxCreateMtlxDocumentFromHdNetwork. You will notice that the colorspace was not added back to the tiledimage node: <input name="file" type="filename" value="Images\brass_color.jpg" />

Separate question: A procedural material example was added recently to MaterialX and fails conversion via UsdMtlx._TestFile( ): MaterialX/resources/Materials/Examples/StandardSurface/standard_surface_brick_procedural.mtlx
This is possibly due to the usage of an inlined node definition. Will this be a supported workflow?

System Information (OS, Hardware)

Windows 10, x86_64.

Package Versions

USD: tip of dev branch
MaterialX: tip Autodesk adsk_contrib/dev branch, with lighting update PR not yet merged.

Build Flags

-DCMAKE_INSTALL_PREFIX=E:/Ws/UsdBuild/PixarUSD/install
-G Ninja
-DCMAKE_BUILD_TYPE=RelWithDebInfo
-DCMAKE_TOOLCHAIN_FILE=c:\ws\vcpkg2019\scripts\buildsystems\vcpkg.cmake
-DMATERIALX_ROOT=e:\Ws\UsdBuild\MaterialX\install
-DPXR_BUILD_OPENIMAGEIO_PLUGIN=TRUE
-DPXR_BUILD_OPENCOLORIO_PLUGIN=TRUE
-DPXR_BUILD_ALEMBIC_PLUGIN=TRUE
-DPXR_ENABLE_MATERIALX_SUPPORT=TRUE
-DTBB_USE_DEBUG_BUILD=OFF

@JGamache-autodesk
Copy link
Contributor Author

On the left is the provided USD file as a USD stage in Maya. On the right is the same material using a MaterialXSurface shader:
BrassTiled

@spiffmon
Copy link
Member

Hi there, @JGamache-autodesk,
I'd call this currently a known limitation... there are stakeholders at Pixar who are concerned about supporting per-attribute colorSpace metadata in Hydra, both for performance reasons, and because of the implications on all renderers to support it, if we did. We need to discuss this further internally and with the MaterialX folks, but have unfortunately had many competing priorities. So I can't yet give you a timetable for this, though I sympathize that the state we are in is currently really confusing, as USD does allow authoring colorSpace metadata and translates it in the usdMtlx reader.

@jtran56
Copy link

jtran56 commented May 21, 2021

Filed as internal issue #USD-6703

@pmolodo
Copy link
Contributor

pmolodo commented Nov 8, 2022

Hi - we're dealing with some fallout from this failure to roundtrip MaterialX colorspace information as well. Are there any updates?

If not - @spiffmon, can you provide some clarification on how fixing HdMtlxCreateMtlxDocumentFromHdNetwork to preserve colorspace expands the scope of the problem? It's input is a USD file, and output is a MtlxDocument... I could see your case if we were talking about the opposite direction (Mtlx to USD)... but, as you note, usdMtlx is already authoring colorSpace in that direction.

Until / unless you decide to revert that decision (and make usdMtlx NOT author colorSpace in USD), I feel the right thing to do is make HdMtlxCreateMtlxDocumentFromHdNetwork play by the same rules. The current situation is a bit like we've built an offramp that abruptly ends 20 feet from the edge of a ravine. We should either build a bridge over the ravine or tear down the offramp. We can't say we don't have a problem just because we haven't paved the last 20 feet. 😉

From a larger perspective... it's a recurring pattern where we have questions of how to deal with non-homogeneous or poorly formatted input data. Frequently, there's a way to automatically correct it at runtime, but it has performance implications. So in an environment where you can enforce quality control and homogenization on input data (ie, a studio environment), implementing such automatic runtime fixes has little benefit and tangible downside.

I think we could start adopting the general pattern of having an auto-fix system that can be disabled via an option somehow, accompanied with checks for situations that would require the fix.

ie, in this situation, we could add the option to do per-attribute colorspace handling, which is on by default. Studio environments that can ensure input colorspaces are consistent can disable colorspace correctives, and still get the maximum performance. And we add in a validation that there are either no declared colorspaces, or that they have the "expected" value. (The validation isn't really required, but would be nice!) As for renderers - they can declare whether or not they support colorspace correctives (and we can optionally run validations if they don't.)

Thoughts?

@spiffmon
Copy link
Member

Hi @pmolodo , thanks for your patience... the MaterialX community has been patiently waiting for this for some years now. The good news is that we're prioritizing colorSpace work, and expect to start early in the new year. Some of our thoughts about it:

  • We plan to only do the work in Hydra 2.0, which will make it much easier, and allow the implementation to be much more efficient
  • We will at least address the MaterialX roundtripping issue (though the "working colorSpace" will need to come from the USD document itself, as the recently added [renderingColorSpace])(d30157e).
  • Doing so will also require transforming color primvars, pre-render-delegate, in a scene index, and we may initially lean on OCIO exclusively, though that could become a plugin point, as the OM does allow specification of a color management system
  • We also plan to extend colorSpace metadata to be applicable to prims, inheriting hierarchically, as it does in MaterialX
  • A stretch goal for this round would be to upgrade Storm and the glslfx implementation of UsdUVTexture to handle texture color transformations beyond srgb, also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants