Skip to content

Fix all three critical issues from CODE_REVIEW.md #15

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 3 commits into from
Feb 21, 2026
Merged

Fix all three critical issues from CODE_REVIEW.md #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5b52ac
Fix all three critical issues from CODE_REVIEW.md
claude Feb 21, 2026
4b8ff10
Fix all seven moderate issues from CODE_REVIEW.md
claude Feb 21, 2026
302f65b
Fix all six minor issues from CODE_REVIEW.md
claude Feb 21, 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
13 changes: 8 additions & 5 deletions src/TrackLabConstants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,28 @@
// Shared corner radius used by the main side panels.
export const PANEL_CORNER_RADIUS = 8;

// ── Screen layout bounds ───────────────────────────────────────────────────────
// SceneryStack's ScreenView.DEFAULT_LAYOUT_BOUNDS = Bounds2(0, 0, 1024, 618).
const LAYOUT_WIDTH = 1024;
const LAYOUT_HEIGHT = 618;

// ── 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)
export const VIDEO_PLAYER_Y_OFFSET = -20; // video center offset below layout center
export const VIDEO_CENTER_X = LAYOUT_WIDTH / 2; // 512
export const VIDEO_CENTER_Y = LAYOUT_HEIGHT / 2 + VIDEO_PLAYER_Y_OFFSET; // 289

// ── 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
export const CONTROL_PANEL_LEFT_MARGIN = 10; // control panel inset from layout left edge
export const TRACK_LIST_LEFT_SPACING = -100; // offset from video right edge (negative moves panels left)
export const DATA_TABLE_TOP_SPACING = 8; // gap between track list bottom and data table
Expand Down
10 changes: 10 additions & 0 deletions src/i18n/StringManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ import { LocalizedString, type ReadOnlyProperty } from "scenerystack";
import strings_en from "./strings_en.json";
import strings_fr from "./strings_fr.json";

// ── Compile-time key-parity check ─────────────────────────────────────────────
// These assignments are never executed at runtime; they exist solely so
// TypeScript verifies that both language files share identical key structures.
// If a key is added to one file but not the other, a type error will appear
// here before the app is ever run.
// biome-ignore lint/correctness/noUnusedVariables: compile-time type guard only
const _enMatchesFr: typeof strings_fr = strings_en;
// biome-ignore lint/correctness/noUnusedVariables: compile-time type guard only
const _frMatchesEn: typeof strings_en = strings_fr;

