Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix dependency issues in effect-selection #93

Closed
wants to merge 5 commits into from
Closed

[WIP] Fix dependency issues in effect-selection #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8b61f1
Make hook dependencies errors
aganders3 Jun 1, 2024
1c9431e
Move fetching logic into dispatcher, dispatch to set selection
aganders3 Jun 1, 2024
ee85b24
Use newCanvas in reducer - not sure this makes a difference but feels…
aganders3 Jun 1, 2024
1e5e457
Trying to make track adding idempotent
aganders3 Jun 3, 2024
f1aab72
Fix highlighting points on selection
aganders3 Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
56 changes: 19 additions & 37 deletions src/components/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 = () => {
Expand Down Expand Up @@ -145,46 +145,28 @@ 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 = selectedPoints.get(pointsID) || [];
const selected = Array.from(selectedPoints);
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
Expand Down Expand Up @@ -304,7 +286,7 @@ export default function App() {
>
<Scene
ref={sceneDivRef}
isLoading={isLoadingPoints || isLoadingTracks}
isLoading={isLoadingPoints || numLoadingTracks > 0}
initialCameraPosition={initialViewerState.cameraPosition}
initialCameraTarget={initialViewerState.cameraTarget}
/>
Expand Down
65 changes: 57 additions & 8 deletions src/hooks/usePointCanvas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -97,6 +113,7 @@ interface MinMaxTime {

// setting up a tagged union for the actions
type PointCanvasAction =
| SyncTracks
| AutoRotate
| CameraProperties
| CurTime
Expand All @@ -106,6 +123,7 @@ type PointCanvasAction =
| PointsPositions
| Refresh
| RemoveAllTracks
| Selection
| SelectionMode
| ShowTracks
| ShowTrackHighlights
Expand Down Expand Up @@ -136,7 +154,7 @@ function reducer(canvas: PointCanvas, action: PointCanvasAction): PointCanvas {
newCanvas.controls.autoRotate = action.autoRotate;
break;
case ActionType.HIGHLIGHT_POINTS:
newCanvas.highlightPoints(action.points);
newCanvas.highlightPoints([...action.points].map((p) => p % newCanvas.maxPointsPerTimepoint));
break;
case ActionType.INIT_POINTS_GEOMETRY:
newCanvas.initPointsGeometry(action.maxPointsPerTimepoint);
Expand All @@ -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;
Copy link
Collaborator Author

@aganders3 aganders3 Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need a new Map(action.selection) sometimes? Otherwise we just need to be careful (that's a big "just") not to merely modify the selection and then dispatch this.

case ActionType.SELECTION_MODE:
newCanvas.setSelectionMode(action.selectionMode);
break;
Expand All @@ -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);
Copy link
Collaborator

@andy-sweet andy-sweet Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find much official documentation on whether calling an async function in a react reducer is recommended. I found some posts on SO/Medium, but nothing too authoritative.

Given that reducers should be pure, my guess is that it's probably not recommended.

I guess the reduction of all dispatched actions will be the same even with async because those will not have started yet. But the instance will be modified in the future when the async functions run. And each dispatch will mutate different instances, which makes this feel fragile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're definitely right. Fragile is a kind word for it.

I'm trying to think of how we can do this without having an effect depend on canvas. One idea is to split the canvas back into state + ref, but that's another pretty big change at this point.

Another idea is maybe we do want an effect that runs whenever canvas changes to sync the state. Maybe that's fine as long as it's somewhat smart about skipping unnecessary work?

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;
Expand Down Expand Up @@ -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 });
},
Comment on lines +254 to +255
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the IDs of the points from indices within the timeframe to the calculated pointID. As noted there are still possible mismatch problems, so maybe we need to do this conversion even earlier?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about this issue now. What are the remaining possible mismatch problems? This will be executed immediately after the selection is made, so curTime and maxPointsPerTimepoint will not change. Are their values assumed not to change in a later side-effect related to this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may be sufficient. I need to re-test systematically, my above comment may be inaccurate.

[canvas.curTime, canvas.maxPointsPerTimepoint],
);

// set up the canvas when the div is available
// this is an effect because:
Expand Down
9 changes: 6 additions & 3 deletions src/lib/BoxPointSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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.
Expand All @@ -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;
}

Expand All @@ -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) {
Expand Down
10 changes: 5 additions & 5 deletions src/lib/PointCanvas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand All @@ -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();
Copy link
Collaborator

@andy-sweet andy-sweet Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also be useful over on #91: https://github.com/aganders3/points-web-viewer/pull/91/files#diff-86963609bec6fda3d34233676d5ae33f6aa4b9bbe2758a10cd4863b704b37093R219


showTracks = true;
showTrackHighlights = true;
Expand All @@ -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();
Expand Down Expand Up @@ -107,7 +107,7 @@ export class PointCanvas {
return newCanvas as PointCanvas;
}

get selectedPoints(): PointsCollection {
get selectedPoints(): PointSelection {
return this.selector.selection;
}

Expand Down
8 changes: 4 additions & 4 deletions src/lib/PointSelectionBox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -192,5 +193,4 @@ function isPoints(obj: Object3D): obj is Points {
return Boolean(obj && "isPoints" in obj && obj.isPoints);
}

export type { PointsCollection };
export { PointSelectionBox };
Loading
Loading