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

Line series with small values causes negative color to apply across the whole chart with second line #19130

Closed
runfaj opened this issue Jun 9, 2023 · 21 comments · Fixed by #19709

Comments

@runfaj
Copy link

runfaj commented Jun 9, 2023

Expected behaviour

Negative color should only appear on points that are actually negative.

Actual behaviour

For context, here's what the chart looks like when only one series. I included this so you can see how the data is structured and how the negative values apply appropriately:

image

Fiddle: https://jsfiddle.net/runfaj/0u9gxtzb/1/

However, once another series is added with large values, causing the first series to scale very close to the 0 axis line, you can see now it is actually rendering two lines where the negative values is across the length of the chart.

image

Fiddle: https://jsfiddle.net/runfaj/21ycwasv/1/

I don't believe this is a configuration issue since 99.9% of all our charts have worked fine, but this edge case has popped up for a couple customers to complain. From what I can tell, it looks like there are two generated svgs for the tiny values line series and both appear to be generating across the whole chart. I think in this scenario, I would probably expect to see the essentially the same as the first screenshot above, but nearly a flat line displayed with the first 9/10ths or so purple and that last 1/10th is red.

Product version

highcharts 11.0.0

Affected browser(s)

any

@highsoft-bot highsoft-bot added this to To do in Development-Flow via automation Jun 9, 2023
@karolkolodziej
Copy link
Contributor

Hi @runfaj!

I'm not sure if that is a bug that is how the positive/negative colour is applied.
If you check the DOM you can see a separate path for the negative colour- demo.

In my opinion, the extremes are not correctly selected for given data.
Have you considered moving that series which oscillates around 0 to a separate axis where it will be visible better?

@runfaj
Copy link
Author

runfaj commented Jun 14, 2023

@karolkolodziej Thanks for the update. The main issue is that all our charts are dynamically generated and there's very valid use cases that people run into where the extremes can happen. The example I provided in the description was actually from a replicated chart, but omitted 4 other series and an extra axis on the chart that the customer has in addition to the example two series. The extremes in their cases are as correct as they could be, but the complaint is that when it approaches zero, instead of breaking up the displayed line with the positive and negative where it actually is positive and negative, we see the given behavior where everything becomes both positive and negative.

One solution I thought I could maybe implement is just disabling the negative values color if it becomes super close to the zero axis, but then you still get an inaccurate representation for those negative values. Another thought is maybe doing per-point coloring instead, but that definitely doesn't work with the turbothreshold. Imo, this seems like it should be a library adjustment because the default highcharts functionality for extending past the given x values just doesn't make sense. However, I also recognize this could be a potentially difficult code change if there are lots of variations of values crossing the positive and negative in the given series.

@karolkolodziej
Copy link
Contributor

@runfaj you are right, having in mind all the different possible scenarios codding that into the core would be difficult.
But you can update the series dynamically based on the available range -demo.
You will probably find some threshold when the negativeColor should be enabled.

@runfaj
Copy link
Author

runfaj commented Jun 19, 2023

@karolkolodziej I tried what you suggested with a few different variants, but couldn't find anything that seems to really address the issue. One thing I was thinking might be a possibility would be how the svg itself renders. I haven't dived into that source code, but one option could be to have a flag for lines that indicates a) if the line is horizontally flat (or close to), and b) negative values are present, instead of drawing a straight line, line segments could be drawn from each axis intercept. I think those are already auto-calculated since there's already logic for the negative values somewhere, so seems like having a slightly different logic for drawing the actual line svg might be doable.

@karolkolodziej
Copy link
Contributor

You can always divide the series into positive and negative parts as I did in the demo.
With that, you don't have a problem with negative SVG zones on the series.

@runfaj
Copy link
Author

runfaj commented Jun 20, 2023

I'll dig into this a bit and get back to you. For both the demo links you provided though, I don't see that split happening as expected for a single series. This one doesn't seem to render any negative colors and this one as two separate series doesn't work well when you've got many other series also added to the chart (many of our customers have at least 3-4 in addition to the problem series shown in this example).

@runfaj
Copy link
Author

runfaj commented Jul 10, 2023

@karolkolodziej This would still be an awesome thing for HC to fix. However, in the meantime, I ended up just disabling the negative color if the overall extremes for the series is < 1% of the axis extremes. This isn't ideal, but at least visually isn't as bad. Also, the setExtremes and afterSetExtremes never worked for me (maybe because I'm handling min/max myself?), so I ended up having to do this update in the chart render event.

  if (lineSeries.length) {
    chartRenderFunctions.push(function render() {
      const chart = this;
      _.forEach(chart.yAxis, yAxis => {
        const { series, dataMax, dataMin } = yAxis;
        const maxExtreme = _.max([Math.abs(dataMax), Math.abs(dataMin)]);
        _.forEach(series, s => {
          if (s.options.negativeColor) {
            const seriesExtreme = _.max([Math.abs(s.dataMax), Math.abs(s.dataMin)]);
            const hasPosAndNeg = s.dataMax > 0 && s.dataMin < 0;
            const extremePercent = seriesExtreme / maxExtreme;
            if (hasPosAndNeg && extremePercent < 0.01) {
              s.update({ negativeColor: '' });
            }
          }
        });
      });
    });
  }

@karolkolodziej
Copy link
Contributor

@runfaj thank you for sharing that ;)

@runfaj
Copy link
Author

runfaj commented Jul 13, 2023

@karolkolodziej unfortunately we had another customer report this. This time, even though the line has completely negative values, it still breaks. Here's a screenshot of an example I've replicated where the middle line is completely negative, but the positive green is still getting rendered.
image

I replicated here where shrinking the height makes a little more obvious: https://jsfiddle.net/runfaj/ua36978v/4/. If you look at the second data series, you can see the values are all negative, so the negativeColor is taking effect, but I would definitely never expect to see the positive green line in this scenario.

Upon investigation in the dom, it appears that highcharts is always rendering two lines for positive and negative. In this same fiddle, that series-0 is completely positive, but you can see in the dom that it is still rendering a negative line (since I have negativeColor defined) that apparently is just hiding behind the positive one all the time:
image

I don't think there's anything I can possibly do on my side to capture and try to fix all edge cases here, since apparently negative lines can even be rendered when all values are positive. I don't have the option of being able to split series into two separate positive and negative series, so this seems like a highcharts bug that would need to be addressed regardless.

Without knowing the actual source code, and thinking at a high level - I wonder if it might make sense in the svg path generating instead of rendering two separate lines, if negativeColor is present, only rendering the line segments. You could potentially calculate each point on the line the same way it currently is, but have one extra step that detects those zero intercepts and adds extra breaks in the svg paths, so each is guaranteed to only render the appropriate positive and negative sections. Those extra negativeColor paths or the positive paths could also just be completely skipped from drawing if all the associated values don't have any zero intercepts.

@runfaj
Copy link
Author

runfaj commented Aug 29, 2023

@karolkolodziej Have gotten a couple more reports of this issue for customers over the past few weeks. Do you know if this will get fixed or not (even if months down the road)? Mostly to give them a general yes or no. Thanks!

@karolkolodziej
Copy link
Contributor

Let me bring this to an internal discussion. I will let you know as soon as I know something 😉

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Aug 30, 2023

Workaround

Hi, I did some experimenting with this problem, and found out that stroke-dasharray can be used to achieve what we want. See https://jsfiddle.net/highcharts/7qh391zr/. The snippet identifies the distance on the graph where it crosses between zones, and applies the dasharray accordingly.

On the pro side, this is quite fast and effective, and we can remove the clipping zones so we probably come out with simpler and leaner code. Except we may need the clipping for area series...

On the con side, it breaks the dashStyle option. Maybe we are able to mix in the existing dasharray to the zone segments, but that will be complex at best. May be worth a try though. Edit: Animation (addPoint etc) may also be a challenge.

As for alternative solutions, one approach could be to flip the clipping rectangles. Consider my fiddle at the top of this comment. Instead of one horizontal clipping rectangle per zone, we would split the clipping so it is vertical and representing each segment. Although that too might interfere with the area series.

@TorsteinHonsi
Copy link
Collaborator

@runfaj What do you think?

@runfaj
Copy link
Author

runfaj commented Aug 31, 2023

@TorsteinHonsi I think the idea could work - I just plugged in the original fiddle data in the description and it doesn't seem to work quite right: https://jsfiddle.net/runfaj/c3vt4681/4/ (I had to remove the dashStyle being defined on the series like you mentioned). I would expect in this one to be getting a purple section starting at where I screenshotted the cursor:
image

However, it does seem to somewhat fix the issue on the later fiddle I posted: https://jsfiddle.net/runfaj/c3vt4681/7/ (original: #19130 (comment)), although there is some weirdness happening with the the start/end points on the graph, which only seems to appear when the dashStyle is removed.
image

Regarding the area series, I suspect that might not be too much of an issue to just not render the area fill since this only happens when a fairly flat line very close to the zero axis, so the fill wouldn't be too missed if it disappeared.

@TorsteinHonsi
Copy link
Collaborator

Thanks for your feedback @runfaj , I have a new revision that deals with both those cases. The first case was due to using rounded values for the threshold.

Regarding your dashStyle: 'Solid' setting, you should know that Highcharts handles this differently than when it is left out (in spite of the docs saying it defaults to Solid). When a dashStyle setting is applied, we don't apply linecap. That's why it makes a difference.

@runfaj
Copy link
Author

runfaj commented Sep 1, 2023

@TorsteinHonsi thanks for the updates/clarification! I'll spend some time integrating this into our platform to see if it fixes the customer cases we've seen and follow up soon!

@runfaj
Copy link
Author

runfaj commented Sep 5, 2023

@TorsteinHonsi I think this will work for most cases after testing through cases that have come up (and a few other ones saved in our platform). I had to make a small adjustment in the zone for loop. There was an error occurring - I just needed to add a continue statement:

const zone = series.zones[zoneI];
if (!zone) { // added this
    continue;
}
if (zone !== lastZone) {

I went through and tested cases where there were varying pos/neg values along the lines and it seemed to work well in all solid line scenarios. However, like you mentioned, this does break the dashStyle, which I did find a few customers setting that (we only allow "solid" or "dash"). Seems like it fails (like you mentioned it would), but wondering if it could be resolved with something like:

let modifiedDashArray = dashArray.map((position, idx) => (
  // Make all the positions relative to the previous
  position - (dashArray[idx - 1] || 0)
));
if (series.userOptions.dashStyle === 'Dash') {
  // "Dash" is 8,6
  modifiedDashArray = modifiedDashArray.map(v => (Math.round(v / 16) * 16));
  let newDashArray = [];
  for(let n = 0; n < modifiedDashArray[modifiedDashArray.length - 1]; n += 16) {
    // convert the pos/neg splits to the 8,6 sections for "Dash" style
    newDashArray.push(...);
  }
}
path.show().attr({
  'clip-path': 'none',
  'stroke-dasharray': newDashArray.join(' '),
});

I'm guessing it would have to be set back to a solid style first for the calculations though. Maybe that would require 2 event loops - one to detect this scenario and set it back to solid, then another to do all the dashArray calculations.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Sep 6, 2023

Thanks for working with me on this @runfaj .

Your idea with splicing in the dashStyle would work, but I fear it would be pretty fragile. There's also the mentioned problem with animation with this dasharray approach.

Workaround 2: Perpendicular clips

There's another possible solution to this issue. If we flipped the clip rectangles and instead created multiple vertical clipping segments, we would be able to clip the graph at exactly the x position where it crosses the threshold. I have created a POC for that on https://jsfiddle.net/highcharts/857pbrty/. It has some advantages that I can see:

  • It preserves the dashStyle setting
  • It is more likely to work well with animation and updating
  • It most likely performs faster because the SVGPath.getPointAtLength is pretty slow.
  • It would also give us a new feature for free - the options to create perpendicular zones on area charts. Which I believe would be a very useful feature. For example it would make it easy to highlight when the readings drop below a critical value: https://jsfiddle.net/highcharts/857pbrty/3/
Skjermbilde 2023-09-06 kl  20 35 53

@runfaj
Copy link
Author

runfaj commented Sep 8, 2023

@TorsteinHonsi I think this would be an excellent solution - I used that first example against our application and it looks like it solves all the cases really well! I had to add one check here in the case of only a single point was rendering in one chart instance:

if (!lastPoint.zone.segments.at(-1)) {
  return;
}
lastPoint.zone.segments.at(-1).end = lastPoint.plotX;

For the second example, we use a custom goal lines setup for our charting configurations and the additional zones would be incredibly useful there as well! I think the only thought that I would maybe request if that was going to be built out officially would be to enable zones by segment. So instead of limiting to a single horizontal value for each zone, maybe the zones could be setup per data point or something. Here's a quick example of that UI where a user can input a goal value for each point:
image
and here's how that might look rendered (the green line is the goal)
image
It would be really cool to somehow make basically all of the points on that pink line that are below the green line automatically red with the idea of these zone clips.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Sep 14, 2023

After a bit more effort on this, I found that vertical clips pose a different problem - if the lines are close to vertical, we get exactly the same problem as before, only vertical. I'm now proceeding with an approach where the clips are horizontal, but the boundary line is adapted to the line crossing. Clips are visualized by the dotted lines:

Skjermbilde 2023-09-14 kl  14 42 11

@runfaj
Copy link
Author

runfaj commented Sep 18, 2023

@TorsteinHonsi I didn't even think to check with verticals - good catch. I suspect some of our clients might be running into that with some of our bar/line type charts. I'll have to see if I can find cases to test against.

Development-Flow automation moved this from To do to Done Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

4 participants