From 0c0d8ef055923181971cea40f4171b9f46544de5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 22 Feb 2026 12:46:31 +0000 Subject: [PATCH] Apply codebase quality recommendations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Fix silent async failures — add console.error to VideoPlayerNode.play() catch and AutoTrackerNode initFromVideo catch so failures are visible in the console rather than silently discarded. 2. WASM error boundary — AutoTrackerNode now renders an in-video error message (below the drag hint) when OpenCV initialisation fails, giving the user actionable feedback instead of a silent no-op. 3. PlottableProperty registry — extract the nine kinematics quantities from KinematicsGraphNode into a new canonical file (src/screen-name/graph/kinematics-plottable-properties.ts) with a buildKinematicsPlottableProperties(model) factory. Adding a new physical quantity now requires a single-file edit. 4. Model-view transform safety — add SimModel.pixelToModelCoords(Vector2) as the single, named entry point for converting pixel/scene coordinates to model coordinates. Update AutoTrackerNode and DigitizingOverlayNode to use it; update the tracksProperty INVARIANT comment to reference the new method instead of the raw MVT call. 5. Graph subsystem documentation — replace the one-liner doc comment on ConfigurableGraph with a full subsystem overview: file table, data-flow diagram, gesture-coordination guidance, and visibility/axis-selection notes. 6. Standardize listener cleanup — AutoTrackerNode now uses a private disposeAutoTrackerNode lambda (same pattern as all other view nodes) instead of storing bound*-prefixed fields and calling unlink/remove directly from dispose(). Redundant bound* class fields removed. https://claude.ai/code/session_01JerFgrXUb6fycACVYTA971 --- src/screen-name/graph/ConfigurableGraph.ts | 52 +++++++++++++- .../graph/kinematics-plottable-properties.ts | 72 +++++++++++++++++++ src/screen-name/model/SimModel.ts | 25 +++++-- src/screen-name/view/AutoTrackerNode.ts | 65 ++++++++++------- src/screen-name/view/DigitizingOverlayNode.ts | 3 +- src/screen-name/view/KinematicsGraphNode.ts | 70 ++---------------- src/screen-name/view/VideoPlayerNode.ts | 3 +- 7 files changed, 191 insertions(+), 99 deletions(-) create mode 100644 src/screen-name/graph/kinematics-plottable-properties.ts diff --git a/src/screen-name/graph/ConfigurableGraph.ts b/src/screen-name/graph/ConfigurableGraph.ts index f7b6a17..a2ff99e 100644 --- a/src/screen-name/graph/ConfigurableGraph.ts +++ b/src/screen-name/graph/ConfigurableGraph.ts @@ -1,6 +1,54 @@ /** - * Configurable graph that allows users to select which properties to plot on each axis. - * This provides a flexible way to explore relationships between any two quantities. + * ConfigurableGraph — interactive X-Y plot panel for the kinematics graph subsystem. + * + * ## Subsystem overview + * + * The graph subsystem lives in `src/screen-name/graph/` and consists of five files: + * + * | File | Role | + * |------|------| + * | `ConfigurableGraph.ts` | Top-level SceneryStack node; owns the bamboo chart, axis labels, control buttons, and coordinates all sub-modules | + * | `GraphDataManager.ts` | Accumulates (x, y) data points, drives auto-scaling, and owns the tick-spacing algorithm | + * | `GraphInteractionHandler.ts` | All pointer/touch/keyboard gestures: pan, pinch-zoom, axis-drag, header-drag, corner-resize | + * | `GraphControlsPanel.ts` | Builds the axis-selector dropdowns and header bar UI | + * | `PlottableProperty.ts` | `PlottableProperty` union type — the interface any quantity must satisfy to appear in the axis selector | + * | `kinematics-plottable-properties.ts` | Canonical registry of all kinematics quantities; the sole place to add a new plottable quantity | + * + * ## Data flow + * + * ``` + * KinematicsGraphNode (view layer — owns track selection and wires model → graph) + * └─ ConfigurableGraph (graph node — layout, bamboo chart, buttons) + * ├─ GraphDataManager (data store, auto-scale, tick math) + * ├─ GraphInteractionHandler + * │ ├─ ZoomGestureHandler + * │ ├─ PanGestureHandler + * │ ├─ AxisGestureHandler + * │ ├─ HeaderGestureHandler + * │ └─ ResizeGestureHandler + * └─ GraphControlsPanel (axis selector UI) + * ``` + * + * ## Gesture coordination + * + * `GraphInteractionHandler` is the largest file (~930 lines). It delegates to + * five single-responsibility sub-handlers, each in its own file. Before adding + * new gesture logic, read the existing `zoom()` / `pan()` / `rescaleAxes()` + * helpers — many edge cases (pinch-centre preservation, manual-zoom locking, + * axis-specific gestures) are already handled there. + * + * ## Axis selection + * + * The two `Property` values (`xPropertyProperty`, + * `yPropertyProperty`) drive everything: axis labels, data mapping, and the + * dropdown UI. They are exposed via `getXPropertyProperty()` / + * `getYPropertyProperty()` so that `KinematicsGraphNode` can react to changes. + * + * ## Visibility + * + * `graphVisibleProperty` (initially `false`) gates the entire graph, header, + * title panel, and resize handles. Set it to `true` after construction to + * show the graph; the ToolsControlPanel checkbox is the standard toggle. */ import { BooleanProperty, Property, type TReadOnlyProperty } from "scenerystack/axon"; diff --git a/src/screen-name/graph/kinematics-plottable-properties.ts b/src/screen-name/graph/kinematics-plottable-properties.ts new file mode 100644 index 0000000..43af43a --- /dev/null +++ b/src/screen-name/graph/kinematics-plottable-properties.ts @@ -0,0 +1,72 @@ +/** + * kinematics-plottable-properties.ts + * + * Canonical registry of every quantity that can appear on a kinematics graph + * axis. Centralising the list here means: + * + * - Adding a new physical quantity (e.g. kinetic energy) requires a change in + * exactly one place rather than hunting through KinematicsGraphNode. + * - The accessor keys (e.g. "vx", "aMag") are co-located with the + * PlottableProperty definitions, making key–accessor mismatches obvious. + * - Unit properties from the model are wired in a single factory call, + * keeping KinematicsGraphNode free of quantity-specific knowledge. + * + * ## Adding a quantity + * + * 1. Compute it in `KinematicsComputer.ts` and add it to `TrackKinematicsPoint`. + * 2. Add a `createPlottableProperty(...)` call below with the matching key. + * 3. Expose it in the `dataPoints` map inside `KinematicsGraphNode.updateGraph()`. + * + * That's it — the graph axis selector picks it up automatically because it + * renders whatever is in this array. + */ + +import type { TReadOnlyProperty } from "scenerystack/axon"; +import type { SimModel } from "../model/SimModel.js"; +import type { PlottableProperty } from "./PlottableProperty.js"; + +function createPlottableProperty( + name: string, + unit: string | TReadOnlyProperty, + accessor: (point: Record) => number, +): PlottableProperty { + return { name, unit, accessor }; +} + +/** + * Build the ordered list of kinematics quantities available for axis selection. + * Called once per `KinematicsGraphNode` instance; the result is passed directly + * to `ConfigurableGraph`. + * + * @param model - Provides the reactive unit-string properties so that axis + * labels update automatically when the user changes the calibration unit. + */ +export function buildKinematicsPlottableProperties(model: SimModel): PlottableProperty[] { + return [ + // ── Time ────────────────────────────────────────────────────────────── + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("t", "s", (pt) => pt["t"] ?? 0), + + // ── Position ────────────────────────────────────────────────────────── + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("x", model.distanceUnitProperty, (pt) => pt["x"] ?? 0), + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("y", model.distanceUnitProperty, (pt) => pt["y"] ?? 0), + + // ── Velocity ────────────────────────────────────────────────────────── + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("vx", model.velocityUnitProperty, (pt) => pt["vx"] ?? 0), + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("vy", model.velocityUnitProperty, (pt) => pt["vy"] ?? 0), + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("speed", model.velocityUnitProperty, (pt) => pt["speed"] ?? 0), + + // ── Acceleration ────────────────────────────────────────────────────── + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("ax", model.accelerationUnitProperty, (pt) => pt["ax"] ?? 0), + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("ay", model.accelerationUnitProperty, (pt) => pt["ay"] ?? 0), + // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation + createPlottableProperty("|a|", model.accelerationUnitProperty, (pt) => pt["aMag"] ?? 0), + ]; +} diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index 73b2cc3..947d374 100644 --- a/src/screen-name/model/SimModel.ts +++ b/src/screen-name/model/SimModel.ts @@ -148,7 +148,7 @@ export class SimModel { // Never write raw pixel coordinates or coordinates from a different MVT // into this property. All externally-sourced positions (digitizing // clicks, auto-tracker output) must first be converted with - // modelViewTransformProperty.value.inversePosition2() before storage. + // pixelToModelCoords() before storage. public readonly tracksProperty = new Property([]); public readonly activeTrackIdProperty = new Property(null); public readonly canAddTrackProperty = new BooleanProperty(true); @@ -230,9 +230,9 @@ export class SimModel { * Any code that writes to `tracksProperty` must store positions in the * coordinate system of the *current* MVT. Positions coming from outside * (digitizing clicks, auto-tracker pixel output) must be converted with - * `modelViewTransformProperty.value.inversePosition2()` before storage. - * Writing raw pixel coordinates or stale model coordinates directly into - * `tracksProperty` will silently corrupt the track data. + * `pixelToModelCoords()` before storage. Writing raw pixel coordinates or + * stale model coordinates directly into `tracksProperty` will silently + * corrupt the track data. */ private retransformTrackPoints(prevMvt: Transform3, newMvt: Transform3): void { const tracks = this.tracksProperty.value; @@ -288,6 +288,23 @@ export class SimModel { this.tracksProperty.value = this.tracksProperty.value.filter((t) => t.id !== id); } + /** + * Convert a point from pixel/scene space to model coordinates using the + * current model-view transform. + * + * All externally-sourced positions (digitizing clicks, auto-tracker output) + * **must** go through this method before being stored in `tracksProperty`. + * Calling `modelViewTransformProperty.value.inversePosition2()` directly + * bypasses this contract and risks storing coordinates in the wrong space + * if the MVT is ever replaced by a subclass or indirection. + * + * @param pixelPoint - A point in scene/pixel coordinate space. + * @returns The equivalent position in model (real-world) coordinates. + */ + public pixelToModelCoords(pixelPoint: Vector2): Vector2 { + return this.modelViewTransformProperty.value.inversePosition2(pixelPoint); + } + /** * Record a digitized position for `frame` on the track identified by `id`. * If a point at the same frame already exists it is replaced (update diff --git a/src/screen-name/view/AutoTrackerNode.ts b/src/screen-name/view/AutoTrackerNode.ts index ba8769b..ce492b8 100644 --- a/src/screen-name/view/AutoTrackerNode.ts +++ b/src/screen-name/view/AutoTrackerNode.ts @@ -1,7 +1,7 @@ import type { TReadOnlyProperty } from "scenerystack/axon"; import { Vector2 } from "scenerystack/dot"; import { Shape } from "scenerystack/kite"; -import { DragListener, Line, Node, Path, Rectangle, Text } from "scenerystack/scenery"; +import { DragListener, Line, Node, Path, Rectangle, Text, VBox } from "scenerystack/scenery"; import { PhetFont } from "scenerystack/scenery-phet"; import { Tandem } from "scenerystack/tandem"; import { StringManager } from "../../i18n/StringManager.js"; @@ -50,6 +50,7 @@ export class AutoTrackerNode extends Node { private readonly recordedFrames = new Set(); private readonly hintText: Text; + private readonly errorText: Text; private readonly selectionRect: Rectangle; private readonly trailPath: Path; private readonly crosshairH: Line; @@ -65,12 +66,7 @@ export class AutoTrackerNode extends Node { // previous drag from overwriting a more recent one. private initVersion = 0; - // Kept for removeEventListener / unlink in dispose() - private readonly boundVideoElement: HTMLVideoElement; - private readonly boundOnFrame: () => void; - private readonly boundClearRecordedFrames: () => void; - private readonly boundAutoTrackingShownProperty: TReadOnlyProperty; - private readonly boundAutoTrackingShownListener: (shown: boolean) => void; + private readonly disposeAutoTrackerNode: () => void; /** * @param videoElement - The video element used both for pixel capture and frame events. @@ -102,8 +98,22 @@ export class AutoTrackerNode extends Node { font: new PhetFont({ size: HINT_FONT_SIZE, weight: "bold" }), fill: TrackLabColors.trackerHintFillProperty, }); - this.hintText.center = new Vector2(VIDEO_WIDTH / 2, VIDEO_HEIGHT / 2); - this.addChild(this.hintText); + + // ── Error text (shown when OpenCV fails to load or tracking init fails) ─ + this.errorText = new Text("", { + font: new PhetFont({ size: HINT_FONT_SIZE, weight: "bold" }), + fill: TrackLabColors.trackerCrosshairStrokeProperty, + visible: false, + }); + + // Stack hint and error vertically so both are centred in the video area. + const centeredLabels = new VBox({ + children: [this.hintText, this.errorText], + spacing: 8, + align: "center", + center: new Vector2(VIDEO_WIDTH / 2, VIDEO_HEIGHT / 2), + }); + this.addChild(centeredLabels); // ── Selection rectangle ─────────────────────────────────────────────── this.selectionRect = new Rectangle(0, 0, 0, 0, { @@ -155,6 +165,7 @@ export class AutoTrackerNode extends Node { this.trailPath.shape = null; this.trailPath.visible = false; this.hintText.visible = false; + this.errorText.visible = false; this.selStart = this.globalToLocalPoint(event.pointer.point); this.selecting = true; @@ -231,8 +242,13 @@ export class AutoTrackerNode extends Node { this.hintText.visible = true; } }) - .catch((_err) => { + .catch((err: unknown) => { + console.error("AutoTracker: failed to initialise OpenCV tracker:", err); if (this.initVersion === capturedVersion) { + const message = + err instanceof Error ? err.message : "Tracking initialisation failed. Try again."; + this.errorText.string = message; + this.errorText.visible = true; this.hintText.visible = true; } }); @@ -274,9 +290,9 @@ export class AutoTrackerNode extends Node { // O(1) duplicate-frame check via Set (vs O(n) linear scan). if (!this.recordedFrames.has(frame)) { - // Convert video-pixel coords to global coords, then to model coords. + // Convert video-pixel coords to global/scene coords, then to model coords. const globalPt = this.localToGlobalPoint(new Vector2(pt.x, pt.y)); - const modelPt = model.modelViewTransformProperty.value.inversePosition2(globalPt); + const modelPt = model.pixelToModelCoords(globalPt); model.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y); this.recordedFrames.add(frame); } @@ -285,16 +301,11 @@ export class AutoTrackerNode extends Node { videoElement.addEventListener("timeupdate", onFrame); videoElement.addEventListener("seeked", onFrame); - // Store refs so dispose() can remove the listeners. - this.boundVideoElement = videoElement; - this.boundOnFrame = onFrame; - // Clear the recorded-frames set whenever the user switches to a different // track so frames from the previous track don't suppress recording on the // new one. const clearRecordedFrames = () => this.recordedFrames.clear(); model.activeTrackIdProperty.lazyLink(clearRecordedFrames); - this.boundClearRecordedFrames = clearRecordedFrames; // ── Show/hide based on combined "video loaded && autoTracking" ──────── const autoTrackingShownListener = (shown: boolean) => { @@ -307,8 +318,15 @@ export class AutoTrackerNode extends Node { } }; autoTrackingShownProperty.link(autoTrackingShownListener); - this.boundAutoTrackingShownProperty = autoTrackingShownProperty; - this.boundAutoTrackingShownListener = autoTrackingShownListener; + + // ── Centralised cleanup (mirrors the disposeXxx pattern used elsewhere) ─ + this.disposeAutoTrackerNode = () => { + videoElement.removeEventListener("timeupdate", onFrame); + videoElement.removeEventListener("seeked", onFrame); + model.activeTrackIdProperty.unlink(clearRecordedFrames); + autoTrackingShownProperty.unlink(autoTrackingShownListener); + this.model.tracker.dispose(); + }; } private setCrosshairVisible(visible: boolean): void { @@ -339,7 +357,7 @@ export class AutoTrackerNode extends Node { this.hintText.visible = false; } - /** Clear tracking state (template, trail, visuals). */ + /** Clear tracking state (template, trail, visuals, and any displayed error). */ public reset(): void { this.model.tracker.dispose(); this.trailHead = 0; @@ -350,14 +368,11 @@ export class AutoTrackerNode extends Node { this.trailPath.shape = null; this.trailPath.visible = false; this.setCrosshairVisible(false); + this.errorText.visible = false; } public override dispose(): void { - this.boundVideoElement.removeEventListener("timeupdate", this.boundOnFrame); - this.boundVideoElement.removeEventListener("seeked", this.boundOnFrame); - this.model.activeTrackIdProperty.unlink(this.boundClearRecordedFrames); - this.boundAutoTrackingShownProperty.unlink(this.boundAutoTrackingShownListener); - this.model.tracker.dispose(); + this.disposeAutoTrackerNode(); super.dispose(); } } diff --git a/src/screen-name/view/DigitizingOverlayNode.ts b/src/screen-name/view/DigitizingOverlayNode.ts index edaa2c1..6a037be 100644 --- a/src/screen-name/view/DigitizingOverlayNode.ts +++ b/src/screen-name/view/DigitizingOverlayNode.ts @@ -324,8 +324,7 @@ export class DigitizingOverlayNode extends Node { const time = model.currentTimeProperty.value; const frame = Math.round(time * model.frameRateProperty.value); - const mvt = model.modelViewTransformProperty.value; - const modelPt = mvt.inversePosition2(localPt); + const modelPt = model.pixelToModelCoords(localPt); model.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y); onPointAdded(); diff --git a/src/screen-name/view/KinematicsGraphNode.ts b/src/screen-name/view/KinematicsGraphNode.ts index f38baf5..4aff91d 100644 --- a/src/screen-name/view/KinematicsGraphNode.ts +++ b/src/screen-name/view/KinematicsGraphNode.ts @@ -5,12 +5,12 @@ * Users can select which variables to plot on each axis (t, x, y, vx, vy, speed, ax, ay, |a|). */ -import { Property, type TReadOnlyProperty } from "scenerystack/axon"; +import { Property } from "scenerystack/axon"; import { HBox, Node, Text, VBox } from "scenerystack/scenery"; import { PhetFont } from "scenerystack/scenery-phet"; import { ComboBox, type ComboBoxItem } from "scenerystack/sun"; import ConfigurableGraph from "../graph/ConfigurableGraph.js"; -import type { PlottableProperty } from "../graph/PlottableProperty.js"; +import { buildKinematicsPlottableProperties } from "../graph/kinematics-plottable-properties.js"; import type { SimModel } from "../model/SimModel.js"; // Graph dimensions @@ -18,14 +18,6 @@ const GRAPH_WIDTH = 300; const GRAPH_HEIGHT = 200; const MAX_DATA_POINTS = 5000; -function createPlottableProperty( - name: string, - unit: string | TReadOnlyProperty, - accessor: (point: Record) => number, -): PlottableProperty { - return { name, unit, accessor }; -} - export class KinematicsGraphNode extends VBox { private readonly graph: ConfigurableGraph; private readonly model: SimModel; @@ -45,61 +37,9 @@ export class KinematicsGraphNode extends VBox { this.listParent = listParent; this.selectedTrackProperty = new Property(null); - // Create plottable properties using unit properties from the model. - // Accessor functions return 0 for undefined values (filtered out later by NaN check). - // NOTE: Using bracket notation required by TypeScript's noUncheckedIndexedAccess - const plottableProperties: PlottableProperty[] = [ - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - createPlottableProperty("t", "s", (pt) => pt["t"] ?? 0), - createPlottableProperty( - "x", - model.distanceUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["x"] ?? 0, - ), - createPlottableProperty( - "y", - model.distanceUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["y"] ?? 0, - ), - createPlottableProperty( - "vx", - model.velocityUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["vx"] ?? 0, - ), - createPlottableProperty( - "vy", - model.velocityUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["vy"] ?? 0, - ), - createPlottableProperty( - "speed", - model.velocityUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["speed"] ?? 0, - ), - createPlottableProperty( - "ax", - model.accelerationUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["ax"] ?? 0, - ), - createPlottableProperty( - "ay", - model.accelerationUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["ay"] ?? 0, - ), - createPlottableProperty( - "|a|", - model.accelerationUnitProperty, - // biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation - (pt) => pt["aMag"] ?? 0, - ), - ]; + // Build the registry of plottable quantities from the canonical definition. + // To add a new quantity, edit kinematics-plottable-properties.ts — not here. + const plottableProperties = buildKinematicsPlottableProperties(model); // Default: plot y vs x (trajectory) const initialXProperty = plottableProperties[1]; diff --git a/src/screen-name/view/VideoPlayerNode.ts b/src/screen-name/view/VideoPlayerNode.ts index c4c37c6..d6bbde7 100644 --- a/src/screen-name/view/VideoPlayerNode.ts +++ b/src/screen-name/view/VideoPlayerNode.ts @@ -85,7 +85,8 @@ export class VideoPlayerNode extends Node { // ── Play / Pause ─────────────────────────────────────────────────────── const isPlayingListener = (isPlaying: boolean) => { if (isPlaying) { - this.videoElement.play().catch(() => { + this.videoElement.play().catch((err: unknown) => { + console.error("Video playback failed:", err); model.isPlayingProperty.value = false; }); } else {