[Pending rebase] Some lines in line chart not rendering for sparse data

[danielr]

For sparse data sets (that is, if there is not an entry for every x-index), when zoomed in and panning around, sometimes the outer lines of the line chart would disappear.

See this (sorry for the mediocre quality, I thought GIF is more appropriate here than a video):

In this demo, I use 15-minute intervals for the x-values, but there is not an entry for every x-index (thus "sparse data set").

I've had a look at the code and also fixed the issue. The following is an in-depth explanation of the issue at the code level:
In fact, ChartRendererBase took precautions to set _minX and _maxX to be always outside of the visible viewport, however, if the data set is sparse, this is not enough: If there is no entry at _minX, dataSet.entryForXIndex(_minX) will often return an entry with xIndex > _minX (not always, though, because of the nature of binary search). Fixed this by adding two more entry query functions to ChartDataSet: entryBeforeXIndex and entryAfterXIndex, which are guaranteed to return entries with a lower/higher x-index (if they exist).

[liuxuan30]

I saw the same issue on my side, but pending fix.
you need to solve the conflict first :) and the code style of "{}"

[danielr]

Oh, sorry, I overlooked the braces for the while loop. Fixed.

Regarding the conflict: I've intentionally based by branch on the last stable tag (2.1.6), because I use the fork myself and didn't want the latest unstable Realm-stuff in master. But you're right, I should probably merge master for the PR.

[danielr]

@liuxuan30 Just tried to rebase this onto master, but I noticed that there are fundamental changes to the data sets due to Realm (different implementations with base/interface). I currently don't have a lot of experience with Realm and, to be honest, not the time to implement the necessary changes to the RealmBaseDataSet.

[liuxuan30]

Your fix does not touches the Realm stuff, just how the entries should be fetched; so should be easy to port the fix; Realm is just a DB, and defined the interfaces;
That's fine if you don't have time to port, we value the idea to fix. We can keep this PR open, when you have time you could do it. On the other side, @danielgindi or me or others may fix it on his side since he is going to make a release soon.

[danielr]

Yes, you'd need to port the entryBeforeXIndexand entryAfterXIndexmethods to the Realm data set. This is certainly not difficult, but I'd first need to have a closer look at that implementation and especially setup some Realm data to be able to test the implementation.

[danielgindi]

I'll try to work with this soon. If somebody has the time and wants to rebase this before I get to - that's also nice...

One thing bothers me: The first/last points to draw have been proven to be some kind of a bottleneck in the past, in other situations. Have you tested the performance implications of this change?

[danielr]

One thing bothers me: The first/last points to draw have been proven to be some kind of a bottleneck in the past, in other situations. Have you tested the performance implications of this change?

I have not explicitly profiled the code. However, the change is actually very minimal from a performance point of view. The heavy lifting is still done by the binary search (the new methods delegate to entryIndex). The only thing that has been added is the search for the next entry whose x-index lies beyond the specified index (before/after). Even though this is implemented as a while loop, it only ever iterates more than once if there is more than one entry for this x-index. In 50% of the cases it doesn't even do anything, because the index returned by entryIndex already lies on the "correct" side of the specified index (due to the nature of binary search it depends from which side it is approached). BTW, this is also why the bug only appeared for every 2nd point. ;)

[alyapozdnyakova]

This looks really good. Did you stop working on this? I'm having the same problem and tried your fix (+ some refactoring) and it works great.

[danielr]

@ylibatsya I created this pull request while v2.1.6 was the latest stable release, so my fix is based on that. In the meantime, a lot has changed, especially due to the Realm integration, so someone (read: someone with more Realm experience than me), should rebase this PR onto the current master, which would require to port my fix to the Realm-based dataset implementation, too. Other than that, it is "finished".

[liuxuan30]

@danielr it's just redefined the data portion interfaces, so we have common interfaces, so realm and standard can have their own implementation. I'm thinking your added methods should be in the common interfaces, nothing to do with the realm details? It should not be hard to adapt your change to the new architecture.
BTW, if you don't have much time, you can wait for us to do it. busy for all of us :)

[danielgindi]

I think that this PR is irrelevant anymore. Can't reproduce.