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

Propagate uv scale and offset #1864

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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?

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
2 participants