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

feat(eraser_brush): erasable=deep #7100

Merged
merged 181 commits into from
Jul 26, 2021

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jun 4, 2021

Changes

This PR introduces fine grained control over a group's erasable property.
Added erasable property value: deep, erasable now accepts boolean | 'deep'.

The deep option will erase nested objects in a group if they are erasable,
true will erase the entire group (once the group changes the eraser is propagated to its children for proper functionality),
false will leave all objects including the group untouched.

Fixes

  1. Erasing huge groups like the result of SprayBrush or CircleBrush.
  2. Eraser behavior in a group after the group has changed (object added/removed). Because the group's dimensions change after add/remove the eraser must be handled or else it will be transformed incorrectly according to the group's previous bounding box. To handle this, before the change is applied to the group the eraser is propagated to the relevant objects and is removed from the group. This happens when the group's erasable property is set to true.
  3. Object resizing is now supported out of the box, including canvas resizing.

Test It

https://codesandbox.io/s/exciting-architecture-bcgej?file=/src/App.tsx

Motivation

@ShaMan123

fabricjs/fabricjs.com#306

Thank you!
However, It seems to be incompatible with the SprayBrush.
After spraying, I used an eraser and my computer's memory boiled.
https://codepen.io/matsu-motsu/pen/xxqWjdZ

Originally posted by @matsu-motsu in #6994 (comment)

Context

Behavior before this PR

Once erasing started EraserBrush traversed through all objects, nested or not to render the layers for erasing.
Once erasing had completed (mouseup) all objects were iterated through, including objects that were nested in groups. If an object was erasable a path would have been added to it's eraser.
EraserBrush didn't add erasing paths to groups to enable a group to have both erasable and non erasable objects.

In the case of SprayBrush, what happened was:

  1. Rendering the layers for erasing was extremely heavy because it iterated through all the dots, unnecessarily.
  2. When erasing had completed an erasing path was created for each dot of the spray, resulting in a huge and extremely heavy operation.

This behavior is exactly what the deep option does and is undesirable as default behavior as shown above.
So now, with this PR, a single path will be added only to the group's eraser as long as erasable=true and that's it.


This tests object and canvas resizing with eraser:
https://codesandbox.io/s/romantic-williams-fwbz9?file=/src/App.tsx
Erase the blue overlay and then change the canvas size by dragging the window borders. Notice that the eraser is now sized automatically; the red rect changes size while it's eraser stays in place.

Originally posted by @ShaMan123 in #7100 (comment)


I have come across an edge case.
I am interested in your feedback.
Say there's a group with a clip path. When erasing the group the eraser path will be clipped properly.
Once the eraser paths propagate to child objects the group clip path will not. This will make nested erasing with nested clipping hard to handle from the event path:created |before:path:created.
How should we address this?
It's easy enough to check if the parent group of an object has a clip path and if it does to apply it to the object. But what object nested objects in nested groups with clip paths?

SOLVED 82a3d7d -> e2c6774
https://codesandbox.io/s/angry-browser-49sc3?file=/src/EraserBrush.js

Originally posted by @ShaMan123 in #7100 (comment)

ShaMan123 added 30 commits April 7, 2021 01:30
This reverts commit 02f3360.
This reverts commit 196867a.
This fixes eraser being positioned against current object transform instead of initial object transform
This reverts commit cce4ac1.
handle transform in dedicated clip path group instead of in canvas
Only erasable objects will be visibly affected by the eraser brush.
       * In order to support selective erasing all non erasable objects are rendered on the main ctx
       * while the entire canvas is rendered on the top ctx.
       * When erasing occurs, the path clips the top ctx and reveals the main ctx.
       * This achieves the desired effect of seeming to erase only erasable objects.
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jun 26, 2021

Updated CHANGELOG
I want to move forward with this PR

@ShaMan123 ShaMan123 requested a review from asturur July 11, 2021 18:50
@ShaMan123
Copy link
Contributor Author

@asturur This PR fixes a major bug and adds a major feature. It is sad seeing it going stale.
Could you reply on the open matters so we can agree on how to finalize it?

@stale
Copy link

stale bot commented Jul 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jul 26, 2021
@stale stale bot removed the stale Issue marked as stale by the stale bot label Jul 26, 2021
@asturur
Copy link
Member

asturur commented Jul 26, 2021

This is fine, i have no time arguing over the deep/erasable naming.

@asturur asturur merged commit 6a69cf7 into fabricjs:master Jul 26, 2021
@ShaMan123
Copy link
Contributor Author

Thanks!
I didn't mean to argue. Just want this to be a good feature that both you and I are happy about.

@ShaMan123 ShaMan123 deleted the group-erasable-option branch July 27, 2021 04:22
@asturur
Copy link
Member

asturur commented Jul 27, 2021

Argue is the wrong word, I apologize. I just meant is not a detail to which i can dedicate time nor stop you with.

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jul 27, 2021 via email

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

Successfully merging this pull request may close these issues.

2 participants