mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.64k stars 35.37k forks source link

GLTFLoader: Incorrect bind pose for newly loaded armatures #24772

Closed mattrossman closed 1 year ago

mattrossman commented 2 years ago

Describe the bug

Skeleton.pose() relies on the initial world matrix of each bone to reset bind pose. However, the world matrices of bones loaded in a glTF asset are not updated when the model first loads, causing .pose() to squash the bones into the origin rather than the position they initially appear in.

If I render the scene (or manually call .updateMatrixWorld() on the asset) prior to creating the the skeleton, then it sets the correct bind pose.

This issue of non-updated bone matrices also affects the result of calling .clone() on newly loaded glTF models.

To Reproduce

Steps to reproduce the behavior:

  1. Load a glTF model with an armature
  2. Create a skeleton from its bones
  3. Call .pose() on the skeleton
  4. Render the scene

OR

  1. Load a glTF model with an armature
  2. Clone the glTF scene root and add it to the scene
  3. Render the scene

Code

const loader = new GLTFLoader()
loader.load("/character.glb", (gltf) => {
  scene.add(gltf.scene)

  const bones = []
  gltf.scene.traverse((o) => (o instanceof THREE.Bone) && bones.push(o))

  const skeleton = new THREE.Skeleton(bones)
  skeleton.pose()
})

OR

const loader = new GLTFLoader()
loader.load("/character.glb", (gltf) => {
  const cloned = gltf.scene.clone()
  scene.add(cloned)
})

Live example

Expected behavior

The skeleton should reset to the initial pose as stored in the glTF asset.

While the workaround (manually calling .updateMatrixWorld() prior to these operations) is simple, I don't think users should have to know the internals of how Skeleton.pose() works to get the expected output.

I suppose the fix is to update the world matrices within GLTFLoader. IIUC these matrices are already available in the loader here, they just aren't filled into the bone .matrixWorld properties.

Screenshots

Incorrect bind pose:

image

Correct bind pose:

image

Platform:

mattrossman commented 2 years ago

There is a similar issue with FBXLoader too. I haven't tested other loaders. I get a different result with versus without a .updateMatrixWorld invocation prior to constructing a skeleton. In my case, the skeleton appears much larger without the updated world matrices.

Maybe the PR should include fixes needed for these other loaders too?

Mugen87 commented 2 years ago

Let's focus on GLTFLoader first otherwise the PR might get more complex to review if more loaders are included.

donmccurdy commented 2 years ago

The skeleton should reset to the initial pose as stored in the glTF asset.

If we overwrite the initial matrices of the joints with bind matrices, we are losing some information ... I'm not sure that's what all users will want here, or that we should consider it a bug to omit.

Also -- I think it's possible for two skinned meshes to rely on the same bones, with different inverse bind matrices.

mattrossman commented 2 years ago

Thanks for the feedback. What if instead of copying the bind matrices, we add an .updateMatrixWorld() so the joint world matrices match their transform in the glTF doc? I think that addresses the points you bring up.

// GLTFLoader.js

+ mesh.updateMatrixWorld()

  mesh.bind( new Skeleton( bones, boneInverses ), mesh.matrixWorld );

I'm not sure if there are conventions for when joint world matrices should be initialized. If this doesn't belong in the loader, then alternative the update could be added in Skeleton.pose() and somewhere in the .clone() flow.

donmccurdy commented 2 years ago

I believe that should be OK — even without skinning involved, it does seem better if GLTFLoader's results are initialized with correct world matrices when the loader returns.

Ideally, we'd call updateMatrixWorld() once on the root of each scene in the glTF file, propagating updates to all nodes in the scene. I'm not sure whether there's a good place to put that call in the loading process, or if the skeleton is constructed before the full scene hierarchy is assembled.

/cc @takahirox FYI

Mugen87 commented 2 years ago

Looking at the code, the skeletons are indeed created during the node hierarchy build. So there is not a place where world matrices can be computed once for all skeletons.

BTW: If you need correctly computed world matrix, use mesh.updateWorldMatrix( true, false );.

takahirox commented 1 year ago

If I'm right, the default bindMode "attached" doesn't use bindMatrix (the second argument of .bind()), so bindMatrix isn't used anywhere and inserting mesh.updateMatrixWorld() doesn't have any effect, does it?

https://threejs.org/docs/#api/en/objects/SkinnedMesh.bindMode https://github.com/mrdoob/three.js/blob/0ccddfd057512e611fe5faa0c3eaee99f3036563/src/objects/SkinnedMesh.js#L101-L103

Are you thinking of detached bindMode? But if we use updated skinned mesh world matrix, doesn't it mismatch the glTF core spec?

Only the joint transforms are applied to the skinned mesh; the transform of the skinned mesh node MUST be ignored.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#joint-hierarchy

And found another implementation note in the core spec.

Implementation Note The matrix defining how to pose the skin’s geometry for use with the joints (also known as “Bind Shape Matrix”) should be premultiplied to mesh data or to Inverse Bind Matrices.

takahirox commented 1 year ago

So, if I read the spec correctly

Implementation Note The matrix defining how to pose the skin’s geometry for use with the joints (also known as “Bind Shape Matrix”) should be premultiplied to mesh data or to Inverse Bind Matrices.

Only the joint transforms are applied to the skinned mesh; the transform of the skinned mesh node MUST be ignored.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#joint-hierarchy

I think passing an identity matrix to SkinnedMesh.bind() as the second argument seems to match the spec.

And bind matrix doesn't seem to be used in Skeleton.pose().

https://github.com/mrdoob/three.js/blob/f3149bad3464ad439bef28bda11cb6db55df726f/src/objects/Skeleton.js#L87-L128

I guess the root cause in your case would be that bind shape matrix is not premultiplied to mesh data or to inverse bind matrices in your glTF model?

takahirox commented 1 year ago

I think this issue can be closed because GLTFLoader follows the core specification. Please reopen with details if it doesn't follow the specification.

mattrossman commented 1 year ago

inserting mesh.updateMatrixWorld() doesn't have any effect, does it?

I didn't understand this part -- mesh.updateMatrixWorld() does have an effect from my testing, it fixes the bind pose from being wiped.

I still think there is a bug here, perhaps not with GLTFLoader if it follows the spec but maybe somewhere else. It doesn't make sense to me that this works but this doesn't, the only difference being whether I rendered the scene before posing the skeleton. The bind pose data is there and .pose() works most of the time, just not immediately after loading. Furthermore if call .pose() directly after loading then the data is lost, and subsequent calls to .pose() after rendering don't work.

mattrossman commented 1 year ago

I notice the skeletons produced within GLTFLoader (i.e. those that get bound to the SkinnedMeshes within it) do have a correct bind pose. The difference in my other examples is that I'm calling the Skeleton constructor myself on a list of Bones found in the model.

There's a couple of patterns I now use to access the correct skeleton(s) within a glTF model. On is by searching for a SkinnedMesh:

const skinnedMesh = gltf.scene.getObjectByProperty('isSkinnedMesh', true)
const skeleton = skinnedMesh.skeleton // can safely call .pose() on this

A limitation of that approach is it only works for skeletons that are bound to a mesh, not for plain skeletons. A more robust approach is to use the GLTFParser:

const skeleton = await gltf.parser.loadSkin(0) // gltf.json.skins shows available indices

Here's an updated fiddle that correct displays the bind pose without explicitly updating world matrices:

https://jsfiddle.net/mattrossman/wa5bz7r8/8/

I didn't see a similar pattern for FBXLoader and didn't check in other loaders, but as long as I'm using GLTFLoader I have some resolution.