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

GLTFExporter: Support individual morph target animation. #15011

Merged
merged 10 commits into from
Jan 25, 2019

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Oct 5, 2018

  • Adds AnimationUtils.insertKeyframe().
  • Adds AnimationUtils.mergeMorphTargetTracks().
  • Adds AnimationClip.prototype.clone().
  • Adds KeyframeTrack.prototype.clone().

While three.js allows animation clips to manipulate a single morph target at a time, glTF currently does not. This PR enables GLTFExporter to automatically merge animation tracks affecting individual morph targets, and write filler data if needed. I used a version of this code to complete #14747, converting the Rome models to GLB, which relied heavily on morph target animation.

The addition of track.clone() and clip.clone() provide a methods I've needed before — both also appear in #13430.

/cc @takahirox @fernandojsg

@takahirox
Copy link
Collaborator

I locally do the similar thing to convert from MMD to glTF. This's very helpful for sure.

Can't we move mergeMorphTargetTracks to AnimationUtils?

@donmccurdy
Copy link
Collaborator Author

Yeah we can certainly move mergeMorphTargetTracks to AnimationUtils. That does mean it would be in src/ rather than examples/js, I just wasn't sure if that was OK.

@takahirox
Copy link
Collaborator

I'm ok with that but it's very up to @mrdoob. IMO at least mergeMorphTargetTracks would be better out of the exporter for the reusability of the function and the simplicity of the exporter.

var keyframeIndex = THREE.AnimationUtils.insertKeyframe( mergedTrack, sourceTrack.times[ j ] );
mergedTrack.values[ keyframeIndex * targetCount + targetIndex ] = sourceTrack.values[ j ];

}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm just realized this isn't quite right, even though it was sufficient for the Rome models. If mergedTrack and sourceTrack contain:

mergedTrack:
0 0 0 0 // frame 1 
0 0 0 0 // frame 2
0 0 0 0 // ...
0 0 0 0
0 0 0 0

sourceTrack:
0 // frame 1
1 // frame 4
0 // frame 5

... we need to overwrite frames 2-3 with interpolated values from sourceTrack, not just overwrite frame 0, 4, and 5. Will fix this...

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 is fixed. ✅

@donmccurdy donmccurdy force-pushed the feat-gltfexporter-multimorphanimations branch from 315ba26 to faa89d5 Compare October 9, 2018 05:40
@donmccurdy
Copy link
Collaborator Author

I've moved mergeMorphTargetTracks() into AnimationUtils, and fixed the issue in my earlier comment. If we'd rather keep the extra code in GLTFExporter I'm also fine with that.

@mrdoob mrdoob requested a review from takahirox October 9, 2018 22:40
@mrdoob mrdoob added this to the r98 milestone Oct 9, 2018
@Mugen87 Mugen87 mentioned this pull request Oct 21, 2018
14 tasks
@mrdoob mrdoob modified the milestones: r98, r99 Oct 31, 2018
@donmccurdy
Copy link
Collaborator Author

@takahirox other than the remaining question of whether insertKeyframe and mergeMorphTargetTracks should be in GLTFExporter or AnimationUtils, any other issues with this PR?

@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018
@donmccurdy
Copy link
Collaborator Author

The only remaining question on this PR is whether the new code (insertKeyframe, mergeMorphTargetTracks) should be inside of GLTFExporter, or the AnimationUtils class — @mrdoob any preference on this?

@mrdoob
Copy link
Owner

mrdoob commented Dec 11, 2018

@donmccurdy I think I would put it in GLTFExporter for now.

@donmccurdy donmccurdy force-pushed the feat-gltfexporter-multimorphanimations branch from eda81a8 to 7303890 Compare December 14, 2018 02:22
@mrdoob mrdoob modified the milestones: r100, r101 Dec 31, 2018
@donmccurdy donmccurdy force-pushed the feat-gltfexporter-multimorphanimations branch from b2387a5 to 7303890 Compare January 4, 2019 07:33
@donmccurdy
Copy link
Collaborator Author

Ok, I've made the changes. @takahirox this could probably use another round of review when you have the chance. I put the new functions onto GLTFExporter.Utils.* so the unit tests can access them; the tests were pretty helpful for this code.

Copy link
Collaborator

@fernandojsg fernandojsg left a comment

Choose a reason for hiding this comment

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

Just added a comment out of curiosity, the rest of the code lgtm, and I prefer too to have it directly on the GLTFExporter.Utils until we will have modules available on /examples.
Nice job on the tests too, really useful for this kind of issues 👍

examples/js/exporters/GLTFExporter.js Show resolved Hide resolved
Copy link
Collaborator

@takahirox takahirox left a comment

Choose a reason for hiding this comment

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

Hm, I'm still in favor of putting Animation clip operation in AnimationUtil but yeah we could move another chance if necessary.

BTW I couldn't check functionality because of lack of time...

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 22, 2019

IMO using AnimationUtils would be an easier decision if it weren't in src/*. Maybe moving it out or splitting AnimationUtils would make sense eventually. In the meantime lets go with GLTFExporter.Utils here.

Thanks @takahirox and @fernandojsg!

@mrdoob mrdoob merged commit 5e55ef6 into mrdoob:dev Jan 25, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2019

Thanks!

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