Three.js state refactor

[andy-sweet]

This refactors the three.js state that is currently stored as individual references in the main React component into a separate class and module.

This makes the main React component simpler, makes the initialization effect easier to read, and also means the code in the class constructor/methods don't need to look up current for references. The separation is relatively clean, though there's some overlap in the DOM and callbacks.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
points-web-viewer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 29, 2024 5:12pm

[andy-sweet]

Still a WIP. Will let you know when this is ready for review.

[andy-sweet]

This is ready for review now. I decided to move the three.js class into its own module, which seems like a reasonable way to keep a clean interface between react and three for now.

Didn't think too hard about names - happy to take suggestions!

[aganders3]

This looks great!

As for naming I think Canvas is definitely overloaded here. Maybe PointsCanvas, but I suppose that would need to change when we start adding tracks. What abut TrackCanvas?

The other thought that came during review is to use a custom hook. It's perhaps not important for reuse at this point, but it moves more of the related logic into canvas.ts. On the other hand it brings react (but not JSX) into that file. For example:

diff --git a/src/canvas.ts b/src/canvas.ts
index d348202..bf4ee33 100644
--- a/src/canvas.ts
+++ b/src/canvas.ts
@@ -1,3 +1,4 @@
+import { useEffect, useRef } from 'react';
 import {
     AdditiveBlending,
     AxesHelper,
@@ -208,4 +209,24 @@ export class Canvas {
         }
         this.selectionHelper.dispose();
     }
-}
\ No newline at end of file
+}
+
+export function useCanvas(
+    div: React.RefObject<HTMLDivElement>,
+    initRenderWidth: number,
+    initRenderHeight: number,
+) {
+    const canvas = useRef<Canvas>();
+    useEffect(() => {
+        // initialize the canvas
+        canvas.current = new Canvas(initRenderWidth, initRenderHeight);
+        div.current?.appendChild(canvas.current.renderer.domElement);
+        canvas.current.animate();
+        return () => {
+            canvas.current?.renderer.domElement.remove();
+            canvas.current?.dispose();
+        }
+    }, [/* no dependencies, run only on init */]);
+
+    return canvas.current || null;
+}
diff --git a/src/scene.tsx b/src/scene.tsx
index 62e05db..c977b32 100644
--- a/src/scene.tsx
+++ b/src/scene.tsx
@@ -1,6 +1,6 @@
 import { useEffect, useRef, useState } from 'react';
 import { InputSlider, InputText, InputToggle } from "@czi-sds/components";
-import { Canvas } from './canvas';
+import { Canvas, useCanvas } from './canvas';
 
 // @ts-expect-error
 import { ZarrArray, slice, openArray } from "zarr";
@@ -9,7 +9,7 @@ import { ZarrArray, slice, openArray } from "zarr";
 const DEFAULT_ZARR_URL = new URL("https://public.czbiohub.org/royerlab/zebrahub/imaging/single-objective/tracks_benchmark/ZSNS001_nodes.zarr");
 
 interface SceneProps {
-    renderWidth: number;
+    renderWidth?: number;
     renderHeight?: number;
 }
 
@@ -30,19 +30,13 @@ export default function Scene(props: SceneProps) {
     // * manage objects that should never change, even when the component re-renders
     // * avoid triggering re-renders when these *do* change
     const divRef: React.RefObject<HTMLDivElement> = useRef(null);
-    const canvas = useRef<Canvas>();
+    const canvas: Canvas | null = useCanvas(
+        divRef, renderWidth, renderHeight
+    );
 
     // this useEffect is intended to make this part run only on mount
     // this requires keeping the dependency array empty
     useEffect(() => {
-        // initialize the canvas
-        canvas.current = new Canvas(renderWidth, renderHeight);
-
-        // append renderer canvas
-        const divCurrent = divRef.current;
-        const renderer = canvas.current!.renderer;
-        divCurrent?.appendChild(renderer.domElement);
-
         const keyDown = (event: KeyboardEvent) => {
             console.log("keyDown: %s", event.key);
             if (event.repeat) { return; } // ignore repeats (key held down)
@@ -62,18 +56,13 @@ export default function Scene(props: SceneProps) {
         document.addEventListener('keydown', keyDown);
         document.addEventListener('keyup', keyUp);
 
-        // start animating - this keeps the scene rendering when controls change, etc.
-        canvas.current.animate();
-
         return () => {
-            renderer.domElement.remove();
-            canvas.current?.dispose();
             document.removeEventListener('keydown', keyDown);
             document.removeEventListener('keyup', keyUp);
         }
     }, []); // dependency array must be empty to run only on mount!
 
-    canvas.current?.setSelecting(selecting);
+    canvas?.setSelecting(selecting);
 
     // update the array when the dataUrl changes
     useEffect(() => {
@@ -92,7 +81,7 @@ export default function Scene(props: SceneProps) {
 
     // set the controls to auto-rotate
     useEffect(() => {
-        canvas.current && (canvas.current.controls.autoRotate = autoRotate);
+        canvas && (canvas.controls.autoRotate = autoRotate);
     }, [autoRotate]);
 
     // playback time points
@@ -112,7 +101,7 @@ export default function Scene(props: SceneProps) {
     // update the geometry buffers when the array changes
     useEffect(() => {
         if (!array) return;
-        canvas.current?.initPointsGeometry(array.shape[1] / 3);
+        canvas?.initPointsGeometry(array.shape[1] / 3);
     }, [array]);
 
     // update the points when the array or timepoint changes
@@ -129,7 +118,7 @@ export default function Scene(props: SceneProps) {
                     console.debug('IGNORE SET points at time %d', curTime);
                     return;
                 }
-                canvas.current?.setPointsPositions(data);
+                canvas?.setPointsPositions(data);
             });
         } else {
             console.debug('IGNORE FETCH points at time %d', curTime);
@@ -141,7 +130,7 @@ export default function Scene(props: SceneProps) {
 
     // update the renderer and composer when the render size changes
     // TODO: check performance and avoid if unchanged
-    canvas.current?.setSize(renderWidth, renderHeight);
+    canvas?.setSize(renderWidth, renderHeight);
 
     // set up marks for the time slider
     const spacing = 100;

[aganders3]

Should this also update the camera aspect ratio?

[andy-sweet]

Yes, looks like we're not doing it on main. But makes sense to catch it in this PR.

[andy-sweet]

As for naming I think Canvas is definitely overloaded here. Maybe PointsCanvas, but I suppose that would need to change when we start adding tracks. What abut TrackCanvas?

I also thought of PointsCanvas, so let's go with that. Once we have tracks, I'd be down to rename this to TrackCanvas, but let's do that when we need to.

The other thought that came during review is to use a custom hook. It's perhaps not important for reuse at this point, but it moves more of the related logic into canvas.ts. On the other hand it brings react (but not JSX) into that file.

Part of the motivation of this PR is to keep React code separate, so I'd rather not put the custom hook in canvas.ts. But I'd be fine with defining it in scene.tsx (and maybe somewhere else in the future). Though, as we're only using it once, I'm not convinced it adds too much (especially once we have a fix for #12).

[andy-sweet]

As for naming I think Canvas is definitely overloaded here. Maybe PointsCanvas, but I suppose that would need to change when we start adding tracks. What abut TrackCanvas?

I also thought of PointsCanvas, so let's go with that. Once we have tracks, I'd be down to rename this to TrackCanvas, but let's do that when we need to.

I went for PointCanvas to be consistent with PointSelectionBox and named the file similarly.

[aganders3]

All good with me - merge when you're ready.

In my mind this pretty much wraps up the migration to React, and we can hopefully move on to adding features again.