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.

Sign up for GitHub

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

Jump to bottom

Support monotone cubic interpolation for vertical line charts #9084

Merged
merged 2 commits into from May 14, 2021
Merged

Support monotone cubic interpolation for vertical line charts #9084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40c8395
Support monotone cubic interpolation for vertical line charts
nagix May 13, 2021
b4ec37b
Use more more intuitive veriable names
nagix May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/controllers/controller.line.js
View file
Open in desktop
Expand Up @@ -99,7 +99,8 @@ export default class LineController extends DatasetController {
}

draw() {
this._cachedMeta.dataset.updateControlPoints(this.chart.chartArea);
const meta = this._cachedMeta;
meta.dataset.updateControlPoints(this.chart.chartArea, meta.iScale.axis);
super.draw();
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/elements/element.line.js
View file
Open in desktop
Expand Up @@ -257,12 +257,12 @@ export default class LineElement extends Element {
}
}

updateControlPoints(chartArea) {
updateControlPoints(chartArea, indexAxis) {
const me = this;
const options = me.options;
if ((options.tension || options.cubicInterpolationMode === 'monotone') && !options.stepped && !me._pointsUpdated) {
const loop = options.spanGaps ? me._loop : me._fullLoop;
_updateBezierControlPoints(me._points, options, chartArea, loop);
_updateBezierControlPoints(me._points, options, chartArea, loop, indexAxis);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about it, but it could be better to include the indexAxis in options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's possible to include it in options, and it's more flexible than adding an argument to functions. My concern is that indexAxis is rarely set per element, so exposing this option to users may increase complexity of the configuration. @etimberg Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinkin about the lineElement, which is per dataset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it is the lineElement. But even in that case, it is not usual for each line to have a different index axis (x or y).

Copy link
Member

Choose a reason for hiding this comment

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

Right, did not read your concern properly. It already is possible to configure the indexAxis per dataset:
https://codepen.io/kurkle/pen/vYxEpLG

But I agree it would not be common.

It might have been a mistake to move the control point update procedure to the line element, in the MVC point of view, which I rarely think of. But since its there and now needs to know the index axis to work properly, I think the indexAxis can be considered to be a line option.

I don't see a big difference on how the option is passed to the line, but it would remove the need for that argument from the updateControlPoints call. It would also make it available to external code maybe in a bit more straight forward way. (When interacting a line, you'd get it from line.options)

If an plugin calls the updateControlPoints (like the filler does), it would be possible to provide a contradicting value compared to the line controller. Containing the value in the options would prevent those kinds of mistakes.

All that said, I think its really not a big issue either way, so lets hear what @etimberg thinks about it :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't have strong opinions on which way. 😄 I slightly prefer the options just for the external interaction case but I won't hold up the PR over it.

me._pointsUpdated = true;
}
}
Expand Down
35 changes: 20 additions & 15 deletions src/helpers/helpers.curve.js
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ import {_isPointInArea} from './helpers.canvas';

const EPSILON = Number.EPSILON || 1e-14;
const getPoint = (points, i) => i < points.length && !points[i].skip && points[i];
const getValueAxis = (indexAxis) => indexAxis === 'x' ? 'y' : 'x';

export function splineCurve(firstPoint, middlePoint, afterPoint, t) {
// Props to Rob Spencer at scaled innovation for his post on splining between points
Expand Down Expand Up @@ -71,9 +72,10 @@ function monotoneAdjust(points, deltaK, mK) {
}
}

function monotoneCompute(points, mK) {
function monotoneCompute(points, mK, indexAxis = 'x') {
const valueAxis = getValueAxis(indexAxis);
const pointsLen = points.length;
let deltaX, pointBefore, pointCurrent;
let delta, pointBefore, pointCurrent;
let pointAfter = getPoint(points, 0);

for (let i = 0; i < pointsLen; ++i) {
Expand All @@ -84,16 +86,17 @@ function monotoneCompute(points, mK) {
continue;
}

const {x, y} = pointCurrent;
const iPixel = pointCurrent[indexAxis];
const vPixel = pointCurrent[valueAxis];
if (pointBefore) {
deltaX = (x - pointBefore.x) / 3;
pointCurrent.cp1x = x - deltaX;
pointCurrent.cp1y = y - deltaX * mK[i];
delta = (iPixel - pointBefore[indexAxis]) / 3;
pointCurrent[`cp1${indexAxis}`] = iPixel - delta;
pointCurrent[`cp1${valueAxis}`] = vPixel - delta * mK[i];
}
if (pointAfter) {
deltaX = (pointAfter.x - x) / 3;
pointCurrent.cp2x = x + deltaX;
pointCurrent.cp2y = y + deltaX * mK[i];
delta = (pointAfter[indexAxis] - iPixel) / 3;
pointCurrent[`cp2${indexAxis}`] = iPixel + delta;
pointCurrent[`cp2${valueAxis}`] = vPixel + delta * mK[i];
}
}
}
Expand All @@ -113,8 +116,10 @@ function monotoneCompute(points, mK) {
* cp2x?: number,
* cp2y?: number,
* }[]} points
* @param {string} indexAxis
*/
export function splineCurveMonotone(points) {
export function splineCurveMonotone(points, indexAxis = 'x') {
const valueAxis = getValueAxis(indexAxis);
const pointsLen = points.length;
const deltaK = Array(pointsLen).fill(0);
const mK = Array(pointsLen);
Expand All @@ -132,10 +137,10 @@ export function splineCurveMonotone(points) {
}

if (pointAfter) {
const slopeDeltaX = (pointAfter.x - pointCurrent.x);
const slopeDelta = pointAfter[indexAxis] - pointCurrent[indexAxis];

// In the case of two points that appear at the same x pixel, slopeDeltaX is 0
deltaK[i] = slopeDeltaX !== 0 ? (pointAfter.y - pointCurrent.y) / slopeDeltaX : 0;
deltaK[i] = slopeDelta !== 0 ? (pointAfter[valueAxis] - pointCurrent[valueAxis]) / slopeDelta : 0;
}
mK[i] = !pointBefore ? deltaK[i]
: !pointAfter ? deltaK[i - 1]
Expand All @@ -145,7 +150,7 @@ export function splineCurveMonotone(points) {

monotoneAdjust(points, deltaK, mK);

monotoneCompute(points, mK);
monotoneCompute(points, mK, indexAxis);
}

function capControlPoint(pt, min, max) {
Expand Down Expand Up @@ -177,7 +182,7 @@ function capBezierPoints(points, area) {
/**
* @private
*/
export function _updateBezierControlPoints(points, options, area, loop) {
export function _updateBezierControlPoints(points, options, area, loop, indexAxis) {
let i, ilen, point, controlPoints;

// Only consider points that are drawn in case the spanGaps option is used
Expand All @@ -186,7 +191,7 @@ export function _updateBezierControlPoints(points, options, area, loop) {
}

if (options.cubicInterpolationMode === 'monotone') {
splineCurveMonotone(points);
splineCurveMonotone(points, indexAxis);
} else {
let prev = loop ? points[points.length - 1] : points[0];
for (i = 0, ilen = points.length; i < ilen; ++i) {
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/plugin.filler.js
View file
Open in desktop
Expand Up @@ -575,7 +575,7 @@ export default {
continue;
}

source.line.updateControlPoints(area);
source.line.updateControlPoints(area, source.axis);
if (draw) {
drawfill(chart.ctx, source, area);
}
Expand Down
0 ...t.line/cubicInterpolationMode/monotone.js → ...cInterpolationMode/monotone-horizontal.js
View file
Open in desktop
File renamed without changes.
0 ....line/cubicInterpolationMode/monotone.png → ...InterpolationMode/monotone-horizontal.png
View file
Open in desktop
File renamed without changes
28 changes: 28 additions & 0 deletions test/fixtures/element.line/cubicInterpolationMode/monotone-vertical.js
View file
Open in desktop
@@ -0,0 +1,28 @@
module.exports = {
config: {
type: 'line',
data: {
datasets: [
{
data: [{x: 10, y: 1}, {x: 0, y: 5}, {x: -10, y: 15}, {x: -5, y: 19}],
borderColor: 'red',
fill: false,
cubicInterpolationMode: 'monotone'
}
]
},
options: {
indexAxis: 'y',
scales: {
x: {display: false, min: -15, max: 15},
y: {type: 'linear', display: false, min: 0, max: 20}
}
}
},
options: {
canvas: {
height: 256,
width: 512
}
}
};
Binary file added test/fixtures/element.line/cubicInterpolationMode/monotone-vertical.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion types/helpers/helpers.curve.d.ts
View file
Open in desktop
Expand Up @@ -31,4 +31,4 @@ export interface MonotoneSplinePoint extends SplinePoint {
* between the dataset discrete points due to the interpolation.
* @see https://en.wikipedia.org/wiki/Monotone_cubic_interpolation
*/
export function splineCurveMonotone(points: readonly MonotoneSplinePoint[]): void;
export function splineCurveMonotone(points: readonly MonotoneSplinePoint[], indexAxis?: 'x' | 'y'): void;
2 changes: 1 addition & 1 deletion types/index.esm.d.ts
View file
Open in desktop
Expand Up @@ -1711,7 +1711,7 @@ export interface LineHoverOptions extends CommonHoverOptions {
export interface LineElement<T extends LineProps = LineProps, O extends LineOptions = LineOptions>
extends Element<T, O>,
VisualElement {
updateControlPoints(chartArea: ChartArea): void;
updateControlPoints(chartArea: ChartArea, indexAxis?: 'x' | 'y'): void;
points: Point[];
readonly segments: Segment[];
first(): Point | false;
Expand Down