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

calcMinMax() during scrolling / panning #89

Closed
AlBirdie opened this issue May 15, 2015 · 9 comments
Closed

calcMinMax() during scrolling / panning #89

AlBirdie opened this issue May 15, 2015 · 9 comments
Labels

Comments

@AlBirdie
Copy link
Contributor

For finance charts it is basically the industry standard to constantly adjust the chart's min and max values for the currently visible X axis range.

That would mean that we'd need to call calcMinMax with the chart's lowestVisibleXIndex and highestVisibleXIndex instead of just looping from 0 to data.count.
In order not to break any existing implementations, that would ideally be configurable with a boolean.

@danielgindi In case you consider this to be included into the library, I'd be happy to implement it on the current master branch and create a pull request for it (same for Android, but that would have to wait a couple of days). I'm just hoping that this doesn't put too much effort on the CPU, but I reckon that'll be fine as long as the datasets aren't that large.

@danielgindi
Copy link
Collaborator

@AlBirdie That sounds great!

If you could do a PR just a few notes:

  1. Keep it as a feature toggled by a boolean, as you said
  2. Do performance benchmarks on different datasets - before and after, so we'd know it it has affected performance and how... It would be great to include notes about performance in the documentation of the feature!

I appreciate your participation in this project very much! :-)

@AlBirdie
Copy link
Contributor Author

Sure, not a problem. Do you have any preferences as to how to test performance?
I'd probably do a loop that invalidates (hence calling drawRect on the chart eventually) and measure the time it takes to execute drawRect and create an average of that. Sounds reasonable?
I intended to to this for a long time already (mainly for the line chart performance issue), but haven't gotten around it, sorry for that. Now that the main implementation is done I can finally work on that.

@danielgindi
Copy link
Collaborator

That sounds good. Actually if you have the time to implement Tests based on
the test platform that's already in Xcode it will be the best as in one
click you could run a test without all the hassle..

On Fri, May 15, 2015 at 1:01 PM, AlBirdie notifications@github.com wrote:

Sure, not a problem. Do you have any preferences as to how to test
performance?
I'd probably do a loop that invalidates (hence calling drawRect on the
chart eventually) and measure the time it takes to execute drawRect and
create an average of that. Sounds reasonable?
I intended to to this for a long time already (mainly for the line chart
performance issue), but haven't gotten around it, sorry for that. Now that
the main implementation is done I can finally work on that.


Reply to this email directly or view it on GitHub
#89 (comment)
.

@danielgindi danielgindi changed the title [Feature] calcMinMax() during scrolling / panning calcMinMax() during scrolling / panning May 15, 2015
@AlBirdie
Copy link
Contributor Author

Just a quick update, unfortunately it's gonna take a while for me to finish work on this, got a lot of other things to do this week. But we'll get there eventually. ;)

@danielgindi
Copy link
Collaborator

Ok it was merge, along with some bugfixes.
Can you make sure that everything is working ok according to your expectations? Before I go ahead and append to the Android version...

@AlBirdie
Copy link
Contributor Author

Sure, just give me a few minutes. The other changes I've talked about in #92 will then be based on the new master branch. I'm going to make a separate pull request for each feature, that'll make it much easier to spot bugs and make it easier to merge.

@danielgindi
Copy link
Collaborator

Yes that's a good idea :-)

@AlBirdie
Copy link
Contributor Author

Ok, tried the current master branch with the LineChart1ViewController (pinchZoomEnabled = NO, no limit lines, no custom Min/Max) and though the re-draw loop is almost gone, there are still occasional situations where you get like 2 or 3 redraws during panning resulting in a slight flicker of the chart.
The error basically remains, but because of the additions @dorsoft made in the BarLineChartViewBase
s drawRect ( if(_lastLowestVisibleXIndex != lowestVisibleXIndex....) ) much less severe.
The question is if we can live with that and merge it into the Android version anyways?

Personally, I haven't seen this error in our application at all, just in the demo iosCharts demo application. That doesn't mean that the error won't occur in other real-world implementations either.
To date I couldn't find out why this error is happening... :(

@danielgindi
Copy link
Collaborator

It may be the calculating min/max causes base values to change so that calculating another min/max is required. But I think I've fixed that situation so you need to debug it.

If you still encounter that issue, you may want to breakpoint on all calls to setNeedsDisplay!

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

No branches or pull requests

2 participants