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

normalize joint weights #10539

Merged
merged 3 commits into from
Jan 27, 2024
Merged

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Nov 13, 2023

Objective

allow automatic fixing of bad joint weights.
fix #10447

Solution

  • remove automatic normalization of vertexes with all zero joint weights.
  • add Mesh::normalize_joint_weights which fixes zero joint weights, and also ensures that all weights sum to 1. this is a manual call as it may be slow to apply to large skinned meshes, and is unnecessary if you have control over the source assets.

note: this became a more significant problem with 0.12, as weights that are close to, but not exactly 1 now seem to use Vec3::ZERO for the unspecified weight, where previously they used the entity translation.

@robtfm robtfm added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Nov 13, 2023
@alice-i-cecile alice-i-cecile added the A-Animation Make things move and change over time label Nov 13, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Non-blocking suggestion: could we add a check that the joint weights are normalized when running in debug mode?

@robtfm
Copy link
Contributor Author

robtfm commented Nov 13, 2023

Non-blocking suggestion: could we add a check that the joint weights are normalized when running in debug mode?

i can do this, but the warning becomes unavoidable ... if you load a gltf and once loaded you call normalize_joint_weights, then still the mesh is uploaded once to the gpu before the normalization, so the warning is still triggered.

up to you?

@alice-i-cecile
Copy link
Member

Hmm 🤔 I think we can leave it. The real requirement here is for a general-purpose mesh-validation asset preprocessor IMO.

@JMS55 JMS55 added this to the 0.13 milestone Nov 23, 2023
};

for weights in joints.iter_mut() {
let sum: f32 = weights.iter().sum();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth verifying the weights are positive first? If someone accidentally uses with_inserted_attribute with a mixture of positive and negative floats, this will do weird things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

negative weights are technically not allowed in the spec, whereas not summing to precisely 1 is allowed due to rounding issues.

but sure, no reason not to. i arbitrarily chose to clamp negatives to zero, i guess other treatments might be better (like if all are negative, maybe you should just flip the sign?) but since we're in invalid-data territory there's probably no perfect answer. if someone wants a different treatment they can always write their own normalize function.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Nice work!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 27, 2024
Merged via the queue into bevyengine:main with commit b35b9e5 Jan 27, 2024
22 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

allow automatic fixing of bad joint weights.
fix bevyengine#10447 

## Solution

- remove automatic normalization of vertexes with all zero joint
weights.
- add `Mesh::normalize_joint_weights` which fixes zero joint weights,
and also ensures that all weights sum to 1. this is a manual call as it
may be slow to apply to large skinned meshes, and is unnecessary if you
have control over the source assets.

note: this became a more significant problem with 0.12, as weights that
are close to, but not exactly 1 now seem to use `Vec3::ZERO` for the
unspecified weight, where previously they used the entity translation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

translated skinned mesh weirdness
4 participants