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

Add support for rounded corners for BarChart #3754

Closed

Conversation

skarol
Copy link

@skarol skarol commented Nov 21, 2018

Issue Link πŸ”—

Rounding the corners of bars #1066

Goals ⚽

image

  • Add ability to draw rounded corners for BarChartView and HorizontalBarChartView.
  • Support also stacked bar charts.
  • Corner radius should be calculated automatically, depending on chart's width and height.
  • To use it simply add dataSet.roundedCorners = [.allCorners].

Implementation Details 🚧

  • Add roundedCorners property for BarChartDataSet, where you can specify which corners should be rounded.
  • Introduce changes in drawDataSet function in BarChartRenderer and HorizontalBarChartRenderer, where UIBezierPath is created to clip bar filling.
  • Similar changes apply to drawHighlighted function in BarChartRenderer.

Testing Details πŸ”

Add testRoundedCorners test to BarChartTests.

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 4, 2018

except for CI errors, is problem solved for what I asked #1066 (comment) ?

@skarol
Copy link
Author

skarol commented Dec 4, 2018

@liuxuan30

  1. CI errors occurred due to a slight difference in drawing bars approach. Look at the diffs. IMO it's not possible (without hacking - like massive if with old and new approach) to implement this feature and have CI passing.
  2. Do you have bars thickness in mind? It should work for thin and fat bars as well.

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 11, 2018

@skarol I mean if the bars are thin enough, it becomes an oval more like a bar with rounded corners. That's what I said a flaw in #1066 (comment), and hold similar PRs till now. Have you solved this? (sorry I don't have time to download your PR and test it on my side so far)

If so, I could take a deep look and maybe we could fix the CI errors and get this merged.

@skarol
Copy link
Author

skarol commented Dec 11, 2018

@liuxuan30 yes, I've solved it. Basically, corner radius is half of bar's width for BarCharView and half of bar's height for HorizontalBarChart. For very thin bars it will look like this:
image
and for fat ones:
image

@liuxuan30
Copy link
Member

That's fantastic. we will take time for this. But maybe longer :(
have to take a look at CI errors first

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 24, 2018

@jjatie which branch do you think we should use to merge this PR? I'm asking because this is a nice feature that waited for too long (if it solves my questions), so I'm hoping to add them soon in master. But I'm seeing conflicts with 4.0 refactor.

@@ -377,9 +377,9 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer
// draw the bar shadow before the values
if dataProvider.isDrawBarShadowEnabled
{
for j in stride(from: 0, to: buffer.rects.count, by: 1)
for firstIndexInBar in stride(from: 0, to: buffer.rects.count, by: 1)
Copy link
Member

Choose a reason for hiding this comment

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

why change name here? seems nothing about first index

Copy link
Collaborator

@jjatie jjatie Dec 25, 2018

Choose a reason for hiding this comment

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

I have the same question. If anything the change should be:

 for barRect in buffer.rects
    where viewPortHandler.isInBoundsLeft(barRect.maxX)
{
    guard viewPortHandler.isInBoundsRight(barRect.origin.x) else
    {
          break
    }
    context.setFillColor(dataSet.barShadowColor.cgColor)
    context.fill(barRect)
}

@@ -896,4 +934,32 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer

return element
}

