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

add drawDot func to pencilbrush #4743

Merged
merged 5 commits into from
Feb 21, 2018
Merged

add drawDot func to pencilbrush #4743

merged 5 commits into from
Feb 21, 2018

Conversation

boomyao
Copy link
Contributor

@boomyao boomyao commented Feb 19, 2018

add simplePencil_brush that _render not needs to redraw everything, suitable for low-profile phone.

close #4680

@boomyao boomyao changed the title add simplePencil_brush that _render not needs to redraw everything, s… add drawDot func to pencilbrush Feb 19, 2018
@asturur
Copy link
Member

asturur commented Feb 19, 2018

Ok so this is what i meant.

  • _render stays untouched like in your PR
  • drawDot ( i renamed _drawSegment ) is just a small pieces of _render that can draw just a part
  • _render uses drawDot for code reuse.

In this way i achieved exactly your goal, but in addition i minimized code changes. It make easeir to see what changed.

@boomyao
Copy link
Contributor Author

boomyao commented Feb 19, 2018

cool ! so beautiful code you do.

@asturur
Copy link
Member

asturur commented Feb 19, 2018

Do not worry is just practice.
But for contributing to an open project is important to minimize changes, it makes easier for the reviewer to understand what is going on, and also makes easier to understand if something is going to break with the change.

In this way the capture logic is unchanged, and we should not be worried. The drawing logic still needs to be tested manually but it looks good to me.

Please confirm me that it draws as intended and that performances are improved on slow phones, and i ll merge it.

@asturur
Copy link
Member

asturur commented Feb 20, 2018

@boomyao, does it works good?

@boomyao
Copy link
Contributor Author

boomyao commented Feb 21, 2018

@asturur, maybe this PR should be close.
if don't exec ctx.beginPath() everytime, ctx.stroke() will render the path many time, then the path is bold, so i think it's the reason to draw all points when mousemove.

@asturur
Copy link
Member

asturur commented Feb 21, 2018

no i think we can fix it. let's try

@asturur
Copy link
Member

asturur commented Feb 21, 2018

i think we are good with this latest change.
Thanks @boomyao

@asturur asturur merged commit f855bbc into fabricjs:master Feb 21, 2018
@asturur asturur mentioned this pull request Feb 21, 2018
@boomyao
Copy link
Contributor Author

boomyao commented Feb 21, 2018

@asturur Great ! ! Thank you for give me a lesson!

AndrewJDR added a commit to AndrewJDR/fabric.js that referenced this pull request May 4, 2018
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
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.

freedrawing slowly on low-profile phone.
2 participants