/**
* Manages all localized strings for the simulation
*/
Expand Down
13 changes: 11 additions & 2 deletions src/screen-name/graph/GraphInteractionHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,11 @@ export default class GraphInteractionHandler {
if (mouseDragStartY !== null && mouseDragInitialYRange) {
const deltaY = event.pointer.point.y - mouseDragStartY;

// Convert delta to model coordinates
// Convert delta to model coordinates.
// Screen Y increases downward while model Y increases upward, so a
// positive screen delta (drag down) corresponds to a positive model
// delta (shift range up). The result is that content follows the
// drag — dragging down pans the view down — matching the X-axis UX.
const modelDeltaY =
deltaY * (mouseDragInitialYRange.getLength() / this.graphHeight);

Expand Down Expand Up @@ -766,7 +770,12 @@ export default class GraphInteractionHandler {
if (mouseDragStartX !== null && mouseDragInitialXRange) {
const deltaX = event.pointer.point.x - mouseDragStartX;

// Convert delta to model coordinates
// Convert delta to model coordinates.
// Screen X and model X share the same direction, so without the
// negation a rightward drag would shift the range right and the
// content would move LEFT. The negation makes the content follow
// the drag — dragging right pans the view right — matching the
// Y-axis UX above.
const modelDeltaX =
-deltaX * (mouseDragInitialXRange.getLength() / this.graphWidth);

Expand Down
254 changes: 105 additions & 149 deletions src/screen-name/model/SimModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ const CALIB_CENTER_INITIAL = new Vector2(
const CALIB_P1_INITIAL = CALIB_CENTER_INITIAL.plusXY(-CALIB_HALF_LENGTH, 0);
const CALIB_P2_INITIAL = CALIB_CENTER_INITIAL.plusXY(CALIB_HALF_LENGTH, 0);

// ── Bounds for clamping the coordinate-system origin ─────────────────────────
// The origin must stay within the video area so the axes are always visible.
// These are layout / pixel-space bounds, matching the view-layer video rectangle.
const COORD_ORIGIN_BOUNDS_MIN_X = VIDEO_CENTER_X - VIDEO_WIDTH / 2;
const COORD_ORIGIN_BOUNDS_MAX_X = VIDEO_CENTER_X + VIDEO_WIDTH / 2;
const COORD_ORIGIN_BOUNDS_MIN_Y = VIDEO_CENTER_Y - VIDEO_HEIGHT / 2;
const COORD_ORIGIN_BOUNDS_MAX_Y = VIDEO_CENTER_Y + VIDEO_HEIGHT / 2;

// ── Model-view transform builder ───────────────────────────────────────────
/**
* Builds a Transform3 from the coordinate-system tool and calibration tool.
Expand Down Expand Up @@ -90,6 +98,36 @@ function buildModelViewTransform(
}

// ── Kinematics computation ───────────────────────────────────────────────

/**
* Scalar finite difference at index i within an array of n values.
*
* Uses forward difference at the first point, backward difference at the last,
* and central differences for all interior points. Returns null if the array
* has fewer than 2 elements, if the time interval is non-positive, or if
* either endpoint value is null.
*
* @param getValue Returns the scalar quantity at index j (null = unknown).
* @param getTime Returns the time stamp at index j.
* @param i Index to differentiate at.
* @param n Total number of elements.
*/
function finiteDifference(
getValue: (j: number) => number | null,
getTime: (j: number) => number,
i: number,
n: number,
): number | null {
if (n < 2) return null;
const [prevIdx, nextIdx] =
i === 0 ? [0, 1] : i === n - 1 ? [n - 2, n - 1] : [i - 1, i + 1];
const prev = getValue(prevIdx);
const next = getValue(nextIdx);
const dt = getTime(nextIdx) - getTime(prevIdx);
if (prev === null || next === null || dt <= 0) return null;
return (next - prev) / dt;
}

/**
* Computes velocity and acceleration for each point in a track.
* Uses central differences where possible for better accuracy.
Expand All @@ -110,153 +148,27 @@ function computeTrackKinematics(track: Track): TrackKinematics {
return { ...track, points: [] };
}

// Helper to safely get a point (returns undefined if out of bounds)
const getPoint = (idx: number): TrackPoint | undefined => points[idx];

// Compute velocity at index i using finite differences
const computeVelocity = (
i: number,
): { vx: number | null; vy: number | null } => {
if (n < 2) {
return { vx: null, vy: null };
}

const curr = getPoint(i);
if (!curr) return { vx: null, vy: null };

if (i === 0) {
// Forward difference for first point
const next = getPoint(1);
if (!next) return { vx: null, vy: null };
const dt = next.time - curr.time;
if (dt <= 0) return { vx: null, vy: null };
return {
vx: (next.x - curr.x) / dt,
vy: (next.y - curr.y) / dt,
};
}

if (i === n - 1) {
// Backward difference for last point
const prev = getPoint(n - 2);
if (!prev) return { vx: null, vy: null };
const dt = curr.time - prev.time;
if (dt <= 0) return { vx: null, vy: null };
return {
vx: (curr.x - prev.x) / dt,
vy: (curr.y - prev.y) / dt,
};
}

// Central difference for interior points
const prev = getPoint(i - 1);
const next = getPoint(i + 1);
if (!prev || !next) return { vx: null, vy: null };
const dt = next.time - prev.time;
if (dt <= 0) return { vx: null, vy: null };
return {
vx: (next.x - prev.x) / dt,
vy: (next.y - prev.y) / dt,
};
};

// First pass: compute velocities
const velocities = points.map((_, i) => computeVelocity(i));

// Helper to safely get velocity
const getVelocity = (
idx: number,
): { vx: number | null; vy: number | null } | undefined => velocities[idx];

// Compute acceleration at index i using finite differences on velocities
const computeAcceleration = (
i: number,
): { ax: number | null; ay: number | null } => {
if (n < 2) {
return { ax: null, ay: null };
}

const curr = getPoint(i);
if (!curr) return { ax: null, ay: null };

if (i === 0) {
// Forward difference
const v0 = getVelocity(0);
const v1 = getVelocity(1);
const next = getPoint(1);
if (!v0 || !v1 || !next) return { ax: null, ay: null };
if (
v0.vx === null ||
v1.vx === null ||
v0.vy === null ||
v1.vy === null
) {
return { ax: null, ay: null };
}
const dt = next.time - curr.time;
if (dt <= 0) return { ax: null, ay: null };
return {
ax: (v1.vx - v0.vx) / dt,
ay: (v1.vy - v0.vy) / dt,
};
}
const getTime = (j: number) => points[j]?.time ?? 0;
const getX = (j: number) => points[j]?.x ?? null;
const getY = (j: number) => points[j]?.y ?? null;

if (i === n - 1) {
// Backward difference
const vPrev = getVelocity(n - 2);
const vCurr = getVelocity(n - 1);
const prev = getPoint(n - 2);
if (!vPrev || !vCurr || !prev) return { ax: null, ay: null };
if (
vPrev.vx === null ||
vCurr.vx === null ||
vPrev.vy === null ||
vCurr.vy === null
) {
return { ax: null, ay: null };
}
const dt = curr.time - prev.time;
if (dt <= 0) return { ax: null, ay: null };
return {
ax: (vCurr.vx - vPrev.vx) / dt,
ay: (vCurr.vy - vPrev.vy) / dt,
};
}

// Central difference
const vPrev = getVelocity(i - 1);
const vNext = getVelocity(i + 1);
const prev = getPoint(i - 1);
const next = getPoint(i + 1);
if (!vPrev || !vNext || !prev || !next) return { ax: null, ay: null };
if (
vPrev.vx === null ||
vNext.vx === null ||
vPrev.vy === null ||
vNext.vy === null
) {
return { ax: null, ay: null };
}
const dt = next.time - prev.time;
if (dt <= 0) return { ax: null, ay: null };
return {
ax: (vNext.vx - vPrev.vx) / dt,
ay: (vNext.vy - vPrev.vy) / dt,
};
};
// First pass: velocities via finite difference of position
const vxArr = points.map((_, i) => finiteDifference(getX, getTime, i, n));
const vyArr = points.map((_, i) => finiteDifference(getY, getTime, i, n));

// Second pass: compute accelerations
const accelerations = points.map((_, i) => computeAcceleration(i));
// Second pass: accelerations via finite difference of velocity
const axArr = points.map((_, i) =>
finiteDifference((j) => vxArr[j] ?? null, getTime, i, n),
);
const ayArr = points.map((_, i) =>
finiteDifference((j) => vyArr[j] ?? null, getTime, i, n),
);

// Combine all data into KinematicPoints
const kinematicPoints: KinematicPoint[] = points.map((pt, i) => {
const vel = velocities[i];
const acc = accelerations[i];
const vx = vel?.vx ?? null;
const vy = vel?.vy ?? null;
const ax = acc?.ax ?? null;
const ay = acc?.ay ?? null;

const vx = vxArr[i] ?? null;
const vy = vyArr[i] ?? null;
const ax = axArr[i] ?? null;
const ay = ayArr[i] ?? null;
return {
frame: pt.frame,
time: pt.time,
Expand Down Expand Up @@ -376,11 +288,27 @@ export class SimModel {
public readonly canAddTrackProperty = new BooleanProperty(true);

// ── Derived kinematics for all tracks ───────────────────────────────────
// Automatically computes velocity and acceleration from position data
// Cache keyed by track ID; only recomputes kinematics for tracks whose
// point array reference has changed since the last derivation. Because
// addPointToTrack() always creates a new points array, reference equality
// is sufficient to detect modifications.
private readonly kinematicsCache = new Map<
string,
{ points: Track["points"]; kinematics: TrackKinematics }
>();

public readonly trackKinematicsProperty: TReadOnlyProperty<
readonly TrackKinematics[]
> = new DerivedProperty([this.tracksProperty], (tracks) =>
tracks.map((track) => computeTrackKinematics(track)),
tracks.map((track) => {
const cached = this.kinematicsCache.get(track.id);
if (cached && cached.points === track.points) {
return cached.kinematics;
}
const kinematics = computeTrackKinematics(track);
this.kinematicsCache.set(track.id, { points: track.points, kinematics });
return kinematics;
}),
);
// Symbols are assigned sequentially (A → Z) and intentionally not reused
// after a track is removed. Stable, unique symbols matter for data export
Expand All @@ -389,6 +317,27 @@ export class SimModel {
private nextSymbolCode = TRACK_SYMBOL_FIRST_CODE;

public constructor() {
// ── Clamp coord origin to the video area ────────────────────────────────
// Validation lives here rather than in the view so that any writer of
// coordOriginProperty (drag listener, programmatic reset, etc.) benefits
// from the constraint without needing per-call clamping logic.
// The guard `if (clampedX !== pos.x || clampedY !== pos.y)` prevents
// infinite recursion: after the clamped value is written, the listener
// fires again but finds the condition false and exits.
this.coordOriginProperty.lazyLink((pos) => {
const clampedX = Math.max(
COORD_ORIGIN_BOUNDS_MIN_X,
Math.min(COORD_ORIGIN_BOUNDS_MAX_X, pos.x),
);
const clampedY = Math.max(
COORD_ORIGIN_BOUNDS_MIN_Y,
Math.min(COORD_ORIGIN_BOUNDS_MAX_Y, pos.y),
);
if (clampedX !== pos.x || clampedY !== pos.y) {
this.coordOriginProperty.value = pos.copy().setXY(clampedX, clampedY);
}
});

this.modelViewTransformProperty.lazyLink((newMVT) => {
if (this.prevModelViewTransform === null) {
this.prevModelViewTransform = newMVT;
Expand Down Expand Up @@ -455,10 +404,16 @@ export class SimModel {
const tracks = this.tracksProperty.value.map((track) => {
if (track.id !== id) return track;

// Reject duplicate frames. Without this guard a second click on the
// same frame (manual digitizing) or a repeated timeupdate event would
// add a second point at the same frame index, corrupting kinematics.
if (track.points.some((p) => p.frame === frame)) return track;
// If the user re-digitizes a point at the same frame (e.g. to correct a
// misclick), replace the existing coordinates rather than silently
// discarding the new position. Adding a second point at the same frame
// would corrupt kinematics, so replacement is the only safe update path.
const existingIndex = track.points.findIndex((p) => p.frame === frame);
if (existingIndex !== -1) {
const updatedPoints = [...track.points];
updatedPoints[existingIndex] = { frame, time, x, y };
return { ...track, points: updatedPoints };
}

const point: TrackPoint = { frame, time, x, y };
const updated: Track = { ...track, points: [...track.points, point] };
Expand All @@ -469,6 +424,7 @@ export class SimModel {

public reset(): void {
this.prevModelViewTransform = null;
this.kinematicsCache.clear();
this.isPlayingProperty.reset();
this.currentTimeProperty.reset();
this.durationProperty.reset();
Expand Down
Loading