chore(TS): animation #8297

Lazauya · 2022-09-18T01:32:31Z

Changes

Refactor animation logic into classes:
- AnimationBase contains all the logic
- Subclasses handle initialization logic and impl calculate that returns the current value of the animation
animate color
- closes Animate color seems broken in current master #8241
- align class and signature with peers
  - animateColor(from ,to ,duration, options) => animateColor(options)
  - colorEasing integrated into default easing of ColorAnimation
- fixed byValue option
fix end value calculation:
there was a conflict happening in the cases:
- passing byValue without endValue since defaults are assigned to both, endValue and byValue diverged
- passing diverged byValue, endValue
- this fix uncovered bugs in easing functions that I fixed
Color:
- accept more param types (color source, color instance) in constructor and deprecate fromSource as it has become redundant
BREAKING:
return animation instance from animate instead of a cancel function and remove findAnimationByXXX from AnimationRegistry

Gist

@asturur there are too many threads in this PR. All are pretty much stale accept #8297 (review) that I would like you to comment on.

Proposals

What about making AnimationBase extend Observable and fire state events?

const context = animate({ ... })
context.on('change', () => {})

It is possible to make animate accept also a color animation options object

animate({
  startValue: [r,g,b,a] // TS will find this hard to handle since it is an array and will fall into the array animation case
  type: 'color' // this will do the job
})

accept color values in color animation

animate({
  startValue: 'colorString' // TS will find this easy to understand and handle as `ColorAnimation`
})
animate({
  startValue: new Color('colorString') // even better, making passing `type: color` redundant
})

accept color space option for color animation

animate({
  startValue: new Color('colorString') // even better, making passing `type: color` redundant
  colorSpace: 'hsl'
})

type fixes, mergeWithDefaults for animate ran npm audit

# Conflicts: # src/constants.ts # src/typedefs.ts # src/util/anim_ease.ts # src/util/animate.ts # src/util/animate_color.ts # src/util/lang_object.ts

ShaMan123

I left a few comments, nothing major
Checkout package-lock.json from master (upstream probably) so it is removed from the PR diff
And run prettier

src/util/anim_ease.ts

src/util/animate.ts

src/util/anim_ease.ts

src/util/animate.ts

src/util/animate_color.ts

src/constants.ts

src/util/anim_ease.ts

src/util/animate.ts

asturur · 2022-09-18T22:41:02Z

src/util/animate_color.ts

-      }
-    },
+    onChange: (current, valuePerc, timePerc) =>
+      onChange?.(


we slipped in a bunch of optional chaining operator, but those are not supported before chrome 80.
We didn't fully decide yet which browsers are we going to support, in this case better leave the code as it was.

I can remove the chaining ops, but typescript compiles it down into unchained anyway so I don't get why we should remove them

I didn't know that's a problem

because typescript compile down a lot of stuff that we don't let it do.
For now we let the browsersupport of rollup compile the stuff for us.
I couldn't get Typescript to choose browser granularity in chosing what to compile to, and at the end what you have to support is browsers.

src/util/animate.ts

src/util/animate_color.ts

asturur · 2022-09-18T22:53:35Z

there are at least 3 behavor change i would like to see reverted, and it seems to me you added a target new option that should be proposed on a separate PR.

Lazauya · 2022-09-18T23:08:16Z

there are at least 3 behavor change i would like to see reverted, and it seems to me you added a target new option that should be proposed on a separate PR.

The only ACTUAL behavior change is that the currentState of a color animation is now a TColorAlphaSource instead of a color string. See my other comments about the target stuff. I can change it but the problem is its SUPER hard to generify the animate util function to work with "normal" animations and color animations. It took me a long time to realize that just changing animateColor match animate would be more clean in the end. I can make another PR specifically for that I suppose if that would be better.

ShaMan123

typed animation registry to avoid confusion

it's weird that when I comment and review the comments are displayed out of context

src/util/animate.ts

ShaMan123 · 2022-09-19T05:52:05Z

src/util/animate_color.ts

-      }
-    },
+    onChange: (current, valuePerc, timePerc) =>
+      onChange?.(


I didn't know that's a problem

src/util/animate.ts

src/util/animate_color.ts

asturur · 2022-09-19T09:26:04Z

there are at least 3 behavor change i would like to see reverted, and it seems to me you added a target new option that should be proposed on a separate PR.

The only ACTUAL behavior change is that the currentState of a color animation is now a TColorAlphaSource instead of a color string. See my other comments about the target stuff. I can change it but the problem is its SUPER hard to generify the animate util function to work with "normal" animations and color animations. It took me a long time to realize that just changing animateColor match animate would be more clean in the end. I can make another PR specifically for that I suppose if that would be better.

I think the 3 behavor change are:

removal of ...restOptions
remove of calculateColor where @ShaMan123 pointed out
on the onChange part assuming that the value is always an array and removing the array check
Arent' those changes?

Adding in pr description modified the color animate function to be more generic doesn't help much in understanding why are you changing things, on top of that those changes should be factored away from a typing pr.

TODO: will modify this in another PR

Lazauya · 2022-09-23T22:35:30Z

I went ahead and removed animate color stuff. I'm putting that in a new PR where I'll clean it up.

Lazauya · 2022-09-30T22:13:23Z

@ShaMan123 @asturur Any outstanding issues with this?

ShaMan123

What is the status of the isMany typing?
I would like that in this PR

src/util/animate.ts

ShaMan123 · 2022-10-01T10:52:23Z

In order to pass tests:

Add a changelog entry describing changes
run npm run prettier:write and commit
lint is not your fault

ShaMan123

minor patch color
updated from master
updated description
seems good to go

ShaMan123 · 2022-12-27T10:50:42Z

@asturur let's merge this one
Too long has it been waiting

asturur · 2022-12-29T18:41:53Z

@asturur let's merge this one Too long has it been waiting

We had a list of priorities i m going in order.
I have to re-catch up because i have no idea what is this anymore.
Re-reading now.

asturur · 2022-12-29T18:49:18Z

Ok fixed merge conflicts that is step 1

asturur · 2022-12-29T18:57:14Z

notes for me since this is a long review

check the regex match in color. https://github.com/fabricjs/fabric.js/pull/8297/files#diff-52c2e42c3516c3b6ccf6a73c0fe7f280f5dc9500283b88f6a0555defb0eacf7dR291
remove barrel import if they end up defining objects in the code https://github.com/fabricjs/fabric.js/pull/8297/files#diff-7a7d2850d56c6fbd2bd2e6cbe154e4fe2205e19dcbc44f3653fd5f50a78b767dR1-R5 ( yes they do )

src/util/animation/Animation.ts

asturur · 2022-12-29T21:38:33Z

this build
313352 Dec 29 22:31 fabric.min.js
master build
311733 Dec 29 22:37 fabric.min.js

1.6KB extra, not sure why, they usually reduce, but not an issue

asturur · 2022-12-29T22:14:14Z

Will take me tomorrow to finish reading this.
I wish there was a little bit more explanation on the pr message.
Is it clear that the external interface isn't changed ( apart for the returned value ) but a bit more of text on AnimationBase and Animation, ArrayAnimation and ColorAnimation would have been nice.
You have to consider the order you get the files to review is not the execution order and you start reading having no idea what is going to happen.

src/util/animation/AnimationBase.ts

…c.js into pr/8297

ShaMan123 · 2022-12-29T22:54:27Z

src/shapes/Object/AnimatableObject.ts

+    if (!propIsColor && typeof to === 'string') {
+      // check for things like +=50
+      // which should animate so that the thing moves by 50 units in the positive direction
+      to = to.includes('=')


can we dump this?

ShaMan123 · 2022-12-29T22:56:32Z

You have to consider the order you get the files to review is not the execution order and you start reading having no idea what is going to happen.

Point taken

asturur · 2022-12-30T16:34:08Z

I think this PR is good, thanks both.
I want to simplify something, maybe i can do that following up with a different pr.
Example:

object animatabale could not use util animate but just start the animation on its own
get rid of all custom syntax += syntax for animation.
remove the FX_DURATION concepts and all the 4 premade effects and relegate them to a folder called examples.

I have noticed there are type tests, and i see they can't run for now. is that correct?

ShaMan123 · 2022-12-30T19:18:20Z

object animatabale could not use util animate but just start the animation on its own

didn't get what you meant.

The rest I agree.

I have noticed there are type tests, and i see they can't run for now. is that correct?

Correct. #8394 handles the test framework.

Co-authored-by: kristpregracke <kpregracke@redfoundry.com> Co-authored-by: ShaMan123 <shacharnen@gmail.com> Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com> Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

kristbp2 added 6 commits September 10, 2022 18:49

initial undocumented types

827cf88

documenting types,

9224364

type fixes, mergeWithDefaults for animate ran npm audit

remove defaults obj for animate

fc9300f

Merge remote-tracking branch 'origin/master' into laz-animate-typing

badf63c

# Conflicts: # src/constants.ts # src/typedefs.ts # src/util/anim_ease.ts # src/util/animate.ts # src/util/animate_color.ts # src/util/lang_object.ts

run prettier

c263864

Typing for color animation and fixes for animate

8c39dd5

ShaMan123 suggested changes Sep 18, 2022

View reviewed changes

asturur reviewed Sep 18, 2022

View reviewed changes

src/util/anim_ease.ts Outdated Show resolved Hide resolved

asturur reviewed Sep 18, 2022

View reviewed changes

src/util/animate.ts Outdated Show resolved Hide resolved

asturur reviewed Sep 18, 2022

View reviewed changes

src/util/animate.ts Outdated Show resolved Hide resolved

asturur reviewed Sep 18, 2022

View reviewed changes

src/util/animate_color.ts Outdated Show resolved Hide resolved

kristbp2 and others added 3 commits September 18, 2022 18:28

resolve some comments on the PR

2903682

remove WithReturnType

a0e52f2

animation registry typings

d2075e6

ShaMan123 reviewed Sep 19, 2022

View reviewed changes

kristbp2 added 3 commits September 23, 2022 16:59

revert animate_color.ts

8895a5f

TODO: will modify this in another PR

more explicit typing in animation_registry.ts

4cc9713

Merge remote-tracking branch 'origin/master' into laz-animate-typing

bb86ef1

Lazauya changed the title ~~Animate & Animate Color Typing~~ Animate & Animation Registry Sep 23, 2022

Lazauya changed the title ~~Animate & Animation Registry~~ Animate & Animation Registry Typing Sep 24, 2022

kristbp2 added 2 commits September 24, 2022 14:12

more explicit typing in animation_registry.ts

1ddd449

fix comments

757fd7a

ShaMan123 reviewed Oct 1, 2022

View reviewed changes

src/util/animate.ts Outdated Show resolved Hide resolved

src/util/animate.ts Outdated Show resolved Hide resolved

ShaMan123 added 3 commits December 27, 2022 12:27

Update color.js

6e8861e

Merge branch 'master' into pr/8297

9d6c71e

fix imports

53779e6

ShaMan123 previously approved these changes Dec 27, 2022

View reviewed changes

solved merge conflicts

dab0ade

asturur dismissed ShaMan123’s stale review via dab0ade December 29, 2022 18:48

removed unnecessary changes to object.ts

e1e2c0b

asturur self-assigned this Dec 29, 2022

asturur added 2 commits December 29, 2022 21:11

ooops

e5f3cd7

it builds now

30c5172

asturur reviewed Dec 29, 2022

View reviewed changes

src/util/animation/Animation.ts Outdated Show resolved Hide resolved

asturur reviewed Dec 29, 2022

View reviewed changes

src/util/animation/AnimationBase.ts Outdated Show resolved Hide resolved

ShaMan123 added 3 commits December 30, 2022 00:47

rename: Animation => ValueAnimation

3366fe6

no arrow methods

f644fd2

Merge branch 'laz-animate-typing' of https://github.com/Lazauya/fabri…

02d8c0d

…c.js into pr/8297

ShaMan123 reviewed Dec 29, 2022

View reviewed changes

asturur approved these changes Dec 30, 2022

View reviewed changes

asturur merged commit 695cf10 into fabricjs:master Dec 30, 2022

asturur mentioned this pull request Dec 30, 2022

refactor(): BREAKING Animation removing byValue, removing fxStraigthen. #8547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): animation #8297

chore(TS): animation #8297

Lazauya commented Sep 18, 2022 •

edited by ShaMan123

Loading

ShaMan123 left a comment •

edited

Loading

asturur Sep 18, 2022

Lazauya Sep 18, 2022

ShaMan123 Sep 19, 2022

asturur Sep 19, 2022

asturur commented Sep 18, 2022

Lazauya commented Sep 18, 2022

ShaMan123 left a comment •

edited

Loading

ShaMan123 Sep 19, 2022

asturur commented Sep 19, 2022 •

edited

Loading

Lazauya commented Sep 23, 2022

Lazauya commented Sep 30, 2022

ShaMan123 left a comment

ShaMan123 commented Oct 1, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Dec 27, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022 •

edited

Loading

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

ShaMan123 Dec 29, 2022

ShaMan123 commented Dec 29, 2022

asturur commented Dec 30, 2022

ShaMan123 commented Dec 30, 2022

chore(TS): animation #8297

chore(TS): animation #8297

Conversation

Lazauya commented Sep 18, 2022 • edited by ShaMan123 Loading

Changes

Gist

Proposals

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

asturur Sep 18, 2022

Choose a reason for hiding this comment

Lazauya Sep 18, 2022

Choose a reason for hiding this comment

ShaMan123 Sep 19, 2022

Choose a reason for hiding this comment

asturur Sep 19, 2022

Choose a reason for hiding this comment

asturur commented Sep 18, 2022

Lazauya commented Sep 18, 2022

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 Sep 19, 2022

Choose a reason for hiding this comment

asturur commented Sep 19, 2022 • edited Loading

Lazauya commented Sep 23, 2022

Lazauya commented Sep 30, 2022

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 commented Oct 1, 2022 • edited Loading

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Dec 27, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022 • edited Loading

asturur commented Dec 29, 2022

asturur commented Dec 29, 2022

ShaMan123 Dec 29, 2022

Choose a reason for hiding this comment

ShaMan123 commented Dec 29, 2022

asturur commented Dec 30, 2022

ShaMan123 commented Dec 30, 2022

Lazauya commented Sep 18, 2022 •

edited by ShaMan123

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 left a comment •

edited

Loading

asturur commented Sep 19, 2022 •

edited

Loading

ShaMan123 commented Oct 1, 2022 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

asturur commented Dec 29, 2022 •

edited

Loading