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

No way of cloning a mesh so it works with a skinning material #11574

Open
nlra opened this Issue Jun 21, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@nlra

nlra commented Jun 21, 2017

Skinned material need their own geometry to work. This is because the material deforms the geometry, so if you share the same geometry between two meshes with deformation, things are going to get weird (at least that's how I understand it).

Now the problem is, if you load a mesh from a FBX or something else and it contains at some point a skinned mesh, and you need multiple instances of it, here's what happens:

  • You call clone(); on the mesh to generate your new instances
  • The clone function doesn't generate a new geometry, so both models share geometry, and things gets bad (the mesh doesn't appear at all).

Here's a code sample: I was able to replicate this problem in the webgl_loader_fbx.html demo with a simple modification: Just clone the mesh once loaded before using it, and you'll see that the cloned version of it doesn't work:

loader.load( 'models/fbx/xsi_man_skinning.fbx', function( originalObject ) {

	// Using a clone of the mesh, it won't work
	var object = originalObject.clone();
	object.mixer = new THREE.AnimationMixer( object );
	mixers.push( object.mixer );

	var action = object.mixer.clipAction( object.animations[ 0 ] );
	action.play();

	scene.add( object );


}, onProgress, onError );

Is there a way to fix this behaviour? Like a deep clone function that also recreates geometries?

If not, should I just write a function that recursively recreates all the meshes in my object, regenerating their geometry? Or is it a bad idea?

@nlra

This comment has been minimized.

nlra commented Jun 21, 2017

In the case of this demo, the animation mixer crashes (which I assume is a related issue). But even if you comment the code of the animation mixer, there will be no error but the mesh won't appear on screen.

@Mugen87

This comment has been minimized.

Collaborator

Mugen87 commented Jun 22, 2017

