leeoniya / uPlot

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

strange behaviour when scaling multiple y axes via setScale #902

Closed dadebue closed 4 months ago

dadebue commented 4 months ago

I have a chart setup with the following options (excerpt):

plugins: [
  yAxisScalingPlugin(),
  wheelZoomPlugin(),
],
...
cursor: {
  drag: {
    setScale: false,
    x: false,
    y: false,
    uni: 50,
  },
...
hooks: {
  setScale: [
    (u: uPlot, a: string) => {
      console.log('setScale', a)
    },
  ],
},

My scales are created like this with fixed min and max values:

{
  x: {
    time: true,
    auto: true,
    key: 'x',
  },
  'km/h':{
      auto: false,
      range: [min, max],
      key: 'km/h',
    },
  m:{
      auto: false,
      range: [min, max],
      key: 'm',
    }
}

Furthermore I've written this plugin to change the scaling when an axis is clicked and the mouse moved:

export const yAxisScalingPlugin = () => {
  let uPlotEl: uPlot | null = null

  function init(u: uPlot) {
    uPlotEl = u

    const axesRects = Array.prototype.slice.call(
      u.root.querySelectorAll('.u-axis')
    )

    u.axes.forEach((axis, i) => {
      if (axis.scale !== 'x') {
        axesRects[i].addEventListener('mousedown', handleAxisClick, false)
      }
    })
  }

  const handleAxisClick = (e: ClickEvent) => {
    if (!uPlotEl) return

    const scale = getClickedScale(uPlotEl, e)

    if (!scale || scale.name === 'x') return

    scaleY(uPlotEl, scale)
  }

  return {
    hooks: {
      init: init,
    },
  }
}

The scaleY function scales the axis in the end:

export const scaleY = (u: uPlot, scale: ClickedScale) => {
  let curPos: number | null = null
  let percentage: number | null = null

  const watchMouse = (e: MouseEvent) => {
    if (!curPos) {
      curPos = e.offsetY
      return
    }

    let diff = curPos - e.offsetY
    if (diff === 0) return

    if (!percentage) percentage = e.offsetY / scale.height
    const scaleMin = u.scales[scale.name].min ?? -1
    const scaleMax = u.scales[scale.name].max ?? 1
    const scaleDiff = Math.abs(scaleMax - scaleMin)
    const movePerPixel = scaleDiff / scale.height
    let min = scaleMin - movePerPixel * diff
    let max = scaleMax - movePerPixel * diff

    if (e.shiftKey) {
      // zoom
      diff /= 300
      min = scaleMin + scaleDiff * diff * (1 - percentage)
      max = scaleMax - scaleDiff * diff * percentage
    }

    u.setScale(scale.name, { min, max })
    curPos = e.offsetY
  }

  document.addEventListener('mousemove', watchMouse, false)
  document.addEventListener(
    'mouseup',
    () => {
      document.removeEventListener('mousemove', watchMouse, false)
    },
    false
  )
}

So in the end the scaleY function fires the setScale function once for the clicked&dragged axis for every mouse move event. This works fine for one scale.

When I have more than one scale, and update the scales one after the other, the other scale is reset every time. I don't want this and I'm questioning why this is happening? I don't even call the setScale function for the other scale, but nonetheless it is fired (logged via hook).

Here is a quick video showing that the setSelect of both scales are fired, after I change to the second scale.

https://github.com/leeoniya/uPlot/assets/81422785/c325bcae-5f22-4368-9a8e-1fc99132f8eb

I debugged the uPlot code a bit and it seems as if the setScales function is "causing" this. It sets min and max to inf for the other scales in the wipScales for loop:

else {
  wsc.min = inf;
  wsc.max = -inf;
}

this later on sets minMax to null values:

let minMax = wsc.range(
  self,
  wsc.min ==  inf ? null : wsc.min,
  wsc.max == -inf ? null : wsc.max,
  k
);
wsc.min = minMax[0];
wsc.max = minMax[1];

which later on lets the other scales appear "changed", which leads to them being reset I guess:

if (sc.min != wsc.min || sc.max != wsc.max) {
  ...
  changed[k] = anyChanged = true;
}

Thanks for your help. If you need more info please tell me :)

leeoniya commented 4 months ago

