flekschas / regl-scatterplot

Scalable WebGL-based scatter plot library build with Regl
https://flekschas.github.io/regl-scatterplot/
MIT License
185 stars 21 forks source link

Bug: React + Scatterplot subscribe listeners don't register reliably on refresh. #112

Closed NOT-HAL9000 closed 1 year ago

NOT-HAL9000 commented 1 year ago

Introduction

I've been trying to create a custom scatterplot hook that I can drag and drop into my project.

I have it all working however I keep running into a weird event listener bug. I have tried various implementations, but they all end up with this same bug.

I don't know if this as a result of my react code, or regl-scatterplot, but I have recreated the bug locally and on stackblitz.

The reproduction code is at the bottom.

Bug Observations

The issue is that when the page is being refreshed,

  1. If the mouse cursor is left outside of the browser content window bounds, the event listeners ARE registered.
  2. If the mouse cursor is quickly moved within the browser content window bounds, the event listeners ARE NOT registered.
  3. If they didn't register, but you subsequently click outside of the browser content window bounds (either selecting a different window and coming back, or re-selecting the tab), they finally register.

Even if the event listeners didn't register, the scatterplot is still interactive (pan, zoom, select).

https://user-images.githubusercontent.com/70297791/230014160-5c5a45ff-0017-410d-9965-982e0a8b48d3.mp4

Reproduce

The top window in the video was a local implementation, and the bottom window is accessible via the stackblitz link below.

You can find the smallest possible implementation to reproduce the code on stackblitz here. Be aware that you must click the 'Open in New Tab' button on stackblitz to reproduce.

Screen Shot 2023-04-05 at 5 51 10 pm

All the code is in one file and it's only the main page component (to render the canvas) and a hook. It's a very simple implementation.

If you want to re-create the issue locally, comment in the pages/index.js outlines the steps.

1) Install regl-scatterplot via "npm install --save regl regl-scatterplot pub-sub-es" 2) Import useCallback, useEffect, useRef, useState from react 3) Copy 'useScatterplot' hook into the index.js file next to the home component. 4) Use this 'Home' component to register event listeners and render canvas

Let me know if you have any further questions.

NOT-HAL9000 commented 1 year ago

I've narrowed down the problem space a bit. In my example, I would "click" outside of the screen for the events to register. But this is now incorrect, it's when the mouse leaves the canvas space.

I logged from the hover method and realised that it also doesn't get fired when I expect it to. https://github.com/flekschas/regl-scatterplot/blob/71dc8f23dd4c000dcd846bca252006f4f33b5c3f/src/index.js#L749

Moving upstream, I noticed that if I moved the mouse out of the canvas, the mouseLeaveCanvasHandler handler would fire. Then moving it back into the canvas, hover would work. https://github.com/flekschas/regl-scatterplot/blob/71dc8f23dd4c000dcd846bca252006f4f33b5c3f/src/index.js#L3233

It's looking more and more likely to be related to the isMouseInCanvas flag.

When I logged out from the mouseEnterCanvasHandler handler

https://github.com/flekschas/regl-scatterplot/blob/71dc8f23dd4c000dcd846bca252006f4f33b5c3f/src/index.js#L3229

The isMouseInCanvas is fired and logged. Then the event handlers work. No click required.

NOT-HAL9000 commented 1 year ago

I think I have found the issue.

The isMouseInCanvas is initalized as false. https://github.com/flekschas/regl-scatterplot/blob/71dc8f23dd4c000dcd846bca252006f4f33b5c3f/src/index.js#L461

If you are able to move the mouse into the canvas before the initalization (it becomes out of sync). The cursor must then trigger the mouseLeaveCanvasHandler followed by the mouseEnterCanvasHandler to get back into sync.

I was able to fix the issue by setting it to true.

let isMouseInCanvas = true;

If the cursor is not in the canvas on startup (isMouseInCanvas is true), afaik it doesn't matter. When the mouse enters the canvas for the first time,mouseEnterCanvasHandler fires anyway and isMouseInCanvas is re-set to true. So it's in sync the whole time.

I think this assumption is safe? I'm not sure if this solution/assumption has any unintended side effects.

Alternatively, you do a check somewhere to find out if the mouse is within the canvas on boot.

flekschas commented 1 year ago

Thanks a lot for reporting the bug, putting together the reproducible example, and digging into the code!

You're right the isMouseInCanvas can be an issue when initialized to false. I wonder though if setting it to true on init is better or if that might have other side effects.

Let me have a closer look tonight. Another idea that comes to mind is to determine the state of isMouseInCanvas using something like elementFromPoint() with the mouse position at the time the instance is initialized.

NOT-HAL9000 commented 1 year ago

I guess that I have only tested the isMouseInCanvas: true for a full screen canvas. So it's possible that there may be side effects if you are moving the cursor around the page without having entering the canvas element at all.

You would only have to check and trace mouseMoveHandler to get a better understanding.

const mouseMoveHandler = (event) => {
  if (!isInit || (!isMouseInCanvas && !mouseDown)) return;  // <- HERE

  const currentMousePosition = getRelativeMousePosition(event);
  const mouseMoveDist = dist(...currentMousePosition, ...mouseDownPosition);
  const mouseMovedMin = mouseMoveDist >= lassoMinDist;

  if (isMouseInCanvas && !lassoActive) { // <-  <- HERE
    hover(raycast()); 
  }

  if (lassoActive) {
    event.preventDefault();
    lassoManager.extend(event, true);
  } else if (mouseDown && lassoOnLongPress && mouseMovedMin) {
    lassoManager.hideLongPressIndicator({
      time: lassoLongPressRevertEffectTime,
    });
  }

  if (mouseDownTimeout >= 0 && mouseMovedMin) {
    clearTimeout(mouseDownTimeout);
    mouseDownTimeout = -1;
  }

  if (mouseDown) draw = true;
};

Alternatively, you're right, elementFromPoint could definitely work as well.

flekschas commented 1 year ago

I've put together a PR that attempts to fix the issue https://github.com/flekschas/regl-scatterplot/pull/114. if you have the chance, could you give it a try?

NOT-HAL9000 commented 1 year ago

lgtm

flekschas commented 1 year ago

@NOT-HAL9000 New version (v1.6.4) with a fix for this bug is out. Thanks again for reporting the issue and taking a stab at it!