Run-time error if `zoomToPoints` is invoked before points are initialized

[insertmike]

Abstract

zoomToPoints break plot if called before initialized

Context:

Suppose a client is developing application, which has the following workflow:

1. Whenever points change, redraw scatter
2. Whenever selected property change, select points and zoom to their cluster

useEffect(() => {
    if (!scatter) return;
    drawPoints({
      scatter,
      points,
      selected,
     ....
    });
  }, [points]);

  useEffect(() => {
    if (!scatter) return;

    if (selected && selected.length > 0) {
      scatter.select(selected);
      scatterPlot.zoomToPoints(selected);
    }
  }, [selected]);

As the order of effects is not guaranteed, nor there is no way of direct checking whether the points have been drawn, if zoomToPoints is invoked before KDBush is initialized, it results in the following error:

gl-scatterplot.esm.js:400 Uncaught TypeError: Cannot read properties of null (reading '0')
    at transformMat4 (regl-scatterplot.esm.js:400:1)
    at getScatterGlPos (regl-scatterplot.esm.js:4601:1)
    at Object.value (regl-scatterplot.esm.js:7472:1)
    at regl-scatterplot.esm.js:2813:1
    at handleRAF (regl.js:10074:1)
    at sentryWrapped (helpers.js:90:1)
t

It is possible to have a workaround by trying out to check whether Scatter returns points when queried, but this is not a solution. Scatter should not throw runtime error when trying to perform this action while trying.

The problem arises because code is trying to access searchIndex but it is undefined in the meantime.

Proposal

There is a couple of things we can do here. I will list my proposal with (MR) and propose additional improvements that we can make to make it nicer.

1. Check whether searchIndex is defined when zoomToPoints is called and return (Could throw a warning too). Otherwise. - This complies with the following improvements as makes the library more robust. It cou

const zoomToPoints = (pointIdxs, options = {}) => {
    if(!pointsInitialized){ // or !pointsInitialized where !pointsInitialized === to (searchIndex !== undefined)
      return;
    }

2. Update tests and documentation.

Further Improvements

@flekschas Can you share your thoughts on the following ones?

1. Introduce a GET parameter which is indicating whether scatter is ready (have drawn points) this will allow the user to make more informed decisions on its actions and wait if needed.

[flekschas]

Have you had a look at the dispatched events? Specifically, init and draw should help you here. Also, scatter.draw() returns a promise that resolves once the points have been drawn.

So a very quick fix would be

const hasDrawn = useRef(false);

useEffect(() => {
  hasDrawn.current = false;
  if (!scatter) return;
  scatter.draw(points)
    .then(() => { hasDrawn.current = true; })
    .catch(() => { hasDrawn.current = false; })
}, [points]);

useEffect(() => {
  if (!scatter || !hasDrawn.current) return;

  if (selected && selected.length > 0) {
    scatter.select(selected);
    scatterPlot.zoomToPoints(selected);
  }
}, [selected]);

[flekschas]

But apart from that I agree that zoomToPoints should not fail when the points have not been drawn yet. :)

[insertmike]

@flekschas

Have you had a look at the dispatched events? Specifically, init and draw should help you here. Also, scatter.draw() returns a promise that resolves once the points have been drawn. So a very quick fix would be ...

That's a good proposal, thank you! However, I was thinking whether it makes sense to delegate this to the scatter itself? A READ_ONLY property. What do you think?

[flekschas]

@insertmike Sure, we can simply allow getting isPointsDrawn in the get() function.