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

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

Merged
merged 29 commits into from Jan 9, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Dec 30, 2022

Motivation

I would like the animation code and interface to be simpler.
So i m proposiing some simplificaiton, it makes also documenting animation easier.

Description

Removal some flexibility in the api in exchange of some code semplification

i also create a folder parkinglot where i put the straighten code and i think i m putting the canvas effects too.
Those needs to be changed in examples or something that the developer can grab, but i don't think they should be part of af the class itself. We have 4 effects, while the possibilities are so many that i think providing 4 of them and nothing else just makes noise.

Other things i would like to solve:

  • keypath in animation. I don't have a solution right now but ideally i would like to list the cases in which a keypat is needed ( gradients i m sure and little else probably )
  • duality between byValue and endValue, i would like to keep one only.

Changes

  • Object.animate takes only an object with one or more properties
  • animate does not take += or -= anymore, we can write an addition or subtraction when we do the animation end value.
  • removed barrel imports
  • Object.animate does not undefine anymore the onChange and onComplete of multiple keys. Reasons are that with requestRenderAll that is not needed anymore, and since you can cancel a single animation, if you cancel the one that carry the actual callbacks the others are not working anymore. We need to be clear on the fact that you shoudn't do expensive things on onChange and that you should use requestRenderAll rather than renderAll.

Some other proposals

naming changes

export type TOnAnimationChangeCallback<T, R = void> = (
  value: T,
  valueRatio: number,
  durationRatio: number
) => R;

into

export type TOnAnimationChangeCallback<T, R = void> = (
  value: T,
  valueProgress: number,
  timeProgress: number
) => R;

Reason: ratio is generic, can be 3, 4, -3 0.5. We have values here that are always between 0 and 1 and represent the progress between start and end. I think this naming would be more clear.

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2022

Build Stats

file / KB (diff) bundled minified
fabric 930.332 (-2.697) 299.836 (-0.902)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2022

Coverage after merging small-animation-preferences into master will be

83.10%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.js35%12.50%0%54.55%431, 433, 433, 433, 433, 433, 435–436, 438, 438, 438–439
src
   cache.ts96.97%90%100%100%56
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   env.ts72.73%53.33%100%79.17%22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%118, 124, 135, 144, 96
   point.class.ts100%100%100%100%
   shadow.class.ts98.48%96%100%100%156
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 23, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   pattern_brush.class.ts97.06%87.50%100%100%21
   pencil_brush.class.ts91.86%85.42%100%93.58%122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
   spray_brush.class.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 77, 89–91, 99
src/canvas
   canvas.class.ts93.15%89.14%94%95.93%1153, 1153–1154, 1157, 1177, 1177, 1239, 1275–1276, 1295–1296, 1304–1305, 1326, 1334, 1447–1448, 1450–1451, 1471–1472, 509, 576–577, 582, 592, 721–722, 724–725, 725, 725, 771–772, 833–834, 887–889, 919, 924–925, 954–955
   canvas_events.ts78.30%75.69%82.81%79.41%1003–1004, 1004, 1004–1006, 1008–1009, 1009, 1009, 1011, 1019, 1019, 1019–1021, 1021, 1021, 1027–1028, 1036–1037, 1037, 1037–1038, 1043, 1045, 1075–1077, 1080–1081, 1085–1086, 1202–1204, 1207–1208, 1280, 1400, 145, 1494–1495, 1501, 1505–1506, 1522, 1544, 1591, 1596, 1635, 170, 318–319, 319, 319–320, 320, 323–327, 332, 334, 347–350, 353, 372, 372, 372–373, 373, 373–374, 382, 387–388, 388, 388–389, 391, 400, 406–407, 407, 407, 443, 447, 447, 447, 447, 447–448, 450, 525, 527, 527, 527–529, 549, 551, 551, 551–552, 552, 552, 555, 555, 555–556, 559, 568–569, 571–572, 574, 574–575, 577–578, 590–591, 591, 591–592, 594–598, 604, 611, 651, 651, 651, 653, 655–659, 665, 671, 671, 671–672, 674, 677, 682, 695, 722, 778–779, 779, 779–780, 782, 785–786, 786, 786–787, 789–790, 793, 793–795, 798–799, 809, 891, 905, 912, 933, 965, 986–987
   static_canvas.class.ts94.67%89.27%97.92%97.10%1118–1119, 1119, 1119–1120, 1240, 1250, 1304–1305, 1308, 1343–1344, 1422, 1431, 1436, 1485–1486, 1714, 1714–1715, 1764, 1767, 1770, 1770, 1770, 1773, 1776, 1776, 1776, 338, 351, 404–405, 407–408, 417, 421–422, 425–426, 878
