Skip to content

refactor: narrow constructor arguments to specific sub-models instead of SimModel #62

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

Merged
veillette merged 2 commits into from
Feb 27, 2026
Merged

refactor: narrow constructor arguments to specific sub-models instead of SimModel #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69ad82a
refactor: narrow constructor arguments to specific sub-models instead…
claude Feb 27, 2026
1903e48
refactor: narrow remaining SimModel constructor arguments to sub-models
claude Feb 27, 2026
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
20 changes: 10 additions & 10 deletions src/screen-name/graph/kinematics-plottable-properties.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/

import type { TReadOnlyProperty } from "scenerystack/axon";
import type { SimModel } from "../model/SimModel.js";
import type { OverlayToolsModel } from "../model/OverlayToolsModel.js";
import type { PlottableProperty } from "./PlottableProperty.js";

function createPlottableProperty(
Expand Down Expand Up @@ -55,7 +55,7 @@ export type KinematicsPlottableGroups = {
* @param model - Provides the reactive unit-string properties so that axis
* labels update automatically when the user changes the calibration unit.
*/
export function buildKinematicsPlottableGroups(model: SimModel): KinematicsPlottableGroups {
export function buildKinematicsPlottableGroups(overlayTools: OverlayToolsModel): KinematicsPlottableGroups {
return {
// ── Time ──────────────────────────────────────────────────────────────
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
Expand All @@ -64,29 +64,29 @@ export function buildKinematicsPlottableGroups(model: SimModel): KinematicsPlott
// ── Position ──────────────────────────────────────────────────────────
position: [
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("x", model.overlayTools.calibUnitProperty, (pt) => pt["x"] ?? 0),
createPlottableProperty("x", overlayTools.calibUnitProperty, (pt) => pt["x"] ?? 0),
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("y", model.overlayTools.calibUnitProperty, (pt) => pt["y"] ?? 0),
createPlottableProperty("y", overlayTools.calibUnitProperty, (pt) => pt["y"] ?? 0),
],

// ── Velocity ──────────────────────────────────────────────────────────
velocity: [
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("vx", model.overlayTools.velocityUnitProperty, (pt) => pt["vx"] ?? 0),
createPlottableProperty("vx", overlayTools.velocityUnitProperty, (pt) => pt["vx"] ?? 0),
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("vy", model.overlayTools.velocityUnitProperty, (pt) => pt["vy"] ?? 0),
createPlottableProperty("vy", overlayTools.velocityUnitProperty, (pt) => pt["vy"] ?? 0),
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("speed", model.overlayTools.velocityUnitProperty, (pt) => pt["speed"] ?? 0),
createPlottableProperty("speed", overlayTools.velocityUnitProperty, (pt) => pt["speed"] ?? 0),
],

// ── Acceleration ──────────────────────────────────────────────────────
acceleration: [
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("ax", model.overlayTools.accelerationUnitProperty, (pt) => pt["ax"] ?? 0),
createPlottableProperty("ax", overlayTools.accelerationUnitProperty, (pt) => pt["ax"] ?? 0),
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("ay", model.overlayTools.accelerationUnitProperty, (pt) => pt["ay"] ?? 0),
createPlottableProperty("ay", overlayTools.accelerationUnitProperty, (pt) => pt["ay"] ?? 0),
// biome-ignore lint/complexity/useLiteralKeys: TypeScript requires bracket notation
createPlottableProperty("|a|", model.overlayTools.accelerationUnitProperty, (pt) => pt["aMag"] ?? 0),
createPlottableProperty("|a|", overlayTools.accelerationUnitProperty, (pt) => pt["aMag"] ?? 0),
],
};
}
62 changes: 36 additions & 26 deletions src/screen-name/view/AutoTrackerNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { TReadOnlyProperty } from "scenerystack/axon";
import { type Dimension2, Vector2 } from "scenerystack/dot";
import { type Dimension2, type Transform3, Vector2 } from "scenerystack/dot";
import { Shape } from "scenerystack/kite";
import { DragListener, Line, Node, Path, Rectangle, Text, VBox } from "scenerystack/scenery";
import { PhetFont } from "scenerystack/scenery-phet";
Expand All @@ -15,7 +15,15 @@ import { StringManager } from "../../i18n/StringManager.js";
import TrackLabColors from "../../TrackLabColors.js";
import { VIDEO_HEIGHT, VIDEO_WIDTH } from "../../TrackLabConstants.js";
import trackLab from "../../TrackLabNamespace.js";
import type { SimModel } from "../model/SimModel.js";
import type { TrackingModel } from "../model/TrackingModel.js";

/** Narrowed dependencies passed to AutoTrackerNode at construction time. */
type AutoTrackerNodeOptions = {
tracking: TrackingModel;
videoDimensionsProperty: TReadOnlyProperty<Dimension2>;
frameRateProperty: TReadOnlyProperty<number>;
modelViewTransformProperty: TReadOnlyProperty<Transform3>;
};

const MAX_TRAIL = 150;
const CROSSHAIR_SIZE = 16;
Expand Down Expand Up @@ -47,7 +55,7 @@ const LABELS_SPACING = 8; // vertical gap between hint text and error text
* DOM node at position (0,0).
*/
export class AutoTrackerNode extends Node {
private readonly model: SimModel;
private readonly tracking: TrackingModel;

// ── Trail: O(1) ring buffer ────────────────────────────────────────────
// Using a fixed-size circular buffer instead of a plain array so that the
Expand Down Expand Up @@ -82,16 +90,18 @@ export class AutoTrackerNode extends Node {
/**
* @param videoElement - The video element used both for pixel capture and frame events.
* @param autoTrackingShownProperty - Combined gate (video loaded AND toggle on); controls visibility.
* @param model - Provides active track state and receives recorded positions via addPointToTrack.
* @param options - Narrowed model dependencies: tracking sub-model, specific playback properties,
* and the model-view transform for converting pixel coordinates to model coordinates.
*/
public constructor(
videoElement: HTMLVideoElement,
autoTrackingShownProperty: TReadOnlyProperty<boolean>,
model: SimModel,
options: AutoTrackerNodeOptions,
) {
super({ visible: false });

this.model = model;
const { tracking, videoDimensionsProperty, frameRateProperty, modelViewTransformProperty } = options;
this.tracking = tracking;

const autoTrackerStrings = StringManager.getInstance().getAutoTracker();

Expand Down Expand Up @@ -171,7 +181,7 @@ export class AutoTrackerNode extends Node {
this.trailSize = 0;
// Bump version so any in-flight initTracker call is discarded when it resolves.
this.initVersion++;
this.model.tracking.resetTracker();
this.tracking.resetTracker();
this.setCrosshairVisible(false);
this.trailPath.shape = null;
this.trailPath.visible = false;
Expand Down Expand Up @@ -216,32 +226,32 @@ export class AutoTrackerNode extends Node {

if (region.w > MIN_REGION_SIZE && region.h > MIN_REGION_SIZE) {
// Auto-create a track if none is active
if (!this.model.tracking.activeTrackIdProperty.value) {
this.model.tracking.addTrackAndActivate();
if (!this.tracking.activeTrackIdProperty.value) {
this.tracking.addTrackAndActivate();
}

// initTracker is async (loads WASM on first call); tracking begins
// automatically once isTrackerReady becomes true.
// Capture the current version so stale results from a previous drag
// (still awaiting WASM load) are discarded if a new drag has started.
const capturedVersion = this.initVersion;
this.model.tracking
this.tracking
.initTracker(videoElement, region)
.then(() => {
if (this.initVersion !== capturedVersion) {
// A newer drag has already started; discard this result.
this.model.tracking.resetTracker();
this.tracking.resetTracker();
return;
}
// Guard against the race condition where the user removes the
// active track while WASM was loading. If the track no longer
// exists, abort tracking so the crosshair doesn't appear with
// nowhere to record points.
const activeId = this.model.tracking.activeTrackIdProperty.value;
const activeId = this.tracking.activeTrackIdProperty.value;
const trackStillExists =
activeId !== null && this.model.tracking.tracksProperty.value.some((t) => t.id === activeId);
activeId !== null && this.tracking.tracksProperty.value.some((t) => t.id === activeId);
if (!trackStillExists) {
this.model.tracking.resetTracker();
this.tracking.resetTracker();
this.hintText.visible = true;
}
})
Expand Down Expand Up @@ -269,7 +279,7 @@ export class AutoTrackerNode extends Node {
hitArea.setRect(0, 0, dims.width, dims.height);
centeredLabels.center = new Vector2(dims.width / 2, dims.height / 2);
};
model.playback.videoDimensionsProperty.link(videoDimensionsListener);
videoDimensionsProperty.link(videoDimensionsListener);

// ── Track on every video frame ────────────────────────────────────────
// OpenCV template matching (track()) is a heavy synchronous operation.
Expand All @@ -278,14 +288,14 @@ export class AutoTrackerNode extends Node {
// event callbacks from piling up and freezing the main thread.
const processFrame = async () => {
this.pendingFrameId = 0;
if (!(this.visible && this.model.tracking.isTrackerReady)) {
if (!(this.visible && this.tracking.isTrackerReady)) {
return;
}

this.trackInProgress = true;
let pt: { x: number; y: number } | null = null;
try {
pt = await this.model.tracking.trackFrame(videoElement);
pt = await this.tracking.trackFrame(videoElement);
} catch {
// Tracker was disposed mid-flight (e.g. new selection started); skip frame.
return;
Expand All @@ -306,21 +316,21 @@ export class AutoTrackerNode extends Node {
this.updateTrackerVisuals(pt);

// ── Record position to model if a track is active ─────────────────
const activeId = model.tracking.activeTrackIdProperty.value;
const activeId = tracking.activeTrackIdProperty.value;
if (activeId) {
const time = videoElement.currentTime;
// Multiply by frame rate directly rather than dividing by frameDuration
// (1/fps) to avoid cascading floating-point error at non-integer fps values
// like 29.97, which could cause two adjacent timestamps to map to the same
// frame or skip a frame entirely.
const frame = Math.round(time * model.playback.frameRateProperty.value);
const frame = Math.round(time * frameRateProperty.value);

// O(1) duplicate-frame check via Set (vs O(n) linear scan).
if (!this.recordedFrames.has(frame)) {
// Convert video-local pixel coords directly to model coords.
// The MVT operates in video-local space, matching these coordinates.
const modelPt = model.pixelToModelCoords(new Vector2(pt.x, pt.y));
model.tracking.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y);
const modelPt = modelViewTransformProperty.value.inversePosition2(new Vector2(pt.x, pt.y));
tracking.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y);
this.recordedFrames.add(frame);
}
}
Expand All @@ -338,7 +348,7 @@ export class AutoTrackerNode extends Node {
// track so frames from the previous track don't suppress recording on the
// new one.
const clearRecordedFrames = () => this.recordedFrames.clear();
model.tracking.activeTrackIdProperty.lazyLink(clearRecordedFrames);
tracking.activeTrackIdProperty.lazyLink(clearRecordedFrames);

// ── Show/hide based on combined "video loaded && autoTracking" ────────
const autoTrackingShownListener = (shown: boolean) => {
Expand All @@ -357,10 +367,10 @@ export class AutoTrackerNode extends Node {
videoElement.removeEventListener("timeupdate", onFrame);
videoElement.removeEventListener("seeked", onFrame);
this.cancelPendingFrame();
model.tracking.activeTrackIdProperty.unlink(clearRecordedFrames);
tracking.activeTrackIdProperty.unlink(clearRecordedFrames);
autoTrackingShownProperty.unlink(autoTrackingShownListener);
model.playback.videoDimensionsProperty.unlink(videoDimensionsListener);
this.model.tracking.resetTracker();
videoDimensionsProperty.unlink(videoDimensionsListener);
this.tracking.resetTracker();
};
}

Expand Down Expand Up @@ -403,7 +413,7 @@ export class AutoTrackerNode extends Node {
public reset(): void {
this.cancelPendingFrame();
this.trackInProgress = false;
this.model.tracking.resetTracker();
this.tracking.resetTracker();
this.trailHead = 0;
this.trailSize = 0;
this.recordedFrames.clear();
Expand Down
37 changes: 21 additions & 16 deletions src/screen-name/view/CalibrationToolNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import {
OVERLAY_SHIFT_DRAG_SPEED,
} from "../../TrackLabConstants.js";
import trackLab from "../../TrackLabNamespace.js";
import { CALIBRATION_UNITS } from "../model/OverlayToolsModel.js";
import type { SimModel } from "../model/SimModel.js";
import { CALIBRATION_UNITS, type OverlayToolsModel } from "../model/OverlayToolsModel.js";

const FONT = new PhetFont(14);
const WARNING_FONT = new PhetFont({ size: 11, weight: "bold" });
Expand Down Expand Up @@ -62,9 +61,15 @@ export class CalibrationToolNode extends Node {
/**
* @param videoLoadedProperty - Controls visibility; node is hidden until a video is loaded.
* @param listParent - Scene-graph node used as the popup list parent for the unit ComboBox.
* @param model - Provides calibration properties and receives user-entered values.
* @param overlayTools - Provides calibration properties and receives user-entered values.
* @param activeTrackIdProperty - When non-null, dims and locks the tool to prevent accidental moves while digitizing.
*/
public constructor(videoLoadedProperty: TReadOnlyProperty<boolean>, listParent: Node, model: SimModel) {
public constructor(
videoLoadedProperty: TReadOnlyProperty<boolean>,
listParent: Node,
overlayTools: OverlayToolsModel,
activeTrackIdProperty: TReadOnlyProperty<string | null>,
) {
super();

const calibrationStrings = StringManager.getInstance().getCalibration();
Expand Down Expand Up @@ -135,14 +140,14 @@ export class CalibrationToolNode extends Node {
});
// Pattern shown inside the dialog as "Range: {{min}} – {{max}} <unit>"
const rangePatternProperty = new DerivedProperty(
[model.overlayTools.calibUnitProperty],
[overlayTools.calibUnitProperty],
(unit) => `{{min}} – {{max}} ${unit}`,
);

// ── Midpoint panel ────────────────────────────────────────────────────
// Button showing current value + unit; clicking it opens the keypad.
const buttonLabelProperty = new DerivedProperty(
[model.overlayTools.calibDistanceProperty, model.overlayTools.calibUnitProperty],
[overlayTools.calibDistanceProperty, overlayTools.calibUnitProperty],
(dist, unit) => `${dist.toFixed(CALIBRATION_DECIMAL_PLACES)} ${unit}`,
);

Expand All @@ -156,9 +161,9 @@ export class CalibrationToolNode extends Node {
listener: () => {
keypadDialog.beginEdit(
(value: number) => {
model.overlayTools.calibDistanceProperty.value = value;
overlayTools.calibDistanceProperty.value = value;
},
model.overlayTools.calibDistanceProperty.range,
overlayTools.calibDistanceProperty.range,
rangePatternProperty,
() => {
/* no-op: keypad close callback not needed */
Expand All @@ -178,7 +183,7 @@ export class CalibrationToolNode extends Node {
}),
tandemName: `${unit}Item`,
}));
const unitComboBox = new ComboBox(model.overlayTools.calibUnitProperty, unitItems, listParent, {
const unitComboBox = new ComboBox(overlayTools.calibUnitProperty, unitItems, listParent, {
buttonFill: TrackLabColors.comboBoxButtonFillProperty,
listFill: TrackLabColors.comboBoxListFillProperty,
highlightFill: TrackLabColors.comboBoxHighlightFillProperty,
Expand Down Expand Up @@ -213,8 +218,8 @@ export class CalibrationToolNode extends Node {

// ── Update geometry when endpoints move ───────────────────────────────
const updateGeometry = () => {
const p1 = model.overlayTools.calibPoint1Property.value;
const p2 = model.overlayTools.calibPoint2Property.value;
const p1 = overlayTools.calibPoint1Property.value;
const p2 = overlayTools.calibPoint2Property.value;

// Update both shadow and main lines
calibrationLineShadow.setLine(p1.x, p1.y, p2.x, p2.y);
Expand Down Expand Up @@ -249,14 +254,14 @@ export class CalibrationToolNode extends Node {
// is rebuilt once per change event regardless of which endpoint moved,
// and disposal is managed in one place.
const calibMultilink = Multilink.multilink(
[model.overlayTools.calibPoint1Property, model.overlayTools.calibPoint2Property],
[overlayTools.calibPoint1Property, overlayTools.calibPoint2Property],
updateGeometry,
);

// ── Drag listeners for endpoints ──────────────────────────────────────
endpoint1.addInputListener(
new RichDragListener({
positionProperty: model.overlayTools.calibPoint1Property,
positionProperty: overlayTools.calibPoint1Property,
keyboardDragListenerOptions: {
dragSpeed: OVERLAY_DRAG_SPEED,
shiftDragSpeed: OVERLAY_SHIFT_DRAG_SPEED,
Expand All @@ -266,7 +271,7 @@ export class CalibrationToolNode extends Node {
);
endpoint2.addInputListener(
new RichDragListener({
positionProperty: model.overlayTools.calibPoint2Property,
positionProperty: overlayTools.calibPoint2Property,
keyboardDragListenerOptions: {
dragSpeed: OVERLAY_DRAG_SPEED,
shiftDragSpeed: OVERLAY_SHIFT_DRAG_SPEED,
Expand All @@ -289,12 +294,12 @@ export class CalibrationToolNode extends Node {
this.pickable = !isDigitizing;
this.opacity = isDigitizing ? DIGITIZING_DIM_OPACITY : 1;
};
model.tracking.activeTrackIdProperty.link(onActiveTrackChange);
activeTrackIdProperty.link(onActiveTrackChange);

this.disposeCalibrationToolNode = () => {
calibMultilink.dispose();
videoLoadedProperty.unlink(onVideoLoaded);
model.tracking.activeTrackIdProperty.unlink(onActiveTrackChange);
activeTrackIdProperty.unlink(onActiveTrackChange);
rangePatternProperty.dispose();
buttonLabelProperty.dispose();
};
Expand Down
Loading