leeoniya / uPlot

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

Series stroke at scale limits is clipped to half width when scale is explicitly ranged #859

Closed zefirka closed 9 months ago

zefirka commented 9 months ago

See here: https://jsfiddle.net/Zeffirsky/k273abz8/11/

Even with padding set up, top and bottom series (which sticked to the top and bottom of scales) are going beyond chart and so series has different width. I see a workaround with playing scales min/max modifications, but it's really nasty path cuz possible there will be two axes with different scale range and so It will become really hard to find matching scale change to ensure axes grid will have the same values on same height.

Can we consider something like internal padding for chart or scales or something like this?

leeoniya commented 9 months ago

i think the series path clipping rect just needs to be expanded by half the series.width instead of using exactly u.bbox.

leeoniya commented 9 months ago

it already does this halfWidth expansion here if it sees the series values hitting the scale min/max, but maybe it's not hitting that condition?

https://github.com/leeoniya/uPlot/blob/master/src/uPlot.js#L1521

zefirka commented 9 months ago

I looked into uplot.series[number].min/max and found out that there's null. Possibly that's the reason?

leeoniya commented 9 months ago

probably yes. e.g. if auto: false uplot will not scan the series data for min/max and will not cache those on the series objects.

i was actually planning on decoupling this scanning+caching from the follow-up auto-ranging by making series.scan and scale.scan and not only rely on series.auto and scale.auto for both things.

leeoniya commented 9 months ago

this works, though. hmm...

  scales: {
    x: {
      time: false
    },
    y: {
      range: {
        min: {pad: 0},
        max: {pad: 0},
      },
    }
  },
leeoniya commented 9 months ago

i think this is mostly a problem with explicitly setting scales, whether via cursor or via api. when you set the scales manually there's no reason for uPlot to scan the data for auto-ranging, so it skips this.

an easy way to avoid this is to unconditionally expand the stroke clipping region by half width. it would fix the flat-min or flat-max case but would have the not-so-great side-effect of the stroke going outside the grid area by halfWidth when the data limits are outside the y scale bounds. generally that's probably less-bad behavior than the current one.

leeoniya commented 9 months ago

hmm, not sure i like the unconditional approach. the alternative is to always scan the data. there will still be edge cases there, for example if you have a bunch of values at 0 and one value at -1 and your scale min is 0.

EDIT: there was a maths error that double multiplied the required offset.

image

zefirka commented 9 months ago

@leeoniya can you release the fix?

leeoniya commented 9 months ago

this week

zefirka commented 9 months ago

 🥺 👉 👈

leeoniya commented 9 months ago

sorry, will try today.

i'm on parental leave (not getting much sleep 😅 🧟) and doing a release involves a couple hours of careful manual testing in uPlot demos and Grafana dashboards to make sure i didnt break anything!

leeoniya commented 9 months ago

https://www.npmjs.com/package/uplot/v/1.6.25

zefirka commented 9 months ago

Thanks a lot and my congrats!