improvements in barRect height calculation

[potato04]

issue

My last PR #3587 fixed bar height calculation when all y values equals 0.
Recently, I found bars will displayed in an unexpected size when all y values less than 0.
And the black border will be oversized too when voice over is enable.

how to reproduce

replace those codes
https://github.com/danielgindi/Charts/blob/5087a04bbefc9f15995113954ce217bbfb445577/ChartsDemo-iOS/Swift/Demos/AnotherBarChartViewController.swift#L64-L68
with

        let yVals = [
            BarChartDataEntry(x: 0, y: -10),
            BarChartDataEntry(x: 1, y: -20),
            BarChartDataEntry(x: 2, y: -30),
            BarChartDataEntry(x: 3, y: -15)
        ]

other

I moved the offset calculation code out of the loop, calculate once is enough
https://github.com/danielgindi/Charts/blob/5087a04bbefc9f15995113954ce217bbfb445577/Source/Charts/Renderers/BarChartRenderer.swift#L112

[codecov-io]

Codecov Report

Merging #3650 into master will increase coverage by 1.44%.
The diff coverage is 99.14%.

@@            Coverage Diff            @@
##           master   #3650      +/-   ##
=========================================
+ Coverage   31.06%   32.5%   +1.44%     
=========================================
  Files         114     114              
  Lines       10465   10681     +216     
=========================================
+ Hits         3251    3472     +221     
+ Misses       7214    7209       -5

