-
-
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
perf(): reduce setCoords calls #9550
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,6 +251,8 @@ export class LayoutManager { | |
target.setPositionByOrigin(nextCenter, CENTER, CENTER); | ||
// invalidate | ||
target.setCoords(); | ||
(target.subTargetCheck || target.interactive) && | ||
target.forEachObject((object) => object.setCoords()); | ||
target.set('dirty', true); | ||
} | ||
} | ||
|
@@ -286,6 +288,8 @@ export class LayoutManager { | |
left: object.left + offset.x, | ||
top: object.top + offset.y, | ||
}); | ||
// TODO: should we delete aCoords/oCoords here so they are not stale if subTargetCheck is false? | ||
// or should we apply offset instead of recalculating aCoords? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an object without coords could create more damages than one with stale, different typings, always have to check if the coords are there. |
||
} | ||
|
||
protected onAfterLayout( | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1304,19 +1304,16 @@ export class Canvas extends SelectableCanvas implements CanvasOptions { | |||||
transform: Transform, | ||||||
pointer: Point | ||||||
) { | ||||||
const x = pointer.x, | ||||||
y = pointer.y, | ||||||
action = transform.action, | ||||||
actionHandler = transform.actionHandler; | ||||||
let actionPerformed = false; | ||||||
// this object could be created from the function in the control handlers | ||||||
|
||||||
if (actionHandler) { | ||||||
actionPerformed = actionHandler(e, transform, x, y); | ||||||
} | ||||||
const { action, actionHandler, target } = transform; | ||||||
|
||||||
const actionPerformed = | ||||||
!!actionHandler && actionHandler(e, transform, pointer.x, pointer.y); | ||||||
// This call needs to recurse down | ||||||
actionPerformed && target.setCoords(); | ||||||
|
||||||
if (action === 'drag' && actionPerformed) { | ||||||
transform.target.isMoving = true; | ||||||
this.setCursor(transform.target.moveCursor || this.moveCursor); | ||||||
target.isMoving = true; | ||||||
this.setCursor(target.moveCursor || this.moveCursor); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't belong here |
||||||
} | ||||||
transform.actionPerformed = transform.actionPerformed || actionPerformed; | ||||||
} | ||||||
|
@@ -1332,7 +1329,6 @@ export class Canvas extends SelectableCanvas implements CanvasOptions { | |||||
this.setCursor(this.defaultCursor); | ||||||
return; | ||||||
} | ||||||
let hoverCursor = target.hoverCursor || this.hoverCursor; | ||||||
const activeSelection = isActiveSelection(this._activeObject) | ||||||
? this._activeObject | ||||||
: null, | ||||||
|
@@ -1345,17 +1341,11 @@ export class Canvas extends SelectableCanvas implements CanvasOptions { | |||||
target.findControl(this.getViewportPoint(e)); | ||||||
|
||||||
if (!corner) { | ||||||
if ((target as Group).subTargetCheck) { | ||||||
// hoverCursor should come from top-most subTarget, | ||||||
// so we walk the array backwards | ||||||
Comment on lines
-1349
to
-1350
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is wrong exactly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the order of targets is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
this.targets | ||||||
.concat() | ||||||
.reverse() | ||||||
.map((_target) => { | ||||||
hoverCursor = _target.hoverCursor || hoverCursor; | ||||||
}); | ||||||
} | ||||||
this.setCursor(hoverCursor); | ||||||
// hoverCursor should come from top-most subTarget | ||||||
const hoverCursor = | ||||||
(target as Group).subTargetCheck && | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it be:
Suggested change
? |
||||||
this.targets.find((target) => target.hoverCursor)?.hoverCursor; | ||||||
this.setCursor(hoverCursor || target.hoverCursor || this.hoverCursor); | ||||||
} else { | ||||||
const control = corner.control; | ||||||
this.setCursor(control.cursorStyleHandler(e, control, target)); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -836,7 +836,10 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents> | |
while (i--) { | ||
const target = objects[i]; | ||
if (this._checkTarget(target, pointer)) { | ||
if (isCollection(target) && target.subTargetCheck) { | ||
if ( | ||
isCollection(target) && | ||
(target.subTargetCheck || target.interactive) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. target interactive requires subtargetcheck, so we shouldn't be checking for both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought I wrote about this in the description, sorry, updated now.
|
||
) { | ||
const subTarget = this._searchPossibleTargets( | ||
target._objects as FabricObject[], | ||
pointer | ||
|
@@ -1148,9 +1151,10 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents> | |
|
||
if (isActiveSelection(object) && prevActiveObject !== object) { | ||
object.set('canvas', this); | ||
object.setCoords(); | ||
} | ||
|
||
object.setCoords(); | ||
|
||
return true; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,21 +379,7 @@ export class StaticCanvas< | |
* @param {Array} vpt a Canvas 2D API transform matrix | ||
*/ | ||
setViewportTransform(vpt: TMat2D) { | ||
const backgroundObject = this.backgroundImage, | ||
overlayObject = this.overlayImage, | ||
len = this._objects.length; | ||
|
||
this.viewportTransform = vpt; | ||
for (let i = 0; i < len; i++) { | ||
const object = this._objects[i]; | ||
object.group || object.setCoords(); | ||
} | ||
if (backgroundObject) { | ||
backgroundObject.setCoords(); | ||
} | ||
if (overlayObject) { | ||
overlayObject.setCoords(); | ||
} | ||
Comment on lines
-387
to
-396
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why deleting those? control coords need to be set when we change the vpt. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need control coords only for the active object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And regarding corner coords, we need them on these objects only if they do not ignore the vpt There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code we do not even check if these objects are on screen and these objects are not selectable apart from imperatively calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this was here because before we were selecting by lineCoords, while now we select by aCoords that doesn't change by the zoom or pan. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, the active object calls setCoords, the rest should not. Mind that |
||
this.viewportTransform = [...vpt]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this wasn't spread before, it shouldn't be spread now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is unrelated to what we are doing here. no? |
||
this.calcViewportBoundaries(); | ||
this.renderOnAddRemove && this.requestRenderAll(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ export interface ActiveSelectionOptions extends GroupProps { | |
const activeSelectionDefaultValues: Partial<TClassProperties<ActiveSelection>> = | ||
{ | ||
multiSelectionStacking: 'canvas-stacking', | ||
subTargetCheck: true, | ||
interactive: false, | ||
}; | ||
|
||
/** | ||
|
@@ -68,13 +70,6 @@ export class ActiveSelection extends Group { | |
}); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
_shouldSetNestedCoords() { | ||
return true; | ||
} | ||
|
||
/** | ||
* @private | ||
* @override we don't want the selection monitor to be active | ||
|
@@ -149,6 +144,7 @@ export class ActiveSelection extends Group { | |
// the object will be in the objects array of both the ActiveSelection and the Group | ||
// but referenced in the group's _activeObjects so that it won't be rendered twice. | ||
this._enterGroup(object, removeParentTransform); | ||
object.setCoords(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs a mounted decision |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,6 @@ export class Group | |
/** | ||
* Used to allow targeting of object inside groups. | ||
* set to true if you want to select an object inside a group.\ | ||
* **REQUIRES** `subTargetCheck` set to true | ||
* This will be not removed but slowly replaced with a method setInteractive | ||
* that will take care of enabling subTargetCheck and necessary object events. | ||
* There is too much attached to group interactivity to just be evaluated by a | ||
|
@@ -284,13 +283,6 @@ export class Group | |
return this; | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
_shouldSetNestedCoords() { | ||
return this.subTargetCheck; | ||
} | ||
|
||
/** | ||
* Remove all objects | ||
* @returns {FabricObject[]} removed objects | ||
|
@@ -365,7 +357,7 @@ export class Group | |
) | ||
); | ||
} | ||
this._shouldSetNestedCoords() && object.setCoords(); | ||
|
||
object._set('group', this); | ||
object._set('canvas', this.canvas); | ||
this._watchObject(true, object); | ||
|
@@ -412,7 +404,6 @@ export class Group | |
object.calcTransformMatrix() | ||
) | ||
); | ||
object.setCoords(); | ||
} | ||
this._watchObject(false, object); | ||
const index = | ||
|
@@ -489,16 +480,6 @@ export class Group | |
this._drawClipPath(ctx, this.clipPath); | ||
} | ||
|
||
/** | ||
* @override | ||
* @return {Boolean} | ||
*/ | ||
setCoords() { | ||
super.setCoords(); | ||
this._shouldSetNestedCoords() && | ||
this.forEachObject((object) => object.setCoords()); | ||
} | ||
Comment on lines
-496
to
-500
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jiayihu look here, this removes the perf hit you described |
||
|
||
triggerLayout(options: ImperativeLayoutOptions = {}) { | ||
this.layoutManager.performLayout({ | ||
target: this, | ||
|
@@ -689,7 +670,6 @@ export class Group | |
target: group, | ||
targets: group.getObjects(), | ||
}); | ||
group.setCoords(); | ||
return group; | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -534,7 +534,6 @@ export class InteractiveFabricObject< | |
ctx.strokeStyle = options.cornerStrokeColor; | ||
} | ||
this._setLineDash(ctx, options.cornerDashArray); | ||
this.setCoords(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad approach, leading to unnecessary computation and a stale state |
||
this.forEachControl((control, key) => { | ||
if (control.getVisibility(this, key)) { | ||
const p = this.oCoords[key]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -427,7 +427,6 @@ export class FabricText< | |
this.setPathInfo(); | ||
} | ||
this.initDimensions(); | ||
this.setCoords(); | ||
} | ||
|
||
/** | ||
|
@@ -1694,6 +1693,7 @@ export class FabricText< | |
} | ||
if (this._forceClearCache) { | ||
this.initDimensions(); | ||
this.setCoords(); | ||
} | ||
Comment on lines
1694
to
1697
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this whole thing the text has that can change the size just before rendering is wrong, is probably a patch that was done fore some reason, but this shouldn't exist. Render renders, period. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't have a suggestion but is also evident that if there is a dependency between having the correct dimensions and being detected on screen, so for correctness this check and setCoords shoud be called before the onScreen early return, and also Object will do the isOnscreen check again. |
||
super.render(ctx); | ||
} | ||
|
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.
here is a good example of the benefit of separating
setCoords
tosetCornerCoords
andsetControlCoords
because here we need onlysetCornerCoords
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.
Do you have objections doing that?
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.
Also this line is incorrect because it goes down 1 level but should recurse down