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

improvements in barRect height calculation #3650

Merged
merged 24 commits into from
Nov 1, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e2fa55b
fixed barRectCaculation
potato04 Sep 19, 2018
ed9d70d
fixed offset calculation
potato04 Sep 27, 2018
6cbecb8
Fix the mess caused by the setting the min&min value of the y-axis b…
potato04 Sep 27, 2018
526a73a
Fix the mess caused by the setting the min&min value of the y-axis by…
potato04 Sep 27, 2018
4548474
Revert "Fix the mess caused by the setting the min&min value of the y…
potato04 Sep 27, 2018
59d6986
Fix the mess caused by the setting the min&min value of the y-axis by…
potato04 Sep 27, 2018
c774d20
update offset calculation
potato04 Sep 28, 2018
1b6cc38
update code style
potato04 Sep 28, 2018
daff645
update code style
potato04 Sep 28, 2018
a77a358
update offset calculation
potato04 Sep 28, 2018
b67c251
keep barRect calculation untouched
liuxuan30 Sep 29, 2018
9a9edcb
After the correction of min and max , they should be assigned back t…
potato04 Oct 6, 2018
bf86f2c
add demo for bar chart unit test
potato04 Oct 6, 2018
f69c6ee
update unit test
potato04 Oct 7, 2018
ce578a0
revert last commit
potato04 Oct 9, 2018
afac36e
update unit test
potato04 Oct 9, 2018
b57cd0b
make sure max is greater than min &
potato04 Oct 9, 2018
0e7ff42
add new UT and fix some issues.
liuxuan30 Oct 11, 2018
104c0c4
update tvOS images
liuxuan30 Oct 11, 2018
e6098f3
update tolerance to 1% & more swift-y
liuxuan30 Oct 16, 2018
76b1f6f
update tvOS images, removing "Description Label", (not rendered anymore)
liuxuan30 Oct 16, 2018
d5d9abd
update iOS test images, "Description Label" no longer rendered.
liuxuan30 Oct 16, 2018
8da5272
fixed offset calculation in some cases.
potato04 Oct 19, 2018
8ae48b9
Update Source/Charts/Renderers/BarChartRenderer.swift
jjatie Oct 22, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Source/Charts/Components/YAxis.swift
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,17 @@ open class YAxis: AxisBase
var min = _customAxisMin ? _axisMinimum : dataMin
var max = _customAxisMax ? _axisMaximum : dataMax

// Make sure max is greater than min
if min > max {
liuxuan30 marked this conversation as resolved.
Show resolved Hide resolved
liuxuan30 marked this conversation as resolved.
Show resolved Hide resolved
if _customAxisMax && _customAxisMin {
(min, max) = (max, min)
} else if _customAxisMax && !_customAxisMin {
min = max - 1
Copy link
Member

@liuxuan30 liuxuan30 Sep 29, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@potato04 potato04 Oct 6, 2018

Choose a reason for hiding this comment

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

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()

} else if !_customAxisMax && _customAxisMin {
max = min + 1
}
}

// temporary range (before calculations)
let range = abs(max - min)

Expand Down
46 changes: 26 additions & 20 deletions Source/Charts/Renderers/BarChartRenderer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,30 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer
var barRect = CGRect()
var x: Double
var y: Double



// When drawing with an auto calculated y-axis minimum, the renderer actually draws each bar from 0
// to the required value. This drawn bar is then clipped to the visible chart rect in BarLineChartViewBase's draw(rect:) using clipDataToContent.
// While this works fine when calculating the bar rects for drawing, it causes the accessibilityFrames to be oversized in some cases.
// This offset attempts to undo that unnecessary drawing when calculating barRects, particularly when not using custom axis minima.
// This allows the minimum to still be visually non zero, but the rects are only drawn where necessary.
// This offset calculation is not necessary when bars won't be clipped (when y-axis minimum <= 0 and y-axis maximum >= 0)
var heightOffset: CGFloat = 0.0
var originYOffset: CGFloat = 0.0
if let offsetView = dataProvider as? BarChartView {
let offsetAxis = offsetView.leftAxis.isEnabled ? offsetView.leftAxis : offsetView.rightAxis
if !offsetAxis._customAxisMin {
if barData.yMin > 0 {
heightOffset = CGFloat(offsetAxis.axisMinimum)
}
}
if !offsetAxis._customAxisMax {
Copy link
Member

@liuxuan30 liuxuan30 Sep 28, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

if barData.yMax < 0 {
originYOffset = CGFloat(offsetAxis.axisMaximum)
}
}
}

for i in stride(from: 0, to: min(Int(ceil(Double(dataSet.entryCount) * animator.phaseX)), dataSet.entryCount), by: 1)
{
Expand Down Expand Up @@ -139,28 +163,10 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer
bottom *= CGFloat(phaseY)
}

// When drawing with an auto calculated y-axis minimum, the renderer actually draws each bar from 0
// to the required value. This drawn bar is then clipped to the visible chart rect in BarLineChartViewBase's draw(rect:) using clipDataToContent.
// While this works fine when calculating the bar rects for drawing, it causes the accessibilityFrames to be oversized in some cases.
// This offset attempts to undo that unnecessary drawing when calculating barRects, particularly when not using custom axis minima.
// This allows the minimum to still be visually non zero, but the rects are only drawn where necessary.
// This offset calculation also avoids cases where there are positive/negative values mixed, since those won't need this offset.
var offset: CGFloat = 0.0
if let offsetView = dataProvider as? BarChartView {

let offsetAxis = offsetView.leftAxis.isEnabled ? offsetView.leftAxis : offsetView.rightAxis

if barData.yMin.sign != barData.yMax.sign { offset = 0.0 }
else if !offsetAxis._customAxisMin {
offset = CGFloat(offsetAxis.axisMinimum)
}
}

barRect.origin.x = left
barRect.origin.y = top + originYOffset
barRect.size.width = right - left
barRect.origin.y = top
barRect.size.height = bottom == top ? 0 : bottom - top + offset

barRect.size.height = bottom - top + heightOffset - originYOffset
buffer.rects[bufferIndex] = barRect
bufferIndex += 1
}
Expand Down