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

Add UDIM example and fix UDIM bug in MaterialXGraphEditor #2113

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

Conversation

ld-kerley
Copy link
Contributor

Hoping to address #2007 - and also have some data to test while working on improving UDIM support in MaterialX.

I "borrowed" the example from here, and updated the textures a little to be able more easily differentiate the tiles.

Once I modified the textures it became clear that loading this in to MaterialXGraphEditor didn't actually load the correct images in to the correct materials. They would all end up using the fallback material, so this PR includes a small fix to address that.

@kwokcb
Copy link
Contributor

kwokcb commented Nov 14, 2024

Hi @ld-kerley,
I recall that UDIMs worked as is with the sample BB8 model some Siggraph's ago. It has a similar MTLX doc setup to your example. Maybe the geometry needs to be split by UDIM boundaries before-hand which is done in MaterialXView ?

@ld-kerley
Copy link
Contributor Author

@kwokcb Do you know where I can get that BB8 UDIM example? I'm happy to test something we know already works in the graph editor - perhaps there was a code regression or perhaps my new example is subtly broken somehow.

I'm not super familiar with the graph editor code - which is why I was hoping @lfl-eholthouser would be able to take a look too.

The way the current code appears to work it calls RenderView::updateMaterials() with a nullptr here initially - which ends up meaning that materialNode will always be nullptr initially. That means we never end up assigning each of the separate UDIM tile materials here, and instead end up just assigning the fallback material here all the time.

At least thats what I think I was seeing - I do think perhaps if the UDIMs were being assigned to separate pieces of geometry - then the fallback material assignment might be correct, by conicidence, but my understanding of UDIMs is that you're allowed to use multiple tiles on a single shape - at least that how we were using them at Imageworks. Now if only we had a normative specification for UDIMS..... :)

@kwokcb
Copy link
Contributor

kwokcb commented Nov 14, 2024

Hi @ld-kerley,
Best to take the BB8 model offline with @jstone-lucasfilm as it's an private asset.

For the logic, MaterialXView has this option that Jonathan wrote:
image
I believe it needs to enabled to perform the pre-split for UDIMs to work in that viewer.
AFAIK, I don't think the graph editor does this.

For Maya, which builds h/w a texture atlas Jerry and I added code to compute UDIM scale / offset hints which is inserted into the generated shader code. Not sure how to test this out but maybe worth adding in @JGamache-autodesk on the review ?

For the code, I'd need to take a closer look as it's been a while :).

@ld-kerley
Copy link
Contributor Author

Ahh the patch above is for the MaterialXGraphEditor - not MaterialXViewer. I didn't have any problem loading the asset in to MaterialXViewer (when using GLSL backend, MSL backend doesn't currently work).

@lfl-eholthouser
Copy link
Contributor

Hi @ld-kerley ! This seems like a great fix. The initial version of the Graph Editor did not support loading additional geometry so I think UDIMs were just not taken into account originally.

You mentioned the asset working correctly in the Viewer is that without turning on split by udim?

@ld-kerley
Copy link
Contributor Author

I hadn't changed any settings in MaterialXView - and when I load this mesh the split by UDIMs is enabled, and everything appears to render as I'd expect. Toggling this checkbox on/off does not appear to change the image at all for me .

@lfl-eholthouser
Copy link
Contributor

Got it! I was just curious if that was affecting the asset at all.

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.

4 participants