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 default export logic if subdivScheme is none #2713

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

dj-mcg
Copy link
Collaborator

@dj-mcg dj-mcg commented Nov 9, 2022

Skip authoring normals by default when exporting a mesh with its subdivSceme set to NONE. Require the emitNormals tag to be set to TRUE for normals to be written for these meshes.

Skip authoring normals by default when exporting a mesh with its
subdivSceme set to NONE. Require the emitNormals tag to be set to TRUE
for normals to be written for these meshes.
@seando-adsk seando-adsk added the import-export Related to Import and/or Export label Nov 10, 2022
pierrebai-adsk
pierrebai-adsk previously approved these changes Nov 10, 2022
@seando-adsk
Copy link
Collaborator

@dj-mcg The preflight failed. Can you have a look.

@pierrebai-adsk
Copy link
Collaborator

The mesh render test failed due to "px_render" plugin not being found. I had the same error yesterday. Re-running the PF fixed it. I wonder if some PF computer are setup wrong?

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Nov 11, 2022

Ah - I think this dependency snuck into the test maya file. I'll fix it up and kick off another preflight

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Nov 14, 2022

Oops, I guess I'll need to use Maya 2020 (or Maya 2019?) to update the test file... Or maybe I can just hand edit the .ma?

@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 16, 2022
@seando-adsk seando-adsk merged commit f8f82ab into Autodesk:dev Nov 16, 2022
@dj-mcg dj-mcg deleted the pr/EmitNormals_Default_Fix branch November 17, 2022 23:06
@dgovil
Copy link
Collaborator

dgovil commented Dec 5, 2022

Hi I'm curious why this change was necessary? The PR doesn't provide reasoning and I think the logic is incorrect.
If subdivision schema is set to None, is when one should expect normals to be exported?
Per https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usd/usdGeom/schema.usda#L1018 the none type is when one should be exporting normals.

This is breaking our exports as a result because we suddenly don't have normals as of this PR.

@dgovil dgovil mentioned this pull request Dec 5, 2022
@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Dec 6, 2022

You can still emit normals for the none type if you explicitly tag the mesh to do so. If the tag isn't explicitly authored, we shouldn't be writing normals as documented under USD_EmitNormals here:
https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/commands/Readme.md

@dgovil
Copy link
Collaborator

dgovil commented Dec 6, 2022

I think this is a breaking change though and a silent one at that. I acknowledge that y'all documented this years ago but the behaviour did not match the docs however the docs don't match expectation.

You'd suddenly be causing people's scenes to be exporting without normals as ours suddenly did when we pulled from top of tree. It would require everyone to go back and modify all their scenes to conform to a doc spec that wasn't implemented properly.

I also don't understand what workflows this would benefit? It's unusual that someone would want to be opting in to normals vs opting out. I can understand if the case was that the tag opted people out of them, but the default should be to export normals without users having to do anything.

So I posit that the fix should be:

  • Default to normals on.
  • If the USD_EmitNormals is set, then change emitNormals to that value.
  • If it's not set, then emitNormals should be true
  • The doc should be fixed to represent the behaviour that existed prior to this PR

This is IMHO the only way to both conform to that document and not break existing files for export.

This was the behaviour prior to this change, which is why I would request that this change be reverted, or you'll be causing lots of breakages in assets going forward.

@dgovil
Copy link
Collaborator

dgovil commented Dec 6, 2022

I made a PR here to resolve this #2768
I'd really appreciate it if the PR was accepted.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Dec 6, 2022

This had been considered a bug internally at Pixar for a long time - we almost never want normals to be authored by Maya since the downstream applications don't update them with for animated/deformed/simmed geom.

Apologies for the breakage on your end when this got pushed back up - I'm fine with going back to the old behavior for the open source code and dealing with it internally

@dgovil
Copy link
Collaborator

dgovil commented Dec 6, 2022

Ah that makes sense why you'd want it off then.
No worries, I just think the most intuitive default is to export normals when no subdiv is present. I think maybe if it needed to be modulated on a global level, an env var or compile time flag would have been better so Pixar could set it at a studio level?

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Dec 6, 2022

Yeah for now we'll just patch in our current behavior and I'll figure out what to do to handle this long term. But you're right - it was presumptive of me to drop internally requested behavior changes on the wider community here, so I think preserving the behavior that was inadvertently established is probably the right way forward.

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