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
24 changes: 23 additions & 1 deletion src/core/Object3D.js
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,8 @@ Object.assign( Object3D.prototype, EventDispatcher.prototype, {
geometries: {},
materials: {},
textures: {},
images: {}
images: {},
skeletons: {}
};

output.metadata = {
Expand All @@ -620,6 +621,11 @@ Object.assign( Object3D.prototype, EventDispatcher.prototype, {

object.matrix = this.matrix.toArray();

// SkinnedMesh specific

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...

//

function serialize( library, element ) {
Expand Down Expand Up @@ -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 ( meta.skeletons[ this.skeleton.uuid ] === undefined ) {

meta.skeletons[ this.skeleton.uuid ] = this.skeleton.toJSON( meta );

}

object.skeleton = this.skeleton.uuid;

}

//

if ( this.children.length > 0 ) {
Expand All @@ -682,11 +702,13 @@ Object.assign( Object3D.prototype, EventDispatcher.prototype, {
var materials = extractFromCache( meta.materials );
var textures = extractFromCache( meta.textures );
var images = extractFromCache( meta.images );
var skeletons = extractFromCache( meta.skeletons );

if ( geometries.length > 0 ) output.geometries = geometries;
if ( materials.length > 0 ) output.materials = materials;
if ( textures.length > 0 ) output.textures = textures;
if ( images.length > 0 ) output.images = images;
if ( skeletons.length > 0 ) output.skeletons = skeletons;

}

Expand Down
111 changes: 109 additions & 2 deletions src/loaders/ObjectLoader.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import { LineSegments } from '../objects/LineSegments';
import { LOD } from '../objects/LOD';
import { Mesh } from '../objects/Mesh';
import { SkinnedMesh } from '../objects/SkinnedMesh';
import { Skeleton } from '../objects/Skeleton';
import { Bone } from '../objects/Bone';
import { Fog } from '../scenes/Fog';
import { FogExp2 } from '../scenes/FogExp2';
import { HemisphereLight } from '../lights/HemisphereLight';
Expand Down Expand Up @@ -136,6 +138,10 @@ Object.assign( ObjectLoader.prototype, {

var object = this.parseObject( json.object, geometries, materials );

var skeletons = this.parseSkeletons( json.skeletons, object );

this.bindSkeletons( object, skeletons );

if ( json.animations ) {

object.animations = this.parseAnimations( json.animations );
Expand Down Expand Up @@ -519,6 +525,47 @@ Object.assign( ObjectLoader.prototype, {

},

parseSkeletons: function ( json, object ) {

var skeletons = {};

if ( json === undefined ) return skeletons;

for ( var i = 0; i < json.length; i ++ ) {

var skeletonParams = json[ i ];

var uuid = skeletonParams.uuid;
var boneParams = skeletonParams.bones;
var boneInverseParams = skeletonParams.boneInverses;

var bones = [];
var boneInverses = [];

for ( var j = 0, jl = boneParams.length; j < jl; j ++ ) {

var bone = object.getObjectByProperty( 'uuid', boneParams[ j ] );

if ( bone === undefined ) {

console.warn( 'THREE.ObjectLoader: Not found Bone whose uuid is ' + boneParams[ j ] );
bone = new Bone();

}

bones.push( bone );
boneInverses.push( new Matrix4().fromArray( boneInverseParams[ j ] ) );

}

skeletons[ uuid ] = new Skeleton( bones, boneInverses );

}

return skeletons;

},

parseObject: function () {

var matrix = new Matrix4();
Expand Down Expand Up @@ -663,7 +710,33 @@ Object.assign( ObjectLoader.prototype, {

case 'SkinnedMesh':

console.warn( 'THREE.ObjectLoader.parseObject() does not support SkinnedMesh yet.' );
var geometry = getGeometry( data.geometry );
var material = getMaterial( data.material );

var tmpBones;

// If data has skeleton, assumes bones are already in scene graph.
// Then temporarily undefines geometry.bones not to create bones
// in SkinnedMesh constructor.

if ( data.skeleton !== undefined && geometry.bones !== undefined ) {

tmpBones = geometry.bones;
geometry.bones = undefined;

}

object = new SkinnedMesh( geometry, material );

// rebinds with skeleton whose uuid is data.skeleton later.
if ( data.skeleton !== undefined ) object.skeletonUUID = data.skeleton;
if ( data.bindMode !== undefined ) object.bindMode = data.bindMode;
if ( data.bindMatrix !== undefined ) object.bindMatrix.fromArray( data.bindMatrix );
object.updateMatrixWorld( true );

if ( tmpBones !== undefined ) geometry.bones = tmpBones;

break;

case 'Mesh':

Expand All @@ -682,6 +755,12 @@ Object.assign( ObjectLoader.prototype, {

break;

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!

case 'Bone':

object = new Bone();

break;

case 'LOD':

object = new LOD();
Expand Down Expand Up @@ -796,7 +875,35 @@ Object.assign( ObjectLoader.prototype, {

};

}()
}(),

bindSkeletons: function ( object, skeletons ) {

if ( Object.keys( skeletons ).length === 0 ) return;

object.traverse( function ( obj ) {

if ( obj.isSkinnedMesh === true && obj.skeletonUUID !== undefined ) {

var skeleton = skeletons[ obj.skeletonUUID ];

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!


} else {

obj.bind( skeleton, obj.bindMatrix );

}

delete obj.skeletonUUID;

}

} );

}

} );

Expand Down
30 changes: 30 additions & 0 deletions src/objects/Skeleton.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Matrix4 } from '../math/Matrix4';
import { _Math } from '../math/Math';

/**
* @author mikael emtinger / http://gomo.se/
Expand All @@ -9,6 +10,8 @@ import { Matrix4 } from '../math/Matrix4';

function Skeleton( bones, boneInverses ) {

this.uuid = _Math.generateUUID();

// copy the bone array

bones = bones || [];
Expand Down Expand Up @@ -152,6 +155,33 @@ Object.assign( Skeleton.prototype, {

return new Skeleton( this.bones, this.boneInverses );

},

toJSON: function ( meta ) {

var data = {};

var bones = [];
var boneInverses = [];

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

bones.push( this.bones[ i ].uuid );

}

for ( var i = 0, il = this.boneInverses.length; i < il; i ++ ) {

boneInverses.push( this.boneInverses[ i ].toArray() );

}

data.uuid = this.uuid;
data.bones = bones;
data.boneInverses = boneInverses;

return data;

}

} );
Expand Down