isTargetTransparent() now samples the cache directly. #4955
Conversation
AndrewJDR
force-pushed
the
isTargetTransparent_direct_from_cache
branch
2 times, most recently
from
48323cf
to
1047898
Compare
src/canvas.class.js
Outdated
| @@ -492,6 +492,20 @@ | |||
| * @return {Boolean} | |||
| */ | |||
| isTargetTransparent: function (target, x, y) { | |||
| if (target.shouldCache && target._cacheCanvas && !target.isCacheDirty()) { | |||
There was a problem hiding this comment.
the logic that tells us if the target is on its own cache canvas is a little bit more complex ( i have to verify )
There was a problem hiding this comment.
Ah ok. I've now noticed a canvas/path that this code doesn't properly handle, so I'm wondering if it's due to not checking its cache canvas properly or something.
src/canvas.class.js
Outdated
| curRelTargetY = y - center.y; | ||
|
|
||
| curRelTargetX = (target._cacheCanvas.width / 2) + (curRelTargetX / target.zoomX); | ||
| curRelTargetY = (target._cacheCanvas.height / 2) + (curRelTargetY / target.zoomY); |
There was a problem hiding this comment.
for blurriness reasons more than width/2 there is another property where we can see the center of the obect on the canvas, i think is called cacheTranslateX/Y or something like that, you find it in UpdateCacheCanvas function.
Also the translation respect the center should take care of the object rotation using the matrices directly.
There was a problem hiding this comment.
Okay, I tried using cacheTranslateX/Y in various ways, but I was getting weird results. I'll give it another try though.
AndrewJDR
force-pushed
the
isTargetTransparent_direct_from_cache
branch
2 times, most recently
from
e2ccc43
to
f89a9b6
Compare
Previously, isTargetTransparent would do a paint from the cache to the contextCache which has more overhead and is slower. With this commit, fabricjs will test the cache canvas directly using x/y coordinates calculated with respect to the target object center. This is more performant since it avoids an extra render on every call to isTargetTransparent.
AndrewJDR
force-pushed
the
isTargetTransparent_direct_from_cache
branch
from
f89a9b6
to
3665449
Compare
src/canvas.class.js
Outdated
| @@ -492,6 +492,17 @@ | |||
| * @return {Boolean} | |||
| */ | |||
| isTargetTransparent: function (target, x, y) { | |||
| if (target.shouldCache() && target._cacheCanvas && !target.isCacheDirty()) { | |||
There was a problem hiding this comment.
shouldCache() returns the result of this.objectCaching && (!this.group || this.needsItsOwnCache() || !this.group.isOnACache()). Does this seem sufficient?
src/canvas.class.js
Outdated
| @@ -492,6 +492,17 @@ | |||
| * @return {Boolean} | |||
| */ | |||
| isTargetTransparent: function (target, x, y) { | |||
| if (target.shouldCache() && target._cacheCanvas && !target.isCacheDirty()) { | |||
| var normalizedPointer = this._normalizePointer(target, {x: x, y: y}), | |||
There was a problem hiding this comment.
Changed to use _normalizePointer() here, which uses the xform matrix so it handles rotation etc.
|
Updated, accounted for rotations and it also seems to work with retina scaling enabled, now. |
|
I m a little bit busy and i have to wrap up 2 things and release 2.2.4, but i l check this next. |
|
it looks good anyway |
|
i think is good, i m gonna merge it. |
* isTargetTransparent() now samples the cache directly. Previously, isTargetTransparent would do a paint from the cache to the contextCache which has more overhead and is slower. With this commit, fabricjs will test the cache canvas directly using x/y coordinates calculated with respect to the target object center. This is more performant since it avoids an extra render on every call to isTargetTransparent. * remove cacheDirty that should not be needed.
This is a work in progress. It seems to work for the limited things I've tried it with -- mostly hit-testing paths. I know that it doesn't work on apple platforms where retina scaling is enabled, so I'd like some advice on how to get that working properly, and any other feedback/notes.
For example, I check for cache validity with the following:
Is there a better way to be doing this, or other things to consider checking about the state of the cache?
And finally, does the math here correct?
Thank you!
Previously, isTargetTransparent would do a paint from the cache to the contextCache which has more overhead and is slower. With this commit, fabricjs will test the cache canvas directly using x/y coordinates calculated with respect to the target object center. This is more performant since it avoids an extra render
on every call to isTargetTransparent.