-
Notifications
You must be signed in to change notification settings - Fork 79
MeshImporter/XFormImporter refactor (USDU-3 part 3) #261
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
Conversation
clusty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only comment was around need for both: MeshSample and SanitizedMeshSample & friends classes.
Sanitizing the USD Meshes and XForms to Unity standards is now done by the sample itself right after deserialization. This has the
benefit to run in jobs and to simplify the MeshImporter code greatly.
Sanitization code takes care of:
- xform conversion
- triangulation
- de-indexation of attributes
- attribute scope conversion
The SkeletonImporter has been changed and the JointWeights and JointIndices are now deserialized/loaded by the MeshImporter to gain a whole extra mesh deserialization/sanitization.
MeshImporter was always changing handedness regardless of the importOptions. Cameras were not sanitized so I added a new SanitizedCameraSample
eda421b to
08f7228
Compare
jcowles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this change is incredible :)
It's clear that a massive amount of effort went into this, really nice work!
I have a few minor comments, but overall it totally makes sense and is a great simplification.
package/com.unity.formats.usd/Dependencies/USD.NET.Unity/Geometry/MeshSampleBase.cs
Show resolved
Hide resolved
package/com.unity.formats.usd/Dependencies/USD.NET/serialization/Primvar.cs
Outdated
Show resolved
Hide resolved
package/com.unity.formats.usd/Runtime/Scripts/IO/Geometry/CubeImporter.cs
Outdated
Show resolved
Hide resolved
package/com.unity.formats.usd/Runtime/Scripts/IO/Geometry/MeshImporter.cs
Outdated
Show resolved
Hide resolved
cguer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA approved 👍!
Tested on:
- 2021.2.0b10 (Windows + HDRP)
Sorry for the delay 🙈 ...
Purpose of this PR
Ticket/Jira #:
https://jira.unity3d.com/browse/USDU-3
https://jira.unity3d.com/browse/USDU-10
USD geometric Primvars can be given different kind of interpolation across the uv space of a surface primitive:
Each primvar can also be individually indexed for data-compression purpose.
This is an issue for Unity which requires all attributes of a mesh to be either interpolated per vertex or per-vertex per-face.
As a consequence it forces us to sanitize the data on the way in before being pushed to GameObjects.
The sanitization was previously done in the MeshImporter right before setting the Unity mesh buffers which was single threaded and made it impossible to properly cross check all attributes arrays for their interpolation value.
This PR introduces a way to perform sanitization of the Mesh and XForm data right after deserialization while using the job system to lessen the impact of interpolation conversion. For that a new ISanitizable interface and 2 new classes SanitizedMeshSample and XFormMeshSample have been added and the jobified ReadAllJob struct have been modified to provide sanitization service after deserialization.
The sanitization code takes care of:
- xform conversion
- triangulation
- de-indexation of attributes
- attribute scope conversion
As a consequence the MeshImporter has been greatly simplified.
The SkeletonImporter has been changed and the JointWeights and JointIndices are now deserialized/loaded by the MeshImporter to avoid aan extra mesh deserialization/sanitization.
Testing
Functional Testing status:
The sanitization service has almost 100% coverage but not the MeshImporter itself.
@cguer You should check the import with all the regular suspect (kitchen, human female, walking teapot ) and do a before after.
Performance Testing status:
I quickly profile the changes and they are a little bit slower both for static and animated data. Although GC has been greatly reduced. I'll update with the actual numbers after my PTO.
Overall Product Risks
Complexity:
High
Halo Effect:
High
Additional information
Note to reviewers:
Check the SanitizedMeshSample for the actual sanitization code and the MeshImporter to see how the sanitized data is pushed to Unity Meshes.
There are some minor changes to the deserialization process to flag colors as actual primvars which give us access to the indices and interpolation values.
Check the test usda file to get a feel for the kind of crappy edge case that can come up and if I forgot anything.
Reminder: