From d0f3748a1b064bef75c4a25d7cad4a5117e18fb8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 11:38:28 +0000 Subject: [PATCH 1/2] Enforce model-view separation: move layout constants and clean up PlottableProperty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concrete violations of model/view separation were addressed: 1. VIDEO_WIDTH, VIDEO_HEIGHT, VIDEO_CENTER_X, VIDEO_CENTER_Y and CALIB_HALF_LENGTH were defined in SimModel.ts — a model file — even though they describe screen pixel layout. They are now canonical in TrackLabConstants.ts (alongside the other layout constants already there). SimModel imports what it needs (video dimensions for the tracker, center positions for the initial tool positions); view files (CoordinateSystemNode, VideoPlayerNode, DigitizingOverlayNode, AutoTrackerNode) now import directly from TrackLabConstants without touching the model file. 2. KinematicsGraphNode created nine dummy NumberProperty instances whose sole purpose was to satisfy the required property field on PlottableProperty, even though none of their values were ever read (all kinematic data is fed via addDataPointsFromSubSteps / subStepAccessor). PlottableProperty.property is now optional — callers that provide a subStepAccessor covering all data paths may omit it. ConfigurableGraph.addDataPoint() and getValueForAxis() handle the absent-property case. The nine dummy properties are removed from KinematicsGraphNode. No behaviour changes; error count in tsc --noEmit is identical to the baseline. https://claude.ai/code/session_014wtoa1APrzTAN9xNFUe2cc --- src/TrackLabConstants.ts | 16 ++++ src/screen-name/graph/ConfigurableGraph.ts | 7 +- src/screen-name/graph/PlottableProperty.ts | 7 +- src/screen-name/model/SimModel.ts | 25 ++---- src/screen-name/view/AutoTrackerNode.ts | 3 +- src/screen-name/view/CoordinateSystemNode.ts | 2 +- src/screen-name/view/DigitizingOverlayNode.ts | 3 +- src/screen-name/view/KinematicsGraphNode.ts | 89 +++---------------- src/screen-name/view/VideoPlayerNode.ts | 3 +- 9 files changed, 55 insertions(+), 100 deletions(-) diff --git a/src/TrackLabConstants.ts b/src/TrackLabConstants.ts index 0fdde7a..2e954fa 100644 --- a/src/TrackLabConstants.ts +++ b/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 diff --git a/src/screen-name/graph/ConfigurableGraph.ts b/src/screen-name/graph/ConfigurableGraph.ts index 74b3033..3e83bb7 100644 --- a/src/screen-name/graph/ConfigurableGraph.ts +++ b/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; } /** diff --git a/src/screen-name/graph/PlottableProperty.ts b/src/screen-name/graph/PlottableProperty.ts index 7372838..2c4c906 100644 --- a/src/screen-name/graph/PlottableProperty.ts +++ b/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; - // The property to read values from - property: TReadOnlyProperty; + // 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; // 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 diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index a33bd24..5fa192e 100644 --- a/src/screen-name/model/SimModel.ts +++ b/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,8 @@ 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 - -// 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 +46,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 ─────────────────────────────────────────── /** diff --git a/src/screen-name/view/AutoTrackerNode.ts b/src/screen-name/view/AutoTrackerNode.ts index 44243a0..5e609a2 100644 --- a/src/screen-name/view/AutoTrackerNode.ts +++ b/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; diff --git a/src/screen-name/view/CoordinateSystemNode.ts b/src/screen-name/view/CoordinateSystemNode.ts index 121c025..4903ac7 100644 --- a/src/screen-name/view/CoordinateSystemNode.ts +++ b/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; diff --git a/src/screen-name/view/DigitizingOverlayNode.ts b/src/screen-name/view/DigitizingOverlayNode.ts index 1f8053e..0aa9913 100644 --- a/src/screen-name/view/DigitizingOverlayNode.ts +++ b/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; diff --git a/src/screen-name/view/KinematicsGraphNode.ts b/src/screen-name/view/KinematicsGraphNode.ts index 9d9a6a8..7b44d9f 100644 --- a/src/screen-name/view/KinematicsGraphNode.ts +++ b/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, - 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 | 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(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(); }; } diff --git a/src/screen-name/view/VideoPlayerNode.ts b/src/screen-name/view/VideoPlayerNode.ts index 986367d..7f6cc4b 100644 --- a/src/screen-name/view/VideoPlayerNode.ts +++ b/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 From 044ba2b12cf77d3fddcf4eb5d6e31ad8ec35d9d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 11:49:42 +0000 Subject: [PATCH 2/2] Move playback speed out of the view into model.playbackRateProperty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit timeSpeedProperty (EnumerationProperty) in PlaybackControlsNode was directly setting videoElement.playbackRate. model.reset() already resets the analogous frameRateProperty, but the playback speed was silently skipped, so after Reset All the video could continue at half speed. Fix: - Add playbackRateProperty (NumberProperty, default 1.0) to SimModel alongside frameRateProperty. Include it in reset(). - VideoPlayerNode links model.playbackRateProperty → videoElement.playbackRate (the single point where the DOM element is updated). - PlaybackControlsNode keeps its view-local timeSpeedProperty (TimeSpeed is a scenery-phet type that must not enter the model) and syncs bidirectionally: view → model: timeSpeedProperty.link sets model.playbackRateProperty model → view: model.playbackRateProperty.lazyLink updates timeSpeedProperty This mirrors the isDigitizingProperty pattern in TrackListPanel and is safe because Axon deduplicates same-value writes. https://claude.ai/code/session_014wtoa1APrzTAN9xNFUe2cc --- src/screen-name/model/SimModel.ts | 15 +++++++++++++++ src/screen-name/view/PlaybackControlsNode.ts | 17 +++++++++++++++-- src/screen-name/view/VideoPlayerNode.ts | 7 +++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index 5fa192e..ec170e7 100644 --- a/src/screen-name/model/SimModel.ts +++ b/src/screen-name/model/SimModel.ts @@ -36,6 +36,13 @@ 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); +// ── 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) ─────────────────────────── // These default positions are computed from the shared video layout constants. const COORD_ORIGIN_INITIAL = new Vector2( @@ -285,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 = new DerivedProperty([this.frameRateProperty], (fps) => 1 / fps); @@ -453,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(); diff --git a/src/screen-name/view/PlaybackControlsNode.ts b/src/screen-name/view/PlaybackControlsNode.ts index 5c7264e..02447c7 100644 --- a/src/screen-name/view/PlaybackControlsNode.ts +++ b/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 ───── diff --git a/src/screen-name/view/VideoPlayerNode.ts b/src/screen-name/view/VideoPlayerNode.ts index 7f6cc4b..2193f24 100644 --- a/src/screen-name/view/VideoPlayerNode.ts +++ b/src/screen-name/view/VideoPlayerNode.ts @@ -91,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, @@ -148,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);