fix: correct OpenCV ROI clamp and eliminate per-frame DOM rebuild

src/screen-name/model/SimModel.ts

@@ -440,6 +440,11 @@ 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;
+
       const point: TrackPoint = { frame, time, x, y };
       const updated: Track = { ...track, points: [...track.points, point] };
       return updated;

src/screen-name/view/AutoTrackerNode.ts

@@ -46,6 +46,8 @@ const TRAIL_DOT_RADIUS = 3; // radius of each past-position dot in the trail
 export class AutoTrackerNode extends Node {
   private readonly model: SimModel;
   private readonly trail: Array<{ x: number; y: number }> = [];
+  /** Frames already recorded to the active track; cleared on track change or reset. */
+  private readonly recordedFrames = new Set<number>();
 
   private readonly hintText: Text;
   private readonly selectionRect: Rectangle;
@@ -57,9 +59,10 @@ export class AutoTrackerNode extends Node {
   private selecting = false;
   private selStart = Vector2.ZERO;
 
-  // Kept for removeEventListener in dispose()
+  // Kept for removeEventListener / unlink in dispose()
   private readonly boundVideoElement: HTMLVideoElement;
   private readonly boundOnFrame: () => void;
+  private readonly boundClearRecordedFrames: () => void;
 
   public constructor(
     videoElement: HTMLVideoElement,
@@ -222,20 +225,14 @@ export class AutoTrackerNode extends Node {
         const frameDuration = model.frameDurationProperty.value;
         const frame = Math.round(time / frameDuration);
 
-        // Avoid duplicate points for the same frame.
-        const activeTrack = model.tracksProperty.value.find(
-          (t) => t.id === activeId,
-        );
-        const alreadyRecorded = activeTrack
-          ? activeTrack.points.some((p) => p.frame === frame)
-          : false;
-
-        if (!alreadyRecorded) {
+        // O(1) duplicate-frame check via Set (vs O(n) linear scan).
+        if (!this.recordedFrames.has(frame)) {
           // Convert video-pixel coords to global coords, then to model coords.
           const globalPt = this.localToGlobalPoint(new Vector2(pt.x, pt.y));
           const modelPt =
             model.modelViewTransformProperty.value.inversePosition2(globalPt);
           model.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y);
+          this.recordedFrames.add(frame);
         }
       }
     };
@@ -246,6 +243,13 @@ export class AutoTrackerNode extends Node {
     this.boundVideoElement = videoElement;
     this.boundOnFrame = onFrame;
 
+    // Clear the recorded-frames set whenever the user switches to a different
+    // track so frames from the previous track don't suppress recording on the
+    // new one.
+    const clearRecordedFrames = () => this.recordedFrames.clear();
+    model.activeTrackIdProperty.lazyLink(clearRecordedFrames);
+    this.boundClearRecordedFrames = clearRecordedFrames;
+
     // ── Show/hide based on combined "video loaded && autoTracking" ────────
     autoTrackingShownProperty.link((shown) => {
       if (!shown) this.reset();
@@ -280,6 +284,7 @@ export class AutoTrackerNode extends Node {
   public reset(): void {
     this.model.tracker.dispose();
     this.trail.length = 0;
+    this.recordedFrames.clear();
     this.selecting = false;
     this.selectionRect.visible = false;
     this.trailPath.shape = null;
@@ -290,6 +295,7 @@ export class AutoTrackerNode extends Node {
   public override dispose(): void {
     this.boundVideoElement.removeEventListener("timeupdate", this.boundOnFrame);
     this.boundVideoElement.removeEventListener("seeked", this.boundOnFrame);
+    this.model.activeTrackIdProperty.unlink(this.boundClearRecordedFrames);
     this.model.tracker.dispose();
     super.dispose();
   }

src/screen-name/view/DataTableNode.ts

@@ -291,6 +291,43 @@ function buildHTMLTable(
   return wrapper;
 }
 
+/**
+ * Build a single `<tr>` for a data row.
+ * Used by the incremental-update path to avoid rebuilding the whole table.
+ */
+function buildSingleDataRow(
+  row: DataRow,
+  tracks: readonly Track[],
+  cellStyle: string,
+  isEven: boolean,
+  colors: TableColors,
+): HTMLTableRowElement {
+  const tr = document.createElement("tr");
+  tr.style.background = isEven ? colors.rowEven : colors.rowOdd;
+
+  const addCell = (text: string) => {
+    const td = document.createElement("td");
+    td.textContent = text;
+    td.style.cssText = cellStyle;
+    tr.appendChild(td);
+  };
+
+  addCell(String(row.frame));
+  addCell(row.time.toFixed(CELL_DECIMAL_PLACES));
+  for (const track of tracks) {
+    const val = row.values.get(track.id);
+    if (val) {
+      addCell(val.x.toFixed(CELL_DECIMAL_PLACES));
+      addCell(val.y.toFixed(CELL_DECIMAL_PLACES));
+    } else {
+      addCell("—");
+      addCell("—");
+    }
+  }
+
+  return tr;
+}
+
 /**
  * Download icon (simple arrow pointing down).
  */
@@ -309,6 +346,14 @@ export class DataTableNode extends Panel {
   private tableWrapper: HTMLDivElement;
   private readonly disposeDataTable: () => void;
 
+  // ── Incremental-update state ─────────────────────────────────────────────
+  // Tracks whether the table needs a full structural rebuild or just new rows.
+  private lastTrackIds: string[] = [];
+  private lastUnit: string = "";
+  private tableBodyRef: HTMLTableSectionElement | null = null;
+  private readonly frameRowMap: Map<number, HTMLTableRowElement> = new Map();
+  private maxRenderedFrame: number = -Infinity;
+
   public constructor(
     model: SimModel,
     videoLoadedProperty: TReadOnlyProperty<boolean>,
@@ -407,27 +452,123 @@ export class DataTableNode extends Panel {
 
     this.tableWrapper = tableWrapper;
 
+    // ── Full rebuild helper ──────────────────────────────────────────────────
+    // Replaces the entire table DOM and refreshes cached references.
+    const doFullRebuild = (tracks: readonly Track[], unit: string) => {
+      const colors = getTableColors();
+      const newWrapper = buildHTMLTable(tracks, unit, colors, getLabels());
+
+      this.tableWrapper.innerHTML = "";
+      if (newWrapper.firstChild) {
+        this.tableWrapper.appendChild(newWrapper.firstChild);
+      }
+      this.tableWrapper.style.cssText = newWrapper.style.cssText;
+
+      // Cache <tbody> reference and rebuild the frame→row map.
+      const tbody = this.tableWrapper.querySelector("tbody");
+      this.tableBodyRef = tbody instanceof HTMLTableSectionElement ? tbody : null;
+      this.frameRowMap.clear();
+      this.maxRenderedFrame = -Infinity;
+
+      if (this.tableBodyRef) {
+        const dataRows = buildDataRows(tracks);
+        const trs = Array.from(this.tableBodyRef.querySelectorAll("tr"));
+        dataRows.forEach((row, i) => {
+          const tr = trs[i];
+          if (tr instanceof HTMLTableRowElement) {
+            this.frameRowMap.set(row.frame, tr);
+            if (row.frame > this.maxRenderedFrame) {
+              this.maxRenderedFrame = row.frame;
+            }
+          }
+        });
+      }
+
+      this.lastTrackIds = tracks.map((t) => t.id);
+      this.lastUnit = unit;
+    };
+
     // ── Rebuild table function ───────────────────────────────────────────────
+    // Performs a full rebuild on structural changes (track added/removed, unit
+    // or colour change) and an incremental row-append on data-only changes
+    // (new points added to existing tracks).  During auto-tracking this fires
+    // ~30 times/s, so avoiding unnecessary full DOM rebuilds is critical.
     const rebuildTable = () => {
       const tracks = model.tracksProperty.value;
       const unit = unitProperty.value;
+      const trackIds = tracks.map((t) => t.id);
+
+      const isStructural =
+        unit !== this.lastUnit ||
+        trackIds.length !== this.lastTrackIds.length ||
+        trackIds.some((id, i) => id !== this.lastTrackIds[i]);
+
+      if (isStructural || !this.tableBodyRef) {
+        doFullRebuild(tracks, unit);
+        return;
+      }
+
+      // ── Incremental path: same track structure, only new points ────────────
+      const dataRows = buildDataRows(tracks);
 
-      // Build new table with current colors and labels
-      const newWrapper = buildHTMLTable(
-        tracks,
-        unit,
-        getTableColors(),
-        getLabels(),
+      // If any new row would be inserted before an already-rendered row the
+      // sort order of the table would break; fall back to full rebuild in that
+      // rare case (out-of-order manual digitizing on an earlier frame).
+      const hasOutOfOrder = dataRows.some(
+        (row) =>
+          !this.frameRowMap.has(row.frame) &&
+          row.frame < this.maxRenderedFrame,
       );
+      if (hasOutOfOrder) {
+        doFullRebuild(tracks, unit);
+        return;
+      }
 
-      // Replace the content
-      this.tableWrapper.innerHTML = "";
-      if (newWrapper.firstChild) {
-        this.tableWrapper.appendChild(newWrapper.firstChild);
+      const colors = getTableColors();
+      const cellStyle = `padding: 3px 6px; border: 1px solid ${colors.gridStroke}; text-align: center;`;
+
+      // Remove the "no data" placeholder row when the first real rows arrive.
+      if (this.frameRowMap.size === 0 && dataRows.length > 0) {
+        this.tableBodyRef.innerHTML = "";
       }
 
-      // Copy styles
-      this.tableWrapper.style.cssText = newWrapper.style.cssText;
+      for (const row of dataRows) {
+        if (this.frameRowMap.has(row.frame)) {
+          // Update cells in an existing row (a second track filled in this frame).
+          const tr = this.frameRowMap.get(row.frame)!;
+          const cells = tr.querySelectorAll("td");
+          let cellIdx = 2; // skip Frame and Time columns
+          for (const track of tracks) {
+            const val = row.values.get(track.id);
+            const xCell = cells[cellIdx];
+            const yCell = cells[cellIdx + 1];
+            if (xCell)
+              xCell.textContent = val
+                ? val.x.toFixed(CELL_DECIMAL_PLACES)
+                : "—";
+            if (yCell)
+              yCell.textContent = val
+                ? val.y.toFixed(CELL_DECIMAL_PLACES)
+                : "—";
+            cellIdx += 2;
+          }
+        } else {
+          // Append a brand-new row at the bottom.
+          const rowIndex = this.frameRowMap.size; // 0-based index of this row
+          const tr = buildSingleDataRow(
+            row,
+            tracks,
+            cellStyle,
+            rowIndex % 2 !== 0, // isEven flag: index 0 → rowOdd, index 1 → rowEven, …
+            colors,
+          );
+          this.tableBodyRef.appendChild(tr);
+          this.frameRowMap.set(row.frame, tr);
+          if (row.frame > this.maxRenderedFrame) {
+            this.maxRenderedFrame = row.frame;
+          }
+        }
+      }
     };
 
     // ── Reactive updates ─────────────────────────────────────────────────────
@@ -437,11 +578,16 @@ export class DataTableNode extends Panel {
     const unitListener = () => rebuildTable();
     unitProperty.link(unitListener);
 
-    // Rebuild table when color profile or locale changes
-    const tableHeaderBgListener = () => rebuildTable();
+    // Colour profile and locale changes require a full rebuild because cell
+    // colours and label strings are baked into the DOM; they are not captured
+    // by the track-ID / unit structural-change check above.
+    const fullRebuild = () =>
+      doFullRebuild(model.tracksProperty.value, unitProperty.value);
+
+    const tableHeaderBgListener = () => fullRebuild();
     TrackLabColors.tableHeaderBackgroundProperty.lazyLink(tableHeaderBgListener);
 
-    const frameStringListener = () => rebuildTable();
+    const frameStringListener = () => fullRebuild();
     dataTableStrings.frameStringProperty.lazyLink(frameStringListener);
 
     const videoLoadedListener = (loaded: boolean) => {

src/screen-name/view/DigitizingOverlayNode.ts

@@ -1,7 +1,6 @@
 import { Vector2 } from "scenerystack/dot";
 import { Shape } from "scenerystack/kite";
 import {
-  Circle,
   DOM,
   FireListener,
   Line,
@@ -260,32 +259,49 @@ export class DigitizingOverlayNode extends Node {
     });
 
     // ── Mark dots layer ─────────────────────────────────────────────────────
+    // One Path per track is reused across rebuilds.  This avoids allocating
+    // and discarding SceneryStack nodes on every video frame (~30 Hz during
+    // playback), which caused significant GC pressure with many track points.
     const marksLayer = new Node({ pickable: false });
+    const trackPaths = new Map<string, Path>(); // track id → Path
 
     const rebuildMarks = () => {
       const frameDuration = model.frameDurationProperty.value;
       const currentFrame = Math.round(
         model.currentTimeProperty.value / frameDuration,
       );
       const mvt = model.modelViewTransformProperty.value;
-      const circles: Circle[] = [];
-      for (const track of model.tracksProperty.value) {
+      const tracks = model.tracksProperty.value;
+      const activeTrackIds = new Set(tracks.map((t) => t.id));
+
+      // Update or create one Path per track.
+      for (const track of tracks) {
+        let path = trackPaths.get(track.id);
+        if (!path) {
+          path = new Path(null, { fill: track.color, pickable: false });
+          trackPaths.set(track.id, path);
+          marksLayer.addChild(path);
+        }
+        const shape = new Shape();
         for (const point of track.points) {
           if (point.frame <= currentFrame) {
             const localPt = mvt.transformPosition2(
               new Vector2(point.x, point.y),
             );
-            circles.push(
-              new Circle(MARK_DOT_RADIUS, {
-                fill: track.color,
-                x: localPt.x,
-                y: localPt.y,
-              }),
-            );
+            shape.circle(localPt.x, localPt.y, MARK_DOT_RADIUS);
           }
         }
+        path.shape = shape;
+      }
+
+      // Remove paths for tracks that have been deleted.
+      for (const [id, path] of trackPaths) {
+        if (!activeTrackIds.has(id)) {
+          marksLayer.removeChild(path);
+          path.dispose();
+          trackPaths.delete(id);
+        }
       }
-      marksLayer.children = circles;
     };
 
     const currentTimeListener = () => rebuildMarks();
@@ -355,6 +371,10 @@ export class DigitizingOverlayNode extends Node {
       model.frameRateProperty.unlink(frameRateListener);
       model.activeTrackIdProperty.unlink(activeTrackListener);
       model.magnifyVideoProperty.unlink(magnifyListener);
+      for (const path of trackPaths.values()) {
+        path.dispose();
+      }
+      trackPaths.clear();
     };
   }