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

SkinnedMesh/Skeleton serialization #11603

Closed
wants to merge 11 commits into from

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Jun 24, 2017

This PR enables SkinnedMesh/Skeleton serialization.
The code became much simpler than the one I proposed before #8978

Serialized Bones are stored under scene graph("object"), but serialized Skeletons are stored out of scene graph with Bones uuids and boneInverse matrices. Serialized SkinnedMesh has skeleton's uuid, like geometry and material. This's for shared skeleton which we'll support sooner or later.

Bones are under scene graph, so any objects under Bone are also serialized correctly without any special logics.

ObjectLoader temporally undefines geometry.bones at the SkinnedMesh instantiation to prevent Bones creation in SkinnedMesh constructor if serialized SkinnedMesh has Skeleton uuid. Unless we do that, we'll have two duplicated Bone sets (one is from serialization, another is from creation in constructor).

This's an example of the result of SkinnedMesh .toJSON(). (Omitting some parts)

{
    "skeletons": [
        {
            "uuid": "skeleton-uuid",
            "bones": ["bone0-uuid", "bone1-uuid", "bone2-uuid", ...],
            "boneInverses": [...]
        }
    ],
    "object": {
        "uuid": "object-uuid",
        "type": "SkinnedMesh",
        "matrix": [1,0,0,0,0,1,0,0,0,0,1,0,0,0,0,1],
        "geometry": "geometry-uuid",
        "material": "material-uuid",
        "skeleton": "skeleton-uuid",
        "children": [
            "uuid": "bone0-uuid",
            "type": "Bone",
            "matrix": [1,0,0,0,0,1,0,0,0,0,1,0,0,0,0,1],
            "children": [
                ...
            ]
        ]
    }
}

You can confirm it works by running the following code on the console in webgl_animation_skinning_morph example.

Note: targeting BufferGeometry, not Geometry so far. See #8978

mesh.visible = false;
mesh.geometry._bufferGeometry.bones = mesh.geometry.bones;
mesh.geometry._bufferGeometry.animations = mesh.geometry.animations;

// create SkinnedMesh object with BufferGeometry
var m = new THREE.SkinnedMesh( mesh.geometry._bufferGeometry, [ mesh.material[ 0 ].clone() ] );
m.position.copy( mesh.position );
m.quaternion.copy( mesh.quaternion );
m.scale.copy( mesh.scale );
m.position.x += -500;
m.updateMatrix();
scene.add( m );

var mixer2 = new THREE.AnimationMixer( m );
mixer2.clipAction( m.geometry.animations[ 0 ] ).play();
mixer2.update( 1.0 );
m.updateMatrixWorld( true );

// serialization/deserialization
var loader = new THREE.ObjectLoader();
loader.parse( JSON.parse( JSON.stringify( m.toJSON() ) ), function( object ) {

    object.position.x += 1000;
    scene.add( object );

} );

Left is original, right is serialized and then deserialized.

image

I've worked on this PR with @jbaicoianu , thanks!

@jbaicoianu
Copy link
Contributor

Can confirm this works, the skeleton is properly serialized and loaded.

More work is necessary to serialize animation data, we'll submit a separate pull request for that.

There's also a bit more work needed to support cloning a mesh with skeletons (the UUIDs need to be remapped after cloning) - we'll do a separate pull request for that as well.


if ( this.bindMode !== undefined ) object.bindMode = this.bindMode;
if ( this.bindMatrix !== undefined ) object.bindMatrix = this.bindMatrix.toArray();

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this.bindMatrixInverse? For the sake of completeness we should also regard this property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bindMatrixInverse is calculated from bindMatrix in SkinnedMesh.bind() so I didn't think we need to serialize bindMatrixInverse.
Do you think we should do that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just prefer to serialize all public attributes. But your point of view is also valid. The current code is definitely correct. Because i don't have a strong opinion on this, i leave it up to you 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, so let's keep this code so far.

Copy link
Collaborator Author

@takahirox takahirox Jul 1, 2017

Choose a reason for hiding this comment

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

Would you please close change request?

Copy link
Collaborator

@Mugen87 Mugen87 Aug 13, 2017

Choose a reason for hiding this comment

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

@takahirox I'm sorry! I've missed your comment. Approving...

