-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Build Stats
|
@ShaMan123 @Lazauya Let's talk about this small changes here. Also do you have any opinion around endValue and byValue? |
There was a problem hiding this 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
vsrequestRenderAll
, I thought I should always callrenderAll
from within an animation callback since it executes from within a requested frame. If I callrequestRenderAll
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 fordurationProgress
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); |
There was a problem hiding this comment.
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('.')
There was a problem hiding this comment.
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.
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.
The issue with renderAll is that is gonna render N times you call it. requestRenderAll just the last one is going to trigger. Regarding animation controller, no i don't want to work on any new feature logic now.
I want one to disappear. not be warned about. What is the deal with having both? 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.
Yes for me this is the same, i just dislike ratio. |
Ok renames pushed up. |
I didn't want to have both by/end value. It was something I wasn't sure you would accept breaking. |
IMO we could move the key path logic to |
src/util/animation/types.ts
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
That is more a lodash thing, and dangerous becase then people find a way to exploit it with keypath logic can go away as soon as we can animate a gradient or a pattern or a point of polygon. |
@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. |
There was a problem hiding this 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.
Oh well it was the easy way to get there. Is there any particular reason why it sounds bad to you? |
Co-authored-by: Shachar <[email protected]>
There was a problem hiding this 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< |
There was a problem hiding this comment.
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?
It is weird that we calculate |
((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)); |
There was a problem hiding this comment.
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
callback(new Color(rgba).toRgba(), valueProgress, durationProgress)); | |
callback(new Color(rgba), valueProgress, durationProgress)); |
arrays come first in logic
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 |
Looking at the code more closely I think |
|
Let's merge this first part, thanks for the fixes and types improvement and thanks for trying @Lazuya |
What should i do? can i merge or we need to do something else? |
merge |
just realized this PR broke text cursor animation |
found it! since |
@asturur I looked again at |
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 |
introduced by #8547 breaking `Object#animate` signature
…nand '+=' syntax (#8547) Co-authored-by: ShaMan123 <[email protected]>
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:
Changes
Some other proposals
naming changes
into
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