Open jonathanolson opened 7 years ago
@samreid and @pixelzoom, thoughts (lower priority) on the top comment (https://github.com/phetsims/twixt/issues/3#issue-210227781)?
I have a working demo that shows easing handling and damped harmonic animations, and shows that stacking (using 2+ easing animations at a time with deltas) with quadratic in-out has a nice behavior.
(As a clarification, twixt is now a runnable repo for the demo).
Great looking demos! I am wondering what leads to the off-the-axis motion in the easing demo for this case:
UPDATE: Perhaps I was on a stale version. I don't see this issue anymore (or was on an odd easing).
I'm not sure why EaseAnimation sometimes returns dt and sometimes returns the overflow ratio.
What is the 2nd arg in a bind
, for instance: Easing.polynomialEaseOutValue.bind( easing, n );
?
Great looking demos! I am wondering what leads to the off-the-axis motion in the easing demo for this case:
An old version did that (when I had interrupting animations use current velocity), when did you pull? That's an issue if it's with latest master.
I'm not sure why EaseAnimation sometimes returns dt and sometimes returns the overflow ratio.
Looks slightly buggy in the code, but the initial point was "return the amount of DT in the step since the animation ended", so if you are chaining animations one after another, you could call secondAnimation.step( firstAnimationReturnValue )
What is the 2nd arg in a bind, for instance: Easing.polynomialEaseOutValue.bind( easing, n );?
It's currying, so ( someFunc.bind( thisValue, a, b, c ) )( d, e, f )
will execute someFunc( a, b, c, d, e, f )
with thisValue
as the this-reference.
Should deltaCallback
be required?
What is your vision for how to generalize this to Vector2 or Vector3 or Color4, etc? Or a tuple of (Vector2,Color3)?
Should deltaCallback be required?
Will totally depend on what form the final API would take, that's just temporary prototype code.
Here's how the quadratic in-out works on my copy (seems artificially slowed in the gif):
Should deltaCallback be required?
Sorry for the vague question. I meant to ask: in the current implementation (or if the final API shapes up similarly to this), is there any functional advantage to having an EaseAnimation with no deltaCallback
? If not now, do you envision a usefullness in running tweens that have no callback?
What is your vision for how to generalize this to Vector2 or Vector3 or Color4, etc? Or a tuple of (Vector2,Color3)?
I'd like to support passing in any data type that can be eased (for those, e.g. my comment about blend()
), or any numeric tuple type for the damping system. It would use the lower-level "animating a single value" building-block to build the animation.
Sorry for the vague question. I meant to ask: in the current implementation (or if the final API shapes up similarly to this), is there any functional advantage to having an EaseAnimation with no deltaCallback? If not now, do you envision a usefullness in running tweens that have no callback?
If you pass in a locationProperty or tell it to directly change the opacity of a Node, you probably care less about having a callback for whatever high-level API there is. So at that level, it would not be required.
Perhaps an elegant way to do it would be to create a new class Scalar() which has a blend function so all types can be treated uniformly. Animator class could automatically wrap {number} in {Scalar}, though we may pay a performance price.
Perhaps an elegant way to do it would be to create a new class Scalar() which has a blend function so all types can be treated uniformly. Animator class could automatically wrap {number} in {Scalar}, though we may pay a performance price.
Wrapping numbers may end up happening, but it sounds better for performance than creating objects every frame.
The "easing" animator would just need an interpolation function f( a, b, ratio )
, while the "damping" animator wants something that acts like a vector space (presumably we would just handle things equivalent to R^n and animation would be component-wise).
This seems like a great direction to go in, the prototype seems promising and it gives us much more control about how things animate. The next questions I would be asking are about how it would look in the context of a simulation and shaping up the API. Let me know if you want more specific feedback that what I have already provided.
Highly unlikely that I'll be able to review this before the first week of April.
I can't figure out why EaseAnimation was designed to use deltas instead of calling back with the current value, @jonathanolson can you comment?
I can't figure out why EaseAnimation was designed to use deltas instead of calling back with the current value, @jonathanolson can you comment?
This allows multiple animations to run at the same time on one thing, showing the "interpolation" in the quadratic mode (smoothing when destination changes).
That makes perfect sense, thanks!
What do you think about renaming the callback from delta
to callback
and providing 2 parameters, the delta and the value, so that clients without concurrent animations can use the current value? Or should we encourage all easing clients to use deltas so they are more stackable?
Should EaseAnimation also have a callback on completion, or just Animator
when it exists?
I tried writing CCK zooming with and without EaseAnimation, and decided it adds enough value to continue with it. So I'd like to finalize the API for this (not sure if Animator necessary for CCK DC milestone).
That level of refactoring sounds premature, as it's (probably) internal code that will get rewritten, and won't correspond to the public API.
That level of refactoring sounds premature
Which of the preceding comments was this statement referring to?
Should EaseAnimation also have a callback on completion, or just Animator when it exists?
We'd want to support completion callbacks in general.
I tried writing CCK zooming with and without EaseAnimation
Note comments:
* WARNING: PROTOTYPE, see https://github.com/phetsims/twixt/issues/3 before using!
* Not fully documented or stabilized. May be deleted.
The code is definitely not ready to be used in simulations.
So I'd like to finalize the API
I'll plan to take a stab at this very soon then, as a more initial prototype.
CCK picked up a EaseAnimation dependency in https://github.com/phetsims/circuit-construction-kit-common/commit/aabbf782b5cfee8d22dcdd0c30ca84d4e5b07a00 because I decided it was preferable to scaling my own easing, but don't feel any obligation to maintain the API or keep CCK working, just keep me in the loop and I'll revise when updates are made.
Demo is presumably http://localhost/~cmalley/GitHub/twixt/twixt_en.html
I took a look through the code and examples. My feedback is essentially the same as what @samreid said in https://github.com/phetsims/twixt/issues/3#issuecomment-283105194.
I'd also be interested in how this would fit into Sim.stepSimulation
.
Thinking more about this, as the result of an extended Slack conversation on 1/31/18 (which I will not reproduce here)...
What I don't like about the current approach (EaseAnimation, DampedAnimation and their demo) is that the client is responsible for callingstep( dt )
. Since step
for the model and view is called from Sim.js, that results in a cascade of step
definitions. For example, consider the case where I use the "creator pattern" to dynamically create a Node and associated drag listener. At the end of the drag, I want an animation to occur. To do that, I need to instantiate the animation, then somehow call it's step
function.
A better approach might be a mechanism for globally registering an animation, which is then stepped on each clock tick by the registrar. The difficultly here would be in knowing which screen an animation is associated with, so as to only step animations for the active screen.
Mentioning @ariel-phet so he's aware of this.
There has been no work on this issue since 2/2017. Today's Slack developer conversation is reproduced below, so it doesn't disappear when PhET exceeds the 10K message limit.
Main points:
• There is general agreement that we don't want to continue using TWEEN.
• There is currently no animation support in twixt (other than the Easing.js interpolation functions) that is recommended for new development.
• There is confusion about what we should be using in the meantime. @jbphet pointed to Easing.js, a set of interpolation functions which doesn't handle any of the animation. @samreid pointed to EaseAnimation.js, which is marked as "PROTOTYPE...Not fully documented or stabilized". @jonathanolson said that EaseAnimation.js "isn't fully ready", but @samreid and @jonathanolson are both using it in new sim development. I have continued using twixt wrappers around Tween (e.g. MoveTo, OpacityTo) for lack of any other stable animation solution.
• We are creating animations using multiple approaches, which will complicate moving to a better solution.
• No one has had time to think about what a good solution might look like, so there's no way to program defensively. This will also complicate moving to a better solution.
• Continuing to use TWEEN will cause problems for PhET-iO playback.
• We've accumulated almost 1 year's worth of additional technical debt since this issue as created.
• There is no timeline for addressing this.
Full Slack discussion:
Chris Malley [4:44 PM]
Reading scrollback where @jonathanolson asked: “Is it expected that OpacityTo tweens
also change the visibility of the node being animated?” Looks like at least one sim
(Function Builder) relies on this. Should I change? Why would you run OpacityTo on
an invisible node?
Jonathan Olson [4:46 PM]
Not sure, I’m using non-TWEEN.js solutions for the future
Chris Malley [4:46 PM]
What are those solutions?
Chris Malley [5:09 PM]
I’m not a fan of TWEEN. But I was under the impression that TWEEN was what PhET had
collectively decided to use for (typically view-specific) animation. It concerns me
that you’re using “non-TWEEN solutions”, since I would have preferred do the same.
Sam Reid [6:47 PM]
I thought we decided to use our own interpolation functions like EaseAnimation.js and
step them from our own step() functions. (edited) But that it is OK to continue using
tween.js while we move to that new pattern.
John Blanco [9:21 AM]
I agree with what Sam said about having decided to use our own interpolation functions.
I recall changing CoinTerm.step() to use the easing functions from Twixt based on that.
Chris Malley [9:35 AM]
If that’s the case, then why are EaseAnimation and other non-tween animations documented
as “WARNING: PROTOTYPE…Not fully documented or stabilized. May be deleted.”
Sam Reid [9:36 AM]
@jonathanolson raised the same concern in
https://github.com/phetsims/twixt/issues/3#issuecomment-285166120, he would know
best about the status
Chris Malley [9:38 AM]
So while I do recall a conversation about (and preference for) moving to non-tween,
I recall that was predicated on completing related code in twixt. Which as far as I
can tell, has not been done. (edited)
Sam Reid [9:40 AM]
Perhaps Easing.js is more stable? It has fewer disclaimers. (edited)
Chris Malley [9:43 AM]
Easing is a set of interpolation functions. It provides no animation facilities.
E.g. to use Easing for motion animation, the client is responsible for duration,
mapping [0,1] to location, callbacks, etc.
John Blanco [9:47 AM]
Indeed, I've only used the easing functions combined with step-based model changes.
Chris Malley [9:48 AM]
I can see in CoinTerm.step that you had to do boilerplate animation computations there.
Is that really the preferred approach to animation?…
John Blanco [9:50 AM]
Probably not, but we seem to have collectively decided against wide usage of TWEEN,
and TWIXT isn't fully ready, so it seemed like a reasonable approach.
Chris Malley [9:55 AM]
So what is the recommendation for animation in new sims, eg EqEx? Reinvent tween
using Easing?
Sam Reid [9:55 AM]
Let’s postpone discussion until @jonathanolson returns
Jonathan Olson [10:13 AM]
I'm planning to have higher-level animation code (that uses Easing
under the hood) that is stable, but things like EaseAnimation are definitely
not at all final. In the meantime, I think the recommendation was to use Easing's
functions and handle things during a model/view step (so we can avoid the
TWEEN dependency and issues). Ideally I'll have time sometime to finish off the
more ideal animation code
Chris Malley [11:28 AM]
The most recent work on twixt was 2/2017, so I’m skeptical that higher-level
animation code will be available anytime soon. Certainly not in time for me to
use it in EqEx. Using Easing.js directly (ala CoinTerm.step) means more
code/responsibilities in client code, much of it similar/duplicated. So what do
you recommend? Use tween? Or use Easing.js and write a bunch of client code
to do what tween does? (edited)
Jonathan Olson [11:31 AM]
It was my understanding that some of what TWEEN does is incompatible with
our phet-io playback, and presumably any code that uses it would be rewritten
to remove the reference at a future time when it was needed. If that's not a
problem for EqEx, feel free to use it. Either way, it will probably need to get
changed in the future. Sorry, wish I had a good solution right now.
Chris Malley [11:33 AM]
The future sounds like an approach that implements `step` functions in model and view,
called from Sim.js. And that does not match the tween approach at all.
Jonathan Olson [11:34 AM]
The future ideally wouldn't have to be hooked up like that.
Chris Malley [11:34 AM]
How then?
Jonathan Olson [11:36 AM]
When I dive into it, I'll be able to let you know details
What I don't like about the current approach (EaseAnimation, DampedAnimation and their demo) is that the client is responsible for calling
step( dt )
In my opinion, cascaded calls to step() give the most flexibility and control for running animations, and is most consistent with our existing pattern for evolving models or views over time, and compatible with our needs for disposal (no leaks to a registrar).
For example, consider the case where I use the "creator pattern" to dynamically create a Node and associated drag listener. At the end of the drag, I want an animation to occur. To do that, I need to instantiate the animation, then somehow call it's step function.
In that case, you would create the new node and add it to a list (in an appropriate parent), then in the parent's step function, you would call the child's step function.
A better approach might be a mechanism for globally registering an animation, which is then stepped on each clock tick by the registrar. The difficultly here would be in knowing which screen an animation is associated with, so as to only step animations for the active screen.
Tween used a single global registry which seemed problematic, and in https://github.com/phetsims/joist/issues/404#issuecomment-281758745 (and the following few comments) we discussed moving to the cascaded step function approach.
An option we have not yet discussed would be to create a TwixtHandler type (a registrar) like in https://github.com/phetsims/joist/issues/404#issuecomment-281765253, but leave it up to the simulation how they want to handle it: at one end of the spectrum, you could define one TwixtHandler per screen and use it as a "global" registrar (global to that screen). Sims that need to distinguish between model and view tweens, could have one TwixtHandler in the model and one in the view. At the other end of the spectrum, sims that need fine grained control over ordering, the TweenHandler could be defined in the animated instance type, and called via cascaded step functions.
@samreid pointed to EaseAnimation.js, which is marked as "PROTOTYPE...Not fully documented or stabilized". @jonathanolson said that EaseAnimation.js "isn't fully ready", but @samreid and @jonathanolson are both using it in new sim
I am definitely not using it in a new sim. I've been using Easing (which is marked as stable).
After the above commit, EaseAnimation is no longer used in CCK either.
I updated the comment in https://github.com/phetsims/twixt/issues/3#issuecomment-362050037 to name it as TwixtHandler, pointing out that it could be used as a screen global, or passed in to children for explicit calling in step.
At 2/1/18 dev meeting, @ariel-phet asked me to comment on my "wish list" for animation library.
Features ala Tween: • Animate attribute values. Handle the transform between the [0,1] values of easing functions and the range of values in the attribute's domain. • easing (Easing.js already implements this) • callbacks (onStart, onComplete, onStart) • ability to stop (aka cancel) animation • Look through Tween API and decide which features are needed.
Features not in Tween: • Animate multiple attributes simultaneously. E.g. an animation where something translates, scales and fades simultaneously. Possibly over independent durations, but over 1 duration would probably be sufficient. Independent easing for each attribute. (I've had multiple requests for this in Equality Explorer.) • Choice of whether to start animation explicitly (default) or automatically on instantiation • Choice of whether to have the client step the animation (default) or register an animation with something that handles step • For registered animations, associate an animation with a Screen, so that animations play/pause based on the active Screen • For registered animations, ability to easily stop all animations associated with a Screen (or all Screens), e.g. when "Reset All" button is pressed.
Other: • Ability to reuse an animation multiple times, possibly on different targets. • Don't introduce memory management responsibilities for the client.
@jonathanolson and I discussed his current implementation via Zoom. It's looking very good. He's going to summarize a few open questions for developer meeting.
After discussing with @pixelzoom, we have a few general questions for the development team (tagged for developer meeting, and I've heavily rephrased and added questions):
There are two (not necessarily exclusive) options that may work:
Each ScreenView is given an object that tracks active view animations for that screen. When an animation is created, if you pass a ScreenView to it, it will handle registering itself with the ScreenView. This allows the view to step the animations only when it is the active screen, and allows us to stop all animations for a given screen (which is usually desired for "Reset All"). Downside is passing ScreenView references through the code.
Nodes could have addAnimation/removeAnimation added as a feature. All animations of nodes who are fully visible (shows up on the screen) will be stepped each frame (either with Display.updateDisplay, or as a separate manual control that would allow view steps themselves to execute afterwards). Don't have to pass things through the code, but resetting would need to be handled separately (node.stopSubtreeAnimations()
?), and we'd need to decide about what happens when a node would be made invisible (does it stop/interrupt animations under it?)
Can we just manually step any animations used in the model? Do we need to build a parallel structure like above where it is similar to the view?
Stopping an animation currently leaves its value in the state it was in at the moment. There could be an argument that it should move to the "end" value (say an animation gets stopped that's animating something back to a bucket).
Additionally, do we want to support pausing an in-progress animation? Would start()
then resume from the paused location, instead of "restarting"? We may need terminology for "the animation is waiting for a delay, but paused", "it is animating, but paused", and clarification of what "running/animating" means (ideally we'd have a Property that is true when it is un-paused and animating vs a Property that is still true when paused).
It's what would control what causes the animation to get stepped. 'manual'
, 'timer'
, node
and screenView
would all be potential options that could be passed in, based on what we decide. Thoughts?
It may be beneficial for all options to have examples. Using the runnable demo for examples would probably be even better. Is that desired?
@jonathanolson can you please add a few notes from last week's (2/8) dev meeting?
Main notes were:
It's been over a year now. Which items in https://github.com/phetsims/twixt/issues/3#issuecomment-365839043 have been done, and which still remain to do?
@ariel-phet looking at the issues that are currently open in twixt (#12, #13, #14, #16,...), they are all related to this issue in some way. We have a nice prototype that we took almost to the finish line, started using it in production code, then failed to finish. Should we investigate what's needed to resolve these issues?
Marking for developer meeting. It would be good for @kathy-phet to weigh in on priority for this issue.
We discussed this today during developer meeting.
The lowest priority for @jonathanolson is that there are two sims that still use this, but the devs felt more like cleaning up TWEEN
The next highest priority is to work on this issue's features. But we feel like this priority is low until a sim that's being worked on needs these features. Unmarking for dev meeting.
Looking into a prototype for general animation (as noted by https://github.com/phetsims/joist/issues/404).
I've checked in some code that is not refined or final by any means.
Some thoughts/opinions:
first.blend( second, ratio )
function (like Vector2/etc. has) sounds like a good default candidate for interpolating between non-numbers.Animator
that tracks (and steps) multiple animations, removing animations as necessary when they are complete. We may want to just call step() once without having to manually track ephemeral animations (but still want the flexibility to manually reference and step animations themselves).