[WIP] Fix dependency issues in effect-selection

.eslintrc.cjs

@@ -13,8 +13,7 @@ module.exports = {
         "react/react-in-jsx-scope": "off",
         "react/jsx-uses-react": "off",
         "react-hooks/rules-of-hooks": "error", // Enforce Rules of Hooks
-        // TODO: change exhaustive-deps to error
-        "react-hooks/exhaustive-deps": "warn", // Enforce effect dependencies
+        "react-hooks/exhaustive-deps": "error", // Enforce effect dependencies
         "camelcase": "error",
         "spaced-comment": "error",
         "semi": ["error", "always"],

src/components/App.tsx

@@ -32,7 +32,6 @@ export default function App() {
     const numTimes = trackManager?.numTimes ?? 0;
     // TODO: dataUrl can be stored in the TrackManager only
     const [dataUrl, setDataUrl] = useState(initialViewerState.dataUrl);
-    const [isLoadingTracks, setIsLoadingTracks] = useState(false);
 
     // PointCanvas is a Three.js canvas, updated via reducer
     const [canvas, dispatchCanvas, sceneDivRef] = usePointCanvas(initialViewerState);
@@ -42,6 +41,7 @@ export default function App() {
     // this state is pure React
     const [playing, setPlaying] = useState(false);
     const [isLoadingPoints, setIsLoadingPoints] = useState(false);
+    const [numLoadingTracks, setNumLoadingTracks] = useState(0);
 
     // Manage shareable state that can persist across sessions.
     const copyShareableUrlToClipboard = () => {
@@ -145,46 +145,29 @@ export default function App() {
 
     useEffect(() => {
         console.debug("effect-selection");
-        const pointsID = canvas.points.id;
         const selectedPoints = canvas.selectedPoints;
-        if (!selectedPoints || !selectedPoints.has(pointsID)) return;
-        // keep track of which tracks we are adding to avoid duplicate fetching
-        const adding = new Set<number>();
-
-        // this fetches the entire lineage for each track
-        const fetchAndAddTrack = async (pointID: number) => {
-            if (!trackManager) return;
-            const tracks = await trackManager.fetchTrackIDsForPoint(pointID);
-            // TODO: points actually only belong to one track, so can get rid of the outer loop
-            for (const t of tracks) {
-                const lineage = await trackManager.fetchLineageForTrack(t);
-                for (const l of lineage) {
-                    if (adding.has(l) || canvas.tracks.has(l)) continue;
-                    adding.add(l);
-                    const [pos, ids] = await trackManager.fetchPointsForTrack(l);
-                    // adding the track *in* the dispatcher creates issues with duplicate fetching
-                    // but we refresh so the selected/loaded count is updated
-                    canvas.addTrack(l, pos, ids);
-                    dispatchCanvas({ type: ActionType.REFRESH });
-                }
-            }
-        };
+        if (!trackManager || !selectedPoints) {
+            return;
+        }
 
         dispatchCanvas({ type: ActionType.POINT_BRIGHTNESS, brightness: 0.8 });
+        const selected = Array.from(selectedPoints);
+        // TODO: HIGHLIGHT_POINTS still expects the point IDs within the current timeframe
+        // dispatchCanvas({ type: ActionType.HIGHLIGHT_POINTS, points: selected });
 
-        const selected = selectedPoints.get(pointsID) || [];
-        dispatchCanvas({ type: ActionType.HIGHLIGHT_POINTS, points: selected });
-
-        const maxPointsPerTimepoint = trackManager?.maxPointsPerTimepoint ?? 0;
-
-        setIsLoadingTracks(true);
-        Promise.all(selected.map((p: number) => canvas.curTime * maxPointsPerTimepoint + p).map(fetchAndAddTrack)).then(
-            () => {
-                setIsLoadingTracks(false);
-            },
-        );
-        // TODO: add missing dependencies
-    }, [canvas.selectedPoints]);
+        // keep track of which tracks we are adding to avoid duplicate fetching
+        const adding = new Set<number>();
+        selected.forEach((pointId) => {
+            dispatchCanvas({
+                type: ActionType.SYNC_TRACKS,
+                trackManager,
+                pointId,
+                adding,
+                // pass in the setter to decrement the loading counter when we're done
+                setNumLoadingTracks,
+            });
+        });
+    }, [canvas.points.id, canvas.selectedPoints, dispatchCanvas, trackManager]);
 
     // playback time points
     // TODO: this is basic and may drop frames
@@ -304,7 +287,7 @@ export default function App() {
             >
                 <Scene
                     ref={sceneDivRef}
-                    isLoading={isLoadingPoints || isLoadingTracks}
+                    isLoading={isLoadingPoints || numLoadingTracks > 0}
                     initialCameraPosition={initialViewerState.cameraPosition}
                     initialCameraTarget={initialViewerState.cameraTarget}
                 />

src/hooks/usePointCanvas.ts

@@ -1,13 +1,14 @@
-import { useCallback, useEffect, useReducer, useRef, Dispatch, RefObject } from "react";
+import { useCallback, useEffect, useReducer, useRef, Dispatch, RefObject, SetStateAction } from "react";
 
 import { Vector3 } from "three";
 
 import { PointCanvas } from "@/lib/PointCanvas";
-import { PointsCollection } from "@/lib/PointSelectionBox";
-import { PointSelectionMode } from "@/lib/PointSelector";
+import { PointSelection, PointSelectionMode } from "@/lib/PointSelector";
 import { ViewerState } from "@/lib/ViewerState";
+import { TrackManager } from "@/lib/TrackManager";
 
 enum ActionType {
+    SYNC_TRACKS = "SYNC_TRACKS",
     AUTO_ROTATE = "AUTO_ROTATE",
     CAMERA_PROPERTIES = "CAMERA_PROPERTIES",
     CUR_TIME = "CUR_TIME",
@@ -17,13 +18,23 @@ enum ActionType {
     POINTS_POSITIONS = "POINTS_POSITIONS",
     REFRESH = "REFRESH",
     REMOVE_ALL_TRACKS = "REMOVE_ALL_TRACKS",
+    SELECTION = "SELECTION",
     SELECTION_MODE = "SELECTION_MODE",
     SHOW_TRACKS = "SHOW_TRACKS",
     SHOW_TRACK_HIGHLIGHTS = "SHOW_TRACK_HIGHLIGHTS",
     SIZE = "SIZE",
     MIN_MAX_TIME = "MIN_MAX_TIME",
 }
 
+interface SyncTracks {
+    type: ActionType.SYNC_TRACKS;
+    trackManager: TrackManager;
+    pointId: number;
+    adding: Set<number>;
+    // callback to decrement the number of loading tracks
+    setNumLoadingTracks: Dispatch<SetStateAction<number>>;
+}
+
 interface AutoRotate {
     type: ActionType.AUTO_ROTATE;
     autoRotate: boolean;
@@ -68,6 +79,11 @@ interface RemoveAllTracks {
     type: ActionType.REMOVE_ALL_TRACKS;
 }
 
+interface Selection {
+    type: ActionType.SELECTION;
+    selection: PointSelection;
+}
+
 interface SelectionMode {
     type: ActionType.SELECTION_MODE;
     selectionMode: PointSelectionMode;
@@ -97,6 +113,7 @@ interface MinMaxTime {
 
 // setting up a tagged union for the actions
 type PointCanvasAction =
+    | SyncTracks
     | AutoRotate
     | CameraProperties
     | CurTime
@@ -106,6 +123,7 @@ type PointCanvasAction =
     | PointsPositions
     | Refresh
     | RemoveAllTracks
+    | Selection
     | SelectionMode
     | ShowTracks
     | ShowTrackHighlights
@@ -154,6 +172,9 @@ function reducer(canvas: PointCanvas, action: PointCanvasAction): PointCanvas {
             newCanvas.pointBrightness = 1.0;
             newCanvas.resetPointColors();
             break;
+        case ActionType.SELECTION:
+            newCanvas.selector.selection = action.selection;
+            break;
         case ActionType.SELECTION_MODE:
             newCanvas.setSelectionMode(action.selectionMode);
             break;
@@ -168,6 +189,30 @@ function reducer(canvas: PointCanvas, action: PointCanvasAction): PointCanvas {
         case ActionType.SIZE:
             newCanvas.setSize(action.width, action.height);
             break;
+        case ActionType.SYNC_TRACKS: {
+            const { trackManager, pointId, adding, setNumLoadingTracks } = action;
+            const fetchAndAddTrack = async (p: number) => {
+                setNumLoadingTracks((n) => n + 1);
+                const tracks = await trackManager.fetchTrackIDsForPoint(p);
+                // TODO: points actually only belong to one track, so can get rid of the outer loop
+                for (const t of tracks) {
+                    // skip this if we already fetched the lineage for this track
+                    if (newCanvas.rootTracks.has(t)) continue;
+                    newCanvas.rootTracks.add(t);
+                    const lineage = await trackManager.fetchLineageForTrack(t);
+                    for (const l of lineage) {
+                        if (adding.has(l) || newCanvas.tracks.has(l)) continue;
+                        adding.add(l);
+                        const [pos, ids] = await trackManager.fetchPointsForTrack(l);
+                        newCanvas.addTrack(l, pos, ids);
+                    }
+                }
+            };
+            fetchAndAddTrack(pointId).then(() => {
+                setNumLoadingTracks((n) => n - 1);
+            });
+            break;
+        }
         case ActionType.MIN_MAX_TIME:
             newCanvas.minTime = action.minTime;
             newCanvas.maxTime = action.maxTime;
@@ -203,10 +248,14 @@ function usePointCanvas(
 
     // When the selection changes internally due to the user interacting with the canvas,
     // we need to trigger a react re-render.
-    canvas.selector.selectionChanged = useCallback((_selection: PointsCollection) => {
-        console.debug("selectionChanged: refresh");
-        dispatchCanvas({ type: ActionType.REFRESH });
-    }, []);
+    canvas.selector.selectionChanged = useCallback(
+        (selection: PointSelection) => {
+            console.debug("selectionChanged:", selection);
+            const newSelection = new Set([...selection].map((p) => canvas.curTime * canvas.maxPointsPerTimepoint + p));
+            dispatchCanvas({ type: ActionType.SELECTION, selection: newSelection });
+        },
+        [canvas.curTime, canvas.maxPointsPerTimepoint],
+    );
 
     // set up the canvas when the div is available
     // this is an effect because:

src/lib/BoxPointSelector.ts

@@ -1,8 +1,8 @@
-import { PerspectiveCamera, Scene, WebGLRenderer } from "three";
+import { PerspectiveCamera, Points, Scene, WebGLRenderer } from "three";
 import { OrbitControls } from "three/addons/controls/OrbitControls.js";
 import { SelectionHelper } from "three/addons/interactive/SelectionHelper.js";
 
-import { PointSelectionBox, PointsCollection } from "@/lib/PointSelectionBox";
+import { PointsCollection, PointSelectionBox } from "@/lib/PointSelectionBox";
 import { SelectionChanged } from "@/lib/PointSelector";
 
 // Selection with a 2D rectangle to make a 3D frustum.
@@ -11,6 +11,7 @@ export class BoxPointSelector {
     readonly controls: OrbitControls;
     readonly box: PointSelectionBox;
     readonly helper: SelectionHelper;
+    readonly points: Points;
     readonly selectionChanged: SelectionChanged;
 
     // True if this should not perform selection, false otherwise.
@@ -23,13 +24,15 @@ export class BoxPointSelector {
         renderer: WebGLRenderer,
         camera: PerspectiveCamera,
         controls: OrbitControls,
+        points: Points,
         selectionChanged: SelectionChanged,
     ) {
         this.renderer = renderer;
         this.controls = controls;
         this.helper = new SelectionHelper(renderer, "selectBox");
         this.helper.enabled = false;
         this.box = new PointSelectionBox(camera, scene);
+        this.points = points;
         this.selectionChanged = selectionChanged;
     }
 
@@ -52,7 +55,7 @@ export class BoxPointSelector {
     setSelectedPoints(selectedPoints: PointsCollection) {
         console.debug("BoxPointSelector.setSelectedPoints: ", selectedPoints);
         this.box.collection = selectedPoints;
-        this.selectionChanged(selectedPoints);
+        this.selectionChanged(selectedPoints.get(this.points.id) ?? new Set());
     }
 
     pointerUp(_event: MouseEvent) {

src/lib/PointCanvas.ts

@@ -22,8 +22,7 @@ import { OutputPass } from "three/addons/postprocessing/OutputPass.js";
 import { UnrealBloomPass } from "three/addons/postprocessing/UnrealBloomPass.js";
 
 import { Track } from "@/lib/three/Track";
-import { PointSelector, PointSelectionMode } from "@/lib/PointSelector";
-import { PointsCollection } from "@/lib/PointSelectionBox";
+import { PointSelection, PointSelector, PointSelectionMode } from "@/lib/PointSelector";
 
 type Tracks = Map<number, Track>;
 
@@ -38,6 +37,8 @@ export class PointCanvas {
     readonly selector: PointSelector;
 
     readonly tracks: Tracks = new Map();
+    // set of track IDs that have had their lineage fetched
+    readonly rootTracks: Set<number> = new Set();
 
     showTracks = true;
     showTrackHighlights = true;
@@ -48,8 +49,7 @@ export class PointCanvas {
 
     // this is used to initialize the points geometry, and kept to initialize the
     // tracks but could be pulled from the points geometry when adding tracks
-    // private here to consolidate external access via `TrackManager` instead
-    private maxPointsPerTimepoint = 0;
+    maxPointsPerTimepoint = 0;
 
     constructor(width: number, height: number) {
         this.scene = new Scene();
@@ -107,7 +107,7 @@ export class PointCanvas {
         return newCanvas as PointCanvas;
     }
 
-    get selectedPoints(): PointsCollection {
+    get selectedPoints(): PointSelection {
         return this.selector.selection;
     }
 

src/lib/PointSelectionBox.ts

@@ -24,7 +24,8 @@ const _vectemp1 = new Vector3();
 const _vectemp2 = new Vector3();
 const _vectemp3 = new Vector3();
 
-type PointsCollection = Map<number, number[]>;
+// map of id: [indices]
+export type PointsCollection = Map<number, Set<number>>;
 
 class PointSelectionBox {
     camera: OrthographicCamera | PerspectiveCamera;
@@ -163,9 +164,9 @@ class PointSelectionBox {
                 if (frustum.containsPoint(_vec3)) {
                     const objectCollection = this.collection.get(object.id);
                     if (!objectCollection) {
-                        this.collection.set(object.id, [i]);
+                        this.collection.set(object.id, new Set([i]));
                     } else {
-                        objectCollection.push(i);
+                        objectCollection.add(i);
                     }
                 }
             }
@@ -192,5 +193,4 @@ function isPoints(obj: Object3D): obj is Points {
     return Boolean(obj && "isPoints" in obj && obj.isPoints);
 }
 
-export type { PointsCollection };
 export { PointSelectionBox };

src/lib/PointSelector.ts

@@ -1,10 +1,11 @@
 import { PerspectiveCamera, Points, Scene, WebGLRenderer } from "three";
 import { OrbitControls } from "three/addons/controls/OrbitControls.js";
 
-import { PointsCollection } from "@/lib/PointSelectionBox";
 import { BoxPointSelector } from "./BoxPointSelector";
 import { SpherePointSelector } from "./SpherePointSelector";
 
+export type PointSelection = Set<number>;
+
 export enum PointSelectionMode {
     BOX = "BOX",
     SPHERICAL_CURSOR = "SPHERICAL_CURSOR",
@@ -22,7 +23,7 @@ interface PointSelectorInterface {
     dispose(): void;
 }
 
-export type SelectionChanged = (selection: PointsCollection) => void;
+export type SelectionChanged = (selection: PointSelection) => void;
 
 // this is a separate class to keep the point selection logic separate from the rendering logic in
 // the PointCanvas class this fixes some issues with callbacks and event listeners binding to
@@ -34,9 +35,9 @@ export class PointSelector {
     readonly sphereSelector: SpherePointSelector;
 
     selectionMode: PointSelectionMode = PointSelectionMode.BOX;
-    selection: PointsCollection = new Map();
+    selection: PointSelection = new Set();
     // To optionally notify external observers about changes to the current selection.
-    selectionChanged: SelectionChanged = (_selection: PointsCollection) => {};
+    selectionChanged: SelectionChanged = (_selection: PointSelection) => {};
 
     constructor(
         scene: Scene,
@@ -45,7 +46,14 @@ export class PointSelector {
         controls: OrbitControls,
         points: Points,
     ) {
-        this.boxSelector = new BoxPointSelector(scene, renderer, camera, controls, this.setSelectedPoints.bind(this));
+        this.boxSelector = new BoxPointSelector(
+            scene,
+            renderer,
+            camera,
+            controls,
+            points,
+            this.setSelectedPoints.bind(this),
+        );
         this.sphereSelector = new SpherePointSelector(
             scene,
             renderer,
@@ -84,7 +92,7 @@ export class PointSelector {
         return this.selectionMode === PointSelectionMode.BOX ? this.boxSelector : this.sphereSelector;
     }
 
-    setSelectedPoints(selection: PointsCollection) {
+    setSelectedPoints(selection: PointSelection) {
         console.debug("PointSelector.setSelectedPoints:", selection);
         this.selection = selection;
         this.selectionChanged(selection);