Skip to content

Propagate uv scale and offset #1864

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

Conversation

JGamache-autodesk
Copy link
Contributor

Exactly the same as #1840 but on the main branch.

Fixes #1716

Propagate internally added ports when found inside a NodeGraph implementation of a NodeDef.

Testing:

Run render GLSL tests with the shader write option enabled.
Open build/bin/resources/Materials/TestSuite/stdlib/color_management/filename_cm_test/filename_CM_Test_ps.glsl
See that all three image nodes (bl, bl1, and tr1) have uv_scale and uv_offset uniform parameters
See that both NG_tm_test() and NG_tm_retest() now have scale and offset parameters and that these parameters are passed to the mx_image_color3() function.
See that in main() we use all scale and offset uniform parameters when calling either the NodeGraph functions or the regular image node.

Exactly the same as
AcademySoftwareFoundation#1840 but on
the main branch.
// points if that name can be related to the filename input.
// NOTE: This is enough to have UDIMs working with ND_gltf_colorimage
// and ND_UsdUVTexture, so we stop here at this time.
inputSocket = addInputSocket(name, nodeInput->getType());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original comment from @kwokcb:

Hi @JGamache-autodesk,
I don't think this is enough and is inconsistent with how other sockets are "published" using _ name> . This would also resolve the issue that you should be able to support different UDIM scale/offsets for > different image inputs -- i.e. there is no guarantee there is only one set of UDIM information.

Reply by @JGamache-autodesk:

We should definitely split the issue, especially since this PR only attempts to fix single image wrappers like those found for glTf, USD, ADSK, Arnold, etc. Image nodes wrapped in simple NodeGraph implementations.
The second part where we support multi-image nodes with multiple UDIM scales should be indeed discussed and implemented correctly, but maybe should not be a showstopper for first fixing ND_UsdUVTexture and ND_gltf_colorimage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwokcb please advise: Is it a case where I should rework the comment, or is it a case where a fix should only be submitted if it also covers the edge case mentioned in the comment?

@ld-kerley
Copy link
Contributor

Just an FYI - I have some work in progress where I'm trying to deprecate the HwImage.cpp node completely in favor of a data driven node.

@jstone-lucasfilm
Copy link
Member

@ld-kerley @JGamache-autodesk Would you prefer to wait for the upcoming data-driven image node, after which this change would be refactored? Or would you prefer to move forward with this proposal, after which it would be integrated into the upcoming data-driven image node?

@JGamache-autodesk
Copy link
Contributor Author

@jstone-lucasfilm I think we should close this one as it is definitely not compatible with the goals of @ld-kerley.

I will see once the data driven image node lands how much work will be required to have UDIMs working. I already have an evil plan that edits in memory the MaterialX library to add uv_scale and uv_offset ports in case the new data driven image node totally lack any UDIM support of the texture atlas kind.

There is currently no UDIM support in USD at this time unless the shader is the native UsdPreviewSurface. So we will need to see what USD currently does for its UDIM workflow and what this requires on the MaterialX side.

@JGamache-autodesk JGamache-autodesk deleted the bugfix/propagate_uv_scale_and_offset branch March 26, 2025 18:57
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

Successfully merging this pull request may close these issues.

UDIMs uv_scale and uv_offset shader uniform parameters not exposed for tiledimage and UsdUVTexture nodes
3 participants