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

Animation: Interpolants & extensibility overhaul. #7312

Merged
merged 19 commits into from
Oct 25, 2015
Merged

Conversation

tschw
Copy link
Contributor

@tschw tschw commented Oct 8, 2015

What's new?

  • Smooth interpolation! More importantly, an extensible infrastructure for interpolation modes.
  • Well-defined interfaces within the system.
    This way, the internal components can easier be used in a stand-alone fashion and the system becomes easier to maintain and extend. The high level Mixer & Actions API remains 100% compatible.
  • Many bugs and inefficiencies removed (see list below). It's now several factors faster.
  • Caching for very fast .addAction / .removeAction.
  • Better readable source code in many places.

Design Issues Solved

AnimationClip is documented to be reusable. The track-cached index however did result in tons of (linear!) scans when running multiple instances of the same clip (not action) at the same time!

The data model was deeply nested (both for "standard Three" JSON and at run time). Indirections take their toll. Furthermore, the indirections were completely useless.

PropertyBindings had no clearly defined role and .getValue returned something reference or value, only usable with an extra call to a .clone utility function.

Bugs

PropertyBinding.apply: The original value was "copied" without AnimationUtils.clone (incorrect for non-reference properties), also contained a lengthy computation of a one-valued constant.

KeyframeTrack.trim: The loop index in KeyframeTrack.trim was counting into the wrong direction.

KeyframeTrack.getAt: Thenext_is_constant "optimization" did not update after an interval change and also in fact caused a slowdown (the polymorphism caused more overhead than the work saved, also it only works correctly for linear interpolation and requires a very irregular and inefficient data model).

@tschw
Copy link
Contributor Author

tschw commented Oct 8, 2015

window-2015-10-08-18 38 41
window-2015-10-08-18 38 49


gui.add( API, guiNameAddRemove ).onChange( function() {

if ( API[ guiNameAddRemove ] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use ternary operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably having a temporary stupidity attack, but I can't figure it out. Please help!

Copy link
Contributor

Choose a reason for hiding this comment

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

Its probably I who is the stupid one but:

API[ guiNameAddRemove ] ? mixer.addAction( action ) : mixer.removeAction( action )

Another approach would be to have a mixer.setAction and resolve the action (add/remove) inside it to make the consuming code cleaner.. Probably overkill though

@mrdoob
Copy link
Owner

mrdoob commented Oct 8, 2015

The high level Mixer & Actions API remains 100% compatible.

I wouldn't worry about keeping it compatible to Mixer & Action if you're compromising. As this is still new code that no one relies on yet.

@mrdoob
Copy link
Owner

mrdoob commented Oct 8, 2015

/ping @bhouston

@mrdoob
Copy link
Owner

mrdoob commented Oct 8, 2015

Btw, thanks a ton @tschw! 😊

@WestLangley
Copy link
Collaborator

@tschw Wow! And thank you for your well-written inline comments. So helpful.

@gero3 http://threejsworker.com/pullrequests.html Incredibly useful. Thanks.


}
}

if ( tracks.length === 0 ) {
if( tracks.length === 0 ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually code I didn't touch. The diff is against a tidier version of the code that I had to revert for pulling up...

Copy link
Owner

Choose a reason for hiding this comment

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

No worries! I'll tidy things up later 😊

@mrdoob
Copy link
Owner

mrdoob commented Oct 8, 2015

@GGAlanSmithee lets leave the formatting bits out of the review by now.

@@ -52,19 +80,20 @@ THREE.AnimationAction.prototype = {

this.actionTime = this.actionTime + clipDeltaTime;

if ( this.loop === THREE.LoopOnce ) {
if( this.loop === THREE.LoopOnce ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All if statements below needs formatting

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I'll do it later.

@GGAlanSmithee
Copy link
Contributor

Sorry! No more comments about formatting.

@tschw
Copy link
Contributor Author

tschw commented Oct 8, 2015

@mrdoob

@GGAlanSmithee lets leave the formatting bits out of the review by now.

Awwww.... Too late. I used a separate commit for the code I didn't touch. Shall I reset --hard HEAD^..force-push it?

@tschw
Copy link
Contributor Author

tschw commented Oct 8, 2015

Shall I reset --hard HEAD^..force-push it?

Took that commit out for now. We can re-add it later. It's enough of a rewrite already...

@tschw
Copy link
Contributor Author

tschw commented Oct 8, 2015

Also squashed polyfill formatting into the corresponding commits. Once you start gitting...

@tschw
Copy link
Contributor Author

tschw commented Oct 8, 2015

Still bugs me that I've lost some performance over an earlier iteration. It used to be a factor of 5 on Firefox and 3 on Chrome :-(.

Seems the measurements spike out quite a bit, so I probably just had a lucky streak there. Good enough. Enough! Once you start profiling... :-)


// remove from array-based unordered set
actions[ index ] = actions[ actions.length - 1 ];
actions.pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't these two lines become one? :
actions[ index ] = actions.pop();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another mean question for a job interview ;-).
I guess not, because it will re-add the element if it's the last in the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Guess I wouldn't have gotten the job then ;) Lucky me, I already have one :)

Copy link
Owner

Choose a reason for hiding this comment

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

Another mean question for a job interview ;-)

Haha, that made me laugh 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was inspired by the article @GGAlanSmithee cited above, "suggesting" stuff like [] + {} for that :-).

