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

Adjust the size of rectRounded/rectRot points to fit the circle with pointRadius #5858

Merged
merged 1 commit into from Nov 28, 2018

Conversation

4 participants
@nagix
Copy link
Collaborator

commented Nov 23, 2018

Most of the pointStyle shapes are inscribed in the circle that has the radius of pointRadius, but 'roundedRect' and 'rectRot' shapes are inscribed in the rectangle that is inscribed in the circle and they look relatively smaller than others. To achieve more consistent appearance, this PR adjusts the size of those shapes to fit the circle.

This PR also fix the regression introduced by #5319 by removing ctx.translate() and ctx.rotate(), and calculating the absolute coordinates of each rendering point.

Chart.js 2.7.3: https://jsfiddle.net/nagix/ye9kcxaf/
pointstyle1

This PR: https://jsfiddle.net/nagix/6rnfspu3/
pointstyle2

For 'roundedRect', the corner radius is calculated using a new factor of 0.516 which is derived from the following:
slide1

Regarding the performance impact, I performed a benchmark using 50,000 random points on my MacBook and Dell laptop. The following charts show the difference from v2.7.3. The rendering time doesn't increase in most of the symbols except for 'rect' with rotation and 'rectRot'. The performance is even better with Chrome in particular.

https://jsfiddle.net/nagix/4nkucft6/
pointstyle3
pointstyle4
pointstyle5
pointstyle6
pointstyle7
pointstyle8
pointstyle9

Fixes #5839

@simonbrunel
Copy link
Member

left a comment

@nagix awesome!

I'm wondering if we could refactor the rectRounded implementation in helpers.canvas.rectRounded() without breaking #5597. Regarding performances, drawing rotated rectangles on Safari is really slow (and slow on Firefox) compared to the previous version but I guess it's acceptable?! Results in Chrome looks a bit dubious since I think Chrome uses GPU to transform coordinates so it's strange that we are ~2 times faster doing it manually.

Could you also compare performance in IE11 and Edge? And out of curiosity, what's the Chart.min.js size before and after this PR?

Show resolved Hide resolved test/fixtures/element.point/rotation.json Outdated
Show resolved Hide resolved test/fixtures/element.point/rotation.json Outdated
Show resolved Hide resolved src/helpers/helpers.canvas.js Outdated

@simonbrunel simonbrunel requested a review from etimberg Nov 23, 2018

@nagix

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 24, 2018

I have added the benchmark results using Chrome, FireFox, IE11 and Edge on Windows 10 PC. The performance is improved for all the tests. Interestingly, there is also huge improvement in Chrome on Windows. It seems that Chrome has some performance problem in translate() implementation.

The size of Chart.min.js increases from 159,958 to 160,152 bytes.

@benmccann

This comment has been minimized.

Copy link
Collaborator

commented Nov 24, 2018

It would be good to add a comment to the code explaining it or linking to this PR. If I read the code alone without reading the description in this PR I would not be able to follow it or understand where .516 comes from, etc.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 25, 2018

@nagix nagix force-pushed the nagix:issue-5858 branch from 53f3ae4 to 1958353 Nov 25, 2018

@nagix

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 25, 2018

I have refactored rectRounded as well. I removed epsilon as the proposed code checks if the width and height are equal to the radius, and optimizes arc calls.

@nagix

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 25, 2018

I have also added a reference to this PR and #5597 to the comment.

@nagix nagix force-pushed the nagix:issue-5858 branch 2 times, most recently from 31ae280 to 7f16657 Nov 26, 2018

@simonbrunel
Copy link
Member

left a comment

Thanks @nagix for the extra benchmarks.

I like the new rectRounded implementation. I verified that it doesn't break chartjs-plugin-datalabels and it's fine (however, I found another unrelated issue). I thought you would have call rectRounded from drawPoint (as before) but I think it doesn't worth it since you would have to also pass the rotation, so let's keep it like you did.

Show resolved Hide resolved src/helpers/helpers.canvas.js Outdated
Adjust the size of rectRounded/rectRot point to fit pointRadius
- Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius`
- Remove `translate()` and `rotate()` to fix the regression introduced by #5319
- Refactor `rectRounded` for better performance

@nagix nagix force-pushed the nagix:issue-5858 branch from 7f16657 to 85b8440 Nov 27, 2018

@nagix

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2018

I thought you would have call rectRounded from drawPoint (as before) but I think it doesn't worth it since you would have to also pass the rotation, so let's keep it like you did.

Yes, for the best performance, I decided not to call rectRounded from drawPoint.

@simonbrunel
Copy link
Member

left a comment

Thanks @nagix

@simonbrunel simonbrunel merged commit 1f2fa5c into chartjs:master Nov 28, 2018

2 of 3 checks passed

codeclimate 2 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.518%
Details

@nagix nagix deleted the nagix:issue-5858 branch Nov 28, 2018

eeiaao pushed a commit to eeiaao/Chart.js that referenced this pull request Dec 16, 2018

Adjust the size of rectRounded/rectRot point to fit pointRadius (char…
…tjs#5858)

- Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius`
- Remove `translate()` and `rotate()` to fix the regression introduced by chartjs#5319
- Refactor `rectRounded` for better performance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.