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

Fix issue where referenced scenes have incorrect UV set attribute name written out. #1062

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

ysiewappl
Copy link
Contributor

Hi:

This fixes an issue related to referenced Maya scenes being written out with the incorrect varname attribute for the UV set, as it is being determined incorrectly. You can reproduce the issue by attempting to export the cube in the Maya scene with the reference in it and looking at the resulting USDA (the correct file should have map1 as its attribute name.)

Example:

        def Material "usdPreviewSurface1SG"
        {
            token inputs:usd_scene_orig:file1:varname = "map1"
            token outputs:surface.connect = </usd_scene_orig_pCube1/Looks/usdPreviewSurface1SG/usd_scene_orig_usdPreviewSurface1.outputs:surface>

This issue can be demonstrated with the following two scenes, which essentially amount to a single cube with a texture assigned, and then referenced in another scene:

scenes.zip

If you then export the cube in the referenced scene, you get the following without this fix, and even if you have MAYAUSD_EXPORT_MAP1_AS_PRIMARY_UV_SET set appropriately:

        def Material "usdPreviewSurface1SG"
        {
            token inputs:usd_scene_orig:file1:varname = "st"
            token outputs:surface.connect = </usd_scene_orig_pCube1/Looks/usdPreviewSurface1SG/usd_scene_orig_usdPreviewSurface1.outputs:surface>

This is incorrect, and causes USDView to fail to resolve the texture and display it appropriately.

This fix brings up the issue of "how do we determine which is the correct UV set name to write out"? Since we can imagine a scenario like the following, where a shadingEngine could be assigned to multiple meshes; different meshes could then potentially have different data for their currentUVset plug; how would the exporter know which one to then use for writing out the varname attribute for the file node?

image

For now, however, we're fixing the most common case, where a single shadingEngine is associated with a single mesh and single usdPreviewSurface material.

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.

Finding UV linkage is indeed a complex task. I did write code to do this in #876 using a Maya command that simplifies life a lot.

However I did introduce the bug you are seeing at the same time by assuming Maya node names would never contain colons. This is not the case in a referencing context.

The fix for the issue you are having requires correcting my assumption in the following code block by properly reassembling the Maya node name:
https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/fileio/shading/shadingModeExporterContext.cpp#L437
and nothing else. I can submit a PR if you want.

Jerry.

@ysiewappl ysiewappl force-pushed the uvset-name-baked-to-st branch from 59a434e to cc45ac4 Compare January 12, 2021 18:49
@ysiewappl ysiewappl marked this pull request as ready for review January 12, 2021 19:09
@ysiewappl
Copy link
Contributor Author

ysiewappl commented Jan 12, 2021

Finding UV linkage is indeed a complex task. I did write code to do this in #876 using a Maya command that simplifies life a lot.

However I did introduce the bug you are seeing at the same time by assuming Maya node names would never contain colons. This is not the case in a referencing context.

The fix for the issue you are having requires correcting my assumption in the following code block by properly reassembling the Maya node name:
https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/fileio/shading/shadingModeExporterContext.cpp#L437
and nothing else. I can submit a PR if you want.

Jerry.
Hi Jerry:

Thanks for the early reponse! I took a look at the code surrounding this which I wasn't aware of was involved in the mappings and I think I understand the intent behind this a little better now; it still worries me that essentially the UV set name that will win out being written is essentially the one that is being used the most (if I understand the code correctly), which leaves potential for confusing output if an end-user has several meshes with different primary UV set names associated with the same shading engine.

I've gone ahead and updated the code to fix the issue of formatting the node name correctly when a namespace is involved.

Will this work if we are exporting a reference that contains another reference?

You're right; I forgot namespaces will nest. Will change the code as suggested. This is now in 624fa5e35.

@JGamache-autodesk
Copy link
Collaborator

JGamache-autodesk commented Jan 12, 2021

it still worries me that essentially the UV set name that will win out being written is essentially the one that is being used the most (if I understand the code correctly)

The most used one will get the first exported material (the one with the original name). But the code will also generate as many materials as there are UV set names to cover, so the result will match what you have in Maya exactly., The unexpected surprise will be in the number of materials created in the USD file. The import tries to re-merge the extra materials if it can detect that they were generated by Maya.
You might want to try the test scene test\lib\usd\translators\UsdExportUVSetMappingsTest\UsdExportUVSetMappingsTest.ma to get an idea how this looks.

@kxl-adsk kxl-adsk added the import-export Related to Import and/or Export label Jan 12, 2021
@kxl-adsk
Copy link

Thank you for the fix @ysiewappl.

FYI. Some internal build stages run after all tests have trouble to complete and this caused PF to come back red. I checked and all jobs have passed, so we don't have to wait for DevOps to unblock the pipeline before merging this change.

@kxl-adsk kxl-adsk merged commit d8f7792 into Autodesk:dev Jan 13, 2021
@ysiewappl ysiewappl deleted the uvset-name-baked-to-st branch January 13, 2021 18:03
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.

4 participants