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

BakeSkinning: add support for skinning normals with faceVarying interpolation #1695

Merged

Conversation

cameronwhite
Copy link
Contributor

Description of Change(s)

Previously, only normals with vertex or varying interpolation were deformed. This was something we noticed while experimenting with using UsdSkel to deform fractured meshes from rigid body simulations, where faceted normals are quite common.

To reproduce, load the attached file with usdview facevarying_normals.usda --camera /cameras/camera1, and then run UsdSkel.BakeSkinning(usdviewApi.stage.Traverse()) in the interpreter. The undeformed normals can be seen as the cube spins around.

A few notes:

  • I debated between adding a separate UsdSkelSkinFaceVaryingNormalsLBS() method rather than more overloads of UsdSkelSkinNormalsLBS() - feedback on this is welcome!
  • faceVarying normals also are not deformed in hydra, but the issue is less noticeable depending on the renderer (the normals are suppressed, leaving them to be auto-computed post skinning). I'd like to look at this for a future PR, but wanted to keep this one at a reasonable scope
  • faceVarying normals are also not handled for blendshapes. Doing this would require extensions to the schema, which assumes a 1:1 mapping between normals and points.

@jilliene
Copy link

Filed as internal issue #USD-7043

@ucbearkat
Copy link
Contributor

@cameronwhite Thanks for the change! To answer your first question - using a separate UsdSkelSkinFaceVaryingNormalsLBS() method instead of adding yet another override is probably ok in this case since this is about the caller facing api and internally they all end up calling _SkinNormalsLBS anyway.
Your caveats about the scope of the change are also noted.
I've staged this internally for review and will get back to you with any comments that come up.

Copy link
Contributor

@ucbearkat ucbearkat left a comment

Choose a reason for hiding this comment

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

@cameronwhite Sorry it took a while to get back to you on this.
This change is pretty great overall. Noted a couple comments that came up.
Thanks!

pxr/usd/usdSkel/utils.cpp Outdated Show resolved Hide resolved
pxr/usd/usdSkel/utils.h Show resolved Hide resolved
…lsLBS()

- Add a GfMatrix3f overload for UsdSkelSkinFaceVaryingNormalsLBS(),
  refactoring the implementation into a templated function.

- Use const where possible in _FaceVaryingPointIndexFn::GetPointIndex()
@cameronwhite
Copy link
Contributor Author

Thanks! I've pushed a commit with the noted changes

@pixar-oss pixar-oss merged commit 20412dd into PixarAnimationStudios:dev Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants