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

MAYA-105735 Clean up empty map1 texcoord #685

Merged
merged 3 commits into from
Jul 28, 2020

Conversation

JGamache-autodesk
Copy link
Collaborator

If the imported USD mesh does not have a "map1" or a "st" texcoord
stream, then we make sure to get rid of the default created map1
texcoord on the Maya side.

If the imported USD mesh does not have a "map1" or a "st" texcoord
stream, then we make sure to get rid of the default created map1
texcoord on the Maya side.
@kxl-adsk
Copy link

@marsupial You raised some concerns about the UV set support. Please have a look at this PR.

@marsupial
Copy link
Contributor

If there could be a fallback to try for 'uv' if neither 'st' or 'map1' exists, that would be great.
'uv' is the dominant form here, also exported out of Houdini (not LOPs).

@JGamache-autodesk
Copy link
Collaborator Author

@marsupial Maya does always fallback on the UV stream at index 0 on the mesh. Without this fix, the default empty stream "map1" was left in place at index zero and prevented textures from showing correctly.
With the fix, a Houdini USD file with a single UV stream called "uv" will import in a Maya mesh with a single UV stream called "uv". Since this stream is at index zero, the textures will automatically use them and display correctly.

Fixes most of the issues I have seen so far with USD files not showing textures. This does not fix meshes/materials using multiple UV streams. Is this is a common workflow?

@marsupial
Copy link
Contributor

Ok great, sorry for the noise: from reading it looked like only st & map1 were honored.
Right now multiple sets for us aren't super-important, but there are discussions on some workflows that would make it more important in the coming months.

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

Cool, looks good to me! Two super-minor suggestions, but I'd also be ok with it as is.

Thanks @JGamache-autodesk!

// will use that slot. If not, the first texcoord stream to load will replace the default map1
// stream.
bool hasDefaultUVSet = false;
TF_FOR_ALL(iter, primvars)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been slowly yanking out usages of TF_FOR_ALL as we come across them, so these might be good ones to replace with regular range-based for loops:

for (const UsdGeomPrimvar& primvar : primvars) {

Comment on lines 61 to 62
// Default UV set name in Maya
(map1)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have this token available publicly in a place that's accessible to both this file and meshWriteUtils.cpp so we can avoid hard-coding the string there also:

if (setName == "map1") {

Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk left a comment

Choose a reason for hiding this comment

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

Thank you. Aside from what Matt already mentioned, changes makes sense to me.

@kxl-adsk kxl-adsk merged commit 5581cb3 into dev Jul 28, 2020
@kxl-adsk kxl-adsk deleted the t_gamaj/MAYA-105735/fix_empty_map1_on_mesh_import branch July 28, 2020 19:22
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants