feat(animation): animations registry #7528

ShaMan123 · 2021-12-01T07:45:25Z

Motivation

There's a need to cleanup animations in applications.
Currently the dev needs to implement something by themselves.
For example in react while developing when react does a hot reload or whatever and fabric is animating an error raises because the canvas is trying to access the context which react just disposed.
Another use case I can think of is with erasing. It makes sense to iterate over all animations before erasing starts to make the objects non erasable. (I wanted this to be default behavior but lacked the mechanism to do so)
In general, knowing what is animating is crucial information for good UX/UI.

I think this should be handled by fabric.

gist

Exposed fabric.runningAnimations (is this a good name? is fabric the right level to hold this ref?
It is an array containing the options object passed to animate and the cancel function. This means that it can contain additional context if such is passed to animate allowing the dev freedom.
Exposed the following methods: cancelAll, finders
For example we could pass the object/canvas as context, this will make it very simple to cancel relevant animations.

IMO It makes sense that an object holds a ref to all it's running animations and that it's dispose method should cancel them.
What do you think?

src/util/animate.js

c19b2c8

ShaMan123 · 2021-12-05T19:15:04Z

ready

call `removeFromRegistry();` after logic to fit all calls

ShaMan123 · 2021-12-05T19:19:27Z

is this a good name? is fabric the right level to hold this ref?

@asturur regarding fabric.runningAnimations

ShaMan123 · 2021-12-06T10:34:41Z

I have added current state to animation context so now it enables even more freedom of use

ShaMan123 · 2022-01-19T11:56:06Z

I remembered you mentioned there's a need to cleanup runningAnimations from object/canvas dispose.
So I'll commit that.

ShaMan123 · 2022-01-19T14:05:51Z

Once an animation ends it is removed from the registry
added a target property to animation context
all object animations get registered with target set (if called from object.animate)
object dispose cancels the target's animations
canvas dispose cancels all animations (should this be on Canvas or StaticCanvas?)

No memory leaks should exist

ShaMan123 · 2022-01-19T14:19:01Z

src/util/animate.js

        if (cancel) {
          return;
        }
        if (abort(current, valuePerc, timePerc)) {
-          // remove this in 4.0
+          // remove this in 5.0


what about this??

This is good to remove because then the option to pause an animation comes to play.

to support pausing of animations

ShaMan123 · 2022-01-19T16:07:24Z

There's something important!
When using object.animate you can't access the value returned by fabric.util.animate because the function is chainable.
Should we break? I think so. Because it is awkward trying to access the animation context without it. And it may lead to not using object.animate which is a good method or damage the purpose of the PR (making it simple to handle animations after they start).

fabric.js/src/mixins/animation.mixin.js

Line 154 in 7a406e8

return this;

There are a few more places such as this

865be3c, made it a single commit to easily revert

broke a few other to maintain standard

asturur · 2022-01-22T15:00:22Z

src/canvas.class.js

@@ -1200,6 +1200,7 @@
     */
    dispose: function () {
      var wrapper = this.wrapperEl;
+      fabric.runningAnimations.cancelAll();


this needs to be executed per canvas.
EIther you pass in the canvas as argument and cancelAll becomes a cancelAll by canvas, or in an application with 2 canvases this would stop everything.

Good point.
When an objects animates does it have to be attached to a canvas? I guess not. So I'm not sure how to do what you suggest.
I can add a canvas property to the animation context object but what if there's no canvas set to the object??
Or maybe we should iterate over all running animations and check at that moment who is attached to the canvas arg and cancel that?

asturur · 2022-01-22T15:11:01Z

src/mixins/animation.mixin.js

@@ -13,8 +13,7 @@ fabric.util.object.extend(fabric.StaticCanvas.prototype, /** @lends fabric.Stati
   * @param {Object} [callbacks] Callbacks object with optional "onComplete" and/or "onChange" properties
   * @param {Function} [callbacks.onComplete] Invoked on completion
   * @param {Function} [callbacks.onChange] Invoked on every step of animation
-   * @return {fabric.Canvas} thisArg
-   * @chainable


maybe i m wrong, but is chainability still cool to do?
I understand it saves typing and as a result bits in the final bundle.
But i find having half methods chainable and half not is weird. Maybe is something we should drop in the rewrite.

I agree. Chaining is not worth the weirdness.
I say we drop it.

And it is less breaking to have void return values that change (for future impl)

asturur · 2022-01-22T15:20:14Z

src/static_canvas.class.js

     */
    _centerObject: function(object, center) {
      object.setPositionByOrigin(center, 'center', 'center');
      object.setCoords();
      this.renderOnAddRemove && this.requestRenderAll();
-      return this;


i would like to limit breaking changes in 5. Those are not part of the animation changes. What is the reason to remove chainability now?

It's as you said. I thought it's too weird to have the animated version not chainable while the regular one is.

asturur · 2022-01-22T15:23:57Z

i m going to merge this and open a following PR to restore some extra changes that are for now not needed and complicate the release note of 5.0

ShaMan123 · 2022-01-22T15:24:35Z

BTW should I push a small commit for the delay option (#1719 ) or leave it for another PR?

ShaMan123 · 2022-01-22T15:26:08Z

i m going to merge this and open a following PR to restore some extra changes that are for now not needed and complicate the release note of 5.0

If you think it's better to release v5 without this that's alright. We'll make it part of v6.

asturur · 2022-01-22T15:35:27Z

i m fine adding this now, i just don't want to remove the chaining operators that do not return animations.
I have seen the commit for the delay, i wonder what are we doing with the start option then? isn't the start a delay itself?

ShaMan123 · 2022-01-22T15:39:45Z

I don't see a start option. Or anything resembling a delay option

ShaMan123 added 4 commits December 1, 2021 07:23

todos

c19b2c8

feat(animation): running animations registry

d9fb32d

test(animation): running animations

3284141

lint

6f88020

ShaMan123 requested a review from asturur December 1, 2021 08:10

ShaMan123 added 4 commits December 1, 2021 10:11

Update animate.js

8b1d422

Update animate.js

7e55962

fix(): cancel all

5793ced

perf

0466f25

ShaMan123 changed the title ~~feat(animation): running animations registry~~ feat(animation): animations registry Dec 2, 2021