AcademySoftwareFoundation / AcademySoftwareFoundation/MaterialX

color3 to vector3 type decay issue with MDL backend

Open
#1,038 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2.3k
Forks
451
Avg merge
6d 6h
Merged PRs (30d)
5

Description

I'm having trouble using the UsdPreviewSurface node together with the UsdUVTexture node to achieve normal mapping with the MDL backend.

UsdPreviewSurface exposes the _normal_ input (of type `vector3`)
https://github.com/AcademySoftwareFoundation/MaterialX/blob/a255e89372e768b806c63104336dd5e67edf24fc/libraries/bxdf/usd_preview_surface.mtlx#L20-L22
which is then connected to the _rgb_ output of the UsdUVTexture node (type `color3`).
https://github.com/AcademySoftwareFoundation/MaterialX/blob/a255e89372e768b806c63104336dd5e67edf24fc/libraries/bxdf/usd_preview_surface.mtlx#L48-L49

This is not a problem with GLSL, as `color3` is essentially a typedef for `vec3`:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/a255e89372e768b806c63104336dd5e67edf24fc/source/MaterialXGenGlsl/GlslSyntax.cpp#L213-L223

However, in the MDL backend, `color3` is translated to the native `color` type
https://github.com/AcademySoftwareFoundation/MaterialX/blob/a255e89372e768b806c63104336dd5e67edf24fc/source/MaterialXGenMdl/mdl/materialx/core.mdl#L47-L49
which has no implicit conversions defined (MDL spec 6.12.2):
https://raytracing-docs.nvidia.com/mdl/specification/MDL_spec_1.7.2_17Jan2022.pdf.

This means the generated code results in a compilation error.

There are multiple ways to solve this. I think an elegant solution would be to parameterize the UsdUVTexture node with an output type similar to how it's done for the native MaterialX image node.

For reference, the UsdPreviewSurface specification defines the output type of the UsdUVTexture node as `float3`, which is then implicitly converted to `normal3f` and `color3f`:
https://graphics.pixar.com/usd/release/spec_usdpreviewsurface.html#texture-reader.

EDIT: I should probably note that a warning (but no error) is emitted when using the GLSL backend, and the shader compiles fine. Perhaps the spec can be clarified as to whether this kind of implicit conversion is allowed or not, and the validation logic adjusted?

Here's a test file:
[usd_preview_surface_brass_tiled_normalmap.txt](https://github.com/AcademySoftwareFoundation/MaterialX/files/9229054/usd_preview_surface_brass_tiled_normalmap.txt)

From an authoring viewpoint, the issue can be easily resolved by inserting a convertN node between UsdPreviewSurface and UsdUVTexture, however, I'm not sure if that's something which should be required.

EDIT 2: Turns out that if there's no mechanism to prevent color3f-vector3f mismatches, artists will accidentally create graphs where this is the case (in the wild, f.i. the original Chess Set asset). These graphs then are incompatible with the MDL backend. I think it needs to be clearly defined in the spec whether this implicit conversion is allowed or not. In the case it's allowed, the MDL implementation needs to be fixed. If it's not allowed, compilation/validation should fail, and connecting an input to an output of mismatching type should not be possible in an interactive context (f.i. Houdini.)

Contributor guide

Open the contributing guide

Research direction

Start with the linked UsdPreviewSurface and UsdUVTexture definitions and reproduce the supplied usd_preview_surface_brass_tiled_normalmap.txt case through the MDL backend. Compare the type handling in source/MaterialXGenGlsl/GlslSyntax.cpp with source/MaterialXGenMdl/mdl/materialx/core.mdl and review the referenced USD specification. Done means the intended color3/vector3 conversion behavior is defined and validation or generated MDL code handles it consistently.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-graphics
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.