src/color
   color.class.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   changeWidth.ts100%100%100%100%
   control.class.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts86.67%66.67%100%100%122, 129
   drag.ts100%100%100%100%
   polyControl.ts6.35%0%0%11.43%100, 105, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts20%12.50%50%22.22%45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.41%92.68%100%93.59%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/filters
   2d_backend.class.ts92%83.33%100%93.75%35–36
   FilterBackend.ts88.89%88.89%100%85.71%15–16

@asturur
Copy link
Member Author

asturur commented Dec 30, 2022

@ShaMan123 @Lazauya Let's talk about this small changes here. Also do you have any opinion around endValue and byValue?

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

  • Is parkinglot an known thing? I love the name but I am guessing you will rename to something generic like examples.
  • I would change the content of the parkinglot to simple methods, not mixins, e.g. removeObjectWithFading
  • As suggested earlier I would expose an animation controller class that runs numerous animations in parallel (maybe also in sequence) and execute callbacks with an array of values for each animation in the controller. That will make Animatable#_animate dead. It was a mess, now is better but still is pretty shit. I would remove it entirely. The animation API is good, flexible and straight forward. Does this abstraction provide any value?
  • regarding renderAll vs requestRenderAll, I thought I should always call renderAll from within an animation callback since it executes from within a requested frame. If I call requestRenderAll it will request another frame so my animation effects will always be a frame late. Please explain and correct me if I am wrong.
  • byValue endValue, AFAIAC it was solved in chore(TS): animation #8297 . And TS will warn the dev when passing both of them, or should at least.
  • naming changes - I am for. valueProgress is good and I would go for durationProgress cause that is more accurate vs. timeProgress cause time is confusing. Duration is the time since we started so no ambiguity.