Impacted Files	Coverage Δ
Tests/Charts/BarChartTests.swift	100% <100%> (ø)	⬆️
Source/Charts/Renderers/BarChartRenderer.swift	52.18% <100%> (+0.7%)	⬆️
Source/Charts/Components/YAxis.swift	87.3% <86.66%> (+8.13%)	⬆️
Source/Charts/Renderers/LegendRenderer.swift	53.06% <0%> (-0.92%)	⬇️
...ce/Charts/Renderers/CandleStickChartRenderer.swift	0% <0%> (ø)	⬆️
Source/Charts/Utils/Platform+Accessibility.swift	31.57% <0%> (ø)	⬆️
Source/Charts/Charts/BarLineChartViewBase.swift	67.1% <0%> (+1.31%)	⬆️
Source/Charts/Utils/Transformer.swift	49.31% <0%> (+2.73%)	⬆️
Source/Charts/Renderers/AxisRendererBase.swift	67.36% <0%> (+2.77%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b2e873...d5d9abd. Read the comment docs.

[potato04]

@petester42

[liuxuan30]

In like one or two years ago, I tested the negative bar height, and it's fine. Somehow you said it's broken. It seems the main change is the offset introduction. BTW, you mentioned voice over, which i think is irrelevant?

I need to check some old commits

[liuxuan30]

I checked the old commits and your PR. However I found in most cases your fix seems correct, but if you try to set

chartView.leftAxis.axisMaximum = -40.0

and with your test data, it shouldn't draw any bars, but instead, it draws 4 bars and wrong. seems a new issue to me. Not having a fix yet. It seems even I revert all of the changes, it still draw only one bar, and left axis labels are all gone. It seems the axis range calculation and matrix messed up.

Would you mind taking a look if you could have a quick fix? If you couldn't, it's fine, we will file a new issue to track it.

BTW, it's time to add UT for negative bars, and +/- bars, do you know how to add UT?

[potato04]

In like one or two years ago, I tested the negative bar height, and it's fine. Somehow you said it's broken. It seems the main change is the offset introduction. BTW, you mentioned voice over, which i think is irrelevant?

I need to check some old commits

The reason of introducing offset is voice over (Accessibility) , you can check this PR #1060

I checked the old commits and your PR. However I found in most cases your fix seems correct, but if you try to set

chartView.leftAxis.axisMaximum = -40.0

and with your test data, it shouldn't draw any bars, but instead, it draws 4 bars and wrong. seems a new issue to me. Not having a fix yet. It seems even I revert all of the changes, it still draw only one bar, and left axis labels are all gone. It seems the axis range calculation and matrix messed up.

Would you mind taking a look if you could have a quick fix? If you couldn't, it's fine, we will file a new issue to track it.

BTW, it's time to add UT for negative bars, and +/- bars, do you know how to add UT?

Yes, I will check it out and update this PR as soon as possible

[potato04]

I think I solved these problems.

1. chartView.leftAxis.axisMaximum = -40.0 This problem is due to the wrong calculation of originYOffset.
2. It seems even I revert all of the changes, it still draw only one bar, and left axis labels are all gone. When max or min was set manually, sometimes it will cause min>max, so the y-axis will not work properly.
   Please review my new commits.

[liuxuan30]

at first I was thinking a fix like this:

if !offsetAxis._customAxisMin || !offsetAxis._customAxisMax {
    if barData.yMin > 0 {
        heightOffset = CGFloat(offsetAxis.axisMinimum)
    }
    if barData.yMax < 0 {
        originYOffset = CGFloat(offsetAxis.axisMaximum)
    }         
}

I think there is a chance when offsetAxis._customAxisMin is true, and offsetAxis._customAxisMax is false, you still need to set originYOffset. Like all negative values?

[potato04]

After some debugging, it seems that the value of _customAxisMax/_customAxisMin has nothing to do with calculating offset.

[liuxuan30]

really? As the old commit said, if you touch nothing, the bar will draw from zero. So it does not need any offset. I will check the edge

[liuxuan30]

once we find the right fix, we shall add UT for it.

[liuxuan30]

One more issue found:
data set:

        let yVals = [
            BarChartDataEntry(x: 0, y: 10),
            BarChartDataEntry(x: 1, y: 20),
            BarChartDataEntry(x: 2, y: 30),
            BarChartDataEntry(x: 3, y: 15)
        ]

and enable

chartView.leftAxis.inverted = true

it draws wrong:

only uses

barRect.origin.y = top
barRect.size.height = bottom - top

solve the issue. It's not about the min/max bug, but the inverted feature and accessibility.

I tested pure positive values has issue, pure negative values has same issue, +/- combined values don't.

We also need to check highlight code as well. The highlight seems right, as forgot to consider the offset.

The accessibility has more bug then we think :( I think we need add enough UTs for bar chart

[liuxuan30]

It seems the inverted bug starts from barRect.origin.y = top + originYOffset, need to change accordingly, as well as the height. the bottom and top will be flipped and the barRect would mess up easily.

I think we have to think about a clean solution
have to get back to work. used up free time today

[potato04]

Please review the new commits and advise your comments

[liuxuan30]

what you think if we use min = 0.5 * max and max = 1.5 * min? @potato04 @jjatie @petester42 ?
clearly if min > max, and user only set one custom value of them, +/- 1 is not always the good value, for example with large values and decimals

[potato04]

Couldn't agree more, may be we should update those codes too
https://github.com/danielgindi/Charts/blob/a77a358fb0d8a89b6121d4cdcbc122cc42cce395/Source/Charts/Components/YAxis.swift#L170-L178

[potato04]

It seems that the min/max correction is not perfect.

//This will work
chart.leftAxis.axisMinimum = 100
chart.leftAxis.axisMaximum = 0
chart.data = data

//This won't work
chart.data = data
chart.leftAxis.axisMinimum = 100
chart.leftAxis.axisMaximum = 0

chart.data = data
chart.leftAxis.axisMinimum = 100
chart.leftAxis.axisMaximum = 0
// have to call this to make it work
chart.notifyDataSetChanged()

[liuxuan30]

@potato04 Could you grant me push to your branch? Seems I cannot push a commit directly to your fix-barRectCaculation branch. So I use Github editor to add one more commit. Please have a review if my change b67c251 is valid. The idea is to keep barRect calculation the same, move the offset earlier.

[liuxuan30]

@potato04

Check if highlight code need to change as well
Check Horizontal bar chart

I suppose highlight code need to align, right?

[potato04]

@potato04

Check if highlight code need to change as well
Check Horizontal bar chart

I suppose highlight code need to align, right?

Horizontal bar and highlight seem to be working fine
Do you mean that we shall add unit tests?

[liuxuan30]

@potato04

Check if highlight code need to change as well
Check Horizontal bar chart

I suppose highlight code need to align, right?

Horizontal bar and highlight seem to be working fine

highlight code is still drawing from zero line I suppose? Shall we need to align the barRect?

Do you mean that we shall add unit tests?

I don't think we need to add so far.

[potato04]

oh, i got you

highlight code is still drawing from zero line I suppose? Shall we need to align the barRect?

no, we don't need to do that. Because it makes no difference to the UI

[potato04]

Some issues that were fundamental to the calculation of bar rectangle had not yet been adequately addressed. I had to create a commit again...😓
In this commit, First, I moved the offset calculation code back into the loop because the offset of each bar may be different.
Second, replaced originYOffset and heightOffset with topOffset and bottomOffset and added comments to make it easier to understand the calculation process.

[liuxuan30]

oh, i got you

highlight code is still drawing from zero line I suppose? Shall we need to align the barRect?

no, we don't need to do that. Because it makes no difference to the UI

I don't think so, because, I remember the old behavior is drawing from zero line, and we change it to draw from the offset. The old behavior is also not affecting UI. It's like an improvement, so I think we should align both rendering and highlight with the same logic.

I checked the highlight code:

                    y1 = e.y
                    y2 = 0.0
                }
                
                prepareBarHighlight(x: e.x, y1: y1, y2: y2, barWidthHalf: barData.barWidth / 2.0, trans: trans, rect: &barRect)

it is similar, drawing from y2 = 0.0, using the clip trick to cut off unnecessary part of the highlight. Though it's working ok, but I'm kind of concerned if their barRect calculations are not aligned, there would be problem in the future?

@jjatie what you think?

[liuxuan30]

@potato04 more barRects to check:

1. bar border feature;
2. bar shadow feature;

they are involved with rect cuculation. We have to check it as well.

[potato04]

The old behavior is also not affecting UI.

Yes, It will affects the UI when it comes to accessibility.
If you turn on voice over on your iPhone(guide: https://help.apple.com/iphone/10/#/iph3e2e2329), open chart-demo app and select one of the bars in the chart view, then a corresponding black border( VoiceOver Cursor) will appear, before i created this PR, the VoiceOver Cursor was oversized in some cases, because its size depends on the size of the bar being drawn instead of the size after being clipped. please take a look at the image I posted at the top of this thread. The reason is the previous offset calculation did not cover all cases. So the main purpose of my PR is improve offset calculation and fix other problems caused by it.

[jjatie]

@liuxuan30 @potato04 The bar border is fine (it's just a stroke setting, no calculation involved). The highlight seems fine. I'll need to take a closer look at the bar shadows later.

This PR is making me really want to try out Core Animation for our bar Charts.

[liuxuan30]

@jjatie I would support try out CA as well. What you think that do we need to change highlight barRect calculation to align with this PR? I'm kind of unsure. I would agree what @potato04 said above. Highlight code works fine, but the way we calculate the barRect has changed significantly.

[liuxuan30]

@jjatie this means you are ok to leave highlight untouched? then we could merge then