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

Mesh .updateMorphTargets(): Added BufferGeometry support #11285

Merged
merged 5 commits into from May 17, 2017

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 5, 2017

see #11277


for ( m = 0, ml = morphAttribute.length; m < ml; m ++ ) {

name = morphAttribute[ m ].name || m;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be a good idea to make sure the name is always a string? Or do you think it doesn't matter?

Copy link
Collaborator Author

@Mugen87 Mugen87 May 6, 2017

Choose a reason for hiding this comment

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

Yeah, maybe it's cleaner if we always ensure strings. I'll change this 👍

@looeee
Copy link
Collaborator

looeee commented May 9, 2017

Might need to make some updates to PropertyBindings.js too, as it has a special case for morphTargetInfluences where it specifically looks for a `morphTargets' property on the geometry. Here...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 9, 2017

I need to debug this in order to understand the code. We should currently run into this log here because morphTargets is undefined. I actually didn't see this message in my tests.

@looeee
Copy link
Collaborator

looeee commented May 9, 2017

By the way I had a search through code in /src and /examples/js and couldn't find any other places where this will cause an issue. Although I may have missed something of course!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 9, 2017

@looeee It is important, that the name of the morph attribute matches the name of the KeyframeTrack.

const morphAttribute = new THREE.BufferAttribute( morphPositions, 3 );
morphAttribute.name = 'animation001';
geometry.morphAttributes.position.push( morphAttribute );

Have a look at the following nomenclature propertyName[morphAttributeName/morphTargetName].

const track = new THREE.NumberKeyframeTrack( '.morphTargetInfluences[animation001]', [ 0.0, 5 ], [ 0.0, 1.0 ], THREE.InterpolateSmooth );

morphTargetInfluences can be affected by not just a single track of morph target data. A more complex animation consists of many morph targets. The suffix of a name is normally consecutively numbered to represent the order (animation001, animation002, animation003 ...). This information is used by PropertyBinding. The last PR should added support for BufferGeometry based morphTargets in PropertyBinding. But this change needs a review by @tschw

@looeee
Copy link
Collaborator

looeee commented May 10, 2017

So, the name parameter in KeyframeTrack will be slightly different depending on whether it's targeting a morphTarget or a morphAttribute?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 10, 2017

No, they are identical. Normally, clips would not be created manually but like this:

// setup clips with THREE.AnimationClip
// we assume geometry.morphAttributes was created by a loader

const clips = THREE.AnimationClip.CreateClipsFromMorphTargetSequences( geometry.morphAttributes.position, 10 );
const clip = THREE.AnimationClip.findByName( clips, 'animation' );

const mixer = new THREE.AnimationMixer( mesh );
const animationAction = mixer.clipAction( clip );

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 10, 2017

Check out the implementation of AnimationClip .CreateClipsFromMorphTargetSequences() to see, how the naming convention works. There are some helpful comments in the code.

@mrdoob
Copy link
Owner

mrdoob commented May 17, 2017

Should we also remove the hack in MMDLoader? #11277 (comment)

/cc @takahirox

@takahirox
Copy link
Collaborator

takahirox commented May 17, 2017

I haven't followed the entire discussion yet but I suppose
we can remove if facial morphing animation of webgl_loader_mmd* examples work fine without the hack.

@mrdoob mrdoob merged commit b1f7a8a into mrdoob:dev May 17, 2017
@mrdoob
Copy link
Owner

mrdoob commented May 17, 2017

Thanks!

@Mugen87 Mugen87 deleted the mesh branch May 17, 2017 18:06
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.

None yet

4 participants