leeoniya / uPlot

📈 A small, fast chart for time series, lines, areas, ohlc & bars
MIT License
8.51k stars 371 forks source link

undefinied entries in gaps array during #buildClip #257

Closed Cryptkeeper closed 3 years ago

Cryptkeeper commented 3 years ago

After updating to 1.0.10, it seems that undefined values are being pushed into the gaps array within #buildClip, resulting in a TypeError when referencing the value later. Downgrading to a version prior to the linked commit resolves the issue.

TypeError: g is undefined
    uPlot.esm.js:1691:4
    buildClip uPlot.esm.js:1691
    buildPaths uPlot.esm.js:1811
    drawSeries uPlot.esm.js:1580
    forEach self-hosted:225
    drawSeries uPlot.esm.js:1577
    paint uPlot.esm.js:2049
    setScale uPlot.esm.js:2090
    _setScale uPlot.esm.js:2165
    autoScaleX uPlot.esm.js:1218
    _init uPlot.esm.js:2815
    uPlot uPlot.esm.js:2827

I've debugged the issue into the https://github.com/leeoniya/uPlot/commit/5b985552618ab4539df6ea5b50f93d1710a66016 change (specifically, https://github.com/leeoniya/uPlot/commit/5b985552618ab4539df6ea5b50f93d1710a66016#diff-aabd864b5381c3ed4c3bc629b41ab21bR1669-R1672) where the push calls may insert undefined values if the gaps array was already empty (it assumes at least 2 elements).

I am not entirely certain of the conditions, but it seems that an undefined value is being inserted into the gaps array produced within #buildPaths.

buildPaths.gaps output: []
buildPaths.ydata input: [undefined, 1924, 1924, 1924, 1924]

In a simple fix, I tried to only push the headGap and tailGap values into the array if defined. This avoids the TypeError, but introduces a rendering issue where if not initially rendered, the graphs will never render.

I'm assuming this is simply a byproduct of me getting too excited with undefined values and is a usage error, but input is appreciated. Thank you!

Cryptkeeper commented 3 years ago

With some further testing, I've noticed the rendering issue I mentioned is also present in previous builds (1.0.8 & 1.0.9) and is not a regression.

It seems that whenever a graph is initialized, if the resulting gaps array is empty, the graph will never render. Refreshing my page at a later point (with the same data set with a few new entries appended), renders correctly. As a bonus, this means my mentioned fix of simply checking headGap & tailGap are defined prior to insertion works without causing other issues, but may not be the best solution.

I think there's generally an issue with data sets that have <= 1 data points, with the data points being undefined.

leeoniya commented 3 years ago

thanks for the info.

i've pushed a couple commits that hopefully resolve this. tested against these dataset combos:

[],
[],

[1],
[null],

[1,2],
[null,null],
Cryptkeeper commented 3 years ago

