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

highestVisibleXIndex has regression issue #985

Closed
liuxuan30 opened this issue Apr 28, 2016 · 4 comments
Closed

highestVisibleXIndex has regression issue #985

liuxuan30 opened this issue Apr 28, 2016 · 4 comments

Comments

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 28, 2016

@danielgindi
for 375d7ba, you mentioned this closes for #785, #794

However, my old PR: 862de89 was trying to fix other issue.

I think we need review these two commits for highestVisibleXIndex? I had regression on my side:

pt.x now is 25.0, if we print it, but in the mem, it looks like 24.999999993, so floor(pt.x) would be 24, while it should be 25.0?

@liuxuan30 liuxuan30 changed the title lowestVisibleXIndex and highestVisibleXIndex has regression issue highestVisibleXIndex has regression issue Apr 28, 2016
@liuxuan30
Copy link
Member Author

liuxuan30 commented May 3, 2016

So I checked #785, #794, I think they are ONLY related to lowestVisibleXIndex, because old code has "+1" appended. The latest code removed +1.
However, I think highestVisibleXIndex should stick to round as mentioned above.

@danielgindi
Copy link
Collaborator

danielgindi commented Aug 10, 2016

Fixed in Charts 3.0 (v3 branch)

@liuxuan30
Copy link
Member Author

liuxuan30 commented Aug 12, 2016

Is it a fix or a new implementation so this is solved automatically? If so I will close #1000

@danielgindi
Copy link
Collaborator

The whole X concept changed, so it's a new implementation.
There's no "whole" x index, there are floating point x values, so we don't need to consider rounding to either side...

For x indexes there were reasons for both sides, used in multiple situations- now the situation does not exist :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants