Weird curve path if "max" limit is set for line chart

[rgwebcode]

Expected behavior

If the max limit is set for the x axis of a line chart, and the tension is set, or cubicInterpolationMode is set to monotone, then there's a weird curve path at the end of the graph.
If you have two lines on the chart, and you deselect one of them and then reselect it, the lines are being redrawn, and the curve path straightens out to what it should like from the beginning.

This is how it then looks and how I expected it to look without interaction:

Current behavior

And this is how it looks like initially:

Reproducible sample

https://codepen.io/rgwebcode/pen/mdzpzrQ

Optional extra steps/info to reproduce

Happens both in standalone Chart.js and react-chartjs-2.

Possible solution

No response

Context

No response

chart.js version

v4.3.0

Browser name and version

Chrome 112.0.5615.138 & Firefox

Link to your project

No response

[mukham12]

Hello, @etimberg,

I believe I may have pinpointed the underlying cause of this issue, but please correct me if I'm mistaken.

It seems that the problem originates from line 51 of the LineController class. There, we have a conditional check to determine whether the scale ranges have changed. If they have (which seems to be true for the first render), we replace the count variable with the length of the points array. However, in this particular situation, the points array has a length of 8, while a user-defined max value truncates the array at the 5th element. Consequently, setting the count variable to an incorrect value results in the generation of erroneous points within the updateElements function called at line 73.

Since I'm still familiarizing myself with the codebase, I'd like to avoid making hasty decisions. However, my suggestion would be to pass this._drawCount as the third parameter to the updateElements function. I kindly request your guidance on the next steps. I would love to create a PR to address this issue. Thank you!

[mukham12]

Looping in @stockiNail and @LeeLenaleee in case they know how to handle this. Thanks.

[stockiNail]

@mukham12 I am currently in vacation without my laptop. I could have a look at the beginning of July if still open.

[etimberg]

@mukham12 your theory sounds correct. I don't recall why we set it to the number of points in that case. You could try setting it to the truncated length and seeing what happens.

One reason I could see why this code exists is so that points that are animating off the canvas aren't removed suddenly.

[mukham12]

@etimberg,

Setting the value to the truncated length is producing the correct chart (without the “weird” curve). That is what led me to this theory in the first place.

[etimberg]

@mukham12 could you send a PR with those changes? I'd like to test them and see how they work

[mukham12]

@etimberg,

After some reflection, it seems I jumped to a hasty conclusion. The modification I suggested worked fine at first, but it broke when the screen was resized. I will investigate further before making any pull requests. On the bright side, we now know about this issue and are making progress in identifying the bug. I will keep you updated.

@stockiNail your assistance would be appreciated once you return from vacation.

[etimberg]

Perhaps the solution is to only do do the if when it's not the first render. Something like

if (!firstRender && _scaleRangesChanged(meta)) {
 ...
}

Off the top of my head though I'm not sure if we have a way to detect the first render

[mukham12]

I also considered that option, but I'd prefer to keep it as a last resort. There must be a reason why the initial rendering isn't displaying correctly and only adjusts to the correct appearance after resizing the screen or interacting with the legend.

[mukham12]

@etimberg,

I have revisited the code and made another observation worth considering. The directUpdate variable, initialized on line 84, is set to false in the specific scenario we are concerned about. This means that when we attempt to set properties.skip to true on line 94, it does not actually affect individual points. Instead, it modifies a temporary properties object and continues to restart the loop (given that the loop index is greater than or equal to count) having no effect.

As a result, all 8 points will be considered for drawing, since none of them will have skip set to true. This leads me to believe that explicitly setting the skip property for each point, before the continue statement on line 95, may be a viable solution. Like the following

for (let i = 0; i < pointsCount; ++i) {
     const point = points[i];
     const properties = directUpdate ? point : {};

     if (i < start || i >= end) {
       properties.skip = true;
       point.skip = true; // Will ensure the points are not considered for drawing
       continue;
     }
        .
        .
        .
}

Apologies for continuously bringing up new observations, but this approach could address the issue. Please let me know your thoughts and I can create a PR.

You can review the relevant code snippet underneath. Thanks

Chart.js/src/controllers/controller.line.js

Lines 75 to 96 in 57bbd82

	updateElements(points, start, count, mode) {
	const reset = mode === 'reset';
	const {iScale, vScale, _stacked, _dataset} = this._cachedMeta;
	const {sharedOptions, includeOptions} = this._getSharedOptions(start, mode);
	const iAxis = iScale.axis;
	const vAxis = vScale.axis;
	const {spanGaps, segment} = this.options;
	const maxGapLength = isNumber(spanGaps) ? spanGaps : Number.POSITIVE_INFINITY;
	const directUpdate = this.chart._animationsDisabled || reset || mode === 'none';
	const end = start + count;
	const pointsCount = points.length;
	let prevParsed = start > 0 && this.getParsed(start - 1);
	for (let i = 0; i < pointsCount; ++i) {
	const point = points[i];
	const properties = directUpdate ? point : {};
	if (i < start || i >= end) {
	properties.skip = true;
	continue;
	}

[kurkle]

The properties are applied to the points through animations, unless directUpdate is true (meaning props are applied directly without animation)

I think the proposed change should do nothing with direct update and with animation it should make points to be skipped too early.

[mukham12]

@kurkle,

I completely agree with your observations. If the animations are disabled this issue does not occur since directUpdate is set to false, enabling direct point updates. I encourage you to test this behavior on your local machine.

However, with animations enabled, we encounter a different scenario. The points are not directly modified by the code. Consequently, the skip property of the points is not updated because the condition if (i < start || i >= end) evaluates to true. This causes the loop to restart without entering the phase where point updates occur during animation, as you correctly pointed out. As a result, the original points remain unaltered.

Or the issue could be something entirely different.

[stockiNail]

@mukham12 @etimberg @kurkle

I had a look to the code. I think, but maybe I'm wrong, that the line controller is setting all the points (ignoring the scale limits) to the line element.
When the draw of the controller is invoked and it updates the control points for the curve, the splineCurveMonotone function uses all points of the line element.

Locally, I have changed

Chart.js/src/controllers/controller.line.js

Line 60 in 57bbd82

line.points = points;

to

   line.points = points.slice(this._drawStart, this._drawStart + this._drawCount);

where the points of line element are only the "drawable" ones.

Sounds working and also the regression tests.
WDYT?

[stockiNail]

Rethinking a bit that is not the final solution because the line will be drawn only between the visible points and maybe is not correct.

need more time

[mukham12]

Hi @stockiNail. Thanks for looking into this.

No, you are absolutely correct. LineController is setting all the points. I’ve been trying to convey that in my earlier comments and perhaps did not do such a good job.

There are perhaps a few solutions to this, but I was thinking explicitly setting the ‘skip’ property inside the function which creates Point elements would get the job done.

Excited to see what you’ll come up with :)

[stockiNail]

@mukham12 the solution that I have prepared above is working correctly if the scale limits (min and/or max) have the same value of a point. If not, the line will "join" only the point drawable and that's not correct :(.

Therefore I think we could have 2 options (but feel free to correct me):

1. set all points to line element and add a field to the point (not skip because used for other purposes, as @kurkle explained) where we know if the point is drawable. and then change a bit the splineCurveMonotone to skipo the point not needed to craw the line.
2. set only drawable points plus the points before the drawStart and after the drawStart + drawCount, but only if the point is not at the same value of scale limit.

I'm going to go on tomorrow or later.

[mukham12]

@stockiNail, I trust in your expertise to make the best decision here. Whichever option you believe will accomplish the task more efficiently, please go ahead with it. Considering your familiarity with the codebase, you can resolve this more swiftly than myself. Many thanks for your assistance.

[stockiNail]

@mukham12 see PR #11377

[mukham12]

@stockiNail Looks great. Thanks 😊