Thanks for the help. That has resolved the TypeError, but I'm still seeing two rendering issues:

  1. The initial "all or nothing" bug previously described
  2. A lack of scale ticks (I'm not sure if any of the options have changed that might be causing this)

(Neither are producing any errors, please let me know if there's any debug information I can get you to help.)

Untitled
leeoniya commented 3 years ago
  1. The initial "all or nothing" bug previously described

are you referring to "It seems that whenever a graph is initialized, if the resulting gaps array is empty, the graph will never render."?

can you please put up a simple repro? i'm unable trigger this.

  1. A lack of scale ticks (I'm not sure if any of the options have changed that might be causing this)

usually scale ticks go away if there's not data in the zoomed x range, but then you'd have no series line drawn. but i see the series line in your image, so i'm not sure what's going on there. again, a repro will probably be needed.

Cryptkeeper commented 3 years ago

I'll work on putting together a test case. Thanks for the help.

leeoniya commented 3 years ago

ah, here's one that causes weirdness with filling as a result of messed up gaps:

let data = [
  [1,2,3],
  [null,1,null],
];

const opts = {
  width: 1920,
  height: 600,
  title: "Area Fill",
  scales: {
    x: {
      time: false,
    },
  },
  series: [
    {},
    {
      stroke: "red",
      fill: "rgba(255,0,0,0.1)",
      spanGaps: true,
    },
  ],
};

in this case the gaps are duplicated rather than undefined, though.

leeoniya commented 3 years ago

ccd5950 now better handles gaps at the right edge. some details:

the way gap insertion works is to insert a gap for the previous all-null x pixel range when a non-null pixel is encountered. this obviously leads to a situation at the end of the loop if the last value is null that we never end up hitting our gap insertion trigger.

i added a check after the loop exit to make sure the gap is properly inserted/extended.

please re-test.

Cryptkeeper commented 3 years ago

Confirmed fixed with master@https://github.com/leeoniya/uPlot/commit/ccd59500c90fc8d2ef0d237f250e2918e9972eb1. Please let me know when this might be released so I can update my dependency. Thank you for the help!

Cryptkeeper commented 3 years ago

Reopening since I've seen the 2nd rendering issue pop back up (apologies for closing, I think there was a caching issue). No errors in console, going to see if I can create a reproducible test for you.

Untitled
Cryptkeeper commented 3 years ago

For reference this is on each refresh and never renders properly.

I'm seeing this on each graph regardless of data length. Each data array is matching in length, contains consistent values, and has no undefined/null values. Have any of the rendering options changed?

Cryptkeeper commented 3 years ago

Current options for reference: https://github.com/Cryptkeeper/Minetrack/blob/master/assets/js/servers.js#L85

I've tried flipping all show/visibility values and stripping the options down to their minimum without luck. Stripping it further down occasionally will show the Y axis labels. Again, all the data is correct, within range and has no null/undefined values.

Untitled
this._plotInstance = new uPlot({
      height: 100,
      width: 400,
      series: [
        {},
        {}
      ],
      axes: [
        {},
        {}
      ]
    }, this._graphData, document.getElementById(`chart_${this.serverId}`))
leeoniya commented 3 years ago

Have any of the rendering options changed?

not to my recollection :(

do you have a public staging server you can point me to (with either available sourcemaps or non-minified files so i can debug it?)

Cryptkeeper commented 3 years ago

I've setup a staging copy @ https://staging.minetrack.me/

It's not minified and has source maps included.

It's using uPlot master @ https://github.com/leeoniya/uPlot/commit/ccd59500c90fc8d2ef0d237f250e2918e9972eb1 and Minetrack master @ a8a4c23e3a3d912b8fc21d795f52b068b236d2b1

leeoniya commented 3 years ago

ok, i think i see what's going on. it may be related 3fa9cf1f05ea89aa07f966a00359a081cc85d180, but not 100% sure. i don't think this is a uPlot bug, but we'll see if i root-caused this correctly.

long story short is that when you customize the scale names, as you do with 'Players' [1], you have to make sure that your series and axes also have their series.scale and axis.scale changed to match. you can see this being done in the bench: https://github.com/leeoniya/uPlot/blob/master/bench/uPlot.html (though the scales are implicitly initialized there based on the series.scale props).

by default uPlot's scales are named x and y. to match this, series[0] and axes[0] get assigned scale: 'x' defaults; series[1..N] and axes[1..N] get assigned scale: 'y' defaults. this is described in the docs: https://github.com/leeoniya/uPlot/tree/master/docs#series-scales-axes-grid. so if you alter them, it's on you to match them across all 3 sets of opts.

the easiest thing to do in your case is probably to leave the default y scale name since you don't need multiple y scales (like the bench does). but it's easy enough to add the matched scale keys, too.

[1] https://github.com/Cryptkeeper/Minetrack/blob/b7ed5fdd931e4f47e69a83ad08c3037983896873/assets/js/servers.js#L151

Cryptkeeper commented 3 years ago

That has definitely fixed it, thanks for your help.

  1. Any info when you plan to release master as a new version?
  2. Do you take donations? I would love to buy you a couple cups of coffee (in spirit) as a thank you for your help on this.
leeoniya commented 3 years ago
  1. sometime this weekend, assuming none of the demos broke :D
  2. thank you, i am sufficiently caffeinated. but feel free to donate to the eff or a green charity, e.g.
leeoniya commented 3 years ago

https://github.com/leeoniya/uPlot/releases/tag/1.0.11