-
-
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
chore(TS): more conversion of utils #8148
Conversation
src/util/misc/matrix.ts
Outdated
t[0] * p.x + t[2] * p.y + t[4], | ||
t[1] * p.x + t[3] * p.y + t[5] | ||
); | ||
}; |
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.
Probably a point.tansform is a good idea too.
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.
yes!
Code Coverage Summary
|
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 matrix be a class extending Array?
It makes sense IMO and will allow a drop replacement for matrix in the future
I will carefully merge stroke projection into #7475 when it is done
@@ -32,7 +32,7 @@ import { Point } from '../point.class'; | |||
* @type {AnimationContext[]} | |||
*/ | |||
var fabric = global.fabric, RUNNING_ANIMATIONS = []; | |||
fabric.util.object.extend(RUNNING_ANIMATIONS, { | |||
extend(RUNNING_ANIMATIONS, { |
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.
IMO RUNNING_ANIMATIONS
should be a subclass of Array
class AnimationRegistry extends Array<TAnimation> {
...
}
export const RUNNING_ANIMATIONS = new AnimationRegistry()
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.
Doesn't have to be a subclass of array, it can be a class that has an array referenced. It doesn't really matter too much to me. Here i just swapped fabric.util.object because i had issues with circular imports, or at least i think i did.
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.
even better
translateY?: number; | ||
} | ||
|
||
type TScaleMatrixArgs = { |
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.
type TScaleMatrixArgs = Patial<{ ... }>
https://www.typescriptlang.org/docs/handbook/utility-types.html
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.
Why you prefer Partial here?
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.
simply because all props are optional
type projectStrokeOnPointsOptions = { | ||
strokeWidth: number; | ||
strokeLineJoin: StrokeLineJoin; | ||
strokeMiterLimit: number; // https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-miterlimit |
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.
if the comment is /** **/
on top of the prop then it is live and not static text
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.
and perhaps it should be a Pick
type from TStrokeOptions
or TObjectOptions
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 ll recheck this, wasn't really a deep though.
src/util/misc/matrix.ts
Outdated
export const invertTransform = (t: TMat2D): TMat2D => { | ||
const a = 1 / (t[0] * t[3] - t[1] * t[2]), | ||
r = [a * t[3], -a * t[1], -a * t[2], a * t[0], 0, 0] as TMat2D, | ||
{ x, y } = transformPoint({ x: t[4], y: t[5] }, r, true); |
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.
we should decide about the fate of Point and IPoint
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 side with your approach.
Everything should be a Point instance inside fabric
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.
The reason why i didn't overuse the point years ago is because i thought a plain object would be more performant than a function or a class. But i think this is not true.
I never benchmarked it, and i struggle to understand why fabric starts to choke when cycling through an handful of hundreds objects.
I like that a Point can be a Point or its basic interface because it allows you to reuse the data you have without transforming it.
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 think we need to aim for flexibility but tend to use Point in fabric and not a simple object.
I don't think there a real diff in perf.
All major repos use classes so it has to be a good idea and the browser as well
round = 'round', | ||
} | ||
|
||
export type TMat2D = [number, number, number, number, number, number]; |
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.
maybe
export type TMat2D = [number, number, number, number] | [number, number, number, number, number, number];
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 made this initially but then i had to go back because it makes more complicated to write code checking for 4 or 6 variant of the array.
Code Coverage Summary
|
Convert a bit more of utils, and do ad hoc replacemente when the build break.