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 #1840

Conversation

JGamache-autodesk
Copy link
Contributor

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.
    Filename_CM_Test_ps.zip

Custom image nodes like ND_gltf_colorimage and ND_UsdUVTexture did not
produce uv_scale and uv_offset inputs. Fix that by propagating the added
inputs found inside a CompoundNode which represents a NodeGraph
implementation of a NodeDef.

Fixes AcademySoftwareFoundation#1716
// 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

@kwokcb kwokcb May 28, 2024

Choose a reason for hiding this comment

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

Hi @JGamache-autodesk,
I don't think this is enough and is inconsistent with how other sockets are "published" using <node name>_ <input 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@jstone-lucasfilm
Copy link
Member

@JGamache-autodesk We've now merged development work on MaterialX 1.39 back to the main branch of MaterialX, in preparation for the final weeks of development on the 1.39.0 release. When you have a chance, could you retarget this pull request back to the main branch as well?

@jstone-lucasfilm jstone-lucasfilm changed the base branch from dev_1.39 to main May 31, 2024 18:09
@jstone-lucasfilm jstone-lucasfilm changed the base branch from main to dev_1.39 May 31, 2024 18:11
JGamache-autodesk added a commit to autodesk-forks/MaterialX that referenced this pull request Jun 3, 2024
Exactly the same as
AcademySoftwareFoundation#1840 but on
the main branch.
@JGamache-autodesk
Copy link
Contributor Author

Closing. Another pull request targeting main branch has been submitted.

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.

None yet

3 participants