Merge pull request #13 from veillette/claude/refactor-model-view-separation-JkHj2

Refactor constants and playback rate handling

Author: veillette

src/TrackLabConstants.ts

@@ -10,6 +10,22 @@
 // Shared corner radius used by the main side panels.
 export const PANEL_CORNER_RADIUS = 8;
 
+// ── Video display dimensions ───────────────────────────────────────────────────
+// The video element is always rendered at this fixed pixel size.
+// Both the OpenCV tracker and all overlay nodes depend on these values.
+export const VIDEO_WIDTH = 640;
+export const VIDEO_HEIGHT = 360;
+
+// ── Video position in screen (layout) coordinates ────────────────────────────
+// SceneryStack's ScreenView.DEFAULT_LAYOUT_BOUNDS = Bounds2(0, 0, 1024, 618).
+// The video element is centered at layoutBounds.center + (0, VIDEO_PLAYER_Y_OFFSET).
+export const VIDEO_CENTER_X = 512; // 1024 / 2
+export const VIDEO_CENTER_Y = 289; // 618 / 2 + VIDEO_PLAYER_Y_OFFSET  (309 - 20)
+
+// ── Initial calibration tool geometry ─────────────────────────────────────────
+// Half-length of the default calibration segment (pixels from centre to each endpoint).
+export const CALIB_HALF_LENGTH = 100;
+
 // ── Screen layout offsets ─────────────────────────────────────────────────────
 // SceneryStack's ScreenView.DEFAULT_LAYOUT_BOUNDS = Bounds2(0, 0, 1024, 618).
 export const VIDEO_PLAYER_Y_OFFSET = -20; // video center offset below layout center

src/screen-name/graph/ConfigurableGraph.ts

