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

fix(EraserBrush): use rendered objects for pattern #7938

Merged
merged 1 commit into from
May 14, 2022

Conversation

ShaMan123
Copy link
Contributor

Previously the eraser brush used the _objects attribute of canvas to render necessary pattern for the erasing effect.
Fixed to use the _objectsToRender attribute. This way if objects are rendered in a custom way (stack order changes or something) the eraser plays nicely with it.

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.1 |    76.01 |   85.69 |   82.83 |                                               
 fabric.js |    83.1 |    76.01 |   85.69 |   82.83 | ...,30850,30924,30935-31000,31123,31222,31458 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented May 14, 2022

i still have no idea what erarserbrush does and how. And i won't for long probably, so i approve those PR in trust when they do not modify other fabricjs files

@ShaMan123
Copy link
Contributor Author

Thanks!
It's a minor change to align with the changes we made to Canvas#_objectsToRender
I am working on visual tests for the eraser. Will finish soon.
I am facing a weird edge case that I'm struggling to get to the bottom of and you have made me understand the necessity of tests. I am in fear of the eraser breaking due to major changes. Tests will ease my mind.

@ShaMan123 ShaMan123 merged commit 9377ee1 into master May 14, 2022
@ShaMan123 ShaMan123 deleted the eraser-fix-collection-rendering branch May 14, 2022 13:28
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