One implementation for grouped bar chart support in CombinedChartView

[liuxuan30]

I got one implementation for grouped bar chart support in CombinedChartView today to address issue #252

I add a new demo view controller called GroupedCombinedChartViewController, to illustrate multiple bars + multiple lines

I have no intention to merge this PR, as you must have many suggestions and questions :-)

I want to start a discussion about the implementations, and perhaps we can find a good one.

My idea is to add a few properties as the bar chart renderer and transformers, and use bars transformers to locate the xIndex and dataSet to highlight. Other like line/bubble, just use the xIndex and dataSet to highlight their data points.

Pros:
Straightforward, easy to understand and implement

Cons:
I have to create a CombinedChartHighlighter and copy the code from BarChartHighlighter, as I cannot create _highlighters (like CombinedChartRenderer does) nor I can subclass BarChartHighlighter, but I did need the code to calcuate bar stuff.

[liuxuan30]

@PhilJay maybe we can discuss about what you have in mind.

[danielgindi]

#169

[liuxuan30]

Today I found a bug for my implementation for getDataSetByIndex in highlighter. It will search barData first and return its dataSetIndex, however, this index might not be the same on in combinedChartData. It depends on the order of setting lineData and barData. My implementation would prefer barData more than lineData. Gnenerally it works, but when comes to highlight the line dot and chart marker, it cannot get the correct dataSetIndex for line data.

Update: I have fixed the issue I mentioned in commit No.2

[liuxuan30]

I have fixed the issue I mentioned in commit No.2

[danielgindi]

@liuxuan30 I haven't had the time yet to look at the issue with grouped bars in combined charts... Did you have success with it? What's the status of this PR - does it correct it?
Tested to not cause side effects?

[liuxuan30]

Oops, I'm sorry that I just test it again, it has bug while trying to highlight each bar with commit 90a327d.

The second commit is mainly for supporting the chart marker feature, I want it to support single bar and line data marker.

It returns the index in combined chart data set rather than the index of bar data set in commit 90a327d, and it works for the chart marker but break the highlight logic. I'm lost here, because I want to know the dataSet type to know which transformer to use. Old logic just return a dataSetIndex in each sub chart data, so I changed it to return the index in combined chart data, so I can know the type. If we still use old logic, then I have to add a new parameter dataSetType for getMarkerPosition. Please advice.

My project is different logic for highlighting, so I don't find this issue while pushing 90a327d, my bad.

I have been using this implementation for a while, no big issues reported. Just some corner cases, and this chart marker problem.