Skinned material need their own geometry to work. This is because the material deforms the geometry, so if you share the same geometry between two meshes with deformation, things are going to get weird (at least that's how I understand it).

The geometry can be identical. You just need an independent set of bones for each cloned mesh because these entities are the target objects of your animation. The actual problem is that cloning a skinned mesh does not always properly clone the respective skeleton/bones.

@Mugen87 Mugen87 added the Bug label Jun 22, 2017

@nlra

This comment has been minimized.

nlra commented Jun 22, 2017

Does it? The initBones function called by the constructor seems to properly recreate the bones from the data. And since the clone function just runs the constructor they should be properly recreated.

if ( this.geometry && this.geometry.bones !== undefined ) {

    // first, create array of 'Bone' objects from geometry data

    for ( i = 0, il = this.geometry.bones.length; i < il; i ++ ) {

        gbone = this.geometry.bones[ i ];

        // create new 'Bone' object

        bone = new Bone();
        bones.push( bone );

        // apply values

        bone.name = gbone.name;
        bone.position.fromArray( gbone.pos );
        bone.quaternion.fromArray( gbone.rotq );
        if ( gbone.scl !== undefined ) bone.scale.fromArray( gbone.scl );

    }

    // second, create bone hierarchy

    for ( i = 0, il = this.geometry.bones.length; i < il; i ++ ) {

        gbone = this.geometry.bones[ i ];

        if ( ( gbone.parent !== - 1 ) && ( gbone.parent !== null ) && ( bones[ gbone.parent ] !== undefined ) ) {

            // subsequent bones in the hierarchy

            bones[ gbone.parent ].add( bones[ i ] );

        } else {

            // topmost bone, immediate child of the skinned mesh

            this.add( bones[ i ] );

        }

    }

}

Or am I missing something?

@Mugen87

This comment has been minimized.

Collaborator

Mugen87 commented Jun 22, 2017

The initBones function called by the constructor seems to properly recreate the bones from the data.

Basically yes, but you can also create a skinned mesh with a geometry, that does not have any bone data. The skeleton is created separately and binded to the SkinnedMesh in the next step. I assume that FBXLoader does it like that, analogous to GLTF2Loader or ColladaLoader2.

@nlra

This comment has been minimized.

nlra commented Jun 22, 2017

So if I tried to change the code so that the clone function actually takes the bones if they are not part of the geometry, it could work?

I'll look into it and make a pull request if I find a good way of doing that. Thanks for the explanation.

@Mugen87 Mugen87 referenced this issue Jun 22, 2017

Closed

GLTFLoader: Some glTF models cannot be cloned #11573

3 of 12 tasks complete
@Mugen87

This comment has been minimized.

Collaborator

Mugen87 commented Jun 22, 2017

I'm not sure about the best solution in this case. You could implement a .copy() method in SkinnedMesh mesh that calls Mesh.prototype.copy.call( this, source ); first and then copies bindMode, bindMatrix and bindMatrixInverse. The call of the super .copy() automatically clones the bones, which are children of the skinned mesh (that happens here). The problem is the subsequent creation of the skeleton with the new (cloned) bones, because you won't be able to restore the original bones order in the skeleton.bones array. This order is necessary to correctly address skin weights and indices. I think without any enhancement to the core, you won't be able to achieve this.

Maybe we can create a new property .index(not sure about the name) in Bone that stores the index of a bone in the bones array. This information would also be serializable and considered by a clone. With this property, it would be possible to restore the correct order and build the skeleton with independent bones (i think 😉).

@donmccurdy

This comment has been minimized.

Collaborator

donmccurdy commented Jun 23, 2017

... automatically clones the bones, which are children of the skinned mesh

Note that the bones may not necessarily be children of the mesh: #10807 (comment)

I don't think shared skeletons currently work in three.js, but it would be nice to (eventually) enable that, so keeping things working even if bones are in a separate armature node would be ideal. Of course, cloning a SkinnedMesh that shares a skeleton could get dicey. :/

@jbaicoianu

This comment has been minimized.

Contributor

jbaicoianu commented Jun 25, 2017

I've been dealing with this while working on #11603 - the problem I've seen is that after cloning, all your bones have new UUIDs, but the skeleton still references the UUIDs from the original model. I've experimented with adding a function to SkinnedMesh which remaps bone UUIDs by name, but it has problems with some models which load with nameless bones.

Will update this issue with any updates if we find a good solution while working on the other issue.

@takahirox

This comment has been minimized.

Collaborator

takahirox commented Jun 28, 2017

I'd like to start to discuss how to implement shared Skeleton.
Is there any good existing threads where we can?

@zellski

This comment has been minimized.

zellski commented Jul 13, 2017

@jbaicoianu We run into precisely this; both the need to rebind upon clone, and the need to make sure that all nodes have names to make the remapping sane. It would be really wonderful not to have to jump through these hoops (where by hoop I mean hacked-up local copy of GLTF2Loader.)

@donmccurdy

This comment has been minimized.

Collaborator

donmccurdy commented Jul 13, 2017

@jbaicoianu @zellski I assume you have not just a SkinnedMesh, but also animations associated with it, and the animations are where we're targeting UUIDs? We should fix this, I agree, but because it's specific to glTF I am going to address the question in #11573 and let this issue focused on the shared skeleton problem shared by all model formats.

@donmccurdy

This comment has been minimized.

Collaborator

donmccurdy commented Jul 18, 2018

Does anyone know the story behind initBones? It relies on geometry.bones, which isn't documented, but seems to contain JSON representing the bone list. So we have:

  • skeleton.bones (real + documented)
  • geometry.bones (real + undocumented)
  • mesh.bones (probably a typo)

I see MMDLoader, XLoader, JSONLoader, ObjectLoader, and SEA3DLoader using geometry.bones, while GLTFLoader, ColladaLoader, and FBXLoader do not. 😐

@donmccurdy

This comment has been minimized.

Collaborator

donmccurdy commented Jul 18, 2018

Proposed fix: #14494.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment