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

Write non-connected uvSet names #3339

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Sep 21, 2023

This change makes it so that the Maya export will write the uv set names as default values in addition to the connected value.

While the connected value provides a lot of convenience, we're finding that several applications do not handle the connected case. This unfortunately also includes some older versions of RealityKit, but also other DCC.

We're working to resolve this in those other applications and our own libraries, but in the meantime, this increases the compatibility with applications that weren't expecting a connection here.

Since the connection still exists, it means we preserve the existing behaviour if people want to override the value set.

@BigRoy
Copy link
Contributor

BigRoy commented Sep 21, 2023

Could this start influencing how e.g. a renderer like Arnold interprets shaders for a mesh since now it always appears forced to a specific UV set? I assume then that previously a texture could be used in a shader that is applied to two objects with a different "current UV set" (defined on the geom?) maybe? Not sure how that works out in practice but it's possible with maya shaders at least so was wondering whether we might hit an edge case there.

@dgovil
Copy link
Collaborator Author

dgovil commented Sep 21, 2023

It shouldn't have any difference in render time behaviour.
Basically earlier it was set so that the UVSet was a connection to an attribute instead of directly on that input. That attribute that stored the UVSet name was still on the Material itself.

The only advantage was that you could override the UVSets across the material in one spot (in theory anyway, often you'd have multiples pointing to the same thing anyway). With this change, tools that followed the connection wouldn't be affected (since the value is still there), but DCC's that don't would still be able to look up the UVSet.

@seando-adsk seando-adsk added the import-export Related to Import and/or Export label Sep 27, 2023
@JGamache-autodesk
Copy link
Collaborator

Can you hide that code behind an env var so only clients that knowingly export to one of those DCC get affected? The generated USD is correct, but I am a bit worried with bloat and absence of support for multiple UV sets.

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 4, 2023

Sure. What would you like the env var to be called?

@JGamache-autodesk
Copy link
Collaborator

USDMAYA_PROVIDE_DEFAULT_TEXCOORD_PRIMVAR_NAME ?

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 5, 2023

Thanks. I'll make that change.

I was curious though, when you said "the absence of support for multiple UV Sets", would this not just have the same support for multiple UV Sets as the connection only method would? Since it's setting the value to the same thing that the connection's target was being set to anyway.

I don't think there should be a loss in capability, but maybe I'm missing another place where the connections target value is set for an alternate UV Set?

@JGamache-autodesk
Copy link
Collaborator

We initially set a default value of "st" (or equivalent), but there is very interesting post-processing code in _UVMappingManager that will correctly remap complex UVset scenarios, including multiple UV sets as tested in testExportRenamedUVSetMappings.

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 5, 2023

Ah I must have missed that. I'll update the PR to use that and update the test as well to check for these changes. Thanks for the pointer.

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 12, 2023

@JGamache-autodesk , okay updated the code to put it behind an environment flag and support multiple UV Sets based on the manager. Used MAYAUSD_PROVIDE_DEFAULT_TEXCOORD_PRIMVAR_NAME to be consistent with the other flags (MAYAUSD instead of USDMAYA)

I also added a test + assets because I didn't see one that tested just singular material with UVSets in the way we use them.

Let me know if I should add documentation somewhere too. I wasn't sure where to do so.

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

LGTM!

@seando-adsk
Copy link
Collaborator

@dgovil I ran the preflight for you but there is a test failure on testUsdExportUVSetMappings

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 13, 2023

Ah that's odd. I'll take a look. Thanks!

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 13, 2023

Okay, I pushed an update to the Maya sample file that isolates things and removes some of the elements that were causing issues in the log. I believe it should work now.

@JGamache-autodesk
Copy link
Collaborator

Might indeed help. You also might want to search and replace the texture file paths to make them relative since the images are in the same folder as the scene file.

…newer Maya versions and therefore are incompatible
@dgovil
Copy link
Collaborator Author

dgovil commented Oct 13, 2023

Removed the absolute paths , but also had to remove some more setup stuff that seems to only be compatible with Maya 2024. Tried loading the file in Maya 2022 and it loads without warnings/errors so I'm hopeful that's the last of the attributes I missed cleaning out. (I wish there was a Maya file validator :-) )

@seando-adsk if you can run the preflight again when you get a chance. I'm hopeful this is the last failure in the test asset

@seando-adsk
Copy link
Collaborator

@dgovil Seems like testUsdExportUVSetMappings is still failing.

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 16, 2023

Apologies for that, it's just difficult to reproduce the CI tests locally since the tests don't fail here due to the difference in Maya configuration .

I'll try again or see if I can make a file that's generated procedurally.

@JGamache-autodesk
Copy link
Collaborator

For examples of procedurally generated files: grep for any test Python file containing the string vertexUvOne.

@dgovil
Copy link
Collaborator Author

dgovil commented Oct 16, 2023

@seando-adsk Should be resolved now. That one was my bad, I'd missed one commit when cherry-picking my changes over that moved the environment variable to the CMake file instead of inside the unitTest.

I also removed some Python 3 specific code since I'd forgotten that Maya 2022 has a Python 2 only variant for Linux and Windows.

Could you run the pre-flight again when you're able to. Thank you, and apologies again.

@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 17, 2023
@seando-adsk seando-adsk merged commit 6f6a72d into Autodesk:dev Oct 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants