feat(): path animation #9077
feat(): path animation #9077
Conversation
rename Update PathAnimation.ts Update PathAnimation.ts cleaner Update PathAnimation.ts percent expose distance to callback FIX! Update PathAnimation.ts Update index.ts expose init
| @@ -3,8 +3,8 @@ import { capValue } from '../util/misc/capValue'; | |||
|
|
|||
| const RE_PERCENT = /^(\d+\.\d+)%|(\d+)%$/; | |||
|
|
|||
| export function isPercent(value: string | null) { | |||
| return value && RE_PERCENT.test(value); | |||
There was a problem hiding this comment.
checked that all usages are with a defined value
There was a problem hiding this comment.
can we put const RE_PERCENT = /^(\d+\.\d+)%|(\d+)%$/; inside the function isPercent ? i m not sure there is reason to keep a regex alive all the time. The parser is not hot code
There was a problem hiding this comment.
I thought you wanted all regex to be const to be created once.
Can we not hold back on these things for now?
| export type ValueAnimationOptions = TAnimationOptions<number>; | ||
|
|
||
| export type ArrayAnimationOptions = TAnimationOptions<number[]>; | ||
|
|
||
| export type ColorAnimationOptions = TAnimationOptions< | ||
| TColorArg, | ||
| string, | ||
| number[] | ||
| >; | ||
|
|
||
| export type AnimationOptions<T extends number | number[] | TColorArg> = | ||
| T extends TColorArg | ||
| ? ColorAnimationOptions | ||
| : T extends number[] | ||
| ? ArrayAnimationOptions | ||
| : ValueAnimationOptions; |
There was a problem hiding this comment.
moved to the files
|
I want to write a test with playwright but not sure of the best way to do so |
|
Build Stats
|
.github/workflows/build.yml
Outdated
| @@ -42,7 +42,7 @@ jobs: | |||
| size: { fabric: { minified: fs.statSync('${{ env.minified }}').size, bundled: fs.statSync('${{ env.bundled }}').size } } | |||
| }); | |||
| - name: checkout src files | |||
| run: git checkout origin/master -- src fabric.ts index.ts index.node.ts .browserslistrc rollup.config.mjs | |||
| run: git checkout ${{ github.event.pull_request.base.sha }} | |||
There was a problem hiding this comment.
fixed - didn't checkout added files for some reason
|
I also want animateColor to be removed and intergrated into |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs 😔. Thank you for your contributions. |
ShaMan123
added
feature
and removed
stale
| const distance = pathProgress * context.length; | ||
| return callback( | ||
| { | ||
| ...context.getPointOnPath(distance), |
There was a problem hiding this comment.
maybe we need to respect a transform matrix line under group?
|
let me see if i can unblock this today |
|
ok for me here there is a real divergent idea on how features needs to be exposed. The smartest part of this pr is the callback wrapper that changes a value animation to a point on path context. This is exactly what i see as feature creep. To explain myself at best, something like this: fabric.util.animate({
duration: 3000,
endValue: '85%',
easing: fabric.util.ease.easeInOutBack,
// path or pathContext as first argument depending if we think parsing the path is a cost needs to be avoided
onChange: positionOnPathCallbackWrapper(path, ({ x, y }, v, t) {
circle.setXY(new fabric.Point(x, y), 'center', 'center');
canvas.renderAll();
}),
});would be equally clean and require far less code to maintain and doesn't need to be part of the bundle either. On top of that a Path animation for me would be taking path A, path B and move smoothly between path A and path B and not animating a point over a path. All the other animation types do exactly that, change a color from a to b, a number from a to b and so on. |
|
I tried to align to existing patterns. The real problem lies in the code |
|
I was not advocating for a path animation utility indeed. The path iterator was born only for supporting text on a path, so is probable that may not support other use cases. I m not sure what you mean by Here there is a callback wrapper that imho can be useful and light weight if couple with an example on how to use it. If the path iterator can be done better, as long as it supports text on a path as it is today, we have no issue changing it, is not part of the public api per se, it was there because there was no distinction between public and private in fabric 5 because of how the code was written. Then apart the callback wrapper, and the improvement on the path iterator, path iteration is not a fabricJS feature. |
|
I don't think the color animation is useful because it allow only rgb animation, where as hsl animation is what I am used to do. export const parsePathForIteration = (
path: string | fabric.TSimplePathData
) => {
const pathData = fabric.util.makePathSimpler(
Array.isArray(path) ? path : fabric.util.parsePath(path)
);
const data = fabric.util.getPathSegmentsInfo(pathData);
return {
path: pathData,
data,
// this is because in the code the last entry is a sum
length: data[data.length - 1]!.length,
// unintuitve to understand this
getPointOnPath: (distance: number) =>
fabric.util.getPointOnPath(pathData, distance, data)!,
};
}; |
|
so fixing the angles and removing the last entry + refactoring it to expose |
|
Please i don't want to add any more classes. Can we just add an iterate over to fabric.Path so that the intuitive part of: getPointOnPath: (distance: number) =>
fabric.util.getPointOnPath(pathData, distance, data)!,can be saved from the state of the path? getPointOnPath: (distance: number) => {
return fabric.util.getPointOnPath(this.path, distance, this.pathSegmentInfo)!,
}so instead of creating a path iterator new class you just create a fabric path. This could have the added benefit that you can either transform the path with the current transformation or not do that. If you want to use it on a path as string, you can do that by not assigning any properties to the path, otherwise you can add transformation properties to the fabric.Path and get a transformed point. |
|
I like that |
|
the lenght on the last item was a lazy shortcut to get text on a path working. For the transformation part i have no idea how that apply to lenght, i don't think is straight forward at all, maybe path.length is not something we want to expose or we can expose with getTotalLenght that will need to do a full parse of a transformed version of the path before calculating the length ad hoc. there is some thinkering to do for transformations, but definely a method on fabric.Path seems to be easy to discover, easy to document |
|
I don't understand the transform part |
|
we can talk through it, the transform part is optional. Like i have a path long 10, how long it is when my path has scaleX by 2? it could be 20, unless the path is a vertical line so is still 10. this is nice for a relaxing chat maybe |
|
I see, I would transform the points and that is it |
|
anyone interested in this I suggest using bezier-js for the task |
Motivation
#9071
I use this kind of animation in my apps and this is 4x cleaner
Description
Changes
Apart from
PathAnimation:Gist
In Action
https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-4vqpvd?file=%2Fpages%2Findex.tsx%3A52%2C5-77%2C6
Parcel.Sandbox.-.Google.Chrome.2023-07-07.11-46-58_Trim.mp4