options: Partial<TAnimationOptions<T>> = {}
) {
const path = key.split('.');
const propIsColor = this.colorProperties.includes(path[path.length - 1]);
const currentValue = path.reduce((deep: any, key) => deep[key], this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line handled the key path dot notation using key.split('.')

Copy link
Member Author

Choose a reason for hiding this comment

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

i just moved it down, we need to execute it only if startValue is not passed.

@asturur
Copy link
Member Author

asturur commented Dec 30, 2022

  • Is parkinglot an known thing? I love the name but I am guessing you will rename to something generic like examples.
  • I would change the content of the parkinglot to simple methods, not mixins, e.g. removeObjectWithFading

Parking lot is just a middleground between deleting the code and fix it now. Are just files i don't think we should deal with now. Are not even supposed to work, just memory. I tried to keep them in functional state tho.

  • As suggested earlier I would expose an animation controller class that runs numerous animations in parallel (maybe also in sequence) and execute callbacks with an array of values for each animation in the controller. That will make Animatable#_animate dead. It was a mess, now is better but still is pretty shit. I would remove it entirely. The animation API is good, flexible and straight forward. Does this abstraction provide any value?
  • regarding renderAll vs requestRenderAll, I thought I should always call renderAll from within an animation callback since it executes from within a requested frame. If I call requestRenderAll it will request another frame so my animation effects will always be a frame late. Please explain and correct me if I am wrong.

The issue with renderAll is that is gonna render N times you call it. requestRenderAll just the last one is going to trigger.
So for now, if you animate 3 properties ( left, top, angle ) you should use requestRenderAll.

Regarding animation controller, no i don't want to work on any new feature logic now.
Year is basically ended and we are not done yet with migration.
The previous PR was a stretch too, while an improvement animation was already TS and converted.

  • byValue endValue, AFAIAC it was solved in chore(TS): animation #8297 . And TS will warn the dev when passing both of them, or should at least.

I want one to disappear. not be warned about. What is the deal with having both?
Seems to me that byValue is less understandable than endValue.
Is natural to say i want to animate left from 30 to 60 or to 60 only.
is easy to say, i want to move by 30 right, so i will animate left to endValue this.left + 30
byValue instead falls short with colors. i can think of an animation from red to blue, but is a bit stranger to think with byValue, saying that you are animating by value with [-255, 0, 255] in order to cancel red channel and increase the blue.

It also does not make sense with arrays because modify by value with a matrix for example would let me think the natural process is a multiplication, while we would do addition.

  • naming changes - I am for. valueProgress is good and I would go for durationProgress cause that is more accurate vs. timeProgress cause time is confusing. Duration is the time since we started so no ambiguity.

Yes for me this is the same, i just dislike ratio.

@asturur
Copy link
Member Author

asturur commented Dec 30, 2022

Ok renames pushed up.
We can get back to those changes first week of new year, but likely i would like byValue to go.

@ShaMan123
Copy link
Contributor

I didn't want to have both by/end value. It was something I wasn't sure you would accept breaking.
Kill it.

@ShaMan123
Copy link
Contributor

IMO we could move the key path logic to _set or set instead of part of animation logic

@@ -5,13 +5,13 @@ export type AnimationState = 'pending' | 'running' | 'completed' | 'aborted';
/**
* Callback called every frame
* @param {number | number[]} value current value of the animation.
* @param valueRatio ∈ [0, 1], current value / end value.
* @param durationRatio ∈ [0, 1], time passed / duration.
* @param {number} valueProgress ∈ [0, 1], current value / end value.
Copy link
Contributor

Choose a reason for hiding this comment

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

current value / end value.

seems off.

@asturur
Copy link
Member Author

asturur commented Jan 3, 2023

IMO we could move the key path logic to _set or set instead of part of animation logic

That is more a lodash thing, and dangerous becase then people find a way to exploit it with parent.__proto__._proto__.__proto__ and do some strange code pollution things.

keypath logic can go away as soon as we can animate a gradient or a pattern or a point of polygon.
I m sure it can be done already, is probably that this object.animate is supposed to be a shortcut to use util.animate and i adding some constrains. But if we have the animate function clearly used on obejcts, we can remove object.animate entirely.
As an added bonus developers can use any animation engine to change the values on objects.

@asturur
Copy link
Member Author

asturur commented Jan 3, 2023

@ShaMan123 to be clear, i m ok breaking things because we remove code and functionalities, i m not ok breaking things now to add more functionalities or more options.

@asturur asturur changed the title refactor(): Simplify animation code refactor(): BREAKING Animation removing byValue, removing fxStraigthen. Jan 3, 2023
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I don't get why the base class accepts byValue while subclasses accept endValue.
Can't we remove byValue entirely and calc it just for easing?
Updating the ts tests is in order as well.

test/unit/animation.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member Author

asturur commented Jan 6, 2023

I don't get why the base class accepts byValue while subclasses accept endValue. Can't we remove byValue entirely and calc it just for easing? Updating the ts tests is in order as well.

Oh well it was the easy way to get there.
The subclass is not exposed to the final dev, and calculating it for the easing means calculating it every tick.
So all in all i thought that this change was sufficient and not wrong.

Is there any particular reason why it sounds bad to you?

asturur and others added 2 commits January 6, 2023 23:50
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Fixed and simplified types.
The only question remaining is should we merge between animate and animateColor?
I say we should.

@@ -93,41 +94,31 @@ export type TAnimationCallbacks<T> = {
abort: TAbortCallback<T>;
};

export type TAnimationValues<T> =
| {
export type TBaseAnimationOptions<T, TCallback = T, TEasing = T> = Partial<
Copy link
Contributor

Choose a reason for hiding this comment

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

should we rename this? since there is another type called TAnimationBaseOptions or is it clear enough?

test/ts/animation.ts Outdated Show resolved Hide resolved
@ShaMan123
Copy link
Contributor

Is there any particular reason why it sounds bad to you?

It is weird that we calculate byValue according to endValue in the subclass and then calculate endValue according to byValue in the base class but I can live with it.
For sure it makes a lot more sense accepting endValue rather than byValue in the exposed method.

((rgba: TRGBAColorSource, valueRatio: number, durationRatio: number) =>
callback(new Color(rgba).toRgba(), valueRatio, durationRatio));
((rgba: TRGBAColorSource, valueProgress: number, durationProgress: number) =>
callback(new Color(rgba).toRgba(), valueProgress, durationProgress));
Copy link
Contributor

@ShaMan123 ShaMan123 Jan 7, 2023

Choose a reason for hiding this comment

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

I also think that the the color animation callbacks should pass a Color instance instead of a color string

Suggested change
callback(new Color(rgba).toRgba(), valueProgress, durationProgress));
callback(new Color(rgba), valueProgress, durationProgress));

 arrays come first in logic
@ShaMan123
Copy link
Contributor

The only question remaining is should we merge between animate and animateColor?

If we change color animation options to accept only a color instance (which makes sense and is not that breaking) we could merge it into animate w/o a problem by checking instance of Color and extend the isArrayAnimation functionality

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jan 7, 2023

Looking at the code more closely I think byValue (and easing) doesn't belong to the base class...
It is only defined on it but not used by it.
We can define it on the subclasses for their own use.
Thoughts?

@ShaMan123
Copy link
Contributor

The only question remaining is should we merge between animate and animateColor?

If we change color animation options to accept only a color instance (which makes sense and is not that breaking) we could merge it into animate w/o a problem by checking instance of Color and extend the isArrayAnimation functionality

small-animation-preferences...animate-color

@asturur
Copy link
Member Author

asturur commented Jan 8, 2023

Let's merge this first part, thanks for the fixes and types improvement and thanks for trying @Lazuya
Also @Lazauya i don't see why opening a PR from your branch to this branch shouldn't work.
Also @ShaMan123 if you want to commit freely to a pr please squash the commits. Is bad to open N commits and check one by one what did change. Don't change the course of a PR, and if you want to commit squash them so i can at least disagree/agree quickly.

@asturur
Copy link
Member Author

asturur commented Jan 8, 2023

What should i do? can i merge or we need to do something else?

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jan 9, 2023

merge
I can change stuff to accommodate color animation changes in a follow up

@asturur asturur merged commit 8627774 into master Jan 9, 2023
@ShaMan123
Copy link
Contributor

just realized this PR broke text cursor animation
will PR a fix right away

@ShaMan123
Copy link
Contributor

found it! since animate signature has changed _animateCursor should use _animate
And in fact I would ask if it is better to return an object from Object#animate with keys being the props to animate and values being the animation context or at least an array with key, value props.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jan 9, 2023

@asturur I looked again at animateColor and I think that since it used to accept only string value I can merge it into animateColor w/o a problem... What do think?
And in general my direction for ColorAnimation is to have it deal only with Color instances small-animation-preferences...animate-color

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jan 9, 2023

But generally speaking if I were a dev using ColorAnimation I would move to ArrayAnimation since it is much more flexible and isn't fixed to rgb color space

ShaMan123 added a commit that referenced this pull request Jan 11, 2023
introduced by #8547 breaking `Object#animate` signature
ShaMan123 added a commit that referenced this pull request Mar 16, 2024
…nand '+=' syntax (#8547)

Co-authored-by: ShaMan123 <shacharnen@gmail.com>
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

3 participants