Three.js: GLTFLoader: Skinned meshes distorted

[Mugen87]

Description of the problem

I've realized that certain glTF models like the small robot or the cesium man look distorted in dev. Could this be related to the latest changes to SkinnedMesh?

Check out the hands of the robot:
https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_animation_skinning_morph.html

and the torso of the cesium man:
https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_loader_gltf_extensions.html

The skinning example of FBXLoader and ColladaLoader looks fine. Same for the webgl_animation_skinning_blending which uses GLTFLoader.

Three.js version

• [x] Dev
• [ ] r98
• [ ] ...

Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] All of them
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

Hardware Requirements (graphics card, VR Device, ...)

[donmccurdy]

oh no 😅

[donmccurdy]

PR https://github.com/mrdoob/three.js/pull/15314/ removed the call to normalizeSkinWeights() in the constructor. If I add that back, it fixes the issue for the robot model. Weights in these models are supposed to be normalized already, and at some point the glTF validator will warn about it if not. So, if there's a performance benefit to skipping the normalization by default we can just try to correct the models.

[Mugen87]

PR #15314 removed the call to normalizeSkinWeights() in the constructor.

Um, I think I would add it back for now. We can still remove the call of this method at a later point.

[mrdoob]

What's wrong with these models? They look correct to me 😜

Instead of adding the normalizeSkinWeights() back in SkinnedMesh, I would add the call in GLTFLoader instead.

[mrdoob]

Sorry for the breakage but that part of the library sure needs some love 🙂

[Mugen87]

Instead of adding the normalizeSkinWeights() back in SkinnedMesh, I would add the call in GLTFLoader instead.

:+1:

[zeux]

GLTF spec says:

The joint weights for each vertex must be non-negative, and normalized to have a linear sum of 1.0. No joint may have more than one non-zero weight for a given vertex.

The side-effect of normalizeSkinWeights is that models with UNSIGNED_BYTE normalized=true weights are broken because the normalization divides all weights by ~255.

I'd like to fix this but I'm not 100% sure how. As far as I'm concerned, the call to normalizeSkinWeights is a hack that should be removed, because the models that render incorrectly violate the GLTF spec. But obviously if I just submit a PR that removes this, the "bug" will re-appear.

Re-normalizing 8-bit weights is a bit painful and (IMO) unnecessary, again, due to spec enforcing this. We could skip renormalizing unless the weights are floating-point? I'm happy to submit a PR that fixes the issue, but I'd like to understand what's the best solution first.

[donmccurdy]

Thank you @zeux!

I'd be happy to see that normalizeSkinWeights() call removed – as you say, spec-conformant models do not need it. Unfortunately we'll get bug reports from confused users if we do, and it will be difficult to help them – incorrect weights are arguably more common than UNSIGNED_BYTE weights today.

It will help for glTF validator to begin warning about incorrect skin weights (https://github.com/KhronosGroup/glTF-Validator/issues/58), both so that users can diagnose this and so existing exporters will fix bugs. Once that happens, I'm OK with removing this use of normalizeSkinWeights entirely. Until then, it seems OK to only apply it when weights are floating-point.

/cc @bghgary should glTF-Asset-Generator cover skin weights with different component types?

/cc @lexaknyazev

[zeux]

I have a patch that only normalizes floating-point weights, will submit a PR momentarily. Seems like a reasonable compromise.

[bghgary]

should glTF-Asset-Generator cover skin weights with different component types?

You mean like this? 🤔

https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Positive/Animation_SkinType/README.md

[zeux]

@bghgary This is actually a perfect example :) It shows THREE.js not handling BYTE weights at the moment. #16611 should fix this.

[donmccurdy]

You mean like this? 🤔

Exactly like that, thanks! Guess I need to go through those tests more closely. 😅