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: Added serialization/deserialization #15413

Closed
wants to merge 4 commits into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 11, 2018

PR based on #11603 (thanks to @takahirox)

I have tested this PR with multiple examples (e.g. webgl_animation_skinning_blending, webgl_loader_fbx or webgl_loader_collada_skinning) and the serialization/deserialization seems to work. Unfortunately, I have encountered some problems that were really cumbersome to identify and fix^^.

BTW: Before calling SkinnedMesh.toJSON() it might be necessary to call Object3D.updateMatrixWorld() in order to ensure correct transformation matrices. This might be necessary if Object3D.position, Object3D.scale and Object3D.quaternion are not composed into Object3D.matrix yet.

var array = Array.prototype.slice.call( attribute.array );
var array;

if ( attribute.isInterleavedBufferAttribute ) {
Copy link
Collaborator Author

@Mugen87 Mugen87 Dec 11, 2018

Choose a reason for hiding this comment

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

This is necessary since some glTF models might have interleaved buffer data like in webgl_animation_skinning_blending. This code section creates normal BufferAttributes in order to ensure correct buffer data.

// use the supplied bone inverses or calculate the inverses
this.boneMatrices = undefined;
this.boneTexture = undefined;
this.boneTextureSize = undefined;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added all properties of Skeleton to the ctor to improve clarity.


case 'Mesh':

var geometry = getGeometry( data.geometry );
var material = getMaterial( data.material );

if ( geometry.bones && geometry.bones.length > 0 ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is obsolete now.

@takahirox
Copy link
Collaborator

@Mugen87 I wish you could notice me beforehand if you started to work on and use similar code to mine. Actually I was working on #11603 today, and ended up wasting my half day. Yeah, I can understand you didn't bother me, you thought I've been busy because I couldn't respond to anything for a while. But just in case I wanted you to let me know beforehand.

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

var boneUUID = json.bones[ i ];
var bone = object.getObjectByProperty( 'uuid', boneUUID );
Copy link
Collaborator

@takahirox takahirox Dec 11, 2018

Choose a reason for hiding this comment

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

Here can be slow if scene graph is big. Probably making bone lookup table with bone uuid in parseObject is good. I think it may be ok with optimizing when we receive the performance improvement request tho. Adding a comment about performance might be good so far?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good 👍

if ( this.isSkinnedMesh ) {

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.

I don't think .bindMode and .bindMatrix can be undefined here. Yeah more robust tho.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 12, 2018

I wish you could notice me beforehand if you started to work on and use similar code to mine.

Sorry for that. Since you were not that active in the last weeks and noticed multiple time you were busy, I've decided to push this unit of work forward without an announcement.

Actually I was working on #11603 today, and ended up wasting my half day.

I would not say you wasted your time. It's a good thing when more than a single developer deal with important stuff like this one. Similar to VAOs or triangulated lines 👍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 19, 2019

Closing for now. If there is any interest in merging this approach (we might need a different one), I'll reopen and fix the merge conflicts.

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