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 error margin for detecting if a point or line is in the chartArea #5790

Merged
merged 1 commit into from Oct 27, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Oct 24, 2018

Many issues about points/gridline at the top edge of the chart area being cut have been reported. The issue is caused by a decimal round-off error when detecting if a point or line is in the chartArea. This PR adds an err margin, which is already implemented for point clipping at the right and bottom edges.

v2.7.3: https://jsfiddle.net/nagix/z1bx459c/ (To reproduce the problem, you may need to resize the container as this is caused by decimal rounding)
screenshot 2018-10-25 at 2 16 57 am

v2.7.3 + This PR: https://jsfiddle.net/nagix/yv6emrcf/
screenshot 2018-10-25 at 2 17 11 am

Fixes #4579
Fixes #4790
Fixes #5493

@nagix
Copy link
Contributor Author

nagix commented Oct 25, 2018

Maybe 1% margin is too high because it allows 10px off the chart when a border is at 1000px from top/left. 0.01% should be fine.

@simonbrunel
Copy link
Member

Do you think that using a percentage is the best solution or should we express errMargin in pixels instead? (e.g. < left - errMargin)?

@benmccann
Copy link
Contributor

@nagix you may also be interested in the pending PR to fix point clipping on the left and right: #5190

@nagix
Copy link
Contributor Author

nagix commented Oct 25, 2018

Since we don’t need high precision, a margin in pixels seems to be easy to understand.

@nagix
Copy link
Contributor Author

nagix commented Oct 25, 2018

I would like to address #5190 separately because it is about a layout issue while this is about a rounding error which can be easily fixed.

@nagix
Copy link
Contributor Author

nagix commented Oct 25, 2018

Replaced errMargin with epsilon, which is used in the other place.

@benmccann
Copy link
Contributor

Agreed that we should address #5190 separately. It would be useful to get your review on it since you're currently working on this part of the code and know it better than I do. It'd be great to get that one merged too if you think it's a good solution

@nagix
Copy link
Contributor Author

nagix commented Oct 26, 2018

Do we need to fix the codeclimate issue?

@benmccann
Copy link
Contributor

No, I don't think so

@simonbrunel
Copy link
Member

Nice work @nagix. Does 0.0000001 enough in all cases, how did you chose that value?

@simonbrunel simonbrunel added this to the Version 2.8 milestone Oct 26, 2018
@nagix
Copy link
Contributor Author

nagix commented Oct 26, 2018

It’s from your #5597 ;) No one can tell if this distance from the border is inside or outside, and rounding errors usually happen around 10-15 or 10-16

@simonbrunel simonbrunel merged commit cb14217 into chartjs:master Oct 27, 2018
@qreeves
Copy link

qreeves commented Oct 28, 2018

Well done @nagix and thanks for the fix!

@nagix nagix deleted the issue-5790 branch October 28, 2018 06:26
@simonbrunel
Copy link
Member

@nagix I think this PR breaks some cases: if you test this example (code), you will notice that the last points disappear with master (maybe epsilon is too low?):

image

@nagix
Copy link
Contributor Author

nagix commented Nov 27, 2018

@simonbrunel This is related to #5797 and happens only when a category scale is used. As explained in #5797, getPixelForValue rounds the offset value and the x value of the point may exceed chartArea.right depending on whether the decimal part of scale.left is less than 0.5 or not.

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.

None yet

4 participants