@@ -632,8 +632,9 @@ export default class ConfigurableGraph extends Node {
    * Add a new data point based on current property values
    */
   public addDataPoint(): void {
-    const xValue = this.xPropertyProperty.value.property.value;
-    const yValue = this.yPropertyProperty.value.property.value;
+    const xValue = this.xPropertyProperty.value.property?.value;
+    const yValue = this.yPropertyProperty.value.property?.value;
+    if (xValue === undefined || yValue === undefined) return;
 
     this.dataManager.addDataPoint(xValue, yValue);
   }
@@ -697,7 +698,7 @@ export default class ConfigurableGraph extends Node {
     }
     // For properties without sub-step data, fall back to current property value.
     // This handles derived properties like energy, RMS values, etc.
-    return axisProperty.property.value;
+    return axisProperty.property?.value ?? null;
   }
 
   /**

src/screen-name/graph/PlottableProperty.ts

@@ -15,8 +15,11 @@ export type PlottableProperty = {
   // The name to display in the selector (can be a string or a localized string property)
   name: string | TReadOnlyProperty<string>;
 
-  // The property to read values from
-  property: TReadOnlyProperty<number>;
+  // The property to read values from.
+  // Required when subStepAccessor is absent; may be omitted when subStepAccessor
+  // covers all usage paths (e.g. kinematic variables that are always pushed via
+  // addDataPointsFromSubSteps rather than polled with addDataPoint).
+  property?: TReadOnlyProperty<number>;
 
   // Optional unit string for axis label (e.g., "m", "m/s", "J")
   // Can be a static string or a dynamic property for units that depend on calibration

src/screen-name/model/SimModel.ts

@@ -8,10 +8,15 @@ import {
 import { Matrix3, Range, Transform3, Vector2 } from "scenerystack/dot";
 import { TRACK_COLORS } from "../../TrackLabColors.js";
 import {
+  CALIB_HALF_LENGTH,
   MIN_CALIB_DISTANCE,
   MIN_PIXEL_DISTANCE,
   TRACK_SYMBOL_FIRST_CODE,
   TRACK_SYMBOL_LAST_CODE,
+  VIDEO_CENTER_X,
+  VIDEO_CENTER_Y,
+  VIDEO_HEIGHT,
+  VIDEO_WIDTH,
 } from "../../TrackLabConstants.js";
 import { OpenCVTracker } from "../../tracking/OpenCVTracker.js";
 import type {
@@ -21,10 +26,6 @@ import type {
   TrackPoint,
 } from "./Track.js";
 
-// Video display dimensions (used by tracker and views)
-export const VIDEO_WIDTH = 640;
-export const VIDEO_HEIGHT = 360;
-
 // ── Calibration unit type ──────────────────────────────────────────────────
 export const CALIBRATION_UNITS = ["mm", "cm", "m", "km", "in", "ft"] as const;
 export type CalibrationUnit = (typeof CALIBRATION_UNITS)[number];
@@ -35,16 +36,15 @@ export const FRAME_RATE_OPTIONS = [15, 24, 25, 29.97, 30, 50, 60] as const;
 export const DEFAULT_FRAME_RATE = 30;
 export const FRAME_RATE_RANGE = new Range(1, 120);
 
-// ── Layout constants ───────────────────────────────────────────────────────
-// SceneryStack's ScreenView.DEFAULT_LAYOUT_BOUNDS = Bounds2(0, 0, 1024, 618).
-// The VideoPlayerNode is centered at layoutBounds.center + (0, -20).
-const LAYOUT_CENTER_X = 512; // 1024 / 2
-const LAYOUT_CENTER_Y = 309; // 618 / 2
-export const VIDEO_CENTER_X = LAYOUT_CENTER_X; // 512
-export const VIDEO_CENTER_Y = LAYOUT_CENTER_Y - 20; // 289
-const CALIB_HALF_LEN = 100; // pixels from center to each calibration endpoint
+// ── Playback speed multiplier ──────────────────────────────────────────────
+// Stores the actual rate multiplier (1 = normal, 0.5 = slow, 2 = fast).
+// The view maps a TimeSpeed enum to one of these values; the model never
+// imports scenery-phet, so it only sees the numeric rate.
+export const DEFAULT_PLAYBACK_RATE = 1;
+export const PLAYBACK_RATE_RANGE = new Range(0.1, 4);
 
-// Initial tool positions (view / pixel space)
+// ── Initial tool positions (view / pixel space) ───────────────────────────
+// These default positions are computed from the shared video layout constants.
 const COORD_ORIGIN_INITIAL = new Vector2(
   VIDEO_CENTER_X - VIDEO_WIDTH / 4,
   VIDEO_CENTER_Y,
@@ -53,8 +53,8 @@ const CALIB_CENTER_INITIAL = new Vector2(
   VIDEO_CENTER_X,
   VIDEO_CENTER_Y + VIDEO_HEIGHT / 4,
 );
-const CALIB_P1_INITIAL = CALIB_CENTER_INITIAL.plusXY(-CALIB_HALF_LEN, 0);
-const CALIB_P2_INITIAL = CALIB_CENTER_INITIAL.plusXY(CALIB_HALF_LEN, 0);
+const CALIB_P1_INITIAL = CALIB_CENTER_INITIAL.plusXY(-CALIB_HALF_LENGTH, 0);
+const CALIB_P2_INITIAL = CALIB_CENTER_INITIAL.plusXY(CALIB_HALF_LENGTH, 0);
 
 // ── Model-view transform builder ───────────────────────────────────────────
 /**
@@ -292,6 +292,13 @@ export class SimModel {
     range: FRAME_RATE_RANGE,
   });
 
+  // ── Playback speed multiplier (1 = normal, 0.5 = slow, 2 = fast) ────────
+  // The view maps its TimeSpeed enum to this value; the model stays free of
+  // any scenery-phet dependency.
+  public readonly playbackRateProperty = new NumberProperty(DEFAULT_PLAYBACK_RATE, {
+    range: PLAYBACK_RATE_RANGE,
+  });
+
   // Derived frame duration for convenience
   public readonly frameDurationProperty: TReadOnlyProperty<number> =
     new DerivedProperty([this.frameRateProperty], (fps) => 1 / fps);
@@ -460,6 +467,7 @@ export class SimModel {
     this.currentTimeProperty.reset();
     this.durationProperty.reset();
     this.frameRateProperty.reset();
+    this.playbackRateProperty.reset();
     this.axesVisibleProperty.reset();
     this.calibrationVisibleProperty.reset();
     this.magnifyVideoProperty.reset();

src/screen-name/view/AutoTrackerNode.ts

@@ -13,7 +13,8 @@ import { PhetFont } from "scenerystack/scenery-phet";
 import { Tandem } from "scenerystack/tandem";
 import { StringManager } from "../../i18n/StringManager.js";
 import TrackLabColors from "../../TrackLabColors.js";
-import { type SimModel, VIDEO_HEIGHT, VIDEO_WIDTH } from "../model/SimModel.js";
+import { VIDEO_HEIGHT, VIDEO_WIDTH } from "../../TrackLabConstants.js";
+import type { SimModel } from "../model/SimModel.js";
 
 const MAX_TRAIL = 150;
 const CROSSHAIR_SIZE = 16;

src/screen-name/view/CoordinateSystemNode.ts

@@ -6,8 +6,8 @@ import { ArrowNode, PhetFont } from "scenerystack/scenery-phet";
 import { Tandem } from "scenerystack/tandem";
 import { StringManager } from "../../i18n/StringManager.js";
 import TrackLabColors from "../../TrackLabColors.js";
+import { VIDEO_CENTER_X, VIDEO_CENTER_Y, VIDEO_HEIGHT, VIDEO_WIDTH } from "../../TrackLabConstants.js";
 import type { SimModel } from "../model/SimModel.js";
-import { VIDEO_CENTER_X, VIDEO_CENTER_Y, VIDEO_HEIGHT, VIDEO_WIDTH } from "../model/SimModel.js";
 
 const ARROW_LENGTH = 120;
 

src/screen-name/view/DigitizingOverlayNode.ts

@@ -10,7 +10,8 @@ import {
 } from "scenerystack/scenery";
 import { Tandem } from "scenerystack/tandem";
 import TrackLabColors from "../../TrackLabColors.js";
-import { type SimModel, VIDEO_HEIGHT, VIDEO_WIDTH } from "../model/SimModel.js";
+import { VIDEO_HEIGHT, VIDEO_WIDTH } from "../../TrackLabConstants.js";
+import type { SimModel } from "../model/SimModel.js";
 
 const OUTER_R = 12;
 const INNER_R = 2;

src/screen-name/view/KinematicsGraphNode.ts

@@ -6,7 +6,6 @@
  */
 
 import {
-  NumberProperty,
   Property,
   type TReadOnlyProperty,
 } from "scenerystack/axon";