private func findTopRectInBar(barRects: [CGRect], firstIndexInBar: Int, lastIndexInBar: Int) -> CGRect {
Copy link
Member

@liuxuan30 liuxuan30 Dec 24, 2018

Choose a reason for hiding this comment

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

let's put all { to another line. This is the code style for this library

@@ -628,4 +655,31 @@ open class HorizontalBarChartRenderer: BarChartRenderer
{
high.setDraw(x: barRect.midY, y: barRect.origin.x + barRect.size.width)
}

override internal func createBarPath(for rect: CGRect, roundedCorners: UIRectCorner) -> UIBezierPath {
Copy link
Member

Choose a reason for hiding this comment

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

could bar renderer and horizontal bar renderer share the same createBarPath and findMostLeftRectInBar somehow?

@@ -339,4 +339,12 @@ class BarChartTests: FBSnapshotTestCase
chart.notifyDataSetChanged()
FBSnapshotVerifyView(chart, identifier: Snapshot.identifier(UIScreen.main.bounds.size), tolerance: Snapshot.tolerance)
}

func testRoundedCorners() {
Copy link
Member

Choose a reason for hiding this comment

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

not seeing updated images for testing rounded corners.

@@ -402,53 +402,81 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer
{
context.setFillColor(dataSet.color(atIndex: 0).cgColor)
}

context.setStrokeColor(borderColor.cgColor)
Copy link
Member

Choose a reason for hiding this comment

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

it seems you are using drawBorder properties for rounded corners. Why force setting setLineCap(.square)?

But what if people disable drawBorder?


// In case the chart is stacked, we need to accomodate individual bars within accessibilityOrdereredElements
let isStacked = dataSet.isStacked
let stackSize = isStacked ? dataSet.stackSize : 1

for j in stride(from: 0, to: buffer.rects.count, by: 1)
for firstIndexInBar in stride(from: 0, to: buffer.rects.count, by: stackSize)
Copy link
Member

Choose a reason for hiding this comment

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

why stackSize here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it was done for the stacked bar char case, if round each bar when we will have this situation (from my rounding implementation).
StackedBarChar_rounded
Author's example:

Choose a reason for hiding this comment

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

Seems like it was done for the stacked bar char case, if round each bar when we will have this situation (from my rounding implementation).
StackedBarChar_rounded
Author's example:

That's exactly what I wanted to do. Round corner each bar with space in between. Do you have a demo project?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @panncherry
You can check my fork/branch. Do not remember if bothered with an example adjustment as not going to upload a pull request, but the changes should be pretty straightforward to update existing StackedBarChar from the ChartsDemo-iOS

@liuxuan30
Copy link
Member

liuxuan30 commented Dec 24, 2018

@skarol it seems CI failed because multiple bar chart tests failures. If rounded corner is turned on by default, then that's the reason. we need to have a switch to turn it off and add a new rounded bar test.

@@ -377,9 +377,9 @@ open class BarChartRenderer: BarLineScatterCandleBubbleRenderer
// draw the bar shadow before the values
if dataProvider.isDrawBarShadowEnabled
{
for j in stride(from: 0, to: buffer.rects.count, by: 1)
for firstIndexInBar in stride(from: 0, to: buffer.rects.count, by: 1)
Copy link
Collaborator

@jjatie jjatie Dec 25, 2018

Choose a reason for hiding this comment

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

I have the same question. If anything the change should be:

 for barRect in buffer.rects
    where viewPortHandler.isInBoundsLeft(barRect.maxX)
{
    guard viewPortHandler.isInBoundsRight(barRect.origin.x) else
    {
          break
    }
    context.setFillColor(dataSet.barShadowColor.cgColor)
    context.fill(barRect)
}

{
// Set the color for the currently drawn value. If the index is out of bounds, reuse colors.
context.setFillColor(dataSet.color(atIndex: j).cgColor)
for index in firstIndexInBar...lastIndexInBar {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let barRects = buffer.rects[firstIndexInBar...lastIndexInBar]
for (index, rect) in zip(barRects, barRects.indices)
    where viewPortHandler.isInBoundsLeft(barRect.maxX)
{
    ...
}

}

var height: CGFloat = 0
for index in firstIndexInBar...lastIndexInBar {
Copy link
Collaborator

Choose a reason for hiding this comment

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

topRectInBar.size.height = barRects[firstIndexInBar...lastIndexInBar]
    .reduce(0) { $0 + $1.height }

}

/// Creates path for bar in rect with rounded corners
internal func createBarPath(for rect: CGRect, roundedCorners: UIRectCorner) -> UIBezierPath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

internal is default and is not needed.

@jjatie
Copy link
Collaborator

jjatie commented Dec 25, 2018

@liuxuan30 I think we should start only targeting 4.0.0. Will encourage us to get it out faster and with more features in it, encourage people to update and help us keep it bug free.

@samjarman
Copy link

Any update on this @skarol? @liuxuan30? :)

@liuxuan30
Copy link
Member

not really.. have to postpone, working on 3.3 release and PRs

@skarol
Copy link
Author

skarol commented Mar 4, 2019

Sorry, have been busy lately :/ I'll try to address this soon

@samjarman
Copy link

samjarman commented Mar 5, 2019

One more question about this PR for @skarol and @liuxuan30 - the rounded corners seem to be perfectly semicircular - what about exposing the option to round corners with a given corner radius? This would be much more flexible and design agnostic. I'm guessing it is not really in the spirit of such a library to dictate design decisions like this? :)

@skarol
Copy link
Author

skarol commented Mar 5, 2019

@samjarman I decided to not expose corner radius and calculate it as a semicircular on a purpose. When creating a chart you have no idea about bars width and height so it is hard to pass good looking corner radius and semicircular always looks good :)

@samjarman
Copy link

Hmm, I thought it'd be possible for a developer to calculate their own corner radius (such as the one on these input fields and comments on github) based on the width of the bar?

If you can't get the width of the bar, maybe set a corner radius and then a minimum width for the corner radius to be used?

@samjarman
Copy link

I think "semicircular always looks good" is our designers decisions not really a framework's decision, y'know? :)

tastysauce added a commit to life360/Charts that referenced this pull request Mar 7, 2019
@iDevid
Copy link

iDevid commented Apr 24, 2019

Some news about this?

@FrancescoTr
Copy link

Really hoping to see this feature implemented

@igorkulman
Copy link

If the main problem is that it does not look "right" in some situations, why not add an additional parameter?

Something like

enum Radius {
    case none
    case widthPercentage(CGFLoat)
    case absolute(CGFLoat)
}

With .widthPercentage(50.0) matching the current implementation.

@iDevid
Copy link

iDevid commented May 23, 2019

Some news about this?

@brunodalarosa
Copy link

Looking forward to use this feature, any news? Thanks for the great work on this lib :D

@denandreychuk
Copy link

Can someone help me? I got Thread 1: EXC_BAD_ACCESS (code=1, address=0x0) on bar char init when tries to install using pods from this branch:

This one works:
pod 'Charts'

This one doesn't:
pod 'Charts', :git => 'https://github.com/danielgindi/Charts.git', :branch => 'roundedBars'

Anybody faced with this problem?
Seems like I should make some additional steps, because installing from master doesn't work too.

pod 'Charts', :git => 'https://github.com/danielgindi/Charts.git', :branch => 'master'

@denandreychuk
Copy link

Maybe it helps someone. To get this feature works, add these to your pod file:
pod 'Charts', :git => 'https://github.com/tooploox/Charts.git', :branch => 'feature/rounded-corners'

If you have the same crash as me, just reinstall Charts in pod file.


let cornerRadius = rect.width / 2.0

let path = UIBezierPath(roundedRect: rect,

Choose a reason for hiding this comment

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

@skarol I see a problem here when the rect is smaller than the cornerRadius, in this case you'll have a smashed rounded rectangle that looks bad(I haven't tested this code, but I worked in this feature in my fork, and at the start I had used the UIBezierPath like you with the result that I have commented, some screenshots at the end, apologies for the quality).
My proposal to solve this is to split it into two cases(Take into consideration that I haven't used the open var roundedCorners: UIRectCorner = []. I will always round all corners)

  1. rect.height >= rect.width in this case we can perform the UIBezierPath as you do
  2. rect.height < rect.width in this case we can draw a portion of a circumference, I leave the algorithm
let height = rect.size.height
let hypotenuse = width / 2.0
let radius = hypotenuse
let startAngle: CGFloat
let endAngle: CGFloat
if height < radius {
    let oLeg = radius - height
    startAngle = asin(oLeg/hypotenuse)
    endAngle = CGFloat(Double.pi) - startAngle
} else {
    let oLeg = height - radius
    startAngle = -asin(oLeg/hypotenuse)
    endAngle = CGFloat(Double.pi) - startAngle
}
let y = rect.origin.y - (width - rect.size.height)
let center = CGPoint(x: rect.origin.x + radius, y: y + radius)
let bezierPath = UIBezierPath(arcCenter: center, radius: radius, startAngle: startAngle, endAngle: endAngle, clockwise: true)

Before
before2
before3

After
share1
share

@iDevid
Copy link

iDevid commented Jan 23, 2020

why this was closed? @danielgindi

@amit2908
Copy link

Rounded Rect seems an important requirement @danielgindi . Please reopen it and merge changes.

@danielgindi
Copy link
Collaborator

why this was closed? @danielgindi

Friends I can’t remember every PR but please read the history here. There are issues that weren’t addressed and there is another branch that does this.

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 15, 2020

@danielgindi it's weird our branch roundedBars is gone
But I do remember this PR, this is a new PR compared to an old one #1917 , this seems more promising, but the author seemed paused. I was planning to modify and merge it and I found out we have to solve the conflicts, and require feedback from him/her, but no reply yet

@liuxuan30 liuxuan30 mentioned this pull request Apr 17, 2020
2 tasks
@liuxuan30
Copy link
Member

I will reopen this one as this seems the best rounded bar by far. The poblem is we need help from @skarol, or we take it from here.

@liuxuan30 liuxuan30 reopened this Apr 17, 2020
@pylapp
Copy link

pylapp commented May 5, 2020

Hi!

Could you be nice to process this pull request?
It may be cool to have rounded corners for bars :)

Cheers

@rubenmfdeveloper
Copy link

Hi everyone!

There is something I could do to add the feature ASAP?
I really need this feature @skarol

Thanks,

@Pearapps
Copy link

I put up a PR that adds this functionality as well: #4375

No clue if it will get in (likely not, for similar reasons as this one) - but its a more up to date approach you can probably pull in / play with.

@bivant
Copy link
Contributor

bivant commented Jul 28, 2020

Hello.
I think that we need to agree on the functionality that rounding should support. Like corners (top, left...) and radius formula previous suggestion. Probably need to create separate class for the rounding parameters to not be across the several different parameters in the chart class.
Android conterpart seems like allow to achieve this by overriding the renderer
Radius
I suggest formula for the radius: widthFactor * barWidth / 2 + fixedRadiusValue
Obviously widthFactor should apply to the half of the bar as typical case is rounding bar edge. So it seems more intuitive to me that widthFactor == 1.0 leads to that result
This will allow:

  • specify rounding like the author meant - radius is half of a bar width (height if horizontal chart)
  • specify constant radius (the case I need)
  • any combination of above

Edge cases:

  • what to do when formula value exceeds half of the bar width (both parameters are set, bar too narrow to fit in the constant)? Use half of the width or respect fixedRadiusValue?
  • If fixedRadiusValue too big and factor is 0/ignored - lower radius to half of the width or set it to 0 so we see "regular" rectangular bar? The @liuxuan30 issue
  • too small value bar behavior suggestion Add support for rounded corners for BarChartΒ #3754 (comment) . Is this a problem? Should it be configurable?

Corners
It seems like a nice to have control other this as the library should allow to customize the output as much as possible.
What about stacked bar then? Apply bottom corners for the bottom most part and top corners to the topmost one? Or add another parameter so each bar can have the rounding? Is this case real?

@liuxuan30
Copy link
Member

just FYI that we are actively working on the long waited 4.0 merge right now, so if anyone want to contribute on this one, wait until we merge 4.0.
It is a huge update with new features and more importantly, it's more swifty, so some APIs would need a change.

@pratikgajbhiye222
Copy link

just FYI that we are actively working on the long waited 4.0 merge right now, so if anyone want to contribute on this one, wait until we merge 4.0.
It is a huge update with new features and more importantly, it's more swifty, so some APIs would need a change.

Can I use this function for rounded corner?

@antondityativ
Copy link

@liuxuan30 Could you say when this feature will be in the master branch?

@liuxuan30
Copy link
Member

nope, not any estimatable time. I mean it's a big change and the author seems not responding. Besides, we are having 4.0 branch merging ahead, and 4.0 branch is already big enough and will cause more code break for this PR. Without the author's help, I don't think we can merge this fairly.

@bivant
Copy link
Contributor

bivant commented Dec 14, 2020

Hello @liuxuan30
I am ready to dive in the rounding feature implementation in this PR or in my own in the next couple of weeks as master is 4.0 now. But need to decide which features should be supported first.

@haswiniparmar1
Copy link

I want to give a corner radius to shadow in a bar chart view graph. How can I do this? @liuxuan30
Graph's highlight and the value filled portion has curve but the shadow has not. Please let me know if you have any solution for that.

@jjatie jjatie closed this Jan 26, 2021
@jjatie jjatie deleted the branch ChartsOrg:roundedBars January 26, 2021 13:08
@4np 4np mentioned this pull request Jan 29, 2021
@charsdavy
Copy link

why this was closed? @jjatie

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

Successfully merging this pull request may close these issues.