Skip to content
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

Context isolation #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Context isolation #6

wants to merge 6 commits into from

Conversation

ShaMan123
Copy link
Owner

Description

POC to drop object caching logic in favor of a better rendering approach

Ports parts of fabricjs#8298

Object caching and context isolation are 2 different things that were smashed together.
Object caching is a performance optimization targeting apparently panning and transforming (zooming invalidates the cache)
Context isolation is a necessity for supporting rendering operations in an object tree such as clipping a single object. Without it the operation is applied to the entire context, which is not desired.
Currently object caching covers most of the needs of a simple app in terms of context isolation. However if you move out of lines fabric's output is completely broken. e.g. shadows under a group with a clip path.
Also object caching has the blur issue because a clip path is unaware of it's parent, making the result of getTotalObjectScaling wrong always and the cache size off. That should be fixed regardless and of course I have a fix for it in fabricjs#8298 .
Add to that a dev setting object caching to true/false and discovering to their surprise that fabric didn't comply and decided by itself what to do. This is the essence of the confusion and mixing of these 2 concepts.
Another issue I have been looking into is objects that are partially on screen. If they hold a cache it might be extremely wasteful instead of just rendering their visible part.

In this PR I aim to make the rendering logic simpler following a simple rule: Recursion must happen in a single method calling itself rather than a number of methods calling each other as it is currently.

  • exposed renderInIsolation which renders the object at its final transform onto an isolated context and then draws that image onto the main context. For simplicity of the POC I didn't try (yet) to optimize the isolated context size.

  • renamed needsItOwnCache to requiresContextIsolation

  • disabled caching altogether - it can be reinstated if we want or completely refactored to use the isolation output. I think as a first step we should extract all the caching logic to a standalone so it is not on the Object class. If we understand it is a perf opt then it should not remain there. Half the code on Object is related to caching.

This is a big step towards being able to invalidate a region in the canvas.

In Action

trim1.mov
trim2.mov

Copy link

github-actions bot commented May 7, 2024

Build Stats

file / KB (diff) bundled minified
fabric 916.256 (-0.616) 306.202 (+0.547)

@xmejsieoke
Copy link

@ShaMan123 I've tried, but my clipPath doesn't behave as expected compared to before.
image
There is a circle and a rectangle on the canvas. The circle is at [0,0], and a part of the rectangle covers the circle. When I set the rectangle's clipPath to the circle, it behaves as if the circle is drawn from the center of the rectangle, instead of being clipped to the edge of the rectangle as expected.
Isolation:
image
Before:
image

@ShaMan123
Copy link
Owner Author

I have abandoned fabric in favor of my own stuff. One of which is a powerful renderer.
Seems what you describe is a simple fix caused by a wrong origin.

@xmejsieoke
Copy link

Does this line cause the previous transformations to become invalid, even if the matrix was inverted earlier?
image

@ShaMan123
Copy link
Owner Author

This line resets the context transform so that the image is drawn as is, without transformations.

@xmejsieoke
Copy link

I added the following transformations, and it solved my problem.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants