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 arc border with circumference over 2*PI #6215

Merged
merged 6 commits into from
Apr 30, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Apr 18, 2019

Fixes: #6171

Demo

@simonbrunel simonbrunel added this to the Version 2.9 milestone Apr 18, 2019
etimberg
etimberg previously approved these changes Apr 20, 2019
ctx.fillStyle = vm.backgroundColor;

if (vm.circumference > Math.PI * 2) {
Copy link
Contributor

@nagix nagix Apr 24, 2019

Choose a reason for hiding this comment

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

Looks good in case 0 <= circumference <= 2*PI and 2*PI < circumference <= 4*PI, but appearance is not consistent when circumference > 4*PI. How about looping this part (endAngle - startAngle) / (Math.PI * 2) times?

Copy link
Member Author

Choose a reason for hiding this comment

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

Loop implemented, did some more refactoring to keep CC (and me) happy 😄
Demo updated.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of a loop would doing % 2PI be more efficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

@etimberg that was my initial approach, but with transparency it is not consistent with circumference > 4*PI.
Not sure if anyone is going to use circumference that high anyway, but now its drawn consistently.

@nagix
Copy link
Contributor

nagix commented Apr 24, 2019

When borderAlign is set to 'inner', the result doesn't look correct because the border appears at next to the end angle instead of at the start angle.
Screen Shot 2019-04-24 at 11 14 06 PM
Similarly, when borderAlign is 'center', the border should also be drawn and visible at the the start angle as the background is transparent.
Screen Shot 2019-04-24 at 11 14 39 PM
I think we need to fill and stroke arcs multiple times if the circumference is greater than 2*PI. The first arc would have an open path (open at the end edge), the following arcs would have 2 open paths for outer and inner circumferences and the last arc would have an open path again (open at the start edge).
Screen Shot 2019-04-24 at 11 19 53 PM

@kurkle
Copy link
Member Author

kurkle commented Apr 24, 2019

Fixed drawing of borders. But in 2 steps instead of 3, first fully open full circles and then fully closed partial.
Demo updated.

Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. When borderAlign is 'inner' and the overlap is very small, the result is a bit odd, but I know there is no good way to fix. The other parts look perfect, so I think it's ok now.
Screen Shot 2019-04-25 at 2 12 22 PM

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

Successfully merging this pull request may close these issues.

Doughnut chart has incorrect border when overful
6 participants