ChartsOrg / Charts

Beautiful charts for iOS/tvOS/OSX! The Apple side of the crossplatform MPAndroidChart.
Apache License 2.0
27.6k stars 6k forks source link

PROPOSAL: Add new Gradient type #3428

Open jjatie opened 6 years ago

jjatie commented 6 years ago

Motivation

There are many feature requests involving gradients, in addition to the currently existing gradient related features. PRs have been submitted with multiple implementations and concepts as to how to describe a gradient. In an effort to standardize our gradient support, we would like to introduce a new Gradient data type.

credit: @larryonoff

Impletmentation

In a pure Swift implementation, this would be:

struct LinearGradient: NSObject
{
    let start: CGPoint
    let end: CGPoint
    let positions: [CGFloat]
    let colors: [UIColor]
}

However, as we are still currently supporting objc, this is required:

final class LinearGradient: NSObject, NSCopying
{
    let start: CGPoint
    let end: CGPoint
    let positions: [CGFloat]
    let colors: [UIColor]
}

To simplify creation of gradients, we should introduce convenience initializers for standard gradient positions and default colour options.

It may make sense to introduce additional gradient types (radial, etc), however further discussion should be had as to whether it is necessary. If we are going to do so, we should do it the Swift way, by defining a Gradient protocol instead of subclassing to make the eventual transition away from objective-c easier.

jjatie commented 6 years ago

@larryonoff @liuxuan30

larryonoff commented 6 years ago

The similar class can be introduced for radial gradient like func drawRadialGradient(CGGradient, startCenter: CGPoint, startRadius: CGFloat, endCenter: CGPoint, endRadius: CGFloat, options: CGGradientDrawingOptions)

final class RadialGradient: NSObject, NSCopying
{
    let startCenter: CGPoint
    let startRadius: CGFloat
    let endCenter: CGPoint
    let endRadius: CGFloat

    let positions: [CGFloat]
    let colors: [UIColor]
}
larryonoff commented 6 years ago

I think it may be useful adding one more parameter, i.e. let options: CGGradientDrawingOptions

jjatie commented 6 years ago

Seems reasonable. I feel like there should be a default value for this parameter as well in the initializer. I'm thinking []

larryonoff commented 6 years ago

One more protocol can be introduced for extensibility

protocol GradientRenderer {
   func draw(_ gradient: CGGradient, in context: CGContext)
}

so that RadialGradient and LinearGradient conform it.

jjatie commented 6 years ago

Please target the 4.0.0 branch for this PR.

jjatie commented 6 years ago

I think the naming should be different if the Gradient's themselves are conforming to it. Either Drawable and implement it's own draw(in context: CGContext) or, what I think makes more sense, the Renderer is a separate type altogether.

larryonoff commented 6 years ago

@jjatie Is there any place that these types can be used ?

jjatie commented 6 years ago

I've definitely looked at a lot of gradient work, but I can't remember what's merged and what's not. At work right now, so I'll have to check later.

Regardless I think this is a welcome addition, and we should have more things like it.

larryonoff commented 6 years ago

I like draw(in context: CGContext) but it looks that CGGradient should be an input parameter for protocol since of signatures of these two methods below.

func drawLinearGradient(CGGradient, start: CGPoint, end: CGPoint, options: CGGradientDrawingOptions)

func drawRadialGradient(CGGradient, startCenter: CGPoint, startRadius: CGFloat, endCenter: CGPoint, endRadius: CGFloat, options: CGGradientDrawingOptions)
jjatie commented 6 years ago

This is where a separate Renderer type comes in. If we have

protocol Drawable {
    func draw(in context: CGContext)
}

struct RadialGradient: Drawable {
    // properties

    func draw(in context: CGContext) {
    }
}

It doesn't make sense for the gradient to pass itself into it's own method.

It does make sense though to have

protocol GradientRenderer: Renderer {
    func draw(_ gradient: Gradient, in context: CGContext)
}

and attach the protocol on the appropriate renders.

The more I think about this, the more I wonder if we shouldn't abstract this more i.e.

protocol Drawable {
    func draw(in context: CGContext)
}

protocol Renderer {
    func render(_ drawable: Drawable, in context: CGContext)
}

extension Renderer {
    func draw(_ drawable: Drawable, in context: CGContext) {
        drawable.draw(in: context)
    }
}
larryonoff commented 6 years ago

I created some basic PR #3429. That of course needs improvements / discussions / changes.

jjatie commented 6 years ago

@larryonoff I messaged you on Gitter

jjatie commented 6 years ago

The alternative is to rely solely on CGGradient. I think internally we should only rely on it, but the consumer should get a nicer API to work with. A question I have is, how are positions defined in our Gradient type? InCGGradient, they represent a fraction of the total drawn gradient, and the locations on screen are accepted by CGContext.drawLinearGradient method.

larryonoff commented 6 years ago

I see the following options for locations:

  1. locations like in CGGradient from 0 to 1.
  2. locations relative to the same values that axis use.

I think that option 2 is better since it's easier to define gradient transition locations relative to axis values.

We may introduce the following enum to cover both options 1 and 2.

enum GradientLocationsStrategy {
  case relative
  case normalised
}

PS. I don't like using CGGradient in protocol. But I don't see better way to calculate locations yet.

larryonoff commented 6 years ago

@jjatie Hi! I've replied you in Gitter.