From dbc9fbd7ddf3c529efba3c6707c2be8c1cad73c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:10:15 +0000 Subject: [PATCH 1/4] Add frame-based track point deletion with single-level undo Closes the gap behind issue #48: there was no way to remove a single digitized point, and addPointToTrack's first-wins dedupe meant a user who misclicked and clicked again got silence. Rather than making the 2px mark dots clickable (they are batched into one Shape per track on a pickable:false layer, and the overlay already records a point on any click), deletion targets the point identified by (active track, current frame). That needs no hit-testing, no selection state, and no delete mode, and behaves the same on mouse, touch, and keyboard. Model: - removePointFromTrack / restoreLastDeletedPoint / canRestorePointProperty - addOrReplacePointOnTrack: manual digitizing now overwrites the point at a frame, so re-clicking corrects a misclick. AutoTracker keeps the first-wins policy so it never clobbers a hand-placed point. - Points are inserted in frame order. KinematicsComputer differentiates by array index assuming time increases along it; appending only held while digitizing ran forwards, and broke on restore or on digitizing after scrubbing backwards. View: - Eraser and restore buttons in the playback controls, beside the step buttons rather than next to TrackListPanel's per-track trash icon. The eraser is disabled until the current frame holds a point, which is how the interaction teaches itself. - The current frame's point is ringed on the video, so the erase target is visible before committing. - Delete/Backspace on the now-focusable digitizing overlay. - Data table: the current frame's row is outlined and scrolled into view, and clicking a row seeks the video there, making the table a navigation surface into the same erase action. Also fixes a latent bug the feature would have exposed: TableRenderer's incremental path could append rows but never remove them, so a deleted point left its behind showing stale coordinates. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gaCkV6CD5uR1SJpq3BPss --- src/TrackLabColors.ts | 2 + src/TrackLabIcons.ts | 70 ++++++ src/i18n/StringManager.ts | 8 + src/i18n/strings_en.json | 8 +- src/i18n/strings_es.json | 8 +- src/i18n/strings_fr.json | 8 +- src/track-lab/model/TrackLabModel.ts | 28 ++- src/track-lab/model/TrackingModel.ts | 122 ++++++++++- src/track-lab/view/DataTableNode.ts | 14 +- src/track-lab/view/DigitizingOverlayNode.ts | 46 +++- src/track-lab/view/PlaybackControlsNode.ts | 42 +++- src/track-lab/view/TableRenderer.ts | 111 +++++++++- .../view/TrackLabKeyboardHelpContent.ts | 17 ++ src/track-lab/view/TrackLabScreenView.ts | 1 + src/track-lab/view/VideoPlayerNode.ts | 3 + tests/setup.ts | 16 ++ tests/track-lab/model/TrackingModel.test.ts | 205 ++++++++++++++++++ 17 files changed, 688 insertions(+), 21 deletions(-) create mode 100644 tests/track-lab/model/TrackingModel.test.ts diff --git a/src/TrackLabColors.ts b/src/TrackLabColors.ts index 0434907..18b45cd 100644 --- a/src/TrackLabColors.ts +++ b/src/TrackLabColors.ts @@ -253,6 +253,8 @@ const TrackLabColors = { tableEmptyTextProperty: profileColor("tableEmptyText", new Color(136, 136, 136), new Color(120, 120, 120)), tableSymbolShadowProperty: profileColor("tableSymbolShadow", new Color(0, 0, 0, 0.5), new Color(0, 0, 0, 0.3)), tableBackgroundProperty: profileColor("tableBackground", WHITE, new Color(250, 250, 250)), + // Outline marking the row for the frame the video is parked on. + tableCurrentRowProperty: profileColor("tableCurrentRow", new Color(232, 119, 34), new Color(214, 100, 20)), exportButtonProperty: profileColor("exportButton", new Color(76, 175, 80), new Color(60, 150, 65)), // Graph (ConfigurableGraph, GraphDataManager, GraphControlsPanel) diff --git a/src/TrackLabIcons.ts b/src/TrackLabIcons.ts index c7acfad..e8358f3 100644 --- a/src/TrackLabIcons.ts +++ b/src/TrackLabIcons.ts @@ -143,6 +143,76 @@ export function makeTrashIcon(): Node { return new Node({ children: [handle, lid, body, l1, l2, l3] }); } +/** + * Eraser icon: a tilted eraser block with a divider between the rubber and the + * sleeve. Deliberately unlike makeTrashIcon() — the erase-point and + * remove-track actions must never be mistaken for one another. + */ +export function makeEraserIcon(): Node { + const w = 12; // eraser block length + const h = 6.5; // eraser block height + const lw = 1.2; + const sleeveX = 4.5; // divider between rubber tip and sleeve + const tilt = -Math.PI / 6; // block leans like a held eraser + + const body = new Rectangle(0, 0, w, h, 1, 1, { + stroke: TrackLabColors.playbackButtonIconColorProperty, + lineWidth: lw, + fill: null, + }); + const divider = new Line(sleeveX, 0, sleeveX, h, { + stroke: TrackLabColors.playbackButtonIconColorProperty, + lineWidth: lw, + }); + + const block = new Node({ children: [body, divider] }); + block.rotation = tilt; + // Re-wrap so the caller receives an unrotated node whose origin is its centre. + return new Node({ children: [block] }); +} + +/** + * Restore icon: a counter-clockwise arrow looping back on itself ("undo the + * last delete"). + */ +export function makeRestoreIcon(): Node { + const r = 5; // arc radius + const lw = 1.6; + const headLength = 5; // tip-to-base distance of the arrowhead + const headHalfWidth = 3; // half the arrowhead's base + // The glyph is only ~12px on screen, so the head has to be large relative to + // the arc — a subtle one just reads as a letter "C". + const headAngle = Math.PI * 0.4; + const tailAngle = Math.PI * 1.75; + + const arc = new Path(new Shape().arc(0, 0, r, headAngle, tailAngle, false), { + stroke: TrackLabColors.playbackButtonIconColorProperty, + lineWidth: lw, + fill: null, + }); + + // Arrowhead at the arc's start, pointing back against the sweep so the loop + // reads as "go backwards". Tangent at headAngle is (-sin, cos); negate it. + const hx = r * Math.cos(headAngle); + const hy = r * Math.sin(headAngle); + const tipX = Math.sin(headAngle); + const tipY = -Math.cos(headAngle); + // Perpendicular to the tip direction, used to lay out the base corners. + const perpX = -tipY; + const perpY = tipX; + + const head = new Path( + new Shape() + .moveTo(hx + tipX * headLength, hy + tipY * headLength) + .lineTo(hx + perpX * headHalfWidth, hy + perpY * headHalfWidth) + .lineTo(hx - perpX * headHalfWidth, hy - perpY * headHalfWidth) + .close(), + { fill: TrackLabColors.playbackButtonIconColorProperty }, + ); + + return new Node({ children: [arc, head] }); +} + /** * Two small XY arrows for the coordinate-system control / info-dialog row. */ diff --git a/src/i18n/StringManager.ts b/src/i18n/StringManager.ts index 23fbd79..9b3a7af 100644 --- a/src/i18n/StringManager.ts +++ b/src/i18n/StringManager.ts @@ -88,11 +88,13 @@ export class StringManager { titleStringProperty: ReadOnlyProperty; simulationControlsStringProperty: ReadOnlyProperty; graphInteractionsStringProperty: ReadOnlyProperty; + digitizingStringProperty: ReadOnlyProperty; playPauseSimulationStringProperty: ReadOnlyProperty; resetSimulationStringProperty: ReadOnlyProperty; stepBackwardStringProperty: ReadOnlyProperty; stepForwardStringProperty: ReadOnlyProperty; rewindToStartStringProperty: ReadOnlyProperty; + erasePointStringProperty: ReadOnlyProperty; resetZoomStringProperty: ReadOnlyProperty; zoomInOutStringProperty: ReadOnlyProperty; panViewStringProperty: ReadOnlyProperty; @@ -101,11 +103,13 @@ export class StringManager { titleStringProperty: stringProperties.keyboardShortcuts.titleStringProperty, simulationControlsStringProperty: stringProperties.keyboardShortcuts.simulationControlsStringProperty, graphInteractionsStringProperty: stringProperties.keyboardShortcuts.graphInteractionsStringProperty, + digitizingStringProperty: stringProperties.keyboardShortcuts.digitizingStringProperty, playPauseSimulationStringProperty: stringProperties.keyboardShortcuts.playPauseSimulationStringProperty, resetSimulationStringProperty: stringProperties.keyboardShortcuts.resetSimulationStringProperty, stepBackwardStringProperty: stringProperties.keyboardShortcuts.stepBackwardStringProperty, stepForwardStringProperty: stringProperties.keyboardShortcuts.stepForwardStringProperty, rewindToStartStringProperty: stringProperties.keyboardShortcuts.rewindToStartStringProperty, + erasePointStringProperty: stringProperties.keyboardShortcuts.erasePointStringProperty, resetZoomStringProperty: stringProperties.keyboardShortcuts.resetZoomStringProperty, zoomInOutStringProperty: stringProperties.keyboardShortcuts.zoomInOutStringProperty, panViewStringProperty: stringProperties.keyboardShortcuts.panViewStringProperty, @@ -441,6 +445,8 @@ export class StringManager { digitizingAreaStringProperty: ReadOnlyProperty; digitizeTrackStringProperty: ReadOnlyProperty; removeTrackStringProperty: ReadOnlyProperty; + erasePointStringProperty: ReadOnlyProperty; + restorePointStringProperty: ReadOnlyProperty; dataTableStringProperty: ReadOnlyProperty; exportCSVStringProperty: ReadOnlyProperty; measuringTapeBaseStringProperty: ReadOnlyProperty; @@ -485,6 +491,8 @@ export class StringManager { digitizingAreaStringProperty: stringProperties.a11y.digitizingAreaStringProperty, digitizeTrackStringProperty: stringProperties.a11y.digitizeTrackStringProperty, removeTrackStringProperty: stringProperties.a11y.removeTrackStringProperty, + erasePointStringProperty: stringProperties.a11y.erasePointStringProperty, + restorePointStringProperty: stringProperties.a11y.restorePointStringProperty, dataTableStringProperty: stringProperties.a11y.dataTableStringProperty, exportCSVStringProperty: stringProperties.a11y.exportCSVStringProperty, measuringTapeBaseStringProperty: stringProperties.a11y.measuringTapeBaseStringProperty, diff --git a/src/i18n/strings_en.json b/src/i18n/strings_en.json index cbdf2b2..be3bcdc 100644 --- a/src/i18n/strings_en.json +++ b/src/i18n/strings_en.json @@ -42,11 +42,13 @@ "title": "Keyboard Shortcuts", "simulationControls": "Simulation Controls", "graphInteractions": "Graph Interactions", + "digitizing": "Digitizing", "playPauseSimulation": "Play/Pause Simulation", "resetSimulation": "Reset Simulation", "stepBackward": "Step Backward", "stepForward": "Step Forward", "rewindToStart": "Rewind to Start", + "erasePoint": "Erase Point at Current Frame", "resetZoom": "Reset Zoom", "zoomInOut": "Zoom In/Out", "panView": "Pan View" @@ -158,7 +160,7 @@ "addTrackTitle": "4. Add a Track", "addTrackBody": "In the Tracks panel, click “+ Add Track” to create a new track (A, B, C…). Up to 4 tracks can be active at a time.", "digitizeTitle": "5. Digitize Frame by Frame", - "digitizeBody": "Select a track and click the object’s position in the video. Enable the Magnifier for precision. The video advances one frame after each click.", + "digitizeBody": "Select a track and click the object’s position in the video. Enable the Magnifier for precision. The video advances one frame after each click. Click the same frame again to move a point, or press Delete to erase it.", "autoTrackTitle": "Auto-Tracking", "autoTrackBody": "Enable Auto-Tracking in the control panel, then drag a selection box around the object. TrackLab will track it automatically across the remaining frames.", "measuringTapeTitle": "Measuring Tape", @@ -176,9 +178,11 @@ "videoPlayer": "Video player", "videoScrubber": "Video timeline — drag to seek", "rewindToStart": "Rewind to start", - "digitizingArea": "Video digitizing area — click to record the position of the active track", + "digitizingArea": "Video digitizing area — click to record the position of the active track, or press Delete to erase the point at this frame", "digitizeTrack": "Digitize track {{symbol}}", "removeTrack": "Remove track {{symbol}}", + "erasePoint": "Erase the digitized point at the current frame", + "restorePoint": "Restore the last erased point", "dataTable": "Track data", "exportCSV": "Export track data as CSV file", "measuringTapeBase": "Measuring tape base endpoint", diff --git a/src/i18n/strings_es.json b/src/i18n/strings_es.json index 14f6eff..7f0402e 100644 --- a/src/i18n/strings_es.json +++ b/src/i18n/strings_es.json @@ -42,11 +42,13 @@ "title": "Atajos de teclado", "simulationControls": "Controles de simulación", "graphInteractions": "Interacciones con la gráfica", + "digitizing": "Digitalización", "playPauseSimulation": "Reproducir/Pausar la simulación", "resetSimulation": "Reiniciar la simulación", "stepBackward": "Retroceder un paso", "stepForward": "Avanzar un paso", "rewindToStart": "Rebobinar al inicio", + "erasePoint": "Borrar punto en el fotograma actual", "resetZoom": "Restablecer el zoom", "zoomInOut": "Acercar/Alejar", "panView": "Desplazar la vista" @@ -158,7 +160,7 @@ "addTrackTitle": "4. Añadir una pista", "addTrackBody": "En el panel de pistas, haz clic en «+ Añadir pista» para crear una nueva pista (A, B, C…). Pueden estar activas hasta 4 pistas a la vez.", "digitizeTitle": "5. Digitalizar fotograma a fotograma", - "digitizeBody": "Selecciona una pista y haz clic en la posición del objeto en el video. Activa la lupa para mayor precisión. El video avanza un fotograma después de cada clic.", + "digitizeBody": "Selecciona una pista y haz clic en la posición del objeto en el video. Activa la lupa para mayor precisión. El video avanza un fotograma después de cada clic. Vuelve a hacer clic en el mismo fotograma para mover un punto, o pulsa Supr para borrarlo.", "autoTrackTitle": "Seguimiento automático", "autoTrackBody": "Activa el seguimiento automático en el panel de control, luego arrastra un cuadro de selección alrededor del objeto. TrackLab lo seguirá automáticamente a lo largo de los fotogramas restantes.", "measuringTapeTitle": "Cinta métrica", @@ -176,9 +178,11 @@ "videoPlayer": "Reproductor de video", "videoScrubber": "Línea de tiempo del video — arrastra para buscar", "rewindToStart": "Rebobinar al inicio", - "digitizingArea": "Área de digitalización del video — haz clic para registrar la posición de la pista activa", + "digitizingArea": "Área de digitalización del video — haz clic para registrar la posición de la pista activa, o pulsa Supr para borrar el punto de este fotograma", "digitizeTrack": "Digitalizar la pista {{symbol}}", "removeTrack": "Eliminar la pista {{symbol}}", + "erasePoint": "Borrar el punto digitalizado en el fotograma actual", + "restorePoint": "Restaurar el último punto borrado", "dataTable": "Datos de la pista", "exportCSV": "Exportar los datos de la pista como archivo CSV", "measuringTapeBase": "Extremo base de la cinta métrica", diff --git a/src/i18n/strings_fr.json b/src/i18n/strings_fr.json index 4979272..c9be99e 100644 --- a/src/i18n/strings_fr.json +++ b/src/i18n/strings_fr.json @@ -42,11 +42,13 @@ "title": "Raccourcis clavier", "simulationControls": "Contrôles de simulation", "graphInteractions": "Interactions avec le graphique", + "digitizing": "Numérisation", "playPauseSimulation": "Lecture/Pause de la simulation", "resetSimulation": "Réinitialiser la simulation", "stepBackward": "Reculer d'un pas", "stepForward": "Avancer d'un pas", "rewindToStart": "Revenir au début", + "erasePoint": "Effacer le point à l'image actuelle", "resetZoom": "Réinitialiser le zoom", "zoomInOut": "Zoom avant/arrière", "panView": "Déplacer la vue" @@ -158,7 +160,7 @@ "addTrackTitle": "4. Ajouter une piste", "addTrackBody": "Dans le panneau Pistes, cliquez sur « + Ajouter une piste » pour créer une nouvelle piste étiquetée A, B, C, …", "digitizeTitle": "5. Numériser image par image", - "digitizeBody": "Sélectionnez une piste et cliquez sur la position de l’objet dans la vidéo. Activez la Loupe pour plus de précision. La vidéo avance automatiquement d’une image après chaque clic.", + "digitizeBody": "Sélectionnez une piste et cliquez sur la position de l’objet dans la vidéo. Activez la Loupe pour plus de précision. La vidéo avance automatiquement d’une image après chaque clic. Cliquez à nouveau sur la même image pour déplacer un point, ou appuyez sur Suppr pour l’effacer.", "autoTrackTitle": "Suivi automatique", "autoTrackBody": "Activez le Suivi automatique dans le panneau de contrôle, puis faites glisser un cadre de sélection autour de l’objet. TrackLab le suivra automatiquement sur les images restantes.", "measuringTapeTitle": "Ruban de mesure", @@ -176,9 +178,11 @@ "videoPlayer": "Lecteur vidéo", "videoScrubber": "Ligne de temps vidéo — glisser pour naviguer", "rewindToStart": "Revenir au début", - "digitizingArea": "Zone de numérisation vidéo — cliquer pour enregistrer la position de la piste active", + "digitizingArea": "Zone de numérisation vidéo — cliquer pour enregistrer la position de la piste active, ou appuyer sur Suppr pour effacer le point de cette image", "digitizeTrack": "Numériser la piste {{symbol}}", "removeTrack": "Supprimer la piste {{symbol}}", + "erasePoint": "Effacer le point numérisé à l'image actuelle", + "restorePoint": "Restaurer le dernier point effacé", "dataTable": "Données de piste", "exportCSV": "Exporter les données de piste en fichier CSV", "measuringTapeBase": "Extrémité de base du ruban de mesure", diff --git a/src/track-lab/model/TrackLabModel.ts b/src/track-lab/model/TrackLabModel.ts index 60a69b9..8b12383 100644 --- a/src/track-lab/model/TrackLabModel.ts +++ b/src/track-lab/model/TrackLabModel.ts @@ -61,12 +61,36 @@ export class TrackLabModel { * The view passes raw inputs (track id + pixel position); this method * coordinates all sub-model interactions so the view does not need to reach * into playback or overlayTools directly. + * + * Clicking a frame that already holds a point *replaces* it: re-clicking is + * how a user corrects a misclick, and silently ignoring the second click + * reads as the tool being broken. Auto-tracking keeps the first-wins policy + * so it never clobbers a hand-placed point. */ public recordTrackPoint(trackId: string, pixelPoint: Vector2): void { const time = this.playback.currentTimeProperty.value; - const frame = Math.round(time * this.playback.frameRateProperty.value); + // Same derivation as deleteTrackPointAtCurrentFrame(), so the frame a click + // writes is exactly the frame the erase button targets. + const frame = this.playback.currentFrameProperty.value; const modelPt = this.overlayTools.modelViewTransformProperty.value.inversePosition2(pixelPoint); - this.tracking.addPointToTrack(trackId, frame, time, modelPt.x, modelPt.y); + this.tracking.addOrReplacePointOnTrack(trackId, frame, time, modelPt.x, modelPt.y); + } + + /** + * Delete the active track's point at the current playback frame, if any. + * Returns true when a point was actually removed. + */ + public deleteTrackPointAtCurrentFrame(): boolean { + const trackId = this.tracking.activeTrackIdProperty.value; + if (trackId === null) { + return false; + } + const frame = this.playback.currentFrameProperty.value; + if (!this.tracking.hasPointAtFrame(trackId, frame)) { + return false; + } + this.tracking.removePointFromTrack(trackId, frame); + return true; } // ── Video source activation ───────────────────────────────────────────── diff --git a/src/track-lab/model/TrackingModel.ts b/src/track-lab/model/TrackingModel.ts index 63f8b06..e38eb54 100644 --- a/src/track-lab/model/TrackingModel.ts +++ b/src/track-lab/model/TrackingModel.ts @@ -26,6 +26,24 @@ import { OpenCVTracker, type TrackerRegion } from "../../tracking/OpenCVTracker. import { computeTrackKinematics } from "./KinematicsComputer.js"; import type { Track, TrackKinematics, TrackPoint } from "./Track.js"; +/** + * Insert `point` into `points`, keeping the array sorted by ascending frame. + * Always returns a new array so the kinematics cache (keyed on array identity) + * invalidates. + * + * Sorted insertion matters because KinematicsComputer differentiates by array + * index and assumes time increases along it. Appending is only safe while + * digitizing runs forwards; restoring a deleted interior point, or digitizing + * out of order after scrubbing backwards, both break that assumption. + */ +function insertPointSorted(points: readonly TrackPoint[], point: TrackPoint): TrackPoint[] { + const index = points.findIndex((p) => p.frame > point.frame); + if (index === -1) { + return [...points, point]; + } + return [...points.slice(0, index), point, ...points.slice(index)]; +} + /** * Owns all reactive state for particle tracks, kinematics caching, and the * OpenCV template-matching tracker facade. @@ -44,17 +62,30 @@ export class TrackingModel { (tracks) => tracks.length < MAX_TRACKS, ); + // ── Single-level undo for point deletion ────────────────────────────── + // Only the most recently deleted point is remembered. A deletion is a + // deliberate, low-stakes action (the video frame is still there to + // re-digitize), so one level of undo covers the "wrong button" mistake + // without introducing a general command history. + private readonly lastDeletedPointProperty = new Property<{ trackId: string; point: TrackPoint } | null>(null); + + public readonly canRestorePointProperty: TReadOnlyProperty = new DerivedProperty( + [this.lastDeletedPointProperty], + (deleted) => deleted !== null, + ); + // ── Derived kinematics for all tracks ─────────────────────────────────── // Cache keyed by track ID; only recomputes kinematics for tracks whose // point array reference has changed since the last derivation. // // CACHE INVARIANT: validity is determined by object identity // (`cached.points === track.points`). This is correct because every - // mutation path (addPointToTrack, retransformTrackPoints) replaces the - // entire Track object and its points array, so a stale entry always has a - // different reference. removeTrack() explicitly evicts the entry for the - // removed track to prevent an unbounded memory leak when tracks are added - // and removed repeatedly. + // mutation path (addPointToTrack, addOrReplacePointOnTrack, + // removePointFromTrack, retransformTrackPoints) replaces the entire Track + // object and its points array, so a stale entry always has a different + // reference. removeTrack() explicitly evicts the entry for the removed + // track to prevent an unbounded memory leak when tracks are added and + // removed repeatedly. private readonly kinematicsCache = new Map(); public readonly trackKinematicsProperty: TReadOnlyProperty = new DerivedProperty( @@ -119,6 +150,12 @@ export class TrackingModel { } this.tracksProperty.value = this.tracksProperty.value.filter((t) => t.id !== id); this.kinematicsCache.delete(id); + + // The undo stash points at a track that no longer exists; restoring it + // would be a silent no-op, so drop it and let the button disable. + if (this.lastDeletedPointProperty.value?.trackId === id) { + this.lastDeletedPointProperty.value = null; + } } /** @@ -138,12 +175,84 @@ export class TrackingModel { } const point: TrackPoint = { frame, time, x, y }; - const updated: Track = { ...track, points: [...track.points, point] }; + const updated: Track = { ...track, points: insertPointSorted(track.points, point) }; return updated; }); this.tracksProperty.value = tracks; } + /** + * Record a digitized position for `frame` on the track identified by `id`, + * overwriting any position already recorded for that frame. + * + * This is the manual-digitizing entry point. Re-clicking a frame is the + * natural way a user corrects a misclick, so the *last* recorded position + * wins here — the opposite of addPointToTrack()'s first-wins policy, which + * auto-tracking relies on to avoid clobbering hand-placed points. + */ + public addOrReplacePointOnTrack(id: string, frame: number, time: number, x: number, y: number): void { + const point: TrackPoint = { frame, time, x, y }; + + this.tracksProperty.value = this.tracksProperty.value.map((track) => { + if (track.id !== id) { + return track; + } + + const existing = track.points.some((p) => p.frame === frame); + const points = existing + ? track.points.map((p) => (p.frame === frame ? point : p)) + : insertPointSorted(track.points, point); + + return { ...track, points }; + }); + } + + /** + * Delete the point recorded for `frame` on the track identified by `id`. + * Does nothing if the track or the point does not exist. + * + * The removed point is stashed so that restoreLastDeletedPoint() can undo + * this call. Deleting an interior point leaves a gap in the frame series; + * that is safe — KinematicsComputer differentiates against each point's + * recorded timestamp, not against an assumed constant frame interval. + */ + public removePointFromTrack(id: string, frame: number): void { + const track = this.tracksProperty.value.find((t) => t.id === id); + const removed = track?.points.find((p) => p.frame === frame); + if (!(track && removed)) { + return; + } + + this.tracksProperty.value = this.tracksProperty.value.map((t) => + // filter() allocates a new array, which invalidates the kinematics cache. + t.id === id ? { ...t, points: t.points.filter((p) => p.frame !== frame) } : t, + ); + this.lastDeletedPointProperty.value = { trackId: id, point: removed }; + } + + /** + * Re-insert the most recently deleted point. Does nothing if no point has + * been deleted, or if its track has since been removed. + */ + public restoreLastDeletedPoint(): void { + const deleted = this.lastDeletedPointProperty.value; + if (!deleted) { + return; + } + const { trackId, point } = deleted; + this.lastDeletedPointProperty.value = null; + this.addOrReplacePointOnTrack(trackId, point.frame, point.time, point.x, point.y); + } + + /** True when the given track has a point recorded at `frame`. */ + public hasPointAtFrame(id: string | null, frame: number): boolean { + if (id === null) { + return false; + } + const track = this.tracksProperty.value.find((t) => t.id === id); + return track !== undefined && track.points.some((p) => p.frame === frame); + } + /** * Create a new track and immediately make it the active track. * Does nothing if the track limit or symbol limit has been reached. @@ -239,6 +348,7 @@ export class TrackingModel { this.kinematicsCache.clear(); this.tracksProperty.value = []; this.activeTrackIdProperty.value = null; + this.lastDeletedPointProperty.value = null; this.nextSymbolCode = TRACK_SYMBOL_FIRST_CODE; this.initVersion++; this.tracker.dispose(); diff --git a/src/track-lab/view/DataTableNode.ts b/src/track-lab/view/DataTableNode.ts index 008dcb3..7daf102 100644 --- a/src/track-lab/view/DataTableNode.ts +++ b/src/track-lab/view/DataTableNode.ts @@ -40,6 +40,7 @@ import { import TrackLabNamespace from "../../TrackLabNamespace.js"; import { generateCsv } from "../model/TrackExporter.js"; import type { TrackingModel } from "../model/TrackingModel.js"; +import type { VideoPlaybackModel } from "../model/VideoPlaybackModel.js"; import { type A11yLabels, type TableColors, type TableLabels, TableRenderer } from "./TableRenderer.js"; // ── Fonts ──────────────────────────────────────────────────────────────────── @@ -69,6 +70,7 @@ function getTableColors(): TableColors { emptyText: TrackLabColors.tableEmptyTextProperty.value.toCSS(), symbolShadow: TrackLabColors.tableSymbolShadowProperty.value.toCSS(), background: TrackLabColors.tableBackgroundProperty.value.toCSS(), + currentRow: TrackLabColors.tableCurrentRowProperty.value.toCSS(), }; } @@ -86,6 +88,7 @@ export class DataTableNode extends Node { public constructor( tracking: TrackingModel, + playback: VideoPlaybackModel, videoLoadedProperty: TReadOnlyProperty, unitProperty: TReadOnlyProperty, ) { @@ -103,7 +106,12 @@ export class DataTableNode extends Node { }); // ── TableRenderer owns all DOM and incremental-update state ─────────────── - const tableRenderer = new TableRenderer(getTableColors(), getLabels(), getA11yLabels()); + // Clicking a data row seeks the video to that frame, which makes the table + // a navigation surface: find a bad point here, then erase it from the + // playback controls without hunting for a 2px dot on the video. + const tableRenderer = new TableRenderer(getTableColors(), getLabels(), getA11yLabels(), (frame: number) => { + playback.currentTimeProperty.value = frame / playback.frameRateProperty.value; + }); const tableDomNode = new DOM(tableRenderer.wrapper, { allowInput: true }); @@ -204,6 +212,9 @@ export class DataTableNode extends Node { const unitListener = () => runUpdate(); unitProperty.link(unitListener); + const currentFrameListener = (frame: number) => tableRenderer.setCurrentFrame(frame); + playback.currentFrameProperty.link(currentFrameListener); + // Color-theme and locale changes require a full rebuild because cell colours // and label strings are baked into the DOM. const tableHeaderBgListener = () => runRebuild(); @@ -408,6 +419,7 @@ export class DataTableNode extends Node { resizeObserver.disconnect(); tracking.tracksProperty.unlink(tracksListener); unitProperty.unlink(unitListener); + playback.currentFrameProperty.unlink(currentFrameListener); TrackLabColors.tableHeaderBackgroundProperty.unlink(tableHeaderBgListener); dataTableStrings.frameStringProperty.unlink(frameStringListener); videoLoadedProperty.unlink(videoLoadedListener); diff --git a/src/track-lab/view/DigitizingOverlayNode.ts b/src/track-lab/view/DigitizingOverlayNode.ts index 9310f3d..9313acb 100644 --- a/src/track-lab/view/DigitizingOverlayNode.ts +++ b/src/track-lab/view/DigitizingOverlayNode.ts @@ -8,7 +8,7 @@ import type { TReadOnlyProperty } from "scenerystack/axon"; import { type Dimension2, type Transform3, Vector2 } from "scenerystack/dot"; import { Shape } from "scenerystack/kite"; -import { DOM, FireListener, Node, Path, Rectangle } from "scenerystack/scenery"; +import { DOM, FireListener, KeyboardListener, Node, Path, Rectangle } from "scenerystack/scenery"; import { Tandem } from "scenerystack/tandem"; import { StringManager } from "../../i18n/StringManager.js"; import TrackLabColors, { getTrackColor } from "../../TrackLabColors.js"; @@ -25,6 +25,8 @@ type DigitizingOverlayNodeOptions = { modelViewTransformProperty: TReadOnlyProperty; /** Record a digitized point on the given track at the given pixel position. */ recordPoint: (trackId: string, pixelPoint: Vector2) => void; + /** Delete the active track's point at the current frame, if there is one. */ + erasePoint: () => void; }; const OUTER_R = 20; @@ -46,6 +48,12 @@ const MAG_CROSSHAIR_LINE_WIDTH = 1; const MARK_DOT_RADIUS = 2; // radius of each digitized-point dot drawn on the video +// Ring drawn around the active track's point at the current frame — the point +// the erase button and the Delete key act on. Showing the target before the +// user commits is what makes frame-based deletion legible. +const CURRENT_MARK_RING_RADIUS = 6; +const CURRENT_MARK_RING_LINE_WIDTH = 1.5; + // Magnifier canvas box-shadow: "offsetX offsetY blur color" const MAG_SHADOW_OFFSET_X = 0; // px, horizontal shadow offset const MAG_SHADOW_OFFSET_Y = 2; // px, vertical shadow offset @@ -246,6 +254,7 @@ export class DigitizingOverlayNode extends Node { cursor: "none", visible: false, tagName: "div", + focusable: true, accessibleName: a11yStrings.digitizingAreaStringProperty, }); digitizingOverlay.addChild(cursorNode); @@ -308,6 +317,15 @@ export class DigitizingOverlayNode extends Node { const marksLayer = new Node({ pickable: false }); const trackPaths = new Map(); // track id → Path + // A single reused Path holding at most one ring — never one node per point, + // which is what the per-track batching above exists to avoid. + const currentMarkRing = new Path(null, { + stroke: TrackLabColors.digitizingCursorStrokeProperty, + lineWidth: CURRENT_MARK_RING_LINE_WIDTH, + fill: null, + pickable: false, + }); + const rebuildMarks = () => { const currentFrame = playback.currentFrameProperty.value; const mvt = modelViewTransformProperty.value; @@ -340,6 +358,17 @@ export class DigitizingOverlayNode extends Node { trackPaths.delete(id); } } + + // Ring the erase target: the active track's point at the current frame. + const activeId = tracking.activeTrackIdProperty.value; + const activeTrack = activeId === null ? undefined : tracks.find((t) => t.id === activeId); + const currentPoint = activeTrack?.points.find((p) => p.frame === currentFrame); + if (currentPoint) { + const localPt = mvt.transformPosition2(new Vector2(currentPoint.x, currentPoint.y)); + currentMarkRing.shape = new Shape().circle(localPt.x, localPt.y, CURRENT_MARK_RING_RADIUS); + } else { + currentMarkRing.shape = null; + } }; const currentTimeListener = () => { @@ -362,6 +391,9 @@ export class DigitizingOverlayNode extends Node { if (!activeId) { cursorNode.visible = false; } + // The erase-target ring follows the active track, so it must be redrawn + // when the active track changes, not only when points change. + rebuildMarks(); }; tracking.activeTrackIdProperty.link(activeTrackListener); @@ -398,6 +430,17 @@ export class DigitizingOverlayNode extends Node { }), ); + // Delete/Backspace erases the point under the ring. The overlay is + // focusable so keyboard users reach this without a pointer; the erase + // button in the playback controls does the same thing. + digitizingOverlay.addInputListener( + new KeyboardListener({ + keys: ["delete", "backspace"], + fire: () => options.erasePoint(), + }), + ); + + marksLayer.addChild(currentMarkRing); this.addChild(digitizingOverlay); this.addChild(marksLayer); @@ -418,6 +461,7 @@ export class DigitizingOverlayNode extends Node { path.dispose(); } trackPaths.clear(); + currentMarkRing.dispose(); }; } diff --git a/src/track-lab/view/PlaybackControlsNode.ts b/src/track-lab/view/PlaybackControlsNode.ts index 34ac422..3010097 100644 --- a/src/track-lab/view/PlaybackControlsNode.ts +++ b/src/track-lab/view/PlaybackControlsNode.ts @@ -18,8 +18,10 @@ import TrackLabColors from "../../TrackLabColors.js"; const a11yStrings = StringManager.getInstance().getA11y(); +import { makeEraserIcon, makeRestoreIcon } from "../../TrackLabIcons.js"; import TrackLabNamespace from "../../TrackLabNamespace.js"; import GraphDataManager from "../graph/GraphDataManager.js"; +import type { TrackingModel } from "../model/TrackingModel.js"; import type { VideoPlaybackModel } from "../model/VideoPlaybackModel.js"; const LABEL_FONT = new PhetFont(14); @@ -46,9 +48,11 @@ export class PlaybackControlsNode extends HBox { public constructor( playback: VideoPlaybackModel, + tracking: TrackingModel, videoElement: HTMLVideoElement, onStepBackward: () => void, onStepForward: () => void, + onErasePoint: () => void, ) { super({ spacing: CONTROLS_SPACING, align: "center" }); @@ -288,9 +292,45 @@ export class PlaybackControlsNode extends HBox { }, ); - this.children = [timeControlNode, this.scrubber, rewindButton, infoDisplay]; + // ── Erase / restore point buttons ────────────────────────────────────── + // These act on the *current frame* of the *active track*, which together + // identify exactly one digitized point — no hit-testing and no selection + // state. They live beside the step buttons because that is where + // "act on this frame" already lives, and deliberately not in + // TrackListPanel, where they would sit one target away from the trash + // button that deletes an entire track. + const canErasePointProperty = new DerivedProperty( + [tracking.tracksProperty, tracking.activeTrackIdProperty, playback.currentFrameProperty], + (_tracks, activeId, frame) => tracking.hasPointAtFrame(activeId, frame), + ); + + // Dim background and icon together, matching the rewind button. + const dimWhenDisabled = (enabled: boolean, button: import("scenerystack/scenery").Node) => { + button.opacity = enabled ? 1 : 0.45; + }; + + const erasePointButton = createTrackLabButton(makeEraserIcon(), { + baseColor: TrackLabColors.playbackButtonBaseProperty, + enabledAppearanceStrategy: dimWhenDisabled, + enabledProperty: canErasePointProperty, + accessibleName: a11yStrings.erasePointStringProperty, + listener: onErasePoint, + }); + + const restorePointButton = createTrackLabButton(makeRestoreIcon(), { + baseColor: TrackLabColors.playbackButtonBaseProperty, + enabledAppearanceStrategy: dimWhenDisabled, + enabledProperty: tracking.canRestorePointProperty, + accessibleName: a11yStrings.restorePointStringProperty, + listener: () => tracking.restoreLastDeletedPoint(), + }); + + this.children = [timeControlNode, this.scrubber, rewindButton, erasePointButton, restorePointButton, infoDisplay]; this.disposePlaybackControlsNode = () => { + erasePointButton.dispose(); + restorePointButton.dispose(); + canErasePointProperty.dispose(); timeSpeedProperty.unlink(onSpeedChange); playback.playbackRateProperty.unlink(onRateChange); playback.currentTimeProperty.unlink(onTimeChange); diff --git a/src/track-lab/view/TableRenderer.ts b/src/track-lab/view/TableRenderer.ts index 55ff769..203dbd8 100644 --- a/src/track-lab/view/TableRenderer.ts +++ b/src/track-lab/view/TableRenderer.ts @@ -57,8 +57,12 @@ export type TableColors = { emptyText: string; symbolShadow: string; background: string; + currentRow: string; }; +/** Seek the video to `frame`. Invoked when the user clicks a data row. */ +export type FrameSelectedCallback = (frame: number) => void; + export type TableLabels = { frame: string; timeSeconds: string; @@ -83,6 +87,7 @@ function buildHtmlTable( a11y: A11yLabels, maxWidth: number, maxHeight: number, + onFrameSelected: FrameSelectedCallback, ): HTMLDivElement { const dataRows = buildDataRows(tracks); @@ -196,7 +201,7 @@ function buildHtmlTable( if (!row) { continue; } - tbody.appendChild(buildDataRow(row, tracks, makeCellStyle(colors), i % 2 !== 0, colors)); + tbody.appendChild(buildDataRow(row, tracks, makeCellStyle(colors), i % 2 !== 0, colors, onFrameSelected)); } } table.appendChild(tbody); @@ -210,10 +215,22 @@ function buildDataRow( cellStyle: string, isEven: boolean, colors: TableColors, + onFrameSelected: FrameSelectedCallback, ): HTMLTableRowElement { const tr = document.createElement("tr"); tr.style.background = isEven ? colors.rowEven : colors.rowOdd; + // Clicking a row seeks the video to that frame. This is what lets the table + // act as a navigation surface: spot a bad point here, click it, and the + // erase button in the playback controls lights up on the right frame. + tr.style.cursor = "pointer"; + tr.addEventListener("click", (event) => { + // The whole panel is wrapped in a pan DragListener; without this the click + // can be swallowed as the tail of a drag. + event.stopPropagation(); + onFrameSelected(row.frame); + }); + const addCell = (text: string) => { const td = document.createElement("td"); td.textContent = text; @@ -253,9 +270,23 @@ export class TableRenderer { private currentMaxWidth: number; private currentMaxHeight: number; - public constructor(initialColors: TableColors, initialLabels: TableLabels, initialA11y: A11yLabels) { + // ── Current-frame highlight state ────────────────────────────────────────── + // The frame the video is parked on. Re-applied after every rebuild, since a + // rebuild throws away the highlighted . + private currentFrame: number | null = null; + private currentRowColor: string; + private readonly onFrameSelected: FrameSelectedCallback; + + public constructor( + initialColors: TableColors, + initialLabels: TableLabels, + initialA11y: A11yLabels, + onFrameSelected: FrameSelectedCallback, + ) { this.currentMaxWidth = MAX_TABLE_WIDTH; this.currentMaxHeight = MAX_TABLE_HEIGHT; + this.currentRowColor = initialColors.currentRow; + this.onFrameSelected = onFrameSelected; this.wrapper = buildHtmlTable( [], "m", @@ -264,9 +295,40 @@ export class TableRenderer { initialA11y, this.currentMaxWidth, this.currentMaxHeight, + onFrameSelected, ); } + /** + * Mark the row for `frame` as the current one and scroll it into view. + * Cheap — moves styling between two existing rows, never rebuilds. + */ + public setCurrentFrame(frame: number | null): void { + this.currentFrame = frame; + this.applyCurrentFrameHighlight(true); + } + + /** + * Paint the current-frame highlight onto the live DOM. + * + * @param scrollIntoView - true for user-driven frame changes; false after a + * rebuild, where the frame did not actually move and yanking the scroll + * position would fight the user's own scrolling. + */ + private applyCurrentFrameHighlight(scrollIntoView: boolean): void { + for (const [frame, tr] of this.frameRowMap) { + const isCurrent = frame === this.currentFrame; + tr.style.outline = isCurrent ? `2px solid ${this.currentRowColor}` : ""; + // Draw the outline inside the cell box so it is not clipped by the + // scroll container at the first and last rows. + tr.style.outlineOffset = isCurrent ? "-2px" : ""; + tr.setAttribute("aria-current", isCurrent ? "true" : "false"); + if (isCurrent && scrollIntoView) { + tr.scrollIntoView({ block: "nearest" }); + } + } + } + /** Apply user-set max-width / max-height to the wrapper CSS. */ public applyDimensions(maxWidth: number, maxHeight: number): void { this.currentMaxWidth = maxWidth; @@ -297,7 +359,17 @@ export class TableRenderer { labels: TableLabels, a11y: A11yLabels, ): void { - const newWrapper = buildHtmlTable(tracks, unit, colors, labels, a11y, this.currentMaxWidth, this.currentMaxHeight); + this.currentRowColor = colors.currentRow; + const newWrapper = buildHtmlTable( + tracks, + unit, + colors, + labels, + a11y, + this.currentMaxWidth, + this.currentMaxHeight, + this.onFrameSelected, + ); this.wrapper.innerHTML = ""; if (newWrapper.firstChild) { @@ -330,6 +402,10 @@ export class TableRenderer { this.lastTrackIds = tracks.map((t) => t.id); this.lastUnit = unit; + + // The rebuild discarded the highlighted ; restore it on the new DOM. + // No scroll — the frame has not moved, only the rows around it. + this.applyCurrentFrameHighlight(false); } /** @@ -363,6 +439,24 @@ export class TableRenderer { // ── Incremental path: same track structure, only new points ────────────── const dataRows = buildDataRows(tracks); + // The incremental path can append rows but never remove them, so a deleted + // point would leave its behind showing stale coordinates. If any + // rendered frame is gone from the data, fall back to a full rebuild — that + // also re-stripes the positional row backgrounds and resets frameRowMap / + // maxRenderedFrame. Same bail-out idiom as the hasOutOfOrder guard below. + const currentFrames = new Set(dataRows.map((row) => row.frame)); + let hasRemovedRow = false; + for (const frame of this.frameRowMap.keys()) { + if (!currentFrames.has(frame)) { + hasRemovedRow = true; + break; + } + } + if (hasRemovedRow) { + this.rebuild(tracks, unit, colors, labels, a11y); + return; + } + // If any new row precedes the last rendered frame the sort order breaks; // fall back to full rebuild (rare: out-of-order manual digitizing). const hasOutOfOrder = dataRows.some((row) => !this.frameRowMap.has(row.frame) && row.frame < this.maxRenderedFrame); @@ -378,6 +472,7 @@ export class TableRenderer { this.tableBodyRef.innerHTML = ""; } + let appendedRow = false; for (const row of dataRows) { const tr = this.frameRowMap.get(row.frame); if (tr !== undefined) { @@ -399,13 +494,21 @@ export class TableRenderer { } else { // Append a brand-new row. const rowIndex = this.frameRowMap.size; - const newRow = buildDataRow(row, tracks, cellStyle, rowIndex % 2 !== 0, colors); + const newRow = buildDataRow(row, tracks, cellStyle, rowIndex % 2 !== 0, colors, this.onFrameSelected); this.tableBodyRef.appendChild(newRow); this.frameRowMap.set(row.frame, newRow); + appendedRow = true; if (row.frame > this.maxRenderedFrame) { this.maxRenderedFrame = row.frame; } } } + + // A row appended for the current frame starts out unhighlighted — this is + // the common case while digitizing, where each click adds the row the + // video is parked on. + if (appendedRow) { + this.applyCurrentFrameHighlight(true); + } } } diff --git a/src/track-lab/view/TrackLabKeyboardHelpContent.ts b/src/track-lab/view/TrackLabKeyboardHelpContent.ts index f2c0af5..2448888 100644 --- a/src/track-lab/view/TrackLabKeyboardHelpContent.ts +++ b/src/track-lab/view/TrackLabKeyboardHelpContent.ts @@ -15,20 +15,29 @@ * (coordinate system, calibration, tape, angle) * • BasicActionsKeyboardHelpSection — Tab, press buttons, toggle checkboxes, * Reset All, Escape + * • Digitizing — Delete/Backspace on the digitizing overlay + * (hand-written: the binding is a plain + * KeyboardListener, not HotkeyData-backed) */ import { ArrowKeyIconDisplay, BasicActionsKeyboardHelpSection, + KeyboardHelpSection, + KeyboardHelpSectionRow, MoveDraggableItemsKeyboardHelpSection, SliderControlsKeyboardHelpSection, + TextKeyNode, TimeControlsKeyboardHelpSection, TwoColumnKeyboardHelpContent, } from "scenerystack/scenery-phet"; +import { StringManager } from "../../i18n/StringManager.js"; import TrackLabNamespace from "../../TrackLabNamespace.js"; export class TrackLabKeyboardHelpContent extends TwoColumnKeyboardHelpContent { public constructor() { + const shortcutStrings = StringManager.getInstance().getKeyboardShortcutsStrings(); + super( [ new TimeControlsKeyboardHelpSection(), @@ -36,6 +45,14 @@ export class TrackLabKeyboardHelpContent extends TwoColumnKeyboardHelpContent { new SliderControlsKeyboardHelpSection({ arrowKeyIconDisplay: ArrowKeyIconDisplay.LEFT_RIGHT, }), + // Erases the active track's point at the current frame, matching the + // eraser button in the playback controls. + new KeyboardHelpSection(shortcutStrings.digitizingStringProperty, [ + KeyboardHelpSectionRow.labelWithIconList(shortcutStrings.erasePointStringProperty, [ + TextKeyNode.delete(), + TextKeyNode.backspace(), + ]), + ]), ], [ // Coordinate system, calibration tool, measuring tape, angle tool are all diff --git a/src/track-lab/view/TrackLabScreenView.ts b/src/track-lab/view/TrackLabScreenView.ts index 7369c06..08d8fb4 100644 --- a/src/track-lab/view/TrackLabScreenView.ts +++ b/src/track-lab/view/TrackLabScreenView.ts @@ -169,6 +169,7 @@ export class TrackLabScreenView extends ScreenView { // ── Data table (top right, shifts left when window is wider than layoutBounds) ─ const dataTableNode = new DataTableNode( model.tracking, + model.playback, model.playback.videoLoadedProperty, model.overlayTools.calibUnitProperty, ); diff --git a/src/track-lab/view/VideoPlayerNode.ts b/src/track-lab/view/VideoPlayerNode.ts index c6cedac..8c2fc29 100644 --- a/src/track-lab/view/VideoPlayerNode.ts +++ b/src/track-lab/view/VideoPlayerNode.ts @@ -173,6 +173,7 @@ export class VideoPlayerNode extends Node { magnifyVideoProperty: model.overlayTools.magnifyVideoProperty, modelViewTransformProperty: model.overlayTools.modelViewTransformProperty, recordPoint: (trackId, pixelPoint) => model.recordTrackPoint(trackId, pixelPoint), + erasePoint: () => model.deleteTrackPointAtCurrentFrame(), }, () => this.stepForward(), ); @@ -223,9 +224,11 @@ export class VideoPlayerNode extends Node { // ── Playback controls (positioned by TrackLabScreenView at screen bottom) ── this.playbackControlsNode = new PlaybackControlsNode( model.playback, + model.tracking, this.videoElement, () => this.seekByFrames(-1), () => this.seekByFrames(1), + () => model.deleteTrackPointAtCurrentFrame(), ); // Pin to the video width so internal text changes never shift the row. this.playbackControlsNode.preferredWidth = VIDEO_WIDTH; diff --git a/tests/setup.ts b/tests/setup.ts index e6bee0b..59992a2 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -141,6 +141,22 @@ class MockAudioContext { (globalThis as Record)["AudioContext"] = MockAudioContext; (globalThis as Record)["webkitAudioContext"] = MockAudioContext; +// ── Web Worker mock ────────────────────────────────────────────────────────── +// TrackingModel constructs an OpenCVTracker (and therefore a Worker) as a field +// initializer, so model tests need Worker to exist. happy-dom does not provide +// it. Messages are swallowed: no test drives the OpenCV worker protocol, and a +// real worker cannot run under happy-dom anyway. +class MockWorker { + onmessage: ((event: MessageEvent) => void) | null = null; + onerror: ((event: ErrorEvent) => void) | null = null; + postMessage: () => void = noop; + terminate: () => void = noop; + addEventListener: () => void = noop; + removeEventListener: () => void = noop; + dispatchEvent: () => boolean = () => false; +} +(globalThis as Record)["Worker"] = MockWorker; + // ── patch getContext("2d") before any scenerystack import ──────────────────── const origGetContext: typeof HTMLCanvasElement.prototype.getContext = HTMLCanvasElement.prototype.getContext; HTMLCanvasElement.prototype.getContext = function (this: HTMLCanvasElement, contextId: string, ...args: unknown[]) { diff --git a/tests/track-lab/model/TrackingModel.test.ts b/tests/track-lab/model/TrackingModel.test.ts new file mode 100644 index 0000000..124cba5 --- /dev/null +++ b/tests/track-lab/model/TrackingModel.test.ts @@ -0,0 +1,205 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { computeTrackKinematics } from "../../../src/track-lab/model/KinematicsComputer.js"; +import type { TrackPoint } from "../../../src/track-lab/model/Track.js"; +import { TrackingModel } from "../../../src/track-lab/model/TrackingModel.js"; + +/** Frames of the first (and usually only) track, in stored order. */ +function frames(model: TrackingModel, trackId = "track-A"): number[] { + const track = model.tracksProperty.value.find((t) => t.id === trackId); + return (track?.points ?? []).map((p) => p.frame); +} + +function pointAt(model: TrackingModel, frame: number, trackId = "track-A"): TrackPoint | undefined { + const track = model.tracksProperty.value.find((t) => t.id === trackId); + return track?.points.find((p) => p.frame === frame); +} + +/** Digitize frames 0..count-1, one model unit apart along x, at 1 s intervals. */ +function digitize(model: TrackingModel, count: number, trackId = "track-A"): void { + for (let frame = 0; frame < count; frame++) { + model.addOrReplacePointOnTrack(trackId, frame, frame, frame, 0); + } +} + +describe("TrackingModel point deletion", () => { + let model: TrackingModel; + + beforeEach(() => { + model = new TrackingModel(); + model.addTrack(); + }); + + it("removes the point at the given frame and leaves the rest intact", () => { + digitize(model, 4); + + model.removePointFromTrack("track-A", 2); + + expect(frames(model)).toEqual([0, 1, 3]); + }); + + it("removes the first and the last point", () => { + digitize(model, 3); + + model.removePointFromTrack("track-A", 0); + expect(frames(model)).toEqual([1, 2]); + + model.removePointFromTrack("track-A", 2); + expect(frames(model)).toEqual([1]); + }); + + it("removes the only point, leaving an empty track", () => { + digitize(model, 1); + + model.removePointFromTrack("track-A", 0); + + expect(frames(model)).toEqual([]); + expect(model.tracksProperty.value).toHaveLength(1); + }); + + it("is a no-op for a frame with no point, and for an unknown track", () => { + digitize(model, 2); + const before = model.tracksProperty.value; + + model.removePointFromTrack("track-A", 99); + model.removePointFromTrack("track-Z", 0); + + expect(model.tracksProperty.value).toBe(before); + expect(model.canRestorePointProperty.value).toBe(false); + }); + + it("replaces the points array so the kinematics cache invalidates", () => { + digitize(model, 3); + const before = model.trackKinematicsProperty.value[0]; + + model.removePointFromTrack("track-A", 1); + const after = model.trackKinematicsProperty.value[0]; + + expect(after).not.toBe(before); + expect(after?.points.map((p) => p.frame)).toEqual([0, 2]); + }); + + it("recomputes velocity across the gap left by a deleted point", () => { + digitize(model, 3); + + model.removePointFromTrack("track-A", 1); + + // Two points 2 s apart, 2 units apart → 1 unit/s, computed from the + // recorded timestamps rather than an assumed constant frame interval. + const track = model.tracksProperty.value[0]; + expect(track).toBeDefined(); + if (!track) { + return; + } + const kinematics = computeTrackKinematics(track); + expect(kinematics.points[0]?.vx).toBeCloseTo(1, 6); + expect(kinematics.points[1]?.vx).toBeCloseTo(1, 6); + }); +}); + +describe("TrackingModel restore last deleted point", () => { + let model: TrackingModel; + + beforeEach(() => { + model = new TrackingModel(); + model.addTrack(); + }); + + it("round-trips a deleted interior point back into frame order", () => { + digitize(model, 4); + + model.removePointFromTrack("track-A", 1); + expect(model.canRestorePointProperty.value).toBe(true); + + model.restoreLastDeletedPoint(); + + // Restored in sorted position, not appended — KinematicsComputer + // differentiates by array index and assumes time increases along it. + expect(frames(model)).toEqual([0, 1, 2, 3]); + expect(model.canRestorePointProperty.value).toBe(false); + }); + + it("restores the point's original coordinates", () => { + model.addOrReplacePointOnTrack("track-A", 5, 0.5, 12.5, -3.25); + + model.removePointFromTrack("track-A", 5); + model.restoreLastDeletedPoint(); + + expect(pointAt(model, 5)).toEqual({ frame: 5, time: 0.5, x: 12.5, y: -3.25 }); + }); + + it("only remembers the most recent deletion", () => { + digitize(model, 3); + + model.removePointFromTrack("track-A", 0); + model.removePointFromTrack("track-A", 1); + model.restoreLastDeletedPoint(); + + expect(frames(model)).toEqual([1, 2]); + expect(model.canRestorePointProperty.value).toBe(false); + }); + + it("drops the stash when the point's track is removed", () => { + digitize(model, 2); + + model.removePointFromTrack("track-A", 0); + expect(model.canRestorePointProperty.value).toBe(true); + + model.removeTrack("track-A"); + + expect(model.canRestorePointProperty.value).toBe(false); + model.restoreLastDeletedPoint(); + expect(model.tracksProperty.value).toHaveLength(0); + }); + + it("clears the stash on reset", () => { + digitize(model, 1); + model.removePointFromTrack("track-A", 0); + + model.reset(); + + expect(model.canRestorePointProperty.value).toBe(false); + }); +}); + +describe("TrackingModel point insertion policies", () => { + let model: TrackingModel; + + beforeEach(() => { + model = new TrackingModel(); + model.addTrack(); + }); + + it("keeps the first position when auto-tracking re-reports a frame", () => { + model.addPointToTrack("track-A", 0, 0, 1, 1); + model.addPointToTrack("track-A", 0, 0, 9, 9); + + expect(pointAt(model, 0)).toMatchObject({ x: 1, y: 1 }); + }); + + it("overwrites the position when the user re-digitizes a frame", () => { + model.addOrReplacePointOnTrack("track-A", 0, 0, 1, 1); + model.addOrReplacePointOnTrack("track-A", 0, 0, 9, 9); + + expect(pointAt(model, 0)).toMatchObject({ x: 9, y: 9 }); + expect(frames(model)).toEqual([0]); + }); + + it("stores points in frame order when digitized out of order", () => { + model.addOrReplacePointOnTrack("track-A", 4, 4, 0, 0); + model.addOrReplacePointOnTrack("track-A", 1, 1, 0, 0); + model.addPointToTrack("track-A", 2, 2, 0, 0); + + expect(frames(model)).toEqual([1, 2, 4]); + }); + + it("leaves other tracks untouched", () => { + model.addTrack(); + digitize(model, 2, "track-A"); + digitize(model, 2, "track-B"); + + model.removePointFromTrack("track-A", 0); + + expect(frames(model, "track-A")).toEqual([1]); + expect(frames(model, "track-B")).toEqual([0, 1]); + }); +}); From 701bd68c4d929eb323e9c9d96f0b9468f3de4417 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:11:00 +0000 Subject: [PATCH 2/4] Use optional chaining in hasPointAtFrame. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gaCkV6CD5uR1SJpq3BPss --- src/track-lab/model/TrackingModel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track-lab/model/TrackingModel.ts b/src/track-lab/model/TrackingModel.ts index e38eb54..3c25b09 100644 --- a/src/track-lab/model/TrackingModel.ts +++ b/src/track-lab/model/TrackingModel.ts @@ -250,7 +250,7 @@ export class TrackingModel { return false; } const track = this.tracksProperty.value.find((t) => t.id === id); - return track !== undefined && track.points.some((p) => p.frame === frame); + return track?.points.some((p) => p.frame === frame) ?? false; } /** From 554108dd98b7d9cee8f83c94e7ea5b8ba5f2459a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:17:05 +0000 Subject: [PATCH 3/4] Clear remaining Biome warnings in the test suite biome check now reports zero warnings (was 9). - fuzz.spec.ts: annotate the three env-derived constants (useExplicitType). - memory-leak.test.ts: name the two sample plottables so the Property constructions reference them directly instead of indexing SAMPLE_PLOTTABLES, which under noUncheckedIndexedAccess widened to `PlottableProperty | undefined` and needed non-null assertions. The named consts narrow to RecordPlottable from their initializers and Property is invariant in T, so the type argument is given explicitly. Also switched the accessors to bracket access: `point` is a Record and the project sets noPropertyAccessFromIndexSignature. Neither file is in a tsconfig include path, so these type errors were invisible to `npm run check`; both now typecheck under the project's own compiler options. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gaCkV6CD5uR1SJpq3BPss --- tests/fuzz/fuzz.spec.ts | 6 +++--- tests/memory-leak.test.ts | 29 +++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/tests/fuzz/fuzz.spec.ts b/tests/fuzz/fuzz.spec.ts index 153f0bb..fefbd1d 100644 --- a/tests/fuzz/fuzz.spec.ts +++ b/tests/fuzz/fuzz.spec.ts @@ -10,9 +10,9 @@ import { expect, test } from "@playwright/test"; const FUZZ_DURATION = parseInt(process.env["FUZZ_DURATION"] || "15", 10) * 1000; -const FUZZ_SEED = process.env["FUZZ_SEED"] || Math.floor(Math.random() * 1_000_000).toString(); -const FUZZ_RATE = process.env["FUZZ_RATE"] || "100"; -const FUZZ_POINTERS = process.env["FUZZ_POINTERS"] || "1"; +const FUZZ_SEED: string = process.env["FUZZ_SEED"] || Math.floor(Math.random() * 1_000_000).toString(); +const FUZZ_RATE: string = process.env["FUZZ_RATE"] || "100"; +const FUZZ_POINTERS: string = process.env["FUZZ_POINTERS"] || "1"; interface ConsoleMessage { type: string; diff --git a/tests/memory-leak.test.ts b/tests/memory-leak.test.ts index f7f3f3f..3cb5037 100644 --- a/tests/memory-leak.test.ts +++ b/tests/memory-leak.test.ts @@ -30,14 +30,27 @@ async function forceGC(earlyExitRef?: WeakRef): Promise { } } -const SAMPLE_PLOTTABLES: PlottableProperty[] = [ - { name: "t", accessor: (point) => point.t ?? 0 }, - { name: "x", accessor: (point) => point.x ?? 0 }, -]; +// Named separately from the array so callers can reference them without an +// index lookup, which under noUncheckedIndexedAccess would widen to +// `PlottableProperty | undefined` and force a non-null assertion. +// Bracket access because `point` is a Record and the project +// sets noPropertyAccessFromIndexSignature. +const TIME_PLOTTABLE: PlottableProperty = { + name: "t", + accessor: (point: Record) => point["t"] ?? 0, +}; +const POSITION_PLOTTABLE: PlottableProperty = { + name: "x", + accessor: (point: Record) => point["x"] ?? 0, +}; + +const SAMPLE_PLOTTABLES: PlottableProperty[] = [TIME_PLOTTABLE, POSITION_PLOTTABLE]; function createAndDisposeGraphControlsPanel(): WeakRef { - const xPropertyProperty = new Property(SAMPLE_PLOTTABLES[0]!); - const yPropertyProperty = new Property(SAMPLE_PLOTTABLES[1]!); + // Explicit type argument: TypeScript narrows these consts to RecordPlottable + // from their initializers, and Property is invariant in T. + const xPropertyProperty = new Property(TIME_PLOTTABLE); + const yPropertyProperty = new Property(POSITION_PLOTTABLE); const panel = new GraphControlsPanel(SAMPLE_PLOTTABLES, xPropertyProperty, yPropertyProperty, 200); const ref = new WeakRef(panel); panel.dispose(); @@ -64,8 +77,8 @@ describe("Memory leak regression", () => { }); it("double dispose() does not throw", () => { - const xPropertyProperty = new Property(SAMPLE_PLOTTABLES[0]!); - const yPropertyProperty = new Property(SAMPLE_PLOTTABLES[1]!); + const xPropertyProperty = new Property(TIME_PLOTTABLE); + const yPropertyProperty = new Property(POSITION_PLOTTABLE); const panel = new GraphControlsPanel(SAMPLE_PLOTTABLES, xPropertyProperty, yPropertyProperty, 200); panel.dispose(); expect(() => panel.dispose()).not.toThrow(); From c706d54273fcacdceb4a9aa548961dc8ed84f683 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:20:19 +0000 Subject: [PATCH 4/4] Typecheck tests/ in npm run check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests/ was in no tsconfig include path, so `npm run check` never looked at it. Vitest transpiles without typechecking, so a green `npm test` said nothing about whether the specs were type-correct — two type errors sat in tests/memory-leak.test.ts undetected until the previous commit. Adds tsconfig.tests.json, mirroring the existing tsconfig.scripts.json pattern: extends the root config and adds `node` types for `process` and `globalThis.gc`. Kept as a separate program rather than widening the root include so node types do not leak into the src build. Verified by planting a deliberate type error in tests/setup.ts and confirming `npm run check` fails on it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gaCkV6CD5uR1SJpq3BPss --- CLAUDE.md | 7 +++++++ package.json | 2 +- tsconfig.tests.json | 8 ++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 tsconfig.tests.json diff --git a/CLAUDE.md b/CLAUDE.md index 1728232..d40ea01 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,14 +61,21 @@ Fleet-standard Vitest layout: | `tests/setup.ts` | Canvas / AudioContext mocks + `init({ name: "…" })` before SceneryStack imports | | `tests/**/*.test.ts` | Model/physics unit tests — mirror `src/` under `tests/` | | `tests/memory-leak.test.ts` | WeakRef + `forceGC` dispose regression (fleet pattern) | +| `tsconfig.tests.json` | Typecheck config for `tests/` (adds `node` types), run by `npm run check` | Actual specs: - `tests/track-lab/model/KinematicsComputer.test.ts` +- `tests/track-lab/model/TrackingModel.test.ts` - `tests/memory-leak.test.ts` Run `npm test`. CI runs the suite when a `test` script is present. +`npm run check` typechecks three programs — `src` (root `tsconfig.json`), `scripts` +(`tsconfig.scripts.json`), and `tests` (`tsconfig.tests.json`). Vitest transpiles without +typechecking, so a passing `npm test` does not imply the specs typecheck; `npm run check` is what +catches that. + ## Commands ```bash diff --git a/package.json b/package.json index 849aa95..0e9618b 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "lint": "biome check .", "format": "biome format --write .", "fix": "biome check --write .", - "check": "tsc --noEmit && tsc -p tsconfig.scripts.json --noEmit", + "check": "tsc --noEmit && tsc -p tsconfig.scripts.json --noEmit && tsc -p tsconfig.tests.json --noEmit", "test": "vitest run", "test:watch": "vitest", "release": "npm run check && npm run lint && npm run build && npm version patch && git push && git push --tags", diff --git a/tsconfig.tests.json b/tsconfig.tests.json new file mode 100644 index 0000000..331b228 --- /dev/null +++ b/tsconfig.tests.json @@ -0,0 +1,8 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "types": ["node"] + }, + "include": ["tests"] +}