Unify texcoord conventions across render targets#3001
Conversation
This changelist establishes a consistent convention for texture coordinates across the MaterialX codebase, aligning with the MaterialX specification in placing the origin of texture space at the lower left. Geometry loaders now always return texture coordinates in the MaterialX convention, and each render target compensates for the image origin of its own texture lookups at shader generation or texture upload time, replacing the configurable vertical flips previously scattered across loaders, viewers, and tests. The following specific changes are included: - Remove the `texcoordVerticalFlip` argument from `GeometryLoader::load` and `GeometryHandler::loadGeometry`, with `CgltfLoader` now always converting texture coordinates from the upper-left origin of glTF to the lower-left origin of MaterialX, and `TinyObjLoader` passing OBJ coordinates through unchanged. - Assign `fileTextureVerticalFlip` by render target, enabling it for GLSL, MSL, Slang, and OSL to compensate for their upper-left image origins, and disabling it for MDL and for web clients that flip images vertically on upload, with the option's documentation expanded to describe this convention. - Render test geometry in OSL testrender through a new `%render_geometry%` scene template token and `OslRenderer::setRenderGeometry` method, replacing the built-in sphere primitive whose texture layout could not be modified. - Replace the `--materialxtest_mode` flag in MDL render tests with `--uv_scale 0.5 1 --uv_repeat`, aligning the texture layout of the built-in MDL sphere with test-suite geometry without inverting its V coordinates. - Add a `setTargetGenerationOptions` method to `ShaderRenderTester`, along with shared `findRenderGeometry` and `loadRenderGeometry` utilities, removing duplicated logic and filename-based coordinate flips from the render tests for each target. In addition to its benefits to consistency and maintainability, this changelist also improves the visual parity between MSL and OSL renders in GitHub CI, with a few typical examples below (per-pixel RMS error, before -> after): - standard_surface_brick_procedural: 0.00916 -> 0.00882 - standard_surface_gold: 0.01496 -> 0.01276 - open_pbr_carpaint: 0.00720 -> 0.00605 - gooch_shade: 0.00939 -> 0.00738
|
I'm attaching the latest render comparison PDF generated by GitHub CI with these changes: |
|
In addition to the reviewers noted above, I'm CC'ing @krohmerNV and @jreichel-nvidia for their thoughts from the NVIDIA MDL perspective. |
kwokcb
left a comment
There was a problem hiding this comment.
It's great to clear up this "flip" semantic.
I would like to be explicit about the MTLX convention and have gen options explicitly codify the convention (e.g. ORIENTATION_LOWER_LEFT, ORIENTATION_UPPER_LEFT)
The input and target conventions would be both specified and flip is determined if they differ.
One case which I think is worth clarifying is what happens when the renderer's geometry + shader language is based on a upper-left convention -- such as DirectX. The DX geometry loaders I assume would be used (not MTLX ones) + follow the upper-left convention. In the proposal above instead of "flip", you'd have the source geometry orientation and renderer orientation both being ORIENTATION_UPPER_LEFT.
This can occur with the web viewer as well where the renderer's geometry loaders cannot assume to match lower left.
--
One final thing is how does this interact with the "flip" option on image loaders for upper-left and lower-left renderer scenarios. Using both of these has been confusing in the past.
| else if (flipV) | ||
| else | ||
| { | ||
| // Convert texture coordinates from the glTF convention, with the |
There was a problem hiding this comment.
As any geometry loader could be used to load geometry this may not be always true.
There was a problem hiding this comment.
That's a good point, and we should remain flexible about this in the future, though as of today the web viewer loads geometry exclusively through the three.js GLTFLoader, where the glTF convention holds.
| /// when the target samples images with their origin at the lower left | ||
| /// (e.g. MDL), or when the client compensates by flipping images | ||
| /// vertically at upload time, as web clients typically do. | ||
| bool fileTextureVerticalFlip; |
There was a problem hiding this comment.
A suggestion here to be explicit and say what the target orientation is vs "flip". e.g. ORENTATION_LOWER_LEFT, ORIENTATION_UPPER_LEFT. Then a comparison between source (MaterialX) and (renderer) can be made and flip as required.
It would be nice to codify MaterialX lower left orientation somewhere in any case.
If the plans to support USD input graphs (or other) goes forward the default uv convention could be different.
There was a problem hiding this comment.
Thanks for this suggestion, @kwokcb, and I agree that the MaterialX convention deserves to be codified more visibly, with the expanded documentation of this option being a first step in that direction.
I see the appeal of expressing the target as an orientation rather than a flip, though since fileTextureVerticalFlip is public API with bindings in Python and JavaScript, renaming or retyping it would break source compatibility for downstream clients that set it. So my proposal would be to consider this improvement at a future API-transition point for MaterialX, such as the initial development phase of MaterialX v1.40 or v2.0.
On the source side, my sense is that we should keep the MaterialX convention fixed rather than configurable, and this changelist is intended as a step in that direction: geometry loaders normalize texture coordinates to the MaterialX convention at load time, so that shader generation only needs to account for the image orientation of its target. I'd anticipate a future OpenUSD importer following the same pattern, converting texture coordinates at import time if needed, though USD conveniently places its texture origin at the lower left as well.
|
|
||
| StringMap replacementMap; | ||
| replacementMap[OUTPUT_SHADER_TYPE_STRING] = outputShader; | ||
| // POSIX-style separators keep the geometry path valid in scene XML on all platforms. |
There was a problem hiding this comment.
Can you clarify what issue this addresses. Thx.. Not sure what POSIX format and XML have in common and where.
There was a problem hiding this comment.
Good point, and I've clarified this comment in the latest version. The geometry path is written with POSIX-style separators, since testrender supports these on all platforms, including Windows.
| // sphere geometry used by other renderers in the test suite. | ||
| // The default V orientation of the renderer matches the | ||
| // MaterialX convention, with the origin at the lower left. | ||
| command += " --uv_scale 0.5 1 --uv_repeat"; |
There was a problem hiding this comment.
How do you know this is a the default sphere ?
There was a problem hiding this comment.
Good question, and I've clarified this in the latest version as well. Since no scene geometry is specified on the command line, df_vulkan renders its built-in sphere, whose texture coordinates span (0, 0) to (2, 1).
Following up on review feedback from Bernard Kwok, this changelist improves the code comments in OSL and MDL render tests.
|
Thanks for these thoughts, @kwokcb. On the suggestion of explicit orientation enumerations, see my reply in the comment thread above, where my proposal would be to consider this improvement at a future API transition point for MaterialX. For integrations such as DirectX, where the native convention for both geometry and images places the origin at the upper left, my recommendation would follow the same pattern as the geometry loaders in this changelist: the integration converts texture coordinates to the MaterialX convention at import time, and enables On the interaction with vertical flips in image loaders, the guiding principle is that exactly one vertical compensation should be active for a given target: either the generated shader flips the V coordinate of file texture lookups, or the client flips images vertically at upload time, but never both. The web viewer is an example of the latter approach, flipping images on upload and leaving fileTextureVerticalFlip disabled, and I'm hoping the expanded documentation of this option makes these two paths |
| // geometry used by other renderers in the test suite. The | ||
| // default V orientation of df_vulkan matches the MaterialX | ||
| // convention, with the origin at the lower left. | ||
| command += " --uv_scale 0.5 1 --uv_repeat"; |
There was a problem hiding this comment.
The df_vulkan example does not have these fine-grained options (only the DXR example). But we would be willing to add them here, too. Is there a need for --uv_repeat (--materialxtest_mode did only flip+stretch)?
There was a problem hiding this comment.
Thanks @jreichel-nvidia, and that would be much appreciated!
Taking a closer look at the analytic texture coordinates of the built-in df_vulkan sphere, I think you're right that --uv_repeat is unnecessary. Since the sphere's U range spans exactly (0, 2), scaling by 0.5 lands it in (0, 1), and no wrapping should be required.
I've removed --uv_repeat from the render test command in the latest version, so --uv_scale would be the only new option needed in df_vulkan, though we'd welcome the full set of texture coordinate options if you find them valuable for other purposes.
Once these options land in an MDL SDK release, we can raise the minimum df_vulkan version in the MaterialX build from its current value of 2025.0.3, and I'd be interested in your thoughts on sequencing. Would it be reasonable for us to include this MaterialX improvement ahead of the update on the MDL side, with the understanding that MDL render tests would require a df_vulkan build from your upcoming release, or would you recommend that we wait?
Since the built-in `df_vulkan` sphere computes its texture coordinates analytically, with U spanning exactly (0, 2), the `--uv_scale` option is sufficient to place them in the unit square, and no wrapping is required. This minimizes the set of new options needed in the `df_vulkan` example renderer.
This changelist establishes a consistent convention for texture coordinates across the MaterialX codebase, aligning with the MaterialX specification in placing the origin of texture space at the lower left. Geometry loaders now always return texture coordinates in the MaterialX convention, and each render target compensates for the image origin of its own texture lookups at shader generation or texture upload time, replacing the configurable vertical flips previously scattered across loaders, viewers, and tests.
The following specific changes are included:
texcoordVerticalFlipargument fromGeometryLoader::loadandGeometryHandler::loadGeometry, withCgltfLoadernow always converting texture coordinates from the upper-left origin of glTF to the lower-left origin of MaterialX, andTinyObjLoaderpassing OBJ coordinates through unchanged.fileTextureVerticalFlipby render target, enabling it for GLSL, MSL, Slang, and OSL to compensate for their upper-left image origins, and disabling it for MDL and for web clients that flip images vertically on upload, with the option's documentation expanded to describe this convention.%render_geometry%scene template token andOslRenderer::setRenderGeometrymethod, replacing the built-in sphere primitive whose texture layout could not be modified.--materialxtest_modeflag in MDL render tests with--uv_scale 0.5 1 --uv_repeat, aligning the texture layout of the built-in MDL sphere with test-suite geometry without inverting its V coordinates.setTargetGenerationOptionsmethod toShaderRenderTester, along with sharedfindRenderGeometryandloadRenderGeometryutilities, removing duplicated logic and filename-based coordinate flips from the render tests for each target.In addition to its benefits to consistency and maintainability, this changelist also improves the visual parity between MSL and OSL renders in GitHub CI, with a few typical examples below (per-pixel RMS error, before -> after):