@mrdoob
Copy link
Owner

mrdoob commented Oct 24, 2015

I'm blocking Monday for reviewing this. I think I'll have API questions/suggestions.

@tschw
Copy link
Contributor Author

tschw commented Oct 24, 2015

OK, I'm fastening my seatbelt already. Can't we just merge it and refine / simplify the API from there? I have to deploy dependent code and I don't mind subsequent changes, but I do mind the delay - it's already been more of an odyssey than I had planned for...

@mrdoob
Copy link
Owner

mrdoob commented Oct 25, 2015

Haha. Fair point!

mrdoob added a commit that referenced this pull request Oct 25, 2015
Animation: Interpolants & extensibility overhaul.
@mrdoob mrdoob merged commit 2142b89 into mrdoob:dev Oct 25, 2015
@mrdoob
Copy link
Owner

mrdoob commented Oct 25, 2015

Thaaanks!

@mrdoob
Copy link
Owner

mrdoob commented Oct 25, 2015

Can we make webgl_animation_skinning_morph animate once it loads as it used to?

@tschw
Copy link
Contributor Author

tschw commented Oct 25, 2015

@mrdoob

Can we make webgl_animation_skinning_morph animate once it loads as it used to?

See #7443

@AndrewRayCode
Copy link
Contributor

I'm late to the party, but this new API is overly constricting to one specific use case where the user doesn't manage elapsed time. The only methods exposed to update animations take a delta, not an absolute time or an absolute percent. That means if I want to manually step through an animation I can't do it easily with this API. I can only tell this how much time has elapsed. If I want to jump to an animation that's 25% in, for example, this API doesn't allow that.

@tschw
Copy link
Contributor Author

tschw commented Apr 12, 2016

Ooops. Damn! How could I overlook these many pings.

@tschw
Copy link
Contributor Author

tschw commented Apr 12, 2016

@delvarworld
The API was introduced in r73, this PR just made some changes to it.

And AnimationMixer exposes absolute .time, so I don't really get what you are talking about.

@bhouston
Copy link
Contributor

@delvarworld can you suggest the API changes you need more specifically? Maybe as an issue. I would suggest that you even write out the functions or parameters you would like added, especially if you could reference Unity3D or UE4's API docs for comparison.

@tschw
Copy link
Contributor Author

tschw commented Apr 12, 2016

@delvarworld

That means if I want to manually step through an animation I can't do it easily with this API.

Depends on how easy is easy enough for you. Here is what changes to public .time do:

mixer.time = value; // sets the global time, without changing the local time of the actions
action.time = value; // sets the local time of a specific action
mixer.update( - mixer.time + value ); // sets the time of mixer and all running actions
mixer.update( 0 ); // just updates 

Action start, mixing / warping are global. Playback is local.

@AndrewRayCode
Copy link
Contributor

Sorry to prolong this thread, I'm still very new to animating meshes in Three so I'm not sure if I have enough information to make a new ticket yet.

The fundamental problem with this API is it's imperative. It says "go to this next point in the animation, which entirely depends on internal variables you can't see." What I want is a declarative API, to say "go exactly to this point in the animation."

