-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
GLTFExporter: Support individual morph target animation. #15011
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
Conversation
|
I locally do the similar thing to convert from MMD to glTF. This's very helpful for sure. Can't we move |
|
Yeah we can certainly move |
|
I'm ok with that but it's very up to @mrdoob. IMO at least |
| var keyframeIndex = THREE.AnimationUtils.insertKeyframe( mergedTrack, sourceTrack.times[ j ] ); | ||
| mergedTrack.values[ keyframeIndex * targetCount + targetIndex ] = sourceTrack.values[ j ]; | ||
|
|
||
| } |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed. ✅
315ba26 to
faa89d5
Compare
|
I've moved |
|
@takahirox other than the remaining question of whether |
|
The only remaining question on this PR is whether the new code ( |
|
@donmccurdy I think I would put it in |
- Adds AnimationUtils.insertKeyframe(). - Adds KeyframeTrack.clone(). - Enables GLTFExporter to automatically merge animation tracks affecting individual morph targets.
eda81a8 to
7303890
Compare
b2387a5 to
7303890
Compare
|
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 |
fernandojsg
left a comment
There was a problem hiding this 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 👍
takahirox
left a comment
There was a problem hiding this 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...
|
IMO using AnimationUtils would be an easier decision if it weren't in Thanks @takahirox and @fernandojsg! |
|
Thanks! |
Adds AnimationUtils.insertKeyframe().Adds AnimationUtils.mergeMorphTargetTracks().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()andclip.clone()provide a methods I've needed before — both also appear in #13430./cc @takahirox @fernandojsg