-
-
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
Conversation
Build Stats
|
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 made Group#interactive
include subTargetCheck
logically so now it is enough to
new Group(objects, { interactive: true })
This will not change anything else.
There is a decision to make and then I can complete this PR
// hoverCursor should come from top-most subTarget, | ||
// so we walk the array backwards |
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.
wrong
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.
What is wrong exactly?
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 order of targets is [topMost, ..., bottomMost]
?
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.
Sorted in descending z index order, top most object first
src/canvas/SelectableCanvas.ts
Outdated
@@ -190,6 +190,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents> | |||
|
|||
/** | |||
* Keep track of the subTargets for Mouse Events | |||
* Sorted in descending z index order, top most object first |
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.
right
@@ -101,6 +106,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 comment
The reason will be displayed to describe this comment to others. Learn more.
needs a mounted decision
@@ -511,7 +511,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 comment
The reason will be displayed to describe this comment to others. Learn more.
bad approach, leading to unnecessary computation and a stale state
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I wrote about this in the description, sorry, updated now.
Another change I have introduced is
interactive
includingsubTargetCheck
.
To be able to select sub targets both need to be flagged as true.
subTargetCheck
flagssearchPossibleTargets
to traverse the children of a group.
interactive
flags to return the top most sub target found withinsearchPossibleTargets
.
So essentiallyinteractive = true
containssubTargetCheck = true
.
It is more straight forward as well.
for (let i = 0; i < len; i++) { | ||
const object = this._objects[i]; | ||
object.group || object.setCoords(); | ||
} | ||
if (backgroundObject) { | ||
backgroundObject.setCoords(); | ||
} | ||
if (overlayObject) { | ||
overlayObject.setCoords(); | ||
} |
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 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 comment
The 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 comment
The 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 comment
The 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 setActiveObject
(which I believe should call setCoords
if it doesn't) thus making the setCoords
call redundant
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, the active object calls setCoords, the rest should not. Mind that setActiveObject
call setCoords so it seals the gap.
if (this._forceClearCache) { | ||
this.initDimensions(); | ||
this.setCoords(); | ||
} |
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 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 comment
The 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 comment
The 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.
This render method should take in account that if _forceClearCache is true, should execute the initDimension and then move to super.render. the precheck is not useful, is a double cost 99.9% of the time
@@ -248,6 +250,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 comment
The 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.
We should document that if you have a group without subtargetCheck and you plan on using the group inner object coordinates, you have to update them when you use them.
58f98fe
to
4a2a3a7
Compare
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.
rebased
@@ -216,6 +216,8 @@ export class LayoutManager { | |||
target.setPositionByOrigin(nextCenter, CENTER, CENTER); | |||
// invalidate | |||
target.setCoords(); | |||
(target.subTargetCheck || target.interactive) && | |||
target.forEachObject((object) => object.setCoords()); |
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
to setCornerCoords
and setControlCoords
because here we need only setCornerCoords
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
setCoords() { | ||
super.setCoords(); | ||
this._shouldSetNestedCoords() && | ||
this.forEachObject((object) => object.setCoords()); | ||
} |
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.
@jiayihu look here, this removes the perf hit you described
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.
All in all this looks like an important PR
4a2a3a7
to
ec6b63e
Compare
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.
rebased and waiting for response before fixing some issues
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 comment
The reason will be displayed to describe this comment to others. Learn more.
should it be:
(target as Group).subTargetCheck && | |
(target as Group).interactive && |
?
2747199
to
b9ba21b
Compare
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.
cleaned up a small bit
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 comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't belong here
my biggest issue with this PR and the reason why i left it here is because it says it is for performance but never gave a figure of the performance it was improving. |
I also want to do some retrospect on how we got here. |
if (overlayObject) { | ||
overlayObject.setCoords(); | ||
} | ||
this.viewportTransform = [...vpt]; |
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 wasn't spread before, it shouldn't be spread 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.
Are you sure?
When we init the canvas we spread to avoid mutation, why not 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 unrelated to what we are doing here. no?
I suggested @ShaMan123 we should add a benchmark test that simply renders a Group with 100 Groups, with 2 children per group. Then measure how much times it takes and how much this PR affects it. Also it will avoid future regressions.
It was there since v5.3 but not on v3.6 (which we were using at my company before migrating to v6) but it's definitely suspicious to call setCoords during drawing. Maybe it was added to workaround stale coords without minding about performance. |
@asturur this discussion can lead to how fabric uses |
Was added in 4.0 when the ability to call render controls with styleoverrides was added. |
Just keep in mind isOnScreen was used to skip some rendering for first level objects. That was all, there aren't more intention behind it. I m interested to see why is bad, that yes. |
Bear in mind that it's not only about nested oCoords. It still does it for all the canvas objects, which is very expensive. Internally we have changed it to be done only for the needed objects, typically the activeObject. |
If we are doing all objects 'setCoords' every render then we introduced a bug. That was never the intention, again setCoords was something that was called on setViewport and at the end of a transform. Anyway this PR won't be breaking, so we can move forward with a release. |
i also see that calcOcoords with the nested cords for groups interactivity became way slower in the case of groups. so that is also probaly a cause of general slowness When doing setCoords on a group that isn't interactive, setting the oCoords of all the nested objects is a waste, and if the group is interactive but selected, is a waste anyway. But setcoords is imperative, maybe having it going deeply nested was an error because most of the time aCoords do not change and that is all we need for targeting check. But that is topic for a different discussion/pr |
I was surprised to see that all the above are not as expected. Calling setCoords when drawing controls, not calling them otherwise |
The gotcha always said, if something is misaligned doesn't work call setCoords. |
That is what the gotcha says but the code does something entirely different so the gotcha is unrelated. |
@ShaMan123 is not unrelated, things changed pr after pr probably. Also today is still on '_finalizeCurrentTransform' so that aspect is still there, not sure why you say it doesn't |
A red flag i see in this PR is that we remove the nested call of group setCoords done with the subclass and then we comment in 2 places of added code that the call should be recursive, but we just removed the recursiveness from the code. The only changes that are 100% correct here is the one from viewport and then moving the setCoord from the drawControl call earlier to the performAction could be correct without much doubts too. Where are the performance improvement coming from? |
Only during a transform, if for other reasons you need to render the canvas this is better.
agreed Regarding the recursive setCoords call of group. Do we need it? |
This makes my suggestion to delete aCoords/oCoords relevant. If they do not exist they could be recalculated on the fly from |
Then you'd get worse performance having to recompute them everytime, for instance for |
Why? We delete them only if an ancestors invalidates geometry |
my suggestion is to take the no brainers from this PR and to think better what to do for the rest. Do we need to have oCoords cached? at this point i do not know. But of course if you know you are using an override where you can click a control also when the object is not selected, then things change. |
Great point! |
closing in favor of #9793 |
Description
After removing lineCoords we have 2 sets of coords
Corner coords (
aCoords
) used for object geometry (isOnScreen
, intersection etc.) that belong to the parent plane,getCoords
returning them in the scene plane.Control coords (
oCoords
) used for control rendering/hit detection that belong to the viewport plane.This distinction is very important and useful.
It means that viewport changes do not affect corner coords.
Also since they belong to the parent plane they need to be recalculated only if the object itself changes its own transform/size. This means that if an ancestor changes transform we do not need to recalculate the corner coords.
Since control coords exist only on the active object it seems important to separate use cases of
setCoords
to reduce computation.I have found that fabric used to call
setCoords
before drawing the controls (at the end of the rendering cycle) instead of after a transform action. This led to a stale coords state between the 2 calls affecting apps using corner controls in the rendering cycle. It was always a frame behind. This is fixed now.Looking into
setCoords
in general it seems we should decide when they are first set. It is ambiguous.Does it happen when an object mounts the canvas? What about an object added to a group? We can check if the group has a canvas set, therefore it is mounted , therefore
setCoords
should be called.It also means that object geometry should not be used until an object is mounted.
This PR is part of a performance effort I am looking into.
I have a ground breaking idea about a binary index that will reduce computation cost of
isOnScreen
exponentially.I wish to contribute it to the repo but can't wait with it more than a number of days so if it can't merge soon I will depart from fabric (at least with this) or try to expose it as a standalone but that will be a shame.
Another change I have introduced is
interactive
includingsubTargetCheck
when true.To be able to select sub targets both need to be flagged as true.
subTargetCheck
flagssearchPossibleTargets
to traverse the children of a group.interactive
flags to return the top most sub target found withinsearchPossibleTargets
.So essentially
interactive = true
containssubTargetCheck = true
.It is more straight forward as well.
In Action