FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix arc border with circumference over 2*PI by kurkle · Pull Request #6215 · chartjs/Chart.js · GitHub

/ Chart.js Public

Fix arc border with circumference over 2*PI - #6215

Merged
simonbrunel merged 6 commits into
chartjs:masterfrom
kurkle:arc_border_over_2pi
Apr 30, 2019
ConversationCommits (6)ChecksFiles changed
Merged

Fix arc border with circumference over 2*PI#6215
simonbrunel merged 6 commits into
chartjs:masterfrom
kurkle:arc_border_over_2pi

Conversation

kurkle commented Apr 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #6171

Demo

benmccann reviewed Apr 18, 2019
View reviewed changes
Comment thread src/elements/element.arc.js Outdated
simonbrunel added this to the Version 2.9 milestone Apr 18, 2019
etimberg previously approved these changes Apr 20, 2019
View reviewed changes
nagix reviewed Apr 24, 2019
View reviewed changes
Comment thread src/elements/element.arc.js Outdated

ctx.fillStyle = vm.backgroundColor;

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

nagix Apr 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

kurkle Apr 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

etimberg Apr 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

kurkle Apr 25, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 commented Apr 24, 2019

Copy link
Copy Markdown
Contributor

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.

Similarly, when borderAlign is 'center', the border should also be drawn and visible at the the start angle as the background is transparent.

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).

kurkle commented Apr 24, 2019

Copy link
Copy Markdown
Member Author

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

nagix approved these changes Apr 25, 2019
View reviewed changes

nagix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

simonbrunel merged commit 1a2a87b into chartjs:master Apr 30, 2019
kurkle deleted the arc_border_over_2pi branch May 5, 2019 06:45
snyk-bot mentioned this pull request Nov 9, 2019
Merged
This was referenced Mar 4, 2020
Open
Open
vpetersson mentioned this pull request Mar 7, 2020
Merged
This was referenced Mar 9, 2020
Closed
Merged
snyk-bot mentioned this pull request Apr 1, 2020
Open
This was referenced Apr 14, 2020
Open
Open
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Fix arc border with circumference over 2*PI (chartjs#6215)
7d49ec5
This was referenced Jun 1, 2021
Closed
Merged
This was referenced Jun 6, 2021
Closed
Open
Closed
Open
Open
This was referenced Jul 5, 2021
Closed
Open
This was referenced Jul 27, 2021
Closed
Merged
Merged
This was referenced Oct 14, 2021
Merged
Closed
jw mentioned this pull request Jan 17, 2022
Open
1 task
This was referenced Jan 21, 2022
Merged
Closed
This was referenced Feb 4, 2022
Closed
Closed
This was referenced Mar 7, 2022
Open
Open
Open
mend-for-github-com Bot mentioned this pull request Aug 13, 2022
Merged
1 task
This was referenced Nov 29, 2022
Closed
Closed
mend-for-github-com Bot mentioned this pull request Jan 26, 2023
Closed
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

etimberg etimberg left review comments
simonbrunel simonbrunel approved these changes
+3 more reviewers
benmccann benmccann left review comments
nagix nagix approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: bug

Projects

None yet

Milestone

Version 2.9

Development

Successfully merging this pull request may close these issues.

Doughnut chart has incorrect border when overful

5 participants

Back | FazBrowse Home | New Git URL