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(BaseBrush): clipPath #7175

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

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jul 1, 2021

Overview

This PR adds a clipPath property to all brushes, aligning them with fabric.Object.
It BREAKS SprayBrush, it can be avoided with different naming if necessary.

Motivation

closes #7134
closes #6955
closes #5431

Demo

https://codesandbox.io/s/upbeat-microservice-mkbzs?file=/src/App.tsx
Edit fabric_app (forked)

Logic

  • Added a clipPath property to BaseBrush and exposed clip path methods
  • Exposed a common render + _render method on BaseBrush which conforms to Object#render and is in charge of:
    1. Preparing the context (transformation etc.)
    2. calling _render which is implemented by each brush
    3. render clipPath
    4. cleanup
  • Made all subclasses use _render instead of _render - BREAKING SprayBrush to make it compatible with all brushes (renamed it's previous render method to renderChunck), renamed EraserBrush render method to renderAll
  • Added clip path support to segment rendering
  • Added logic to apply the clip path to the resulting object created by the brush

To Do

Watch

Pay no attention to eraser bug. It's fixed by the group PRs.
ezgif com-gif-maker (3)

With Eraser patched up
ezgif com-gif-maker (4)

image

absolutePositioned + viewportTransform
ezgif com-gif-maker (6)

@asturur
Copy link
Member

asturur commented Jul 3, 2021

i do not think we should do that.
The brush should just respect the canvas clipPath if any, the issue is that rendering the topLayer, the contextTop is currently unmanaged, and brush, group selection, text selection, all fight for it.

We should work toward having a clear order of operation there and adding the clipPath there if necessary.

@ShaMan123
Copy link
Contributor Author

Did you try the demo?
What about a use case that you need the draw over parts of the canvas. You can't achieve this right now. Say you want to draw only over some objects.
Ohh you mean that if I use active selection it will be clipped..... I get you. What if we render a cache canvas to workaround this issue?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jul 3, 2021

Thinking again it can't happen. Because once mouseup the brush clears the top context and creates a Path object meaning it won't collide with group/text selection.

@ShaMan123
Copy link
Contributor Author

Isn't canvas clip path applied to all of it? That is not the use case described or the motivation of this PR

@ShaMan123
Copy link
Contributor Author

I think this is a cool feature.
I'll put it at hold for now.

@asturur
Copy link
Member

asturur commented Jul 16, 2021

@ShaMan123 this is a cool feature, yes.
But i want to redo the contextTop or topLayer entirely.
This code will create a feature that will get in the way of reorganizaing.
In the top layer we need to be able to handle:

  • controls
  • group selection
  • brush creation ( not always because for example the eraser brush needs to be in the main canvas )
  • eventually small custom stuff for the developers

brush themselves need to be changed in something that is more similar to custom controls, one class and callbacks for your own rendering.

@ShaMan123
Copy link
Contributor Author

@asturur what about this?
Want this in? I think it's great. Simply need to address the eraser and then the feature is supported by all brush interactions

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated from master.
Suitable for v6.
Eraser supported (needs group PRs to merge into master to function properly)
Something is off with Eraser. I don't know if it's eraser or the clip path.
Need to check deeper.

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge!

@ShaMan123 ShaMan123 requested a review from asturur May 12, 2022 06:47
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed absolutePositioned
Build fails because of async usage.
Visual tests fail but after examining myself I don't see any visual difference.

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated from master, resolved conflicts and tested

@ShaMan123 ShaMan123 added the stale Issue marked as stale by the stale bot label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature stale Issue marked as stale by the stale bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clipPath for canvas ignores free drawing Adding Freehand with ClipPath to Object Group
2 participants