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

Monotone cubic interpolation #3112

Merged
merged 7 commits into from
Aug 22, 2016

Conversation

MatthieuRivaud
Copy link
Contributor

Implement monotone cubic interpolation (see issue #3086).

…ith different interpolation modes on the same graph (updated doc accordingly)

- Added new sample file to demonstrate the monotone cubic interpolation mode
- Fixed a typo in a comment in updateBezierControlPoints
@MatthieuRivaud
Copy link
Contributor Author

Using the CI to run my unit tests is much too tedious (and slow). I'm going to try to fix my karma errors locally before another commit.

@MatthieuRivaud
Copy link
Contributor Author

I had to install manually every karma dependency, but I can finally run unit tests. (I don't know why my npm does not process the dependency tree or some such ; that does not seem like the correct behavior. Oh well ...)

And it's green (at last) !

var points = (meta.data || []).filter(function(pt) { return !pt._model.skip; });
var i, ilen, point, model, controlPoints;

var needToCap = me.chart.options.elements.line.capBezierPoints;
Copy link
Member

Choose a reason for hiding this comment

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

is it right to remove this setting? It's helpful when the graph is zoomed and we want the control points to be allowed to go outside of the drawing area.

Copy link
Contributor Author

@MatthieuRivaud MatthieuRivaud Aug 8, 2016

Choose a reason for hiding this comment

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

It's not removed, if I've done that right. It is moved after the calls to splineCurve or splineCurveMonotone.

I also think it should be faster this way, because very few JavaScript compilers are able to remove or hoist this needToCap check (thus ASM jump) in the capIfNecessary function (called in the loop). It is also a lot more difficult to avoid/not execute/not compile the loop and call.

capControlPoint should be quite easily inlined by most recent compilers, and the loop can be completely ignored (and even not compiled at all for very recent compilers that do branch pruning). Not that it matters much for loops on < 100 items ^^,

Copy link
Member

Choose a reason for hiding this comment

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

Right, I missed that the capControlPoint was behind the setting.

You are right that it will probably be a bit faster :)

@etimberg
Copy link
Member

etimberg commented Aug 8, 2016

+1 to merge
CC @zachpanz88 @simonbrunel

}
if (!pointBefore || pointBefore.model.skip) pointCurrent.mK = pointCurrent.deltaK;
else if (!pointAfter || pointAfter.model.skip) pointCurrent.mK = pointBefore.deltaK;
else if (Math.sign(pointBefore.deltaK) != Math.sign(pointCurrent.deltaK)) pointCurrent.mK = 0;
Copy link
Member

Choose a reason for hiding this comment

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

should use helpers.sign to support IE and Safari

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I somehow missed the compatibility table for Math.sign despite having checked Math.EPSILON ... I'll fix that.

@etimberg etimberg merged commit 89531c6 into chartjs:master Aug 22, 2016
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.

2 participants