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

nagix
Copy link
Contributor

@nagix nagix 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

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@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?

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 Show resolved Hide resolved
@nagix
Copy link
Contributor Author

nagix 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
Copy link
Contributor

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.

etimberg
etimberg previously approved these changes Nov 25, 2018
@nagix
Copy link
Contributor Author

nagix 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
Copy link
Contributor Author

nagix commented Nov 25, 2018

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

@nagix nagix force-pushed the issue-5858 branch 2 times, most recently from 31ae280 to 7f16657 Compare November 26, 2018 03:24
simonbrunel
simonbrunel previously approved these changes Nov 26, 2018
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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.

src/helpers/helpers.canvas.js Outdated Show resolved Hide resolved
- 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
@nagix
Copy link
Contributor Author

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

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @nagix

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
…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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Release v2.7.3 scatter plot CanvasGradient
4 participants