For example, I have a mesh, with an animation of its hand going from down to up. I want to programmatically set the state of this animation. Maybe I want to tie it directly to the rotation of my character. If the character is rotated right, the hand should be up, if the character is rotated forward, the hand should be down. I already know how far rotated my character is and can easily calculate the percent along that rotation. I should be able to pass that same percent to the animation and have it set that current visible frame.

I've found a workaround for now which is similar to what you showed.

mixer.time = 0;
const action = mixer._actions[ 0 ];
const { duration } = action._clip;
action._loopCount = -1;
action.time = 0;
action.startTime = 0;
mixer.update(
    Math.min( duration * 0.999, duration * percentAlongAnimation )
);

This allows me to manually set percentAlongAnimation to whatever programatic value I choose and regain control. Obviously this isn't ideal because I'm mutating variables inside an API I don't own.

As I move on to animation blending and multiple animations, I imagine this will only get more complicated and difficult to achieve.

@bhouston
Copy link
Contributor

@delvarworld would something like a manual mode be useful? Some actions can be set in manual mode and then you have to set their time and weight explicitly?

@tschw
Copy link
Contributor Author

tschw commented Apr 13, 2016

@delvarworld

For example, I have a mesh, with an animation of its hand going from down to up. I want to programmatically set the state of this animation. Maybe I want to tie it directly to the rotation of my character. If the character is rotated right, the hand should be up, if the character is rotated forward, the hand should be down. I already know how far rotated my character is and can easily calculate the percent along that rotation. I should be able to pass that same percent to the animation and have it set that current visible frame.

Sounds more like setting the time of a particular action would be appropriate in this case.

https://github.com/mrdoob/three.js/blob/master/examples/js/TimelinerController.js#L57-L64

is how I did it in

http://threejs.org/examples/misc_animation_authoring.html

I've found a workaround for now which is similar to what you showed.
This allows me to manually set percentAlongAnimation to whatever programatic value I choose and regain control. Obviously this isn't ideal because I'm mutating variables inside an API I don't own.

Hmm... Honestly, this code does not look healthy or advisable in any way, not even for a workaround. I don't understand why you are having to mess with all these values. Also, why use mixer._actions[ 0 ] where there's clipAction to get the action?

Then there's the question, whether someone really should move the global time around. After all, AnimationMixer does not implement a full high level sequencer for the actions, so there really is no persistent global timeline, just a delayed start.

@AndrewRayCode
Copy link
Contributor

Hmm... Honestly, this code does not look healthy or advisable in any way,

Your example worked for me with setting the time prop. I don't think either of our solutions are ideal though, they are both brittle and require knowledge of the internals of the class.

whether someone really should move the global time around

Yes, someone should be able to pass in an exact set of variables to a scene, including time, and get an exact, reproducible output. There's a lot that comes with that, but I don't think it would be productive to drag this thread in that direction.

A good solution might be to separate the time handling logic from the actual animation API. That way end users who want their time to be managed by a third party can use the time API. Similar to the distinction between TweenJS controlling time and easing, and my easing-utils library simply taking in a value and returning the eased value, with no concept of time.

re:

would something like a manual mode be useful?

and

Sounds more like setting the time of a particular action would be appropriate in this case.

There are two things I'm looking for in an animation API:

  1. The ability to manually control animation (with an api for percent based control and absolute time based control, for convenience)
  2. Have the same API work for blended animations. I'm currently working towards multiple animations on my character and don't know how the suggested method by @tschw will work with blending(?). As in I'd like to programmatically set the weights of each animation and the percent of each animation and still have blending work behind the scenes.

@MasterJames
Copy link
Contributor

I sometimes resort to using the greensock TweenMax and/or TweenLite for that. It doesn't need start for animation and other interesting lessons learned over the years as it was originally done in flash but now has a js version.
I used it in my animated spotlights example because the internal Tween didn't like setters/getters (no longer being used that way in the example) and then I realized it's simpler (no need to call start) Anyway I guess I'm suggesting to look at it for ideas and maybe as a solution for setting an animation to a point in between by percentage of 'progress'.

@tschw
Copy link
Contributor Author

tschw commented Apr 13, 2016

@delvarworld

