Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 3, 2023

Related issue: -

Description

The example webgl_loader_bvh.html use the following pattern to animate an instance of SkeletonHelper:

skeletonHelper.skeleton = result.skeleton; 

The purpose of this line is to mimic an instance of SkinnedMesh so the helper can directly be used as the root object of the scene's animation mixer.

To me this is a hack and error-prone since the helper instance is obviously no skinned mesh which can lead to issues in subsequent code that expects "real" skinned meshes. Hence, I suggest a different approach for the example by creating a "dummy" skinned mesh, properly bind the skeleton, adding the bones and then use it with animation mixer.

I propose this change in regard to #25751 where the retargeting methods also rely on the skeletonHelper.skeleton hack (which is no good when studying the related issue).

@mattrossman
Copy link
Contributor

mattrossman commented Apr 10, 2023

@Mugen87 I notice that skeletal animation in glTF gets loaded differently than in BVH. In glTF, the tracks are bound to the target bone directly, whereas in BVH it targets the .bones[...] property of the target object. Thus you need to create the dummy SkinnedMesh since it expects a .bones property on the thing you're binding to.

Could we instead create BVH tracks similar to how the GLTFLoader does it, targeting the bones directly? The dummy skinned mesh pattern seems like almost as much of a hack as the skeletonHelper.skeleton = result.skeleton assignment. What's nice with the glTF pattern is you don't need to worry about the hierarchy of the thing you pass to AnimationMixer, as long as the name is there it will find the target.

// This would work

mixer = new THREE.AnimationMixer( results.skeleton.bones[0] );

// This too

const boneContainer = new THREE.Group();
boneContainer.add( result.skeleton.bones[ 0 ] );

mixer = new THREE.AnimationMixer( boneContainer );

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 11, 2023

I favor to look at skeletons and skinned meshes as coupled entities. A skeleton might be shared across multiple skinned meshes but having skeleton without a skinned mesh in the scene does not sound like a valid use case to me.

@mattrossman
Copy link
Contributor

having skeleton without a skinned mesh in the scene does not sound like a valid use case to me.

Isn't this how GLTFLoader loads animation files without meshes? I often wind up with bare skeletons in my scene when working with glTF animations.

Take for instance this file: [email protected]

image

If you drag that into https://threejs.org/editor/ it adds the bones to the scene and can play the animation on them, yet there is no SkinnedMesh in the hierarchy. The corresponding Skeleton for those bones can be accessed via gltf.parser.loadSkin(0) if I need to access the bind pose.

I use bare skeletons in the scene for visualization animations (SkeletonHelper doesn't require SkinnedMesh input) and for IK retargeting where you may need to compute world positions of joints in the source animation. These use cases don't require a mesh.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 11, 2023

I see. This is indeed a valid use case.

With regard to the glTF spec it's okay to update BVHLoader like suggested. It should be a backwards-compatible change anyway. Would you like to file a PR with your proposal?

@mattrossman
Copy link
Contributor

Sure, I have opened a PR in #25811

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.

2 participants