Skip to content

Add error margin for detecting if a point or line is in the chartArea#5790

Merged
simonbrunel merged 1 commit into
chartjs:masterfrom
nagix:issue-5790
Oct 27, 2018
Merged

Add error margin for detecting if a point or line is in the chartArea#5790
simonbrunel merged 1 commit into
chartjs:masterfrom
nagix:issue-5790

Conversation

@nagix

@nagix nagix commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

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

nagix commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor

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

@nagix

nagix commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

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

@nagix

nagix commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

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

nagix commented Oct 25, 2018

Copy link
Copy Markdown
Contributor Author

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

@benmccann

Copy link
Copy Markdown
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

nagix commented Oct 26, 2018

Copy link
Copy Markdown
Contributor Author

Do we need to fix the codeclimate issue?

@benmccann

Copy link
Copy Markdown
Contributor

No, I don't think so

@simonbrunel

Copy link
Copy Markdown
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

nagix commented Oct 26, 2018

Copy link
Copy Markdown
Contributor Author

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

qreeves commented Oct 28, 2018

Copy link
Copy Markdown

Well done @nagix and thanks for the fix!

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

Copy link
Copy Markdown
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

nagix commented Nov 27, 2018

Copy link
Copy Markdown
Contributor Author

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

4 participants