-
-
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(): optimize perPixelTargetFind
#8770
Conversation
optimize
Build Stats
|
perPixelTargetFind
perPixelTargetFind
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.
Seems ready
Could you help me benchmark the change?
@ShaMan123 I'll gladly have a look personally in the week-end. Do you have any documentation about how to setup the fabric repo locally? What are the steps to open the project that you have used in your screenshots in localhost? Regarding how to analyse the performance, you typically need to record actions that trigger the method a lot Once you select a method in the flame graph, you can also click on the bottom to jump to its file and see the timings of each line. There you can also have a quick evaluation of where most of the time is spent and see if there are significant improvements. This is an example of what I did last week, where I replaced the dynamically created styles declaration with a static one which is significantly more performant for V8 because it has a static shape (we can talk about stuff like this that I noticed in fabric in another time) BeforeAfter |
Interesting! Will take a look later. Thanks @ShaMan123 |
src/canvas/SelectableCanvas.ts
Outdated
// in case the target is the activeObject, we cannot execute this optimization | ||
// because we need to draw controls too. | ||
if (isFabricObjectCached(target) && target !== this._activeObject) { | ||
// optimizatio: we can reuse the cache | ||
if ( |
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 happened that i thought a couple of times of this:
// in case the target is the activeObject, we cannot execute this optimization
// because we need to draw controls too.
And then i asked myself, why do i have to draw the controls?
The controls can be found with the coordinates.
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.
absolutely
I had no idea what was that.
And I think active selection never caches, or at least it didn't in v5. Now I don't remember.
src/canvas/SelectableCanvas.ts
Outdated
@@ -667,10 +667,23 @@ export class SelectableCanvas< | |||
* @return {Boolean} | |||
*/ | |||
isTargetTransparent(target: FabricObject, x: number, y: number): boolean { | |||
const ctx = this.contextCache; | |||
const retina = this.getRetinaScaling(); | |||
const size = Math.max(2 * this.targetFindTolerance * retina, 10); |
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 would you max it out?
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 would rather have a method setTargetFindTolerance
that creates this mini canvas, i m not even sure retina scaling is a requirement since the mouse pixels ( the so called css pixels ) are always without retina.
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 would rather have a method
setTargetFindTolerance
that creates this mini canvas,
will do
I wasn't sure about retina. What will happen in case of ay scaled and blurry image? Will the hit test return a false positive?
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 removed the max value of 10
Truth be told IMO this method doesn't belong to canvas. It is a standalone util. The canvas element can be a const and we could even make it offscreen I believe. |
target.render(ctx); | ||
target.selectionBackgroundColor = selectionBgc; |
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.
to be honest i think selectionBackgroundColor
is just a terrible feature and that it could be deleted, but we shouldn't do it here.
Is a property that pertain the control bounding box and was added to the objects because rush/inexperience
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 met it redoing controls. Managed to get it working there but I agree it is weird because it seems not part of object rendering scope, same as controls, but because it needs to be behind and object stacking I understand why you did it.
src/util/misc/isTransparent.ts
Outdated
tolerance = Math.ceil(Math.max(tolerance, 0)); | ||
const point = new Point(x, y); | ||
const start = point.scalarSubtract(tolerance).floor(); | ||
const end = point | ||
.scalarAdd(Math.max(tolerance, 1)) | ||
.ceil() | ||
.min(new Point(ctx.canvas.width - 1, ctx.canvas.height - 1)); | ||
const boundStart = start.max(new Point()); | ||
const size = end.subtract(boundStart); | ||
if (size.x <= 0 || size.y <= 0) { | ||
// out of bounds | ||
return 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.
i notice now this change, i don't think we should do it. the old loop is so much clear and smaller.
There is something weird in the old code and is that if we set x or y to 0 we do not adjust the tolerance to avoid considering extra pixels, but this change here is definitely extra complication.
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 will try to make simple but seems to me there are edge cases in the logic.
I can't understand the logic
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.
please try to explain it
under group will need more handling ad is such a headache and probably not giving anything because `render` is optimized to use the cache so no reason for that
I think the cache canvas optimization ( reusing the cache ) is still ok and usable for top level obects at least. |
It would be also ok to remove the cacheCanvas optimization if we can see that for already cached object the change in performance is minimal ( maybe is 0 ) since with interactive groups lot of things could be different. |
my code is here |
Let's try tomorrow to finish this. |
src/canvas/SelectableCanvas.ts
Outdated
this.targetFindTolerance, | ||
this.targetFindTolerance, | ||
this.targetFindTolerance |
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 this again shouldn't it be multiplied by retina?
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.
in my branch i completely removed retina from the logic of target transparent.
I don't think it should be involved at all.
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 it is needed for precision in case of blur
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.
considering retina makes the cached/noncached case very similar and cut differences there.
But force us to continuosly round numbers
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.
That isn't too bad, is it?
I am thinking that this feature is used with text and text is heavily impacted by retina
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 it will win the smaller code at the end.
traget transparency isn't meant to be a pixel sniper effect. It has just to guess if is completely transparent or not.
Is not very easy to benchmark this, but i see like improvents here. Master is so higher up right now, sometimes it get the full 16ms to get back a result, and i also notice we run findTarget twice when not needed ( i ll open a PR for that ) |
src/canvas/SelectableCanvas.ts
Outdated
setTargetFindTolerance(value: number) { | ||
this.targetFindTolerance = value; | ||
const size = Math.ceil( | ||
2 * Math.max(this.targetFindTolerance, 1) * this.getRetinaScaling() | ||
); | ||
if (this.pixelFindCanvasEl.width < size) { | ||
this.pixelFindCanvasEl.width = this.pixelFindCanvasEl.height = size; | ||
const retina = this.getRetinaScaling(); | ||
this.pixelFindContext.scale(retina, retina); | ||
} |
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.
after playing around with this branch a bit and the feature, i think this should be slightly different:
setTargetFindTolerance(value: number) { | |
this.targetFindTolerance = value; | |
const size = Math.ceil( | |
2 * Math.max(this.targetFindTolerance, 1) * this.getRetinaScaling() | |
); | |
if (this.pixelFindCanvasEl.width < size) { | |
this.pixelFindCanvasEl.width = this.pixelFindCanvasEl.height = size; | |
const retina = this.getRetinaScaling(); | |
this.pixelFindContext.scale(retina, retina); | |
} | |
setTargetFindTolerance(value: number) { | |
value = Math.floor(value); // floats aren't supported. but just in case get rid of them | |
this.targetFindTolerance = value; | |
const size = 2 * value + 1; | |
this.pixelFindCanvasEl.width = this.pixelFindCanvasEl.height = size; | |
} |
notice that size is 2 * tolerance + 1. 1 is the pixel that we are hoverin and tolerance 3 for example, would mean we always take 3 pixels each side in addition to the original pixel.
So the pixelTargetFind canvas is always an odd number of pixels ( 1,3,5,7... )
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.
sounds correct
src/util/misc/isTransparent.ts
Outdated
const { data } = ctx.getImageData( | ||
x, | ||
y, | ||
tolerance * 2 || 1, | ||
tolerance * 2 || 1 | ||
); |
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.
const { data } = ctx.getImageData( | |
x, | |
y, | |
tolerance * 2 || 1, | |
tolerance * 2 || 1 | |
); | |
const { data } = ctx.getImageData( | |
x - tolerance, | |
y - tolerance, | |
tolerance * 2 + 1, | |
tolerance * 2 + 1, | |
); |
If we write this in this way we can get rid of the if condition above.
Reading getImageData out of bounds seems in spec
The getImageData(sx, sy, sw, sh, settings) method steps are:
If either the sw or sh arguments are zero, then throw an ["IndexSizeError"](https://webidl.spec.whatwg.org/#indexsizeerror) [DOMException](https://webidl.spec.whatwg.org/#dfn-DOMException).
If the [CanvasRenderingContext2D](https://html.spec.whatwg.org/multipage/canvas.html#canvasrenderingcontext2d)'s [origin-clean](https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-origin-clean) flag is set to false, then throw a ["SecurityError"](https://webidl.spec.whatwg.org/#securityerror) [DOMException](https://webidl.spec.whatwg.org/#dfn-DOMException).
Let imageData be a [new](https://webidl.spec.whatwg.org/#new) [ImageData](https://html.spec.whatwg.org/multipage/canvas.html#imagedata) object.
[Initialize](https://html.spec.whatwg.org/multipage/canvas.html#initialize-an-imagedata-object) imageData given sw, sh, [settings](https://html.spec.whatwg.org/multipage/canvas.html#initialize-imagedata-settings) set to settings, and [defaultColorSpace](https://html.spec.whatwg.org/multipage/canvas.html#initialize-imagedata-defaultcolorspace) set to [this](https://webidl.spec.whatwg.org/#this)'s [color space](https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-color-space).
Let the source rectangle be the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx, sy+sh).
Set the pixel values of imageData to be the pixels of [this](https://webidl.spec.whatwg.org/#this)'s [output bitmap](https://html.spec.whatwg.org/multipage/canvas.html#output-bitmap) in the area specified by the source rectangle in the bitmap's coordinate space units, converted from [this](https://webidl.spec.whatwg.org/#this)'s [color space](https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-color-space) to imageData's [colorSpace](https://html.spec.whatwg.org/multipage/canvas.html#dom-imagedata-colorspace) using ['relative-colorimetric'](https://drafts.csswg.org/css-color/#valdef-color-profile-rendering-intent-relative-colorimetric) rendering intent.
Set the pixels values of imageData for areas of the source rectangle that are outside of the [output bitmap](https://html.spec.whatwg.org/multipage/canvas.html#output-bitmap) to [transparent black](https://drafts.csswg.org/css-color/#transparent-black).
Return imageData.
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.
document.createElement('canvas').getContext('2d').getImageData(0,0,1,1)
returns [0,0,0,0] though IMO it should return null
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 that is fine 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.
Set the pixels values of imageData for areas of the source rectangle that are outside of the
OK I see
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.
getImageData(0,0,1,1) should give you a pixel correct? why would you have null?
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 the spec is confusing. I am asking for a non existing pixel and I get one. But for our sake it is fine since it is transparent
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.
oh i see what you mean.
I think is a point of view. Having always pixels is definitely more ergonomic.
Of course if you are hunting for transparent pixels and you go accidentally out of bounds, you are confused.
@ShaMan123 keep this branch steady one moment i m going to fork again and open a PR on your PR. with some changes ( don't want to commit directly or discussing changes is a mess ) |
As far as I am concerned you have taken over this PR and are in charge |
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.
Looks great, unit tests seem to prove accuracy has been reached
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 we should a test under a transformed group
Ok i ll merge now. |
Kind of we forgot that now you can't read how much tolerance you set. |
You mean the enhanced value? I wonder about this bundle size. Is it really accurate? We dropped 50 lines of code and didn't add anything that is compiled down. So what is this data? Whatever @asturur we made it better. |
Motivation
Making perPixelTargetFind faster
Description
limited the canvas size
view the diff w/o ws
Changes
renamed
cacheCanvasEl
=>pixelFindCanvasEl
because it is no longer a generic cache canvasGist
I don't really know how to benchmark this.
Before
After
In Action