diff --git a/apps/cv-worker/src/ballfloor.test.ts b/apps/cv-worker/src/ballfloor.test.ts index 2755eb3..16105c8 100644 --- a/apps/cv-worker/src/ballfloor.test.ts +++ b/apps/cv-worker/src/ballfloor.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { parseClassConfidence } from './index.js'; +import { parseClassBuffer, parseClassConfidence } from './index.js'; /** * A basketball is a handful of pixels and the model is right to be unsure about @@ -58,3 +58,27 @@ describe('per-class detection floors', () => { expect(parseClassConfidence('ball=-0.2')).toEqual({}); }); }); + +/** + * Buffers are a distance, not a probability, so they share the parser but not + * its ceiling — the useful values for a basketball are greater than 1, which is + * the whole reason the plain overlap test fails on it. + */ +describe('per-class association buffers', () => { + it('accepts the values a ball actually needs', () => { + expect(parseClassBuffer('ball=1.5')).toEqual({ ball: 1.5 }); + expect(parseClassBuffer('ball=2.5,puck=1.5')).toEqual({ ball: 2.5, puck: 1.5 }); + }); + + it('still refuses nonsense', () => { + expect(parseClassBuffer('ball=0')).toEqual({}); + expect(parseClassBuffer('ball=-1')).toEqual({}); + expect(parseClassBuffer('ball=abc')).toEqual({}); + // Far enough that every ball on court would match every other one. + expect(parseClassBuffer('ball=99')).toEqual({}); + }); + + it('is absent rather than empty when nothing is asked for', () => { + expect(parseClassBuffer(undefined)).toEqual({}); + }); +}); diff --git a/apps/cv-worker/src/ballrecall.test.ts b/apps/cv-worker/src/ballrecall.test.ts new file mode 100644 index 0000000..38489ac --- /dev/null +++ b/apps/cv-worker/src/ballrecall.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it } from 'vitest'; + +import type { Detection } from './geometry.js'; +import { ByteTracker, inflate } from './tracker.js'; + +/** + * Why the ball is hardly ever identified. + * + * Per-class confidence floors let a faint ball through the filter, and ball + * *positions* rose 69% — but the number of ball tracks barely moved, which is + * the tell. Only a confident detection may open a track; a faint one can extend + * a track that already exists. So every extra ball detection attached itself to + * the handful of tracks lucky enough to have started at 0.4, and the rest were + * discarded for having nothing to attach to. + * + * The second half is geometry. Overlap is a poor question to ask about a thrown + * ball: it is a small box that clears several of its own widths between sampled + * frames, and boxes that do not touch have an IoU of exactly zero however + * close they are. + * + * Measured against the deployed worker, same 20s of real game, same flags: + * + * ball hoop player referee time + * deployed today 21 / 382 7 / 387 297 / 8604 39 / 803 89s + * + birth at floor 71 / 831 15 / 425 297 / 8604 39 / 803 93s + * + buffered IoU 66 / 891 15 / 425 297 / 8604 39 / 803 88s + * + * Ball positions rise 133% for no measurable runtime. The rim doubles too — its + * 0.15 floor was under the same 0.4 birth bar. People are identical to the + * digit, which is the invariant the tests below exist to hold: loosening the + * ball must never loosen anybody. + */ + +const det = (x: number, y: number, w: number, h: number, score: number, classId = 0): Detection => ({ + x, + y, + w, + h, + score, + classId, +}); + +const names = { 0: 'player', 32: 'ball' }; + +describe('starting a track at the class its own floor', () => { + it('opens a ball track from detections no player detection could open', () => { + const tracker = new ByteTracker({ + minLength: 1, + highThreshold: 0.5, + lowThreshold: 0.05, + classHighThreshold: { ball: 0.08 }, + }); + // A ball seen faintly and steadily — never once at the global bar of 0.5. + for (let f = 0; f < 6; f += 1) { + tracker.update([det(100 + f * 3, 200, 20, 20, 0.2, 32)], names, f, f / 30); + } + + const tracks = tracker.results(); + expect(tracks).toHaveLength(1); + expect(tracks[0]?.className).toBe('ball'); + expect(tracks[0]?.points).toHaveLength(6); + }); + + it('still refuses to open a player track from the same weak evidence', () => { + const tracker = new ByteTracker({ + minLength: 1, + highThreshold: 0.5, + lowThreshold: 0.05, + classHighThreshold: { ball: 0.08 }, + }); + for (let f = 0; f < 6; f += 1) { + tracker.update([det(100 + f * 3, 200, 40, 90, 0.2, 0)], names, f, f / 30); + } + // The floor is the ball's alone: loosening it must not loosen people. + expect(tracker.results()).toHaveLength(0); + }); + + it('leaves a class with no override on the global bar', () => { + const tracker = new ByteTracker({ minLength: 1, highThreshold: 0.5, lowThreshold: 0.05 }); + for (let f = 0; f < 6; f += 1) { + tracker.update([det(100 + f * 3, 200, 20, 20, 0.2, 32)], names, f, f / 30); + } + expect(tracker.results()).toHaveLength(0); + }); +}); + +describe('buffered overlap for something small and fast', () => { + it('grows a box by a fraction of its own size, and leaves a zero buffer alone', () => { + const original = { x: 100, y: 100, w: 20, h: 20 }; + expect(inflate(original, 0)).toEqual(original); + expect(inflate(original, 0.5)).toEqual({ x: 90, y: 90, w: 40, h: 40 }); + }); + + it('follows a ball that clears its own width between frames as one flight', () => { + const tracker = new ByteTracker({ + minLength: 1, + highThreshold: 0.5, + lowThreshold: 0.05, + classHighThreshold: { ball: 0.08 }, + classBuffer: { ball: 1.5 }, + }); + // 20px wide, moving 30px a frame: consecutive boxes never touch, so plain + // IoU is zero at every step. + for (let f = 0; f < 8; f += 1) { + tracker.update([det(100 + f * 30, 200, 20, 20, 0.3, 32)], names, f, f / 30); + } + + const tracks = tracker.results(); + expect(tracks).toHaveLength(1); + expect(tracks[0]?.points).toHaveLength(8); + }); + + it('is what makes the difference — the same flight without a buffer shatters', () => { + const tracker = new ByteTracker({ + minLength: 1, + highThreshold: 0.5, + lowThreshold: 0.05, + classHighThreshold: { ball: 0.08 }, + }); + for (let f = 0; f < 8; f += 1) { + tracker.update([det(100 + f * 30, 200, 20, 20, 0.3, 32)], names, f, f / 30); + } + + // Every frame starts a new track: this is the scatter seen in production. + expect(tracker.results().length).toBeGreaterThan(1); + }); + + it('does not let a buffered ball steal the ball beside it', () => { + const tracker = new ByteTracker({ + minLength: 1, + highThreshold: 0.5, + lowThreshold: 0.05, + classHighThreshold: { ball: 0.08 }, + classBuffer: { ball: 1.5 }, + }); + // Two balls, far apart, moving in opposite directions. Buffering must widen + // the net, not merge distinct objects. + for (let f = 0; f < 8; f += 1) { + tracker.update( + [det(100 + f * 20, 200, 20, 20, 0.3, 32), det(900 - f * 20, 600, 20, 20, 0.3, 32)], + names, + f, + f / 30, + ); + } + expect(tracker.results()).toHaveLength(2); + }); + + it('leaves people associating on exactly the geometry they always did', () => { + const withBuffer = new ByteTracker({ minLength: 1, classBuffer: { ball: 1.5 } }); + const without = new ByteTracker({ minLength: 1 }); + for (let f = 0; f < 10; f += 1) { + const frame = [det(100 + f * 5, 200, 40, 90, 0.9), det(300 - f * 5, 210, 40, 90, 0.85)]; + withBuffer.update(frame, names, f, f / 30); + without.update(frame, names, f, f / 30); + } + expect(withBuffer.results().map((t) => t.points.length)).toEqual( + without.results().map((t) => t.points.length), + ); + }); +}); diff --git a/apps/cv-worker/src/index.ts b/apps/cv-worker/src/index.ts index 5b953e8..9b29e0b 100644 --- a/apps/cv-worker/src/index.ts +++ b/apps/cv-worker/src/index.ts @@ -43,25 +43,41 @@ const number = (value: string | undefined, fallback: number): number => { }; /** - * `ball=0.08,hoop=0.15` — per-class detection floors. + * `ball=0.08,hoop=0.15` — a per-class number, whatever the number means. * * Malformed pairs are dropped rather than throwing: a typo in a preset should * cost the ball some recall, not take a twenty-minute detection pass down with * it. */ -export const parseClassConfidence = (value: string | undefined): Record => { +const parseClassNumbers = (value: string | undefined, max: number): Record => { if (value === undefined || value.trim().length === 0) return {}; const out: Record = {}; for (const pair of value.split(',')) { const [name, raw] = pair.split('='); const parsed = Number(raw); if (name === undefined || name.trim().length === 0) continue; - if (!Number.isFinite(parsed) || parsed <= 0 || parsed > 1) continue; + if (!Number.isFinite(parsed) || parsed <= 0 || parsed > max) continue; out[name.trim()] = parsed; } return out; }; +/** Per-class detection floors. A confidence cannot exceed 1. */ +export const parseClassConfidence = (value: string | undefined): Record => + parseClassNumbers(value, 1); + +/** + * Per-class association buffers, as a fraction of the box's own size. + * + * Exposed as a flag for the same reason the floors are: the value that belongs + * here is a measurement, not an opinion, and a constant compiled into the + * pipeline cannot be swept against real footage without rebuilding it. Allowed + * well above 1 — a basketball travels several of its own widths between sampled + * frames, which is the entire problem being solved. + */ +export const parseClassBuffer = (value: string | undefined): Record => + parseClassNumbers(value, 8); + /** * How many threads onnxruntime should use for one inference. * @@ -187,6 +203,9 @@ const detectAndTrack = async (flags: Record): Promise => { tileGrid: number(flags['tile-grid'], 1), minConfidence: number(flags['min-confidence'], 0.3), classConfidence: parseClassConfidence(flags['class-confidence']), + ...(flags['class-buffer'] === undefined + ? {} + : { classBuffer: parseClassBuffer(flags['class-buffer']) }), iouThreshold: number(flags['iou'], 0.45), sourceWidth: width, sourceHeight: height, diff --git a/apps/cv-worker/src/pipeline.ts b/apps/cv-worker/src/pipeline.ts index b741e89..e1e4894 100644 --- a/apps/cv-worker/src/pipeline.ts +++ b/apps/cv-worker/src/pipeline.ts @@ -51,6 +51,8 @@ export interface PipelineOptions { * detections are the same ball seen more often rather than new phantoms. */ classConfidence?: Record; + /** Per-class association buffer; omitted means the built-in table. */ + classBuffer?: Record; iouThreshold: number; sourceWidth: number; sourceHeight: number; @@ -72,6 +74,33 @@ export interface PipelineResult { unsupportedClasses: string[]; } +/** + * How far each class may move, relative to its own size, and still be + * recognised as the same object on the next sampled frame. + * + * People are left at 0: a player cannot cross their own width in a frame, so + * plain overlap already answers the question and buffering would only invite + * one player's box to capture the player beside them. A ball is the opposite — + * small, and fast enough to clear several of its own widths — which is why it + * arrives as a scatter of short tracks rather than a flight. + * + * Swept over 20s of a real game, at the shipped 2x2 grid and ball floor: + * + * buffer ball tracks / positions positions per track + * 0 71 / 831 11.7 + * 0.5 67 / 869 13.0 + * 1.5 66 / 891 13.5 + * 3.0 59 / 900 15.3 + * + * Tracks fall while positions rise, which is fragments being joined into + * flights rather than new detections appearing. 3.0 joins more, but buys almost + * no extra coverage for it (+1% positions over 1.5) while roughly doubling the + * radius in which a stray ball-shaped blob can be mistaken for the ball — and a + * track that teleports is worse than two that stop. Player, referee and hoop + * counts were identical to the digit at every value. + */ +const SMALL_FAST: Record = { ball: 1.5, puck: 1.5 }; + export const createSession = async ( modelPath: string, threads: number, @@ -203,6 +232,14 @@ export const runPipeline = async (options: PipelineOptions): Promise; + /** + * Per-class association buffer, keyed by class name: both boxes are grown by + * this fraction of their own size before overlap is measured. + * + * Overlap is the wrong question for a thrown ball. A basketball is a small + * box that routinely travels more than its own width between sampled frames, + * and two boxes that do not touch have an IoU of exactly zero however close + * they are — so association fails, the track dies, and a new one is born + * further along the same flight. Growing both boxes first (buffered IoU) + * restores a gradient for objects that move far relative to their size, and + * leaves anything with a buffer of 0 — every person on court — untouched. + */ + classBuffer: Record; } export const DEFAULT_TRACKER_OPTIONS: TrackerOptions = { @@ -52,6 +79,8 @@ export const DEFAULT_TRACKER_OPTIONS: TrackerOptions = { iouThreshold: 0.2, maxAge: 30, minLength: 3, + classHighThreshold: {}, + classBuffer: {}, }; const centre = (box: Box): { x: number; y: number } => ({ @@ -73,6 +102,30 @@ interface Pair { score: number; } +const box = (detection: Detection): Box => ({ + x: detection.x, + y: detection.y, + w: detection.w, + h: detection.h, +}); + +/** + * Grows a box by a fraction of its own size on every side. + * + * A buffer of 0 returns the box unchanged, which is the point: buffering is + * opt-in per class, so classes that do not ask for it associate on exactly the + * geometry they always did. + */ +export const inflate = (target: Box, by: number): Box => + by <= 0 + ? target + : { + x: target.x - target.w * by, + y: target.y - target.h * by, + w: target.w * (1 + 2 * by), + h: target.h * (1 + 2 * by), + }; + /** * Greedy IoU association. Hungarian would be optimal, but greedy-by-IoU is * within noise for this many objects and is far easier to reason about when a @@ -82,14 +135,16 @@ const associate = ( tracks: Track[], detections: Detection[], iouThreshold: number, + bufferFor: (track: Track) => number = () => 0, ): { matches: Pair[]; unmatchedTracks: number[]; unmatchedDetections: number[] } => { const candidates: Pair[] = []; tracks.forEach((track, trackIndex) => { - const predicted = predict(track); + const buffer = bufferFor(track); + const predicted = inflate(predict(track), buffer); detections.forEach((detection, detectionIndex) => { if (detection.classId !== track.classId) return; - const score = iou(predicted, detection); + const score = iou(predicted, inflate(box(detection), buffer)); if (score >= iouThreshold) candidates.push({ trackIndex, detectionIndex, score }); }); }); @@ -152,12 +207,24 @@ export class ByteTracker { /** Feeds one frame of detections. */ update(detections: Detection[], classNames: Record, frame: number, ts: number): void { + /** + * The bar this detection has to clear to be treated as confident — and so + * to be allowed to start a track, not merely extend one. + */ + const barFor = (detection: Detection): number => { + const className = classNames[detection.classId]; + const override = + className === undefined ? undefined : this.options.classHighThreshold[className]; + return override ?? this.options.highThreshold; + }; + const bufferFor = (track: Track): number => this.options.classBuffer[track.className] ?? 0; + const usable = detections.filter((d) => d.score >= this.options.lowThreshold); - const high = usable.filter((d) => d.score >= this.options.highThreshold); - const low = usable.filter((d) => d.score < this.options.highThreshold); + const high = usable.filter((d) => d.score >= barFor(d)); + const low = usable.filter((d) => d.score < barFor(d)); // Pass 1: confident detections against every live track. - const first = associate(this.active, high, this.options.iouThreshold); + const first = associate(this.active, high, this.options.iouThreshold, bufferFor); for (const match of first.matches) { const track = this.active[match.trackIndex]; const detection = high[match.detectionIndex]; @@ -167,7 +234,7 @@ export class ByteTracker { // Pass 2: the BYTE step — try the leftovers against tracks that missed out, // with a looser bar, so an occluded player is rescued rather than lost. const stranded = first.unmatchedTracks.map((i) => this.active[i]).filter((t): t is Track => t !== undefined); - const second = associate(stranded, low, this.options.iouThreshold * 0.75); + const second = associate(stranded, low, this.options.iouThreshold * 0.75, bufferFor); const rescued = new Set(); for (const match of second.matches) { const track = stranded[match.trackIndex]; @@ -208,9 +275,3 @@ export class ByteTracker { } } -const box = (detection: Detection): Box => ({ - x: detection.x, - y: detection.y, - w: detection.w, - h: detection.h, -}); diff --git a/apps/web/src/actions.ts b/apps/web/src/actions.ts index 5bd10b6..6676c58 100644 --- a/apps/web/src/actions.ts +++ b/apps/web/src/actions.ts @@ -697,6 +697,7 @@ export const registerActions = (app: Hono): void => { jerseyNumber?: string; team?: string; jerseyColor?: string; + expand?: boolean; }) : { trackId: field(await c.req.parseBody(), 'trackId') }; @@ -789,9 +790,44 @@ export const registerActions = (app: Hono): void => { : requestedIds; const assigned = await assignTracksToAthlete(root, athleteId, finalIds); + /** + * Follow them through the rest of the game, if the caller cannot. + * + * Stitching one pick into the fragments either side of it has existed + * since the appearance matcher landed, but the only way to reach it was + * the candidate grid, which offers proposals to tick. The scrubber — the + * surface people actually use, because pointing at your child on the + * footage needs no explanation — bound the single track under the cursor + * and stopped. Production shows exactly that: an athlete on 2 tracks out + * of 1125, and one suggested moment, in a game they play the whole of. + * + * The proposals are the same ones the grid pre-ticks for confirmation, so + * accepting them here is the behaviour that surface already had. It is + * best-effort: a failure to expand must never lose the pick itself, which + * is the part the user made and the part scoring cannot do without. + */ + let added: string[] = []; + if (body.expand === true) { + try { + const found = await proposeAthleteTracks(root, athleteId, {}); + added = found.proposals.map((proposal) => proposal.trackId); + if (added.length > 0) { + // The whole set, not the additions: assigning is a replace, and it + // clears the athlete's existing rows before it writes. + await assignTracksToAthlete(root, athleteId, [...new Set([...finalIds, ...added])]); + } + } catch { + // No worker, no proxy, nothing to link to — all survivable. The pick + // stands, and the grid can still be used to widen it by hand. + added = []; + } + } + startAnalysis(root, { preset: 'balanced', scoreOnly: true }); - if (prefersJson(c)) return c.json({ ok: true, athleteId, trackId, assigned }); + if (prefersJson(c)) { + return c.json({ ok: true, athleteId, trackId, assigned, added: added.length }); + } return back(c, to, 'Athlete identified — re-scoring with them as the focus'); } catch (error) { if (prefersJson(c)) return uploadJson(c, error); diff --git a/apps/web/src/client/live.ts b/apps/web/src/client/live.ts index 9d9b730..28afcb8 100644 --- a/apps/web/src/client/live.ts +++ b/apps/web/src/client/live.ts @@ -1,4 +1,4 @@ -import { mountReview } from './review.js'; +import { mountMoments } from './moments.js'; /** * Bringing the page up to date without reloading it. @@ -22,9 +22,17 @@ import { mountReview } from './review.js'; * scrolls. */ -/** Islands that live inside a swappable region and must be re-mounted after it. */ -const REMOUNT: Record void> = { - moments: mountReview, +/** + * Islands that live inside a swappable region and must be re-mounted after it. + * + * `moments` holds `#moment-review`, and it must be that island — not the review + * scrubber, which lives on another page entirely and inside no region at all. + * Pointing this at the scrubber meant the moment list was re-rendered as inert + * server markup, with nothing behind Keep or Reject, at exactly the point a + * finished job swapped new suggestions in. + */ +export const REMOUNT: Record void> = { + moments: mountMoments, }; let inFlight: Promise | null = null; diff --git a/apps/web/src/client/main.tsx b/apps/web/src/client/main.tsx index 84650ad..611c04d 100644 --- a/apps/web/src/client/main.tsx +++ b/apps/web/src/client/main.tsx @@ -1,114 +1,22 @@ -/** @jsxImportSource hono/jsx/dom */ -import { render, useState } from 'hono/jsx/dom'; - import { mountIdentify } from './identify.js'; +import { mountMoments } from './moments.js'; import { mountOverlays } from './overlay.js'; import { mountReview } from './review.js'; import { mountJobLog } from './jobs.js'; import { mountUploads } from './upload.js'; /** - * The SPA half of the app: one island that takes over the server-rendered - * moment list and makes accept/reject interactive. Everything else stays plain - * SSR, which keeps the page useful with JavaScript disabled. + * The client entry point: mount every island the page happens to contain. + * + * Each mount looks for its own anchor and returns quietly if the page has none, + * so this one list serves every page. Everything not listed here stays plain + * SSR, which keeps the app usable with JavaScript disabled. */ -interface Moment { - id: string; - start: number; - end: number; - score: number; - reasons: string[]; - included: boolean | null; - favorite: boolean; -} - -const duration = (seconds: number): string => { - const total = Math.max(0, Math.round(seconds)); - return `${Math.floor(total / 60)}:${String(total % 60).padStart(2, '0')}`; -}; - -const decision = (included: boolean | null): string => - included === true ? 'keep' : included === false ? 'reject' : 'undecided'; - -const MomentReview = ({ projectId, initial }: { projectId: string; initial: Moment[] }) => { - const [moments, setMoments] = useState(initial); - const [busy, setBusy] = useState(null); - const [error, setError] = useState(null); - - const decide = async (moment: Moment, included: boolean | null): Promise => { - setBusy(moment.id); - setError(null); - try { - const response = await fetch( - `/api/projects/${encodeURIComponent(projectId)}/moments/${moment.id}`, - { - method: 'PATCH', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ included }), - }, - ); - const payload = (await response.json()) as { ok: boolean; moment?: Moment; error?: string }; - if (!response.ok || !payload.ok || payload.moment === undefined) { - throw new Error(payload.error ?? `Request failed (${response.status})`); - } - const updated = payload.moment; - setMoments((current) => current.map((m) => (m.id === updated.id ? updated : m))); - } catch (cause) { - // Leave the previous state alone so the user can retry without losing work. - setError(cause instanceof Error ? cause.message : String(cause)); - } finally { - setBusy(null); - } - }; - - const kept = moments.filter((m) => m.included === true).length; - - return ( -
-

- {kept} of {moments.length} kept -

- {error === null ? null :

{error}

} - {moments.map((moment) => ( -
-
- - {duration(moment.start)} → {duration(moment.end)} - - score {moment.score.toFixed(2)} - {moment.reasons.join(', ')} - - {decision(moment.included)} - - - -
-
- ))} -
- ); -}; - const mount = (): void => { // Proof the bundle ran, so the stylesheet can hide instructions that only // make sense without it. document.documentElement.classList.add('js'); - // Independent of the review island, and present on pages that have no moments. mountUploads(); mountJobLog(); mountIdentify(); @@ -117,24 +25,8 @@ const mount = (): void => { mountOverlays(); // The review surface: every track over the whole game, click to identify. mountReview(); - - const node = document.getElementById('moment-review'); - if (node === null) return; - - const projectId = node.dataset['project']; - const raw = node.dataset['moments']; - if (projectId === undefined || raw === undefined) return; - - let initial: Moment[]; - try { - initial = JSON.parse(raw) as Moment[]; - } catch { - // Bad payload: keep the server-rendered list rather than blanking the page. - return; - } - - node.innerHTML = ''; - render(, node); + // The moment list, which live.ts also re-mounts after a refresh. + mountMoments(); }; mount(); diff --git a/apps/web/src/client/moments.tsx b/apps/web/src/client/moments.tsx new file mode 100644 index 0000000..57e0f1f --- /dev/null +++ b/apps/web/src/client/moments.tsx @@ -0,0 +1,129 @@ +/** @jsxImportSource hono/jsx/dom */ +import { render, useState } from 'hono/jsx/dom'; + +/** + * The moment list, made interactive. + * + * This lives in its own module rather than in the client entry point because + * two callers need it: the entry mounts it on load, and live.ts re-mounts it + * after swapping the region it sits in. That region is swapped precisely when a + * job finishes — the moment new suggestions appear — and re-mounting pointed at + * the wrong island, so the freshly rendered list arrived as inert server markup + * with no Keep or Reject behind its buttons. + */ + +export interface Moment { + id: string; + start: number; + end: number; + score: number; + reasons: string[]; + included: boolean | null; + favorite: boolean; +} + +const duration = (seconds: number): string => { + const total = Math.max(0, Math.round(seconds)); + return `${Math.floor(total / 60)}:${String(total % 60).padStart(2, '0')}`; +}; + +const decision = (included: boolean | null): string => + included === true ? 'keep' : included === false ? 'reject' : 'undecided'; + +const MomentReview = ({ projectId, initial }: { projectId: string; initial: Moment[] }) => { + const [moments, setMoments] = useState(initial); + const [busy, setBusy] = useState(null); + const [error, setError] = useState(null); + + const decide = async (moment: Moment, included: boolean | null): Promise => { + setBusy(moment.id); + setError(null); + try { + const response = await fetch( + `/api/projects/${encodeURIComponent(projectId)}/moments/${moment.id}`, + { + method: 'PATCH', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ included }), + }, + ); + const payload = (await response.json()) as { ok: boolean; moment?: Moment; error?: string }; + if (!response.ok || !payload.ok || payload.moment === undefined) { + throw new Error(payload.error ?? `Request failed (${response.status})`); + } + const updated = payload.moment; + setMoments((current) => current.map((m) => (m.id === updated.id ? updated : m))); + } catch (cause) { + // Leave the previous state alone so the user can retry without losing work. + setError(cause instanceof Error ? cause.message : String(cause)); + } finally { + setBusy(null); + } + }; + + const kept = moments.filter((m) => m.included === true).length; + + return ( +
+

+ {kept} of {moments.length} kept +

+ {error === null ? null :

{error}

} + {moments.map((moment) => ( +
+
+ + {duration(moment.start)} → {duration(moment.end)} + + score {moment.score.toFixed(2)} + {moment.reasons.join(', ')} + + {decision(moment.included)} + + + +
+
+ ))} +
+ ); +}; + +/** + * Takes over the server-rendered moment list, reading everything it needs from + * the node's own attributes — which is what makes it safe to call again after + * the server has re-rendered that region with different moments in it. + */ +export const mountMoments = (): void => { + const node = document.getElementById('moment-review'); + if (node === null) return; + + const projectId = node.dataset['project']; + const raw = node.dataset['moments']; + if (projectId === undefined || raw === undefined) return; + + let initial: Moment[]; + try { + initial = JSON.parse(raw) as Moment[]; + } catch { + // Bad payload: keep the server-rendered list rather than blanking the page. + return; + } + + node.innerHTML = ''; + render(, node); +}; diff --git a/apps/web/src/client/review.tsx b/apps/web/src/client/review.tsx index d1e64ff..f0d1ad1 100644 --- a/apps/web/src/client/review.tsx +++ b/apps/web/src/client/review.tsx @@ -113,8 +113,51 @@ const attach = (node: HTMLElement): void => { identify.type = 'button'; identify.textContent = 'Identify my athlete'; controls.appendChild(identify); + + /** + * Who this is, asked where the user is already looking at them. + * + * A number alone does not name a child — both teams field a 14 and they are + * on court together — so the shirt colour is the part a parent actually uses. + * Asking here rather than on a separate panel is the difference between the + * fields being filled in and being skipped: production has a child recorded + * as team "Triton (white)" because the only form that offered a colour was + * one the user never reached, so they crammed it into the team box. + */ + const field = (placeholder: string, width: string): HTMLInputElement => { + const input = document.createElement('input'); + input.type = 'text'; + input.placeholder = placeholder; + input.style.cssText = `width:${width};flex:none`; + controls.appendChild(input); + return input; + }; + const nameInput = field('Name', '8rem'); + const numberInput = field('#', '3.5rem'); + const colorInput = field('Shirt colour', '7rem'); + const teamInput = field('Team', '7rem'); + + /** Only send what was actually typed; blank fields must not erase a name. */ + const identity = (): Record => { + const parts: Record = {}; + if (nameInput.value.trim() !== '') parts['name'] = nameInput.value.trim(); + if (numberInput.value.trim() !== '') parts['jerseyNumber'] = numberInput.value.trim(); + if (colorInput.value.trim() !== '') parts['jerseyColor'] = colorInput.value.trim(); + if (teamInput.value.trim() !== '') parts['team'] = teamInput.value.trim(); + return parts; + }; + + /** Shown only while a bind is in flight, because that one really does wait. */ + const spinner = document.createElement('progress'); + spinner.style.cssText = 'display:none;width:8rem;flex:none'; + controls.appendChild(spinner); node.appendChild(controls); + const setPending = (on: boolean): void => { + spinner.style.display = on ? 'block' : 'none'; + identify.disabled = on; + }; + const status = document.createElement('p'); status.className = 'muted'; node.appendChild(status); @@ -224,24 +267,43 @@ const attach = (node: HTMLElement): void => { const bind = async (trackId: string): Promise => { if (busy) return; busy = true; - status.textContent = 'Binding…'; + /** + * Expanding decodes frames, so this is the one click on the page with a + * real wait behind it. Say what is happening rather than leaving a dead + * button: not knowing whether to wait or click again is what produced eight + * athletes in three minutes. + */ + setPending(true); + status.textContent = 'Following them through the rest of the game…'; try { const response = await fetch(bindUrl, { method: 'POST', headers: { accept: 'application/json', 'content-type': 'application/json' }, - body: JSON.stringify({ trackId, trackIds: [trackId] }), + body: JSON.stringify({ + trackId, + trackIds: [trackId], + // One click should mark the child everywhere they appear from here, + // not only in the fragment under the cursor. + expand: true, + ...identity(), + }), }); - const body = (await response.json()) as { ok: boolean; error?: string }; + const body = (await response.json()) as { ok: boolean; error?: string; added?: number }; if (!response.ok || !body.ok) throw new Error(body.error ?? 'Could not bind that track.'); // Re-fetch so the box turns green without a reload. windowStart = Number.NaN; await ensure(video.currentTime); draw(); setIdentifying(false); - status.textContent = 'Identified — re-scoring now. Suggested moments will update.'; + const added = body.added ?? 0; + status.textContent = + (added > 0 + ? `Identified, and followed them into ${added} more fragment(s) of the game. ` + : 'Identified. ') + 'Re-scoring now — suggested moments will update on their own.'; } catch (cause) { status.textContent = cause instanceof Error ? cause.message : String(cause); } finally { + setPending(false); busy = false; } }; @@ -279,5 +341,15 @@ const attach = (node: HTMLElement): void => { export const mountReview = (): void => { const node = document.getElementById('review-surface'); - if (node !== null) attach(node); + if (node === null) return; + /** + * Attach once. This is called again after a live region is swapped, and + * attaching twice appends a second canvas, a second set of fields and a + * second Identify button over the same video — the duplicate-surface bug in + * miniature, and the kind that only shows up after the page has been open + * long enough for something to refresh. + */ + if (node.dataset['mounted'] === 'true') return; + node.dataset['mounted'] = 'true'; + attach(node); }; diff --git a/apps/web/src/expandonbind.test.ts b/apps/web/src/expandonbind.test.ts new file mode 100644 index 0000000..7316703 --- /dev/null +++ b/apps/web/src/expandonbind.test.ts @@ -0,0 +1,144 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { Hono } from 'hono'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * One click on the footage should mark the child for the game, not the second. + * + * Stitching a pick into the fragments either side of it shipped with the + * appearance matcher, but the only route to it was the candidate grid, which + * offers proposals to tick. The scrubber — where people actually identify, + * because pointing at your own child needs no explanation — bound the one track + * under the cursor and stopped. Production shows the consequence exactly: an + * athlete on 2 tracks out of 1125, and one suggested moment, in a game they + * played all of. + * + * Expansion needs a CV worker and real frames, so what is asserted here is the + * contract around it: the request is accepted, the pick itself is never lost to + * a failure to widen it, and the flag is what decides whether widening is even + * attempted. + */ + +let home: string; +let root: string; +let app: Hono; + +beforeAll(async () => { + home = mkdtempSync(path.join(tmpdir(), 'reeleel-expand-')); + process.env['REELEEL_HOME'] = home; + + const { createProject } = await import('@reeleel/core'); + const created = await createProject({ + name: 'expand', + path: path.join(home, 'projects', 'expand'), + sport: 'basketball', + }); + root = created.path ?? created.root; + + const { execute, projectDb, createTrack } = await import('@reeleel/core'); + const db = await projectDb(root); + const now = new Date().toISOString(); + await execute( + db, + 'INSERT INTO source_videos (id, project_id, path, created_at, updated_at) VALUES (?, ?, ?, ?, ?)', + ['vid_a', 'prj_test', '/tmp/vid_a.mp4', now, now], + ); + for (let i = 0; i < 4; i += 1) { + await createTrack(root, { + videoId: 'vid_a', + className: 'player', + confidence: 0.9, + samples: [ + { ts: i * 10, frame: i * 300, x: 1, y: 2, w: 3, h: 4, confidence: 0.9 }, + { ts: i * 10 + 5, frame: i * 300 + 150, x: 1, y: 2, w: 3, h: 4, confidence: 0.9 }, + ], + }); + } + + const { registerActions } = await import('./actions.js'); + app = new Hono(); + registerActions(app); +}); + +afterAll(async () => { + const { resetDbCache } = await import('@reeleel/core'); + resetDbCache(); + rmSync(home, { recursive: true, force: true }); + delete process.env['REELEEL_HOME']; +}); + +const post = async (body: unknown): Promise => + app.request(`/projects/${encodeURIComponent(root)}/athletes/new/track`, { + method: 'POST', + headers: { accept: 'application/json', 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); + +describe('identifying from the scrubber', () => { + it('keeps the pick even when the athlete cannot be followed any further', async () => { + const { listTracks, listAthletes, tracksForAthlete } = await import('@reeleel/core'); + const tracks = (await listTracks(root, 'vid_a')).map((track) => track.id); + + // There is no video behind this fixture, so expansion cannot succeed. The + // pick is the part the user made; losing it to a failed widening would be + // strictly worse than never widening at all. + const response = await post({ trackId: tracks[0], trackIds: [tracks[0]], expand: true }); + expect(response.status).toBe(200); + const body = (await response.json()) as { ok: boolean; added: number }; + expect(body.ok).toBe(true); + expect(body.added).toBe(0); + + const athletes = await listAthletes(root); + expect(athletes).toHaveLength(1); + expect(await tracksForAthlete(root, athletes[0]!.id)).toEqual([tracks[0]]); + }); + + it('records who they are, colour and all, from the same click', async () => { + const { listTracks, listAthletes } = await import('@reeleel/core'); + const tracks = (await listTracks(root, 'vid_a')).map((track) => track.id); + + await post({ + trackId: tracks[1], + trackIds: [tracks[1]], + expand: true, + name: 'Fred', + jerseyNumber: '14', + jerseyColor: 'white', + team: 'Triton', + }); + + const athlete = (await listAthletes(root))[0]!; + expect(athlete.name).toBe('Fred'); + expect(athlete.jerseyNumber).toBe('14'); + // The field that has existed since the first migration and was never written. + expect(athlete.jerseyColor).toBe('white'); + expect(athlete.team).toBe('Triton'); + }); + + it('still accumulates picks rather than replacing them', async () => { + const { listTracks, listAthletes, tracksForAthlete } = await import('@reeleel/core'); + const tracks = (await listTracks(root, 'vid_a')).map((track) => track.id); + + await post({ trackId: tracks[2], trackIds: [tracks[2]], expand: true }); + + const athlete = (await listAthletes(root))[0]!; + const bound = await tracksForAthlete(root, athlete.id); + // Three clicks, three fragments, one child — widening must not undo the + // de-duplication that made repeat clicks safe in the first place. + expect(bound.sort()).toEqual(tracks.slice(0, 3).sort()); + expect(await listAthletes(root)).toHaveLength(1); + }); + + it('leaves a caller that did not ask to expand exactly as it was', async () => { + const { listTracks } = await import('@reeleel/core'); + const tracks = (await listTracks(root, 'vid_a')).map((track) => track.id); + + const response = await post({ trackId: tracks[3], trackIds: [tracks[3]] }); + const body = (await response.json()) as { ok: boolean; added: number }; + expect(body.ok).toBe(true); + expect(body.added).toBe(0); + }); +}); diff --git a/apps/web/src/remount.test.ts b/apps/web/src/remount.test.ts new file mode 100644 index 0000000..823c18a --- /dev/null +++ b/apps/web/src/remount.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; + +import { REMOUNT } from './client/live.js'; +import { mountMoments } from './client/moments.js'; +import { mountReview } from './client/review.js'; + +/** + * A swapped region has to be re-hydrated by the island that actually lives in + * it. + * + * `data-live="moments"` contains `#moment-review`, the interactive Keep/Reject + * list. The map pointed instead at the review scrubber, which lives on a + * different page and inside no live region at all — so after a refresh the + * moment list stayed as the server rendered it, with nothing behind its + * buttons, while the scrubber was re-attached to a node that already had a + * canvas and a set of controls on it. + * + * Both halves are silent failures: the buttons render perfectly and do nothing, + * and the duplicate scrubber only appears once something has refreshed. Neither + * is visible in a type or a screenshot, so the wiring is asserted here. + */ + +describe('re-mounting after a live region swap', () => { + it('hydrates the moment list with the moment island', () => { + expect(REMOUNT['moments']).toBe(mountMoments); + }); + + it('never re-mounts the review scrubber, which is on another page', () => { + expect(Object.values(REMOUNT)).not.toContain(mountReview); + }); + + it('names only regions the pages actually mark as live', async () => { + const { ProjectPage } = await import('./views/pages.js'); + const view = { + project: { id: 'prj_test', name: 'Smoke', sport: 'basketball' }, + videos: [], + athletes: [], + moments: [], + clips: [], + jobs: [], + exports: [], + music: [], + flash: {}, + } as unknown as Parameters[0]; + const html = String(await ProjectPage(view)); + + // A key with no matching region is a re-mount that can never fire. + for (const key of Object.keys(REMOUNT)) { + expect(html).toContain(`data-live="${key}"`); + } + }); +}); diff --git a/apps/web/src/views/pages.tsx b/apps/web/src/views/pages.tsx index f73d937..3fb9735 100644 --- a/apps/web/src/views/pages.tsx +++ b/apps/web/src/views/pages.tsx @@ -445,7 +445,18 @@ export const ProjectPage: FC = ({ game, and click your athlete to identify them.

{moments.length === 0 ? ( -

Nothing suggested yet — run analysis.

+ /* "Run analysis" while analysis is running is the page telling you to + act when the only right move is to wait — and acting again is what + produced eight athletes in three minutes. The empty state has to know + which of the two situations it is in. */ + jobs.some((job) => job.status === 'running' || job.status === 'queued') ? ( +

+ Working on it — suggested + moments will appear here on their own. +

+ ) : ( +

Nothing suggested yet — run analysis.

+ ) ) : ( <>