To me, I have to support grouped bars in combined chart, according to my Boss :( So it solves my problem, and most likely can help others, but I'm not sure if it's the best way to do so. I raise this PR is to discuss. If there are people had similar requests, they can try it out. I don't suggest you merge this, at least we need to think about many stuff, like highlighting, chart marker, etc.

[liuxuan30]

@danielgindi I am occupied with some dev work recently, I will update as soon as I got some time.

[liuxuan30]

I have to delay the merge with master again.. I need feedback about if the idea is good to go, before we could talk about it I would reduce the freq to solve conflicts, too much work on my plate..

[jwardle]

@liuxuan30 can you separate out the PR 1bd8c22 ? It does not have any dependencies on any other changes and is being blocked by the wider PR issues in this thread.

[liuxuan30]

@jwardle wow You even find this change inside a PR. I will take a look at the current code and may file one later

[liuxuan30]

@danielgindi Can you get this notification?

I am having sometime to resolve the conflict. But I stucked at the data provider. My solution will need to fedd two different transformers for bar chart renderer and other charts. However I cannot simply override the getTransformer method to get it done.

Now my way is to modify the barChartDataProvider like this:
func getBarChartTransformer(which: ChartYAxis.AxisDependency) -> ChartTransformer
var barChartXMin: Double { get }
var barChartXMax: Double { get }

So I can provide the correct value for the bar chart. Any better ideas?

The old renderer delegate works because each chart renderer has its own protocol, so I can provide everything they needs.

Is it possible in swift that the combined chart view's getTransformer method could know which renderer is asking for transformer?

[danielgindi]

Well I think that the transformer at this point is the same inside a combined chart, but you're having problems with this you can add more functions to the protocol...

[liuxuan30]

Is it possible in swift that the combined chart view's getTransformer method could know which renderer is asking for transformer?

[danielgindi]

I could think of some options, but that wouldn't be right if custom
renderers are set for example

But I think that the providers should have their function naming more like
the ObjC conventions, and then you can know who is asking for a value

‏בתאריך יום שישי, 30 באוקטובר 2015, Xuan notifications@github.com כתב:

Is it possible in swift that the combined chart view's getTransformer
method could know which renderer is asking for transformer?

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

[liuxuan30]

Alright, I will use barChartXMin, barChartXMax, etc for the bar chart data provider.

Last issue is the highlight - need to figure out a way to highlight the bar, the line dot or the bubble.
Currently they share the same dataSetIndex for each renderer, so, if the line dot highlighted, othre chart type's data of the same dataSetIndex will be highlighted as well due to the highlighter only return the same highlight object

[liuxuan30]

@danielgindi I got a whole day on this and get it work with some flaws.

The biggest change is, I will return the dataSetIndex of combinedChartView.data, not each sub data's. Because line data set can have 0 index, and bar data set could also have one. So it's not unique to identify which dataSet should be used.

Using the data set index of combined data gives me a chance to know which dataSet and chart type it is.

I also rewrite the highlight logic for combined chart view, just to a math to get the data set index of the sub data and call the specific renderer to draw it.

The problem is about the highlight. I had a hard time to figure out a way, and still no good one yet.
Currently we are only using the y value to get the closest data set index. however with the grouped bar support, if the line dots overlays with the bar, it will always get the line dots as the closest one, even if the bar I tapped is on the left of the line dot.

I think there are two things we need to confirm:

1. does my solution make sense and flexible enough and no better options
2. what's the requirement for highlighting. If we need a fine-grained control, we need to take x distance into account while calculating the closest data set.

[liuxuan30]

@danielgindi what's the reason for closing? I know it has been an old PR... But we never discussed about this feature formally.

[danielgindi]

It's because it's not a final solution- you said you have more work on it but hadn't had the time yet.

Feel free to re-open if there's any news :)

[liuxuan30]

@danielgindi I think my work is almost done except for those conflicts; I updated once after the chart data provider is introduced. For the current conflicts, it should not be hard to resolve;
but before that, what I want to know is what do you think about the implementation - we don't have talked about the questions above yet :)
Maybe you could have some time to take a look first how did I support grouped bars. What am I concerned is the solution is not good in flexibility, extensibility, etc. It did solve my problem, but I'm not sure if my way is also suitable from the framework level perspective. Also we need to think about Android portion because it's a serious feature

[danielgindi]

Yes we need to carefully investigate this. I need to find the time for this :-)

[liuxuan30]

synced with master today :) Be aware I had implemented a grouped bar combined view controller in ChartsDemo, to get familiar with my implementation.

[danielgindi]

Man I think that this is not required now - with the new way of calculating bar groups. Thanks for all your efforts on this matter!!!

[liuxuan30]

🎆 happy birthday to this PR one week later and finally closed :D

[danielgindi]

:)

[vojto]

Here's link to wiki that describes the official implementation: https://github.com/PhilJay/MPAndroidChart/wiki/Setting-Data#grouped-barchart

Example code in this PR is misleading, as it never got merged in.

[liuxuan30]

Why you mention this closed PR then. It's my implementation while grouped bars are not supported in Chart v2. However, v3 already supported it.

[vojto]

@liuxuan30 I was just Googling around this problem, and this issue is the prominent result, so I thought I'd leave some pointers for the next guy.

[liuxuan30]

Chart v3 already support grouped bars. Just check out charts demo