@@ -26,17 +25,17 @@ const GRAPH_HEIGHT = 200;
 const MAX_DATA_POINTS = 5000;
 
 /**
- * Creates a PlottableProperty for a kinematic variable with a subStepAccessor.
+ * Creates a PlottableProperty for a kinematic variable driven entirely by
+ * subStepAccessor.  No backing Property is needed because KinematicsGraphNode
+ * always feeds data via addDataPointsFromSubSteps rather than addDataPoint.
  */
 function createPlottableProperty(
   name: string,
   unit: string | TReadOnlyProperty<string>,
-  dummyProperty: NumberProperty,
   accessor: (point: SubStepDataPoint) => number,
 ): PlottableProperty {
   return {
     name,
-    property: dummyProperty,
     unit,
     subStepAccessor: accessor,
   };
@@ -51,17 +50,6 @@ export class KinematicsGraphNode extends VBox {
   private currentComboBox: ComboBox<string | null> | null = null;
   private readonly disposeKinematicsGraph: () => void;
 
-  // Dummy properties for the graph (values aren't used directly, we push data manually)
-  private readonly tProperty = new NumberProperty(0);
-  private readonly xProperty = new NumberProperty(0);
-  private readonly yProperty = new NumberProperty(0);
-  private readonly vxProperty = new NumberProperty(0);
-  private readonly vyProperty = new NumberProperty(0);
-  private readonly speedProperty = new NumberProperty(0);
-  private readonly axProperty = new NumberProperty(0);
-  private readonly ayProperty = new NumberProperty(0);
-  private readonly aMagProperty = new NumberProperty(0);
-
   public constructor(model: SimModel, listParent: Node) {
     super({
       spacing: 8,
@@ -72,58 +60,18 @@ export class KinematicsGraphNode extends VBox {
     this.listParent = listParent;
     this.selectedTrackProperty = new Property<string | null>(null);
 
-    // Create plottable properties using unit properties from the model
-    // Accessor functions return 0 for undefined values (filtered out later by NaN check)
+    // Create plottable properties using unit properties from the model.
+    // Accessor functions return 0 for undefined values (filtered out later by NaN check).
     const plottableProperties: PlottableProperty[] = [
-      createPlottableProperty("t", "s", this.tProperty, (pt) => pt.t ?? 0),
-      createPlottableProperty(
-        "x",
-        model.distanceUnitProperty,
-        this.xProperty,
-        (pt) => pt.x ?? 0,
-      ),
-      createPlottableProperty(
-        "y",
-        model.distanceUnitProperty,
-        this.yProperty,
-        (pt) => pt.y ?? 0,
-      ),
-      createPlottableProperty(
-        "vx",
-        model.velocityUnitProperty,
-        this.vxProperty,
-        (pt) => pt.vx ?? 0,
-      ),
-      createPlottableProperty(
-        "vy",
-        model.velocityUnitProperty,
-        this.vyProperty,
-        (pt) => pt.vy ?? 0,
-      ),
-      createPlottableProperty(
-        "speed",
-        model.velocityUnitProperty,
-        this.speedProperty,
-        (pt) => pt.speed ?? 0,
-      ),
-      createPlottableProperty(
-        "ax",
-        model.accelerationUnitProperty,
-        this.axProperty,
-        (pt) => pt.ax ?? 0,
-      ),
-      createPlottableProperty(
-        "ay",
-        model.accelerationUnitProperty,
-        this.ayProperty,
-        (pt) => pt.ay ?? 0,
-      ),
-      createPlottableProperty(
-        "|a|",
-        model.accelerationUnitProperty,
-        this.aMagProperty,
-        (pt) => pt.aMag ?? 0,
-      ),
+      createPlottableProperty("t", "s", (pt) => pt.t ?? 0),
+      createPlottableProperty("x", model.distanceUnitProperty, (pt) => pt.x ?? 0),
+      createPlottableProperty("y", model.distanceUnitProperty, (pt) => pt.y ?? 0),
+      createPlottableProperty("vx", model.velocityUnitProperty, (pt) => pt.vx ?? 0),
+      createPlottableProperty("vy", model.velocityUnitProperty, (pt) => pt.vy ?? 0),
+      createPlottableProperty("speed", model.velocityUnitProperty, (pt) => pt.speed ?? 0),
+      createPlottableProperty("ax", model.accelerationUnitProperty, (pt) => pt.ax ?? 0),
+      createPlottableProperty("ay", model.accelerationUnitProperty, (pt) => pt.ay ?? 0),
+      createPlottableProperty("|a|", model.accelerationUnitProperty, (pt) => pt.aMag ?? 0),
     ];
 
     // Default: plot y vs x (trajectory)
@@ -216,15 +164,6 @@ export class KinematicsGraphNode extends VBox {
       }
       this.selectedTrackProperty.dispose();
       this.graph.dispose();
-      this.tProperty.dispose();
-      this.xProperty.dispose();
-      this.yProperty.dispose();
-      this.vxProperty.dispose();
-      this.vyProperty.dispose();
-      this.speedProperty.dispose();
-      this.axProperty.dispose();
-      this.ayProperty.dispose();
-      this.aMagProperty.dispose();
     };
   }
 

src/screen-name/view/PlaybackControlsNode.ts

@@ -44,14 +44,27 @@ export class PlaybackControlsNode extends HBox {
     const uiStrings = StringManager.getInstance().getUI();
 
     // ── Playback rate via TimeSpeed ────────────────────────────────────────
-    const timeSpeedProperty = new EnumerationProperty(TimeSpeed.NORMAL);
+    // timeSpeedProperty is view-local (the TimeSpeed enum is a scenery-phet type
+    // that cannot live in the model).  It syncs bidirectionally with the numeric
+    // model.playbackRateProperty so that model.reset() resets the radio buttons.
     const speedMap = new Map([
       [TimeSpeed.FAST, SPEED_FAST],
       [TimeSpeed.NORMAL, SPEED_NORMAL],
       [TimeSpeed.SLOW, SPEED_SLOW],
     ]);
+    const rateToSpeed = new Map(
+      Array.from(speedMap.entries()).map(([k, v]) => [v, k]),
+    );
+    const timeSpeedProperty = new EnumerationProperty(TimeSpeed.NORMAL);
+
+    // view → model
     timeSpeedProperty.link((speed) => {
-      videoElement.playbackRate = speedMap.get(speed) ?? SPEED_NORMAL;
+      model.playbackRateProperty.value = speedMap.get(speed) ?? SPEED_NORMAL;
+    });
+
+    // model → view  (handles reset and any future programmatic rate changes)
+    model.playbackRateProperty.lazyLink((rate: number) => {
+      timeSpeedProperty.value = rateToSpeed.get(rate) ?? TimeSpeed.NORMAL;
     });
 
     // ── TimeControlNode: play/pause + step back + step forward + speed ─────

src/screen-name/view/VideoPlayerNode.ts

@@ -1,7 +1,8 @@
 import { DerivedProperty } from "scenerystack/axon";
 import { DOM, Node, VBox } from "scenerystack/scenery";
 import TrackLabColors from "../../TrackLabColors.js";
-import { type SimModel, VIDEO_HEIGHT, VIDEO_WIDTH } from "../model/SimModel.js";
+import { VIDEO_HEIGHT, VIDEO_WIDTH } from "../../TrackLabConstants.js";
+import type { SimModel } from "../model/SimModel.js";
 
 const MAIN_CONTENT_SPACING = 10; // VBox gap between source control, video layer, and playback
 
@@ -90,6 +91,12 @@ export class VideoPlayerNode extends Node {
     };
     model.isPlayingProperty.lazyLink(isPlayingListener);
 
+    // ── Playback rate (applies model rate to the video element) ──────────
+    const playbackRateListener = (rate: number) => {
+      this.videoElement.playbackRate = rate;
+    };
+    model.playbackRateProperty.link(playbackRateListener);
+
     // ── Playback controls ─────────────────────────────────────────────────
     const playbackControlsNode = new PlaybackControlsNode(
       model,
@@ -147,6 +154,7 @@ export class VideoPlayerNode extends Node {
     this.disposeVideoPlayer = () => {
       TrackLabColors.videoBackgroundColorProperty.unlink(videoBackgroundListener);
       model.isPlayingProperty.unlink(isPlayingListener);
+      model.playbackRateProperty.unlink(playbackRateListener);
       this.videoElement.removeEventListener("loadedmetadata", onLoadedMetadata);
       this.videoElement.removeEventListener("durationchange", updateDuration);
       this.videoElement.removeEventListener("ended", onEnded);