Repository owner deleted a comment from jvhuaxia Jun 24, 2017
@@ -662,6 +668,20 @@ Object.assign( Object3D.prototype, EventDispatcher.prototype, {

}

// SkinnedMesh specific

if ( this.skeleton !== undefined ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi

Why not move this to a toJSON inside SkinnedMesh i think it would make more sense to move mesh serialization specifics to the Mesh and SkinnedMesh objects!

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I can wait until this gets merged and i can try to propose moving and cleanup these after maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

By moving that code to a toJSON method inside SkinnedMesh :)

Copy link
Collaborator Author

@takahirox takahirox Jun 29, 2017

Choose a reason for hiding this comment

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

Can you try to make sample code and share with me?
I tried the same idea before but ended up realizing it'd be simpler if they'd be in Object3D.toJSON().

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this

SkinnedMesh.prototype.toJSON = function(meta)
{
	var data = THREE.Object3D.prototype.toJSON.call(this, meta);

	if(this.bindMode !== undefined)
	{
		data.object.bindMode = this.bindMode;
	}
	if(this.bindMatrix !== undefined)
	{
		data.object.bindMatrix = this.bindMatrix.toArray();
	}

	if(this.skeleton !== undefined)
	{
		if(meta.skeletons[this.skeleton.uuid] === undefined)
		{
			meta.skeletons[this.skeleton.uuid] = this.skeleton.toJSON(meta);
		}

		data.object.skeleton = this.skeleton.uuid;
	}

};

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to do this once - it's trickier than it seems, because of how toJSON is written, it makes it hard to do this without copy-pasting the whole toJSON function.

Ideally, since SkinnedMesh extends Object3D, you should just be able to call Object3D.prototype.toJSON.call(this, meta) - but in practice it's trickier, because Object3D.toJSON defines a bunch of internal functions - not sure if we need those internal functions for the skeleton, but I have run into cases in the past where this made it difficult without duplicating a lot of code, or restructuring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well im doing a ton in nunuStudio i have pretty much overrided more than half of the threejs serialization process to include resources, external lib data etc.

There is some tricks to it sometimes (specially when working with object relations) but for these cases its pretty straight forward just call the thing on Object3D and add whatever you need to add, its always better and cleaner than filling Object3D serialization with a ton of verifications to decide whether or not to include a resource from a specific class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, your code looks good, and this is definitely the way to go rather than piling it all into Object3D. Maybe at some point we can move the serialize and extractFromCache local convenience functions out to be either methods of the Object3D class or part of a utility class, to make it easier to do this for all cases.

Copy link
Collaborator Author

@takahirox takahirox Jun 29, 2017

Choose a reason for hiding this comment

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

I don't think your code is good enough.
I think meta initialization and isRootObject related codes are also necessary in SkinnedMesh.toJSON() in addition to extractFromCache.
Imagine what'll happen with

var skin = new THREE.SkinnesMesh( geometry, material );
skin.toJSON();

I don't really like making duplicated codes in SkinnedMesh.
So IMO, as @jbaicoianu mentioned, we need restructuring if we move this into SkinnedMesh.
Let's make another PR if we do.


if ( skeleton === undefined ) {

console.warn( 'THREE.ObjectLoader: Not found Skeleton whose uuid is ' + obj.sjeletonUUID );
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in console.warn( 'THREE.ObjectLoa.....+ obj.skeletonUUID );

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if im being a bit annoying :), i really want this PR to be merged i was comparing it to what i was doing on nunu xD
Awesome work thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@takahirox
Copy link
Collaborator Author

takahirox commented Aug 13, 2017

@mrdoob Any concerns about this PR?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 13, 2017

I think we should try to move forward with SkinnedMesh/Skeleton serialization so we can enable Skinned Meshes in the editor (#8422).

@takahirox
Copy link
Collaborator Author

Yup, I wanna enable SkinnedMesh edit on the editor!

@takahirox
Copy link
Collaborator Author

@mrdoob Please let me know if there's any concerns in this PR.

@tentone
Copy link
Contributor

tentone commented Aug 24, 2017

@takahirox

I have tested and merged this into nunuStudio everything seems to work just fine (i have to start adding mentions on my commits over there), skeleton serialization is working as expected. Im able to edit bones and load everything without any problem.

Thanks a lot! I think this should be merged 👍

@takahirox
Copy link
Collaborator Author

Solved the conflicts.

@jbaicoianu
Copy link
Contributor

Any chance of getting this merged before the codebase diverges again?

@takahirox
Copy link
Collaborator Author

/ping @mrdoob

1 similar comment
@takahirox
Copy link
Collaborator Author

/ping @mrdoob

@takahirox
Copy link
Collaborator Author

Fixed conflicts.

@takahirox
Copy link
Collaborator Author

Solved the conflicts.

@takahirox
Copy link
Collaborator Author

Solved the conflict.

@takahirox
Copy link
Collaborator Author

Can we add this PR to the r91 milestone together with #11596? 😅

@mrdoob mrdoob added this to the r92 milestone Feb 21, 2018
@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018
@@ -737,6 +810,10 @@ Object.assign( ObjectLoader.prototype, {

}

Choose a reason for hiding this comment

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

Is there a break; missing here for the Mesh case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, thanks. I'll fix later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@mrdoob mrdoob modified the milestones: r93, r94 May 22, 2018
@mrdoob mrdoob modified the milestones: r94, r95 Jun 27, 2018
@mrdoob mrdoob modified the milestones: r95, r96 Jul 31, 2018
@mrdoob mrdoob modified the milestones: r96, r97 Aug 29, 2018
@etr2460
Copy link
Contributor

etr2460 commented Sep 24, 2018

@mrdoob What do you think the odds are that this will get merged in for r97? Is there anything else that needs to be done here?

@mrdoob mrdoob modified the milestones: r97, r98 Sep 25, 2018
@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2018

@etr2460 Sorry for the delay. I just had to find time to look at the current design of that code.

After #15314 I have a better understanding of that code and I'll be more confident when reviewing related code.

Getting there...!

@donmccurdy
Copy link
Collaborator

There are a number of sample glTF models for various tricky skinning cases here:

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

It may be worth testing this PR against them and seeing which work, and which don't. We don't necessarily have to support them all — the glTF format may add restrictions to disallow some of them, even — but it seems like a good way to test this change.

@mrdoob mrdoob modified the milestones: r99, r100 Nov 27, 2018
@takahirox takahirox closed this Dec 11, 2018
@takahirox takahirox deleted the SkinnedMeshSerialization branch December 11, 2018 14:56
@mrdoob mrdoob removed this from the r100 milestone Dec 12, 2018
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.

8 participants