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

Add an optional parameter to specify the tolerance for UsdSkelNormalizeWeights() #1667

Merged

Conversation

cameronwhite
Copy link
Contributor

In Houdini, UsdSkelNormalizeWeights() is used when converting geometry to USD, but this uses a different tolerance than Houdini's skinning operator. Being able to specify our own tolerance would let us get consistent behaviour in edge cases where the total weight is nearly zero

@spiffmon
Copy link
Member

Thanks, @cameronwhite ! I have to say I'm slightly concerned about the potential for different contexts to now get different results. I do not know how much intent was behind the choice of std::numerical_limits other than it being an obvious "rigorous" one.

I also don't know who the stakeholders are at this point, but I can identify the Pixar ones... so it seems maybe worth having a discussion on usd-interest to see if people would be OK aligning on Houdini's choice of epsilon, especially if it's one you've found to produce "better" results?

@cameronwhite
Copy link
Contributor Author

I'm not sure that there's a correct value for all cases - in general, I think this is actually something a DCC might want to expose for user control when authoring USD data (in Houdini we don't need that since a similar tool already exists, see below), or configure to match how their own deformers implicitly normalize the weights. I don't think this method is used anywhere internally in USD, and is just a helper for authoring assets.

For a bit more context:
In Houdini, the skinning operator does not require that the input weights are normalized: it will normalize if necessary as part of a pre-processing step. The user is able to explicitly renormalize the weights ahead of time, though, with another operator Capture Correct that has many common tools for cleaning up capture weights.

In the skinning operator we actually had different epsilons (1e-5 and 0.0) being used for some of its variants, e.g. CPU vs GPU. Going forward we're consolidating these (which led to this PR :) ) and chose 0.0, with the logic being that we should use whatever data the input geometry has and avoid a hidden tolerance that could set a point's weights to 0.0 instead of renormalizing.
The user is able to use the Capture Correct operator if they want to control how the weights are normalized - here the epsilon is a user-controlled parameter (defaulting to 1e-5) since the user may have knowledge about how their capture weights should be cleaned up and what a "near-zero" value should be for the total weight

When converting geometry to USD we invoke UsdSkelNormalizeWeights(), and here we'd want to use the same epsilon as the skinning operator's implicit normalizing (now 0.0) so that the results match. Similar to the skinning operator, we'd be unlikely to expose this epsilon as a parameter since the user can just use the Capture Correct operator ahead of time if desired.
We could, of course, reuse some code from our skinning operator here, but using the USD method happens to be much more convenient given how the exporter works.

@spiffmon
Copy link
Member

Ah, apologies I didn't take the time to do the archeology, and just assumed it was used in some coding path! Since it is documented in the "best practices" to be used during authoring, I think your change makes perfect sense. Thanks for explaining, @cameronwhite !

@jtran56
Copy link

jtran56 commented Oct 29, 2021

Filed as internal issue #USD-6989

@pixar-oss pixar-oss merged commit e254d10 into PixarAnimationStudios:dev Mar 8, 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.

4 participants