-
-
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): utils #8230
chore(TS): utils #8230
Conversation
asturur
commented
Sep 1, 2022
•
edited
Loading
edited
- removed getById in favor of normal browser js
- removed toArray in favor of Array.from
- removed addClass in favor of classList.add
- moved the rotation point logic all in the point class
- removed makeElement
- removed setImageSmoothing
commit 51b93a8 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Sep 1 19:17:52 2022 +0300 fix tests! commit 672ff98 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Sep 1 19:17:42 2022 +0300 Update image.class.ts commit edab4e1 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Sep 1 19:17:39 2022 +0300 Update canvas.class.ts commit 0e23df8 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Thu Sep 1 16:15:39 2022 +0200 ok? commit 09eb1b5 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Thu Sep 1 16:03:55 2022 +0200 addClass removed for classList.add)
95e62dc
to
7ecd0c5
Compare
// but then we remove the lower-canvas specific className | ||
upperCanvasEl.classList.remove('lower-canvas'); | ||
// we add the specific upper-canvas class | ||
upperCanvasEl.classList.add('upper-canvas'); |
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.
much better
limitDimsByArea(ar: number) { | ||
const { perfLimitSizeTotal } = config; | ||
const roughWidth = Math.sqrt(perfLimitSizeTotal * ar); | ||
// we are not returning a point on purpose, to avoid circular dependencies |
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.
VERY STRANGE
And shouldn't happen...
HACKY
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.
no is not hacky and not everything should be a point.
Before was x and y but wasn't a point.
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 for the circular dep cycle is just because of the assignment fabric.Point
that will be removed anyways.
If you assign it in index it will not happen.
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.
Don't you agree this is out of pattern?
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 a function about caching, i don't think returning an array to return 2 values is better or worse than returnin an object and calling it a point. I just really need the 2 values out of there.
}); | ||
const container = fabric.document.createElement('div'); | ||
container.classList.add(this.containerClass); | ||
this.wrapperEl = fabric.util.wrapElement(this.lowerCanvasEl, container); |
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.
drop wrapElement
from utils?
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.
wrapElement probably don't belong in utils exported object indeed. will be just a function imported 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.
it is used only here across fabric, right?
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 i think only here to add a div around the canvas
@@ -1,8 +1,9 @@ | |||
import { TMat2D } from "./typedefs"; | |||
|
|||
export { version as VERSION } from '../package.json'; | |||
export const noop = () => {}; | |||
export function noop() {}; |
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 there a difference between const = () => {}
and function(){}
?
Or just styling?
I don't mind
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.
There is a difference that a fat arrow can't be bound and can't be used as a class member.
So since noop is used in createClass as an empty method, just to be sure i made it a function
@@ -378,4 +386,6 @@ export class Point { | |||
} | |||
} | |||
|
|||
const originZero = new Point(0, 0); |
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.
You want to reuse this? That is why you don't inline it in the function signature?
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 is more readable defined as a const rather than in the function definition.
Does it get defined every function run if is in the function signature?
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.
good question
no idea
src/util/anim_ease.ts
Outdated
* Backwards easing out | ||
* @memberOf fabric.util.ease | ||
*/ | ||
export const easeOutBack = (t, b, c, d, s = 1.70158) => c * ((t = t / d - 1) * t * ((s + 1) * t + s) + 1) + b; |
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 see this number a lot. Maybe define it as const at top of file?
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 there are a bunch of numbers. i wanted to define them, then i realized i didn't know how to call them and moved on for now
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 totally get that
*/ | ||
export const easeOutBounce = (t, b, c, d) => { | ||
if ((t /= d) < (1 / 2.75)) { | ||
return c * (7.5625 * t * t) + b; |
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.
more magic numbers that can/should be defined at top of file?
var docElem, | ||
doc = element && element.ownerDocument, | ||
box = { left: 0, top: 0 }, | ||
return { left, top }; |
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.
can this be a point as well?
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.
not sure, this is style related, it does not need to be transformed or anything else.
function getNodeCanvas(element) { | ||
var impl = fabric.jsdomImplForWrapper(element); | ||
return impl._canvas || impl._image; | ||
return { |
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.
Point?
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.
better off for rtl support as well?
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 those are just for calculating the canvas position for some sort of padding/border issue.
I m not sure we ever need a different measurement.
I also think modern browsers solves this for us giving us an extra pair of mouse event coordinates relative to the canvas element that would be exactly what we need
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 i think that i should look into moving mouse events with offsetX and offsetY and trash all of 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.
i ll open a task
* @param {HTMLElement} element Element to make selectable | ||
* @return {HTMLElement} Element that was passed in | ||
*/ | ||
export function makeElementSelectable(element) { |
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.
merge makeElementSelectable
and makeElementUnselectable
to one method with a flag?
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.
not the kind of refactors or changes i m looking to now.
There is still a lot to do, those are really minors utils, i can merge them yes, if i manage to make everything else work.
This is the list of utils from the utils folder, with some ideas of what to continue to expose and what to expose somewhere else as deprecated ( and to become internals ) fabric.util = {
cos,
sin,
rotateVector,
createVector,
calcAngleBetweenVectors,
getHatVector,
getBisector,
degreesToRadians,
radiansToDegrees,
rotatePoint,
getRandomInt, // hide
removeFromArray, // hide
projectStrokeOnPoints,
transformPoint,
invertTransform,
composeMatrix,
qrDecompose,
calcDimensionsMatrix,
calcRotateMatrix,
multiplyTransformMatrices,
stylesFromArray,
stylesToArray,
hasStyleChanged, // hide
object: {
clone, // hide
extend, // hide
},
createCanvasElement,
createImage,
copyCanvasElement,
toDataURL,
toFixed,
matrixToSVG, // hide
parsePreserveAspectRatioAttribute, // hide
groupSVGElements, // hide
parseUnit,
getSvgAttributes,
findScaleToFit,
findScaleToCover,
capValue, // hide
saveObjectTransform,
resetObjectTransform,
addTransformToObject,
applyTransformToObject,
removeTransformFromObject,
makeBoundingBoxFromPoints,
sendPointToPlane,
transformPointRelativeToCanvas, // this has to be renamed imho
sendObjectToPlane,
string: {
camelize, // hide
capitalize, // hide
escapeXml, // hide
graphemeSplit, // this needs a setter and getter to be overridden
},
getKlass, // hide
loadImage,
enlivenObjects,
enlivenObjectEnlivables,
array: {
min, // hide
max, // hide
},
pick, // hide
joinPath,
parsePath,
makePathSimpler,
getSmoothPathFromPoints,
getPathSegmentsInfo,
getBoundsOfCurve,
getPointOnPath,
transformPath,
getRegularPolygonPath,
request, // hide
setStyle,
isTouchEvent,
getPointer,
removeListener, // hide
addListener, // hide
wrapElement, // hide
getScrollLeftTop, // hide
getElementOffset, // hide
getNodeCanvas,
cleanUpJsdomNode,
makeElementUnselectable, // hide
makeElementSelectable, // hide
isTransparent,
sizeAfterTransform,
mergeClipPaths,
ease,
animateColor,
animate,
requestAnimFrame,
cancelAnimFrame,
}; some i m not sure like getBiSector, getHatVector, if those are just internals for calculation or have some utility for projects. |
Looks good to me |
Ok so i m not sure i understaood that function. to me it just seems it converts from viewportCoordinates to htmlElement coordinates. What does it do on top of that? |
The takeaway from this PRs are:
There are more improvements that can be made to this code probably, but the intention of this PR was to move it to new import/exports and remove the old es5 code hacks. If there are no objections i would like to merge this tomorrow and move to the class/function discussion |
I can review later |
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 still disapprove of limitDimsByArea
return type.
Think of a dev subclassing caching.
But let's move on.
General To Do
- cache can become standalone exports instead of a class once we make everything a map
- add
onStart
to animate - since it complementsdelay
sendPointRelativeToCanvas
- rename tosendPointToWindow
andsendPointToCanvas
? should utilizegetPointer
code from canvas.
src/util/animate.ts
Outdated
})(start); | ||
}; | ||
|
||
setTimeout(() => requestAnimFrame(runner), delay); |
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 like this change since it makes all animations start in delay even if delay is undefined.
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 0 by default is not undefined !
i thought the code semplification was worth a setTimeout 0
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 didn't notice that.
I am not sure is worth...
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 change can be reverted to:
if (delay > 0 ) {
setTimeout(() => requestAnimFrame(runner), delay);
} else {
requestAnimFrame(runner)
}
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.
undefined
should be not setTimeout
0
we need to decide if behaves as undefined
or as setTimeout
0 does
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 removed delay undefined. delay is a number, it will be 0 or a number bigger than 0.
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.
Great, even just if(delay)
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 get why >0
.
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 has been changed back to the more verbose version
* @param {Function} callback Callback to invoke | ||
* @param {DOMElement} element optional Element to associate with animation | ||
*/ | ||
export function requestAnimFrame(...args) { |
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.
isn't args just [callback]?
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, but since the function is just a passtrough for some reason, i keep it generic
return _requestAnimFrame.apply(fabric.window, args); | ||
} | ||
|
||
export function cancelAnimFrame(...args) { |
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.
same
b = c1; | ||
} | ||
// `b` becomes `a`'s clip path so we transform `b` to `a` coordinate plane | ||
applyTransformToObject( |
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.
It must be.
That is exactly what the function does.
We return to it later
It should do that but it doesn't account for canvas offset and it should |
This reverts commit 5ce2a09.
@ShaMan123 i reverted your commit. this is sendObjectToPlane: const t = multiplyTransformMatrices(invertTransform(to), from);
applyTransformToObject(
object,
multiplyTransformMatrices(t, object.calcOwnMatrix())
); it has obect.calcOwnMatrix as extra and 2 matrices to and from. 2 uses of multiplyTransformMatrices. the clipPath function has a multiplication less. applyTransformToObject(
b,
multiplyTransformMatrices(
invertTransform(a.calcTransformMatrix()),
b.calcTransformMatrix()
)
); I told you i have tried it and didn't work. |
That is why the following is correct. - sendObjectToPlane(b, b.calcTransformMatrix, a.calcTransformMatrix())
+ sendObjectToPlane(b, b.group?.calcTransformMatrix, a.calcTransformMatrix())
Taking b's containing plane matrix and multiplying with b's own matrix gives us |
I m not sure i get it. There are no window coordinates ever, we either use design coordinate ( your fabricjs canvas creation, or the htmlCanvas element coordinates ( the distance between the point and the left top HTMLCanvas corner )) |
I don't think so, i think you are inverting a multiplcation order. I ll write a failing unit test, and then you'll owe me a cocacola first time we ever meet. |
We are on! When will we meet and where? |
Let's agree that if we meet in Israel it will something much tastier. Accepted? |
I wrote |
If I win what happens? |
I don't like cocacola |
I added the test in master, as soon as this is merged you can verify if the swap works, and in case do it. |
Any food in budget for the loser is fine. |
Ok i didn't know or i didn't remember this. |
Good point |