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

Update progress graph line algorithm #1239

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

neenjaw
Copy link
Collaborator

@neenjaw neenjaw commented Jul 12, 2021

previously a sliding window algorithm was being used which found the line connect the prev/next points and used as parallel line to create the bezier control points. This was leading to a "dipping" behaviour when there were extreme changes in the datapoints. This is because in order for the bezier line to curve to accomodate all of the point, it sometimes must curve the opposite way to accomodate the extremeness of a curve.

Screenshot from 2021-07-11 20-58-51

Example of the "Dip"

Now using a catmullrom spline to bezier curve algorithm (http://schepers.cc/svg/path/catmullrom2bezier.js) to produce a more predictable curve, and then when there is an extreme curve, points are being interpolated to ease it in. There is a chance that this causes the same "dipping" after the curve rather than preceed it, but this is the limitation of using a spline to graph datapoints rather than computing a best-fit line.

Screenshot from 2021-07-06 00-33-12

Using catmullrom spline (translated to bezier) with interpolated points

I think if there are further aesthetic concerns, probably would be good consider creating a high-resolution (many many data points) poly-line so that it appears smooth, but is more reflective of the true data.

previously a sliding window algorithm was being used which found the
    line connect the prev/next points and used as parallel line to create
    the bezier control points.  This was leading to a "dipping" behaviour
    when there were extreme changes in the datapoints.  This is because in
    order for th ebezier line to curve to accomodate all of the point, it
    sometimes must curve the opposite way to accomodate the extremeness of
    a curve.

    Now using a catmullrom spline to bezier curve algorithm to produce a
    more predictable curve, and then when there is an extreme curve, points
    are being interpolated to ease it in.  There is a chance that this
    causes the same "dipping" after the curve rather than preceed it, but
    this is the limitation of using a spline to graph datapoints rather than
    computing a best-fit line.

    I think if there are further aesthetic concerns, probably would be good
    consider creating a high-resolution (many many data points) poly-line so
    that it appears smooth, but is more reflective of the true data.
Comment on lines +52 to +60
<mask
id={`progress-graph-color-mask-${getRandomId()}`}
x="0"
y="0"
width={width}
height={height}
>
<path d={path} stroke="white" fill="transparent" />
</mask>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As it turns out, when a vertical gradient is applied to a horizontal (flat) line, the svg breaks and shows nothing. This method is that the line creates a mask over a rectangle that is colored with the gradient, and the mask only shows the resulting line and thus works for horizontal lines.

@@ -7,119 +7,221 @@ export type Line = {
length: number
angle: number
}

const SMOOTHING_RATIO = 0.2
const SIMILAR_THRESHOLD_DELTA = 0.01
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if points are within 1% of each other of each other, they will be considered equal for the purposes of smoothing.


const SMOOTHING_RATIO = 0.2
const SIMILAR_THRESHOLD_DELTA = 0.01
const SMOOTHING_THRESHOLD_DELTA = 0.2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If points are more than 20% different from eachother (with respect to the total height of the graph) they will be considered extreme enough to be smoothed

Comment on lines +19 to +21
if (max === 0 && min === 0) {
return data
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cover the case of a list of 0's [0,0,0,0,0]

return data.map((n) => (n - min) / (max - min))
}

export const drawSegmentPath = (dataPoints: Array<DataPoint>): string => {
export const mapToSvgDimensions = (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

function extracted from the component for clarity and ease of comprehension

Comment on lines +64 to +66
const prevPoint = dataPoints[index - 1]
const nextPoint = dataPoints[index + 1]
const nextNextPoint = dataPoints[index + 2]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the current index is near the beginning or the end, these may be undefined, which is why they are being handled carfully on lines 69 and 77

Comment on lines +163 to +164
const quartEaseInFunction = (t: number): number => t * t * t * t
const quartEaseOutFunction = (t: number): number => 1 - --t * t * t * t
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iHiD
Copy link
Member

iHiD commented Jul 12, 2021

Great work again, @neenjaw. My only request would be to add some of those comments inline as they'll be useful in the future. And https://gist.github.com/gre/1650294 specifically as we should reference it in the code as a source. I'll gratefully merge this when that's done! :)

@iHiD iHiD changed the title update progress graph line algorithm Update progress graph line algorithm Jul 14, 2021
@iHiD
Copy link
Member

iHiD commented Jul 14, 2021

@neenjaw I'm going to merge this. If you'd like to follow up with another PR for adding the comments, that'd be great :) Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants