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

Export Normals by default and modify docs to match code behaviour #2768

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Dec 6, 2022

This PR addresses #2767 which was an issue caused from a change in #2713

While I understand that #2713 was done to make the behavior of the plugin match what the doc says, I think the inverse should have happened: the doc should have been updated to match what the code has been doing in production for multiple years now.

So this change reverts the default for exportNormals when subdiv Schema is not set to True, and then updates the document to match the behaviour.

Without this PR, it would require everyone to modify their pre-existing Maya files to get normal export working, and any new meshes would require an extra step of adding a custom attribute before they get normals in their export.

Without normals being exported, many meshes being exported out of Maya are showing up broken in real time renderers that don't apply subdivision. For example, all meshes being exported would be broken when viewed with RealityKit or QuickLook on iOS, because the user authored normals aren't present.

…nt. Fix the doc to match the behaviour of the repo instead.
@neilh-adsk neilh-adsk added bug Something isn't working import-export Related to Import and/or Export labels Dec 6, 2022
pierrebai-adsk
pierrebai-adsk previously approved these changes Dec 6, 2022
Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

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

Looks fine and simple.

@seando-adsk
Copy link
Collaborator

@dgovil Can you have a look at testUsdExportMesh as it failed. The original change #2713 modified the scene file (.ma). Do we need an update to that file again here or an update to the test?

======================================================================
FAIL: testExportAsCatmullClark (testUsdExportMesh.testUsdExportMesh)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/S/jenkins/workspace/ECP/ecg-maya-usd/ecg-mayausd-branch-preflight-python3-linux/ecg-maya-usd/maya-usd/test/lib/usd/translators/testUsdExportMesh.py", line 72, in testExportAsCatmullClark
    self.assertTrue(not m.GetNormalsAttr().Get())
AssertionError: False is not true

@dgovil
Copy link
Collaborator Author

dgovil commented Dec 6, 2022

@seando-adsk ah my bad, I'd reverted the file but forgotten to add it to the commit. Updated the PR now.

@neilh-adsk neilh-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 8, 2022
@seando-adsk seando-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Dec 13, 2022
@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 13, 2022
@seando-adsk
Copy link
Collaborator

@dj-mcg Are you okay with this change?

@dj-mcg
Copy link
Collaborator

dj-mcg commented Dec 13, 2022

In an ideal world we'd be able to fix the behavior to match the docs, since that's what was originally intended and expected within Pixar. However, this has been out in the wild for a while, and a change like this will impact other folks now reliant on this (incorrect) default export behavior. So, I understand @dgovil's position here, and I'm ok to address this on our side if you guys are fine with accepting it.

@seando-adsk seando-adsk merged commit a3d3289 into Autodesk:dev Dec 14, 2022
@oumad
Copy link
Contributor

oumad commented Jan 20, 2023

I confirm having this issue on 0.21.0, it's a serious breaking change for our use case (automatic export of car models coming from nurb surfaces). We're reverting back to 0.20.0 for now until the normal behavior comes back in a later version. Thanks @dgovil for pointing this out. @dj-mcg we're open to re-adapte our pipeline of course if this subject comes back, but in that case a deprecation warning in an intermediary version would be ideal I think.
Thank you everyone for the quick reaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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.

7 participants