Added gradient line drawing to LineChartRenderer. based on PR #3142 #3415

larryonoff · 2018-04-24T11:33:03Z

Some stabilization and code cleanup.

@jjatie could you please take a look at this PR. This PR stabilises and improves your PR #3142

jjatie · 2018-04-25T10:49:05Z

First glance looks okay. Please fix build failures. What do you mean by "stabilization"?

I'll review thoroughly once fixed.

jjatie · 2018-04-25T10:59:04Z

Source/Charts/Renderers/LineChartRenderer.swift

-            context.setStrokeColor(drawingColor.cgColor)
-            context.strokePath()
-        }
+        drawLine(context: context, dataSet: dataSet, spline: cubicPath, matrix: valueToPixelMatrix, drawingColor: drawingColor)


I thought about doing this. I think this correct way to do it is

if dataSet.isDrawLineWithGradientEnabled { drawGradientLine(context: context, dataSet: dataSet, spline: cubicPath, matrix: valueToPixelMatrix) } else { drawLine(in: ...) }

I exposed this logic into a separate function with the name drawLine.

I'm aware. I'm saying that drawLine and drawGradientLine should be separate methods.

Just pushed a change. I think that this's correct now.

jjatie · 2018-04-25T10:59:52Z

Source/Charts/Renderers/LineChartRenderer.swift

@@ -795,6 +806,12 @@ open class LineChartRenderer: LineRadarRenderer

    func drawGradientLine(context: CGContext, dataSet: LineChartDataSetProtocol, spline: CGPath, matrix: CGAffineTransform)
    {
+        guard let gradientPositions = dataSet.gradientPositions else
+        {
+            assertionFailure("Must set `gradientPositions if `dataSet.isDrawLineWithGradientEnabled` is true")


👍 I think we could use more of this.

larryonoff · 2018-04-25T12:08:29Z

@jjatie

First glance looks okay. Please fix build failures. What do you mean by "stabilization"?

fixed out of bounds crash in generateGradientLinePath at for i in (from + 1)..<to by adding guard (from + 1) < to.
fixed how gradient colors are picked, see drawGradientLine, i.e. the first data set color was always selected for points in the middle.

larryonoff · 2018-04-25T12:13:46Z

I'll fix the crash. It's because different return parameters of the func getRed(red:, green:, blue: , alpha:) for NSColor and UIColor.

codecov-io · 2018-04-25T14:36:48Z

Codecov Report

Merging #3415 into 4.0.0 will decrease coverage by 0.15%.
The diff coverage is 37.41%.

@@            Coverage Diff             @@
##            4.0.0    #3415      +/-   ##
==========================================
- Coverage   29.42%   29.26%   -0.16%     
==========================================
  Files         114      114              
  Lines       12605    12683      +78     
==========================================
+ Hits         3709     3712       +3     
- Misses       8896     8971      +75

Impacted Files	Coverage Δ
...ta/Implementations/Standard/LineChartDataSet.swift	`32.65% <ø> (ø)`	⬆️
Source/Charts/Utils/Platform.swift	`12.9% <0%> (-2.49%)`	⬇️
Source/Charts/Renderers/LineChartRenderer.swift	`51.97% <40.31%> (-5.25%)`	⬇️

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 08bd20c...aee20f0. Read the comment docs.

larryonoff · 2018-04-25T14:37:14Z

It looks that gradient locations are calculated wrong. I hope that I'll fix it tomorrow.

jjatie · 2018-04-25T14:37:13Z

Source/Charts/Renderers/LineChartRenderer.swift

+        context: CGContext,
+        dataSet: LineChartDataSetProtocol,
+        spline: CGMutablePath,
+        matrix: CGAffineTransform,


unused parameter

jjatie · 2018-04-25T14:37:23Z

Source/Charts/Renderers/LineChartRenderer.swift

+
+    private func drawLine(
+        context: CGContext,
+        dataSet: LineChartDataSetProtocol,


unused parameter

larryonoff · 2018-04-27T12:17:31Z

What anyone thing about implementing similar approach like for fill, i.e.

open class ChartGradient {
  var start: CGPoint
  var end: CGPoint
  var positions: [CGFloat]?
  var colors: [UIColor]?
}

jjatie · 2018-04-27T13:28:59Z

That sounds like a great idea. I think the properties can all be let decorations. There are many areas I'd like to introduce this sort of data type.

larryonoff · 2018-04-27T13:31:01Z

@jjatie what do you think about class name? is it fine? or better LineChartGradient, LineChartLineGradient?

jjatie · 2018-04-27T13:32:51Z

Hold off on doing it in this PR, a separate one would be greatly appreciated where you implement this across all gradient usage in the framework.

jjatie · 2018-04-27T17:29:44Z

Source/Charts/Renderers/LineChartRenderer.swift

@@ -785,6 +796,11 @@ open class LineChartRenderer: LineRadarRenderer

        // create a new path
        let to = Int(ceil(CGFloat(to - from) * phaseX + CGFloat(from)))
+
+        guard (from + 1) < to else {


Why do we need this?
Maybe let from = from + 1 instead?
Would be nice to have a comment for why we use + 1

Why do we need this?

this's because path generations starts from command move for the first point, and then add line.

jjatie · 2018-04-27T17:31:05Z

Source/Charts/Renderers/LineChartRenderer.swift

-        {
-            fatalError("Must set `gradientPositions if `dataSet.isDrawLineWithGradientEnabled` is true")
+        let boundingBox = spline.boundingBox
+        guard !boundingBox.isNull && !boundingBox.isInfinite && !boundingBox.isEmpty else {


comma instead of ampersand. We're listing requirements, not performing Boolean logic.

jjatie · 2018-04-27T17:37:22Z

Source/Charts/Utils/Platform.swift

@@ -304,6 +320,26 @@ types are aliased to either their UI* implementation (on iOS) or their NS* imple
        }
 	}

+    extension NSUIColor
+    {
+        var nsuiRGBA: (red: CGFloat, green: CGFloat, blue: CGFloat, alpha: CGFloat)? {


Rename to rgba

jjatie · 2018-04-27T17:40:43Z

Source/Charts/Utils/Platform.swift

+
+    extension NSUIColor
+    {
+        var nsuiRGBA: (red: CGFloat, green: CGFloat, blue: CGFloat, alpha: CGFloat)? {


larryonoff · 2018-04-28T09:38:13Z

@jjatie I think it would be great to rebase this PR into 4.0.0 branch. Already did this.

- additional code optimizations

pmairoldi · 2018-06-10T21:48:56Z

Thanks 👍

jjatie reviewed Apr 25, 2018

View reviewed changes

jjatie requested changes Apr 25, 2018

View reviewed changes

larryonoff force-pushed the draw-gradient-line branch from 5bfb844 to 98d4148 Compare April 26, 2018 11:55

larryonoff changed the title ~~Stabilize and clean the code~~ Stabilize and clean the code for draw gradient line PR Apr 26, 2018

larryonoff force-pushed the draw-gradient-line branch from 98d4148 to 7389b76 Compare April 26, 2018 14:25

jjatie requested changes Apr 27, 2018

View reviewed changes

larryonoff force-pushed the draw-gradient-line branch from 73aa129 to 729fb18 Compare April 28, 2018 10:17

larryonoff changed the base branch from draw-gradient-line to 4.0.0 April 28, 2018 10:17

larryonoff changed the title ~~Stabilize and clean the code for draw gradient line PR~~ Added gradient line drawing to LineChartRenderer v2. base on PR #3142 Apr 28, 2018

larryonoff changed the title ~~Added gradient line drawing to LineChartRenderer v2. base on PR #3142~~ Added gradient line drawing to LineChartRenderer. based on PR #3142 Apr 28, 2018

larryonoff force-pushed the draw-gradient-line branch from 617fa81 to 3c20de9 Compare April 28, 2018 11:18

Jacob Christie and others added 8 commits May 2, 2018 13:25

Added gradient line drawing to LineChartRenderer

3db09d9

Stabilize and clean the code

8981f32

Extract line drawing into function

1c65999

Fix macOS build

25eb164

Move drawGradientLine out of drawLine method

a13978f

Remove unused parameters from drawLine function

470328a

Fix gradient location calculation

6bd7bbd

Add toggle gradient line into demo options

9f20679

larryonoff added 4 commits May 2, 2018 13:25

Improvements after code review

342e088

Code cleanup

50b29b4

Remove unnecessary function for generating gradient line

d7814af

- additional code optimizations

Fix: gradient lines peaks are truncated when line width > 1

aee20f0

larryonoff force-pushed the draw-gradient-line branch from 3c20de9 to aee20f0 Compare May 2, 2018 10:26

pmairoldi approved these changes Jun 10, 2018

View reviewed changes

pmairoldi merged commit 1b387d8 into ChartsOrg:4.0.0 Jun 10, 2018

larryonoff deleted the draw-gradient-line branch June 11, 2018 05:46

larryonoff mentioned this pull request Jun 11, 2018

Added gradient line drawing to LineChartRenderer #3142

Closed

Kalvin126 mentioned this pull request Jul 14, 2018

Add a gradient line chart #649

Closed

liuxuan30 mentioned this pull request Apr 16, 2020

Backported @larryonoff’s gradient line drawing implementation from 4.0 to 3.4 #4327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added gradient line drawing to LineChartRenderer. based on PR #3142 #3415

Added gradient line drawing to LineChartRenderer. based on PR #3142 #3415

larryonoff commented Apr 24, 2018 •

edited

Loading

jjatie commented Apr 25, 2018

jjatie Apr 25, 2018

larryonoff Apr 25, 2018 •

edited

Loading

jjatie Apr 25, 2018

larryonoff Apr 25, 2018

jjatie Apr 25, 2018 •

edited

Loading

larryonoff commented Apr 25, 2018

larryonoff commented Apr 25, 2018

codecov-io commented Apr 25, 2018 •

edited

Loading

larryonoff commented Apr 25, 2018

jjatie Apr 25, 2018

larryonoff Apr 25, 2018

jjatie Apr 25, 2018

larryonoff Apr 25, 2018

larryonoff commented Apr 27, 2018

jjatie commented Apr 27, 2018

larryonoff commented Apr 27, 2018

jjatie commented Apr 27, 2018 •

edited

Loading

jjatie Apr 27, 2018

larryonoff Apr 28, 2018

jjatie Apr 27, 2018

jjatie Apr 27, 2018

jjatie Apr 27, 2018

larryonoff commented Apr 28, 2018 •

edited

Loading

pmairoldi commented Jun 10, 2018

Added gradient line drawing to LineChartRenderer. based on PR #3142 #3415

Added gradient line drawing to LineChartRenderer. based on PR #3142 #3415

Conversation

larryonoff commented Apr 24, 2018 • edited Loading

jjatie commented Apr 25, 2018

Choose a reason for hiding this comment

larryonoff Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjatie Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

larryonoff commented Apr 25, 2018

larryonoff commented Apr 25, 2018

codecov-io commented Apr 25, 2018 • edited Loading

Codecov Report

larryonoff commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larryonoff commented Apr 27, 2018

jjatie commented Apr 27, 2018

larryonoff commented Apr 27, 2018

jjatie commented Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larryonoff commented Apr 28, 2018 • edited Loading

pmairoldi commented Jun 10, 2018

larryonoff commented Apr 24, 2018 •

edited

Loading

larryonoff Apr 25, 2018 •

edited

Loading

jjatie Apr 25, 2018 •

edited

Loading

codecov-io commented Apr 25, 2018 •

edited

Loading

jjatie commented Apr 27, 2018 •

edited

Loading

larryonoff commented Apr 28, 2018 •

edited

Loading