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

Fix bug in XBounds calculation of minimum/maximum visible entry index. #4839

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fumoboy007
Copy link

XBounds looks for data entries on the boundary of or outside of the visible range. However, there may not be any such entries for a particular data set.

Imagine a chart with two data sets, one of which has a bigger range of x-values than the other. The chart’s zoomed-out visible range would be the range of the wider data set. When the renderer tries to render the narrower data set, it would fail to correctly calculate the starting and ending entry indices because the starting/ending entries are strictly inside of the visible range.

The fix is to add fallback logic to look for data entries inside of the visible range if the initial algorithm does not find anything.

The XBounds bug fix also requires a bug fix in ChartDataSet’s binary search algorithm.

`XBounds` looks for data entries on the boundary of or outside of the visible range. However, there may not be any such entries for a particular data set.

Imagine a chart with two data sets, one of which has a bigger range of x-values than the other. The chart’s zoomed-out visible range would be the range of the wider data set. When the renderer tries to render the narrower data set, it would fail to correctly calculate the starting and ending entry indices because the starting/ending entries are strictly inside of the visible range.

The fix is to add fallback logic to look for data entries inside of the visible range if the initial algorithm does not find anything.

The `XBounds` bug fix also requires a bug fix in `ChartDataSet`’s binary search algorithm.
Copy link

@k1ran-ak k1ran-ak left a comment

Choose a reason for hiding this comment

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

Reviewed. No crashes. I didn't see any Lower bound <= upper bound errors anymore

@FelixHerrmann
Copy link
Contributor

This conflicts with #4829 where I fixed the partitioningIndex algorithm change

@FelixHerrmann
Copy link
Contributor

My PR might fix your whole problem as you can see in the screenshot below! Could you test my fork and confirm?

image

@fumoboy007
Copy link
Author

@FelixHerrmann Cool! Confirmed that #4829 also partially fixes the XBounds calculation. However, I commented on part of the change, which I believe is incorrect.

@tri75
Copy link

tri75 commented Jul 12, 2022

Please merge this commit soon. thanks.

Joe-BOD pushed a commit to beachbodydigital/Charts that referenced this pull request Aug 2, 2022
* Update for the two fixes in PR now for Charts
* Swift 5.7 - ChartsOrg#4874
* XAxis beyond Chart Dataset - ChartsOrg#4839
@lundjrl
Copy link

lundjrl commented Dec 20, 2022

Hey all, any updates on merging this in? Love the library, awesome work on it!

@FelixHerrmann
Copy link
Contributor

@lundjrl should be fixed with #4829

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

Successfully merging this pull request may close these issues.

5 participants