Yes, someone should be able to pass in an exact set of variables to a scene, including time, and get an exact, reproducible output. There's a lot that comes with that, but I don't think it would be productive to drag this thread in that direction.

A good solution might be to separate the time handling logic from the actual animation API. That way end users who want their time to be managed by a third party can use the time API. Similar to the distinction between TweenJS controlling time and easing, and my easing-utils library simply taking in a value and returning the eased value, with no concept of time.

No, the tweening stuff wouldn't help, because there isn't really a problem on that end.

The animation system was designed to be a "live" system. I pushed the envelope a bit by adding more scheduling functionality. However, and this is the important part, there is currently no sequencer with a a global timeline data structure implemented in Three.js. AnimationMixer only goes as far as it goes, and pretending it was a sequencer would currently be misuse. In particular, an action added to the mixer takes up resources, so it can quickly be started and stopped, and it currently wouldn't scale well to building timelines.

The question that arises is, "can we push the envelope a little further to make AnimationMixer a sequencer?" I think this might be possible, but I'd have to implement some more resource reuse to make it advisable.

Although I have it on my list, it's currently not on the top of it in terms of priorities, so unless we can get some funding for the PR, it would need to wait until I need it / get around to it.

Your example worked for me with setting the time prop. I don't think either of our solutions are ideal though, they are both brittle and require knowledge of the internals of the class.

I think you are misreading my code. The code I referred to only changes internal properties on this :).

@tschw tschw mentioned this pull request Apr 16, 2016
@AndrewRayCode
Copy link
Contributor

I'm still struggling with this, any advice on the correct way to use the API would be appreciated.

My mesh has two animations of different lengths (different number of frames). I need to control the time of each animation independently. In my case it's a walk cycle which always loops (setting its own time independently) and a jump animation (setting its own time independently). I'm controlling the blend between the two using weights.

I've tried doing basically this for each action. I've checked that for each animation, weight is set correctly, and percent is a value from 0 to 1.

action.setEffectiveWeight( weight );
const { duration } = action._clip;
action.time = duration * percent;

and then

mixer.update( 0 );

However my animations don't seem to reach completion - visually it appears as if each animation is still receiving influence from the others, even though I set weights explicitly to 0 (walking) and 1 (jumping). For example, my jump animation in Blender ends with the legs straight up and down, but in my running code, the animation ends with the legs about 80% to that goal. It might not be a weight issue, that's just a hunch.

I've tried debugging the internals of AnimationMixer but it's hard to follow because many functions, like setEffectiveWeight(), have deeply nested side effects.

@tschw
Copy link
Contributor Author

tschw commented Apr 20, 2016

@delvarworld

const { duration } = action._clip;

The loader probably sets a time scale and you're ignoring it.

It might not be a weight issue, that's just a hunch.

Please file a bug report with a simple test that reproduces the "issue". Thanks.

@AndrewRayCode
Copy link
Contributor

Woohoo! Finally found my final problem, which as I suspected, was specific to me. I'm using a custom shader where I computed the gl_Position wrong in the end. Thanks for helping me check my assumptions.

To come full circle, here's the API that I can now use: https://gist.github.com/DelvarWorld/586e9fb3102a39333f130836c416c87b
Under the hood I can do this by calling action.setEffectiveWeight, action.time = ..., and then mixer.update( 0 );. @bhouston While this still requires specific knowledge of the class internals (update( 0 ) especially, since that parameter is supposed to be a delta), I think it's simple enough for me that I don't think I need any API changes made.

@AndrewRayCode
Copy link
Contributor

AndrewRayCode commented Apr 23, 2016

@tschw me again. I'm trying to do this same process with morph targets. I'm trying to control them individually by hand. It doesn't seem quite as straightforward as the rigging blending.

1. I'm trying to rig an eyelid so I can close the upper and lower lid independently, using morph targets. With rigging, as shown above, I can set the weight of each rig animation to blend between them. I thought I could work with the individual upper and lower morph target animations somehow like

clipUpper = THREE.AnimationClip.CreateFromMorphTargetSequence( 'upper', [
        geometry.morphTargets[ 0 ], // open
        geometry.morphTargets[ 1 ], // upper close
], 3 );
actionUpper = mixer.clipAction( clipUpper ).play();

clipLower = THREE.AnimationClip.CreateFromMorphTargetSequence( 'lower', [
        geometry.morphTargets[ 0 ], // open
        geometry.morphTargets[ 3 ], // lower close
], 3 );
actionLower = mixer.clipAction( clipLower ).play();

...

clipUpper.time = ...;
clipLower.time = ...;
mixer.update( 0 );

However in this case only one of the morph animations ever seems to play, which is the first one created. Is it possible to control multiple morph targets independently with the AnimationMixer class? This is necessary for things like programatically setting character expressions.

2. I'm trying to tween the eyelid closing using two morph targets so that it properly tweens around the eye. I have "open", "half closed" and "full closed" targets. However, you can see when I tween between them using

clip = THREE.AnimationClip.CreateFromMorphTargetSequence( 'blink', geometry.morphTargets, 3 );
action = mixer.clipAction( clip ).play();

You can see the problem that occurs in this gif. When closing, the lid successfully tweens through half closed to full closed, which is required so the lid doesn't intersect the eye. However, as it opens again, it doesn't go back through the half way target, so there's an intersection:

blink-no

I tried manually inserting the half close morph target into the sequence again:

clipUpper = THREE.AnimationClip.CreateFromMorphTargetSequence( 'upper', [
        geometry.morphTargets[ 0 ], // open
        geometry.morphTargets[ 1 ], // half close
        geometry.morphTargets[ 2 ], // upper close
        geometry.morphTargets[ 3 ], // half close
], 3 );

However this destroys the animation and the vertices are mangled. Do you have a suggestion for how to reuse morph targets in the same animation without having to duplicate them in Blender? These two questions are related, I tried to go the first route so I could manually set the tween for each morph target, but when I couldn't figure that out, I tried to set a specific order of targets. That didn't work either.

@tschw
Copy link
Contributor Author

tschw commented Apr 23, 2016

@delvarworld
First of, your issues has nothing to do with AnimationMixer or this PR. Therefore you should open a new ticket. There's no magic in what animation mixer does: It interpolates the keyframes in all active actions' clips and applies a weighted state of the results to object properties. At that level, there's no difference between morph targets or bones.

Now, to answer both of your questions:

  1. When you want to do a circular motion with morph targets, you need many of them. So you were going into the right direction, just not far enough.
  2. When you want to apply two different morph target animations to the same object at the same time, these are mixed together. You have to make sure they do or do not overlap in terms of the tracks, depending on whether that is the desired effect or not. Using a common base state for independent morph targets that should run at the same time can't work.

HTH

@bhouston
Copy link
Contributor

  1. When you want to apply two different morph target animations to the same object at the same time, these are mixed together. You have to make sure they do or do not overlap in terms of the tracks, depending on whether that is the desired effect or not. Using a common base state for independent morph targets that should run at the same time can't work.

If Three.JS were to use deltas (e.g.: morph - base)_morphWeight + base) when applying for morph targets rather than blending to the exact set of morph vertices (morph1_morphWeight 1 + morph2*morphWeight2, etc.. ), you can apply multiple at the same time even if they are overlapping. Although the way that Three.JS uses morphs to do full animations like walking may conflict with the idea of using morph targets for layering facial animation.

This is how we implemented our JavaScript-based moprh target code Clara.io's with great effect and it was necessary in order to replicate Maya, 3DS Max, Blender morph target behavior.

@tschw
Copy link
Contributor Author

tschw commented Apr 23, 2016

@bhouston
Where/how to best implement it?

I'm wondering whether doing it right would break legacy content and need to become switchable or could just work.

There's still the state += (1 - sumOfWeights) * stateOnAttach logic in the mixer, so requiring stateOnAtach == morphBaseState might be an option for a shortcut, but maybe to nowhere really worth going...

Can it be done generally - also for bones (see #7913)?

@AndrewRayCode
Copy link
Contributor

Oh, duh! All I wanted was mesh.morphTargetInfluences. That's the only thing I need to use. I can declare morph target percents individually using that, and they automatically blend fine and multiple can run at once.

@AndrewRayCode
Copy link
Contributor

Charisma lives :)

welcome-charisma
https://twitter.com/andrewray/status/724081650242445312

Thanks for your help everyone!

@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2016

Cuteness!

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