sorry, it's hard to assemble snippets of code to understand what's going on here.

can you make a minimal repro in jsFiddle like https://jsfiddle.net/ecm7qo1t/ and maybe a screen rec of how it behaves and what is unexpected?

dadebue commented 4 months ago

Hi of course ☺️

There you go: https://jsfiddle.net/zbw4m5v0/13/

The screen rec is already uploaded here (idk why Github doesn't embed it as video)

https://github.com/leeoniya/uPlot/assets/81422785/c325bcae-5f22-4368-9a8e-1fc99132f8eb

I expect all other scales to stay the same when I drag one of the scales. But somehow all other scales are reset too.

leeoniya commented 4 months ago

great, thanks.

instead of working out what's going wrong in your code, let me see if i can just add a demo of draggable y scales and see if i encounter the same issue.

i think the main issue here is that you're defining static scale ranges, rather than initial ranges for scales, so things are getting reset to those static ranges.

The screen rec is already uploaded here (idk why Github doesn't embed it as video):

i think you need to stick a new line before it :)

leeoniya commented 4 months ago

i did a from-scratch demo for this, and you're right, something weird is going on.

https://leeoniya.github.io/uPlot/demos/y-scale-drag.html

even without explicit ranges, and with the default auto: true, i would not expect that calling setScale() would affect unrelated scales. auto scales should only be re-scaled when double-click unzooming, or calling setData() or calling setScale('x').

leeoniya commented 4 months ago

tried something in another branch, but it breaks some existing stuff, like rescale on series toggle. so will need to take a different approach. auto currently serves as an indicator for a set of two or three behaviors that should be independently controllable, so it's a bit tricky

https://github.com/leeoniya/uPlot/commit/7b76b56ac8173590e84917f6e694e1db91545e31

dadebue commented 4 months ago

No worries, take your time.

Let me know if I can help somehow...

leeoniya commented 4 months ago

a work-around until this is fixed properly is to explicitly set the other scales to their existing min/max values

in the demo, adding the hack block below u.setScale() works:

let mousemove = e => {
  let dy = e.clientY - y0;
  let shiftyBy = dy * unitsPerPx;

  u.setScale(scaleKey, {
    min: min + shiftyBy,
    max: max + shiftyBy,
  });

  // hack to ensure other scales dont re-autoscale
  for (let scaleKey2 in u.scales) {
    if (scaleKey2 != 'x' && scaleKey2 != scaleKey) {
      u.setScale(scaleKey2, {
        min: u.scales[scaleKey2].min,
        max: u.scales[scaleKey2].max,
      });
    }
  }
};
leeoniya commented 4 months ago

demo is fixed by https://github.com/leeoniya/uPlot/commit/bef151e62a5947641b2ba7734ba062823b03af9e.

due to lack of tests, there are some parts in uPlot that are kind of scary to refactor. scaling behavior is one of those parts, since it is at the core of everything else. hopefully i didn't break anything. i think the new logic is a bit more clear than what was there before, but it's always more clear right after you finished writing it :sweat_smile:

thanks for reporting and the investigation :+1:

dadebue commented 4 months ago

OK sounds cool! Whats your release schedule looking like? When do you plan to release the changes ☺️

Thank you!

leeoniya commented 4 months ago

want to get some more changes and testing in. holidays in US, too. so probably in next 2-3 weeks.

if you wanna help test, you can use the repo+commit as a package.json dependency:

"uplot": "leeoniya/uPlot#bef151e62a5947641b2ba7734ba062823b03af9e"
dadebue commented 4 months ago

I've tested a bit and everything except one thing seems quite right! Nice :)

The thing I found is the following:

https://github.com/leeoniya/uPlot/assets/81422785/d609b50b-933f-4f99-bc43-f8d61e275457

The paths of all other series are not redrawn (correctly) when the scaling is used together with the axis size option (to minimize the axis space). In the example the axis size changes and only the path of the series on the changed scale are redrawn correctly. For all other series the datapoints are redrawn correctly, but the paths are still in the old position. Therefore the points and path do not match anymore...

Here is the updated fiddle (with the specified commit if I've done it correctly): https://jsfiddle.net/xvotLbn1/3/

Should I raise a new issue?

leeoniya commented 4 months ago

Should I raise a new issue?

yes, please move this over to another issue