diff --git a/DESIGN.md b/DESIGN.md index e1d99b45..175f7055 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1008,22 +1008,36 @@ gallery badges (#55) — and it **already exists, shipped and unit-tested**: outline; the class label renders only while selected, 11px / 700, anchored at the first vertex, never a pointer target. -**A model's work is marked; a person's is not.** `provenance: "model"` earns a mark on the -canvas label (`class · 62%`, and `class · model` where the model recorded no score) and a -`Sparkles` glyph with the score on the side-panel row, whose accessible name carries the -claim in words and whose tooltip carries the full `model_ref`. Never colour alone — class -colour is already user data and cannot also mean provenance. **Absence is the human case**: -no "manual" badge, no mark on the common path, because the row a reviewer sees a thousand -times is the one that must stay quiet. `import` provenance is unmarked until there is an -importer whose mark would mean something. - -**Confidence has one spelling, and it is whole percent.** `confidencePercent` in -`frontend/annotator/src/adapters/react/paint.ts` — the same shared-helper rule as -`classColor` directly above, and for the same reason: a live suggestion, a committed -annotation's canvas label and its panel row all show the same number, and two notations for -it is a number that disagrees with itself. Two decimals would claim a precision a -confidence does not have. A `null` confidence reads as absent — never as `0`, never as a -low score. +**The canvas label is part of what selection looks like.** A frame carrying forty boxes drew +forty class names over the picture at all times, which hides the asset behind the annotations +of it. The panel is the full inventory; the canvas answers *what is this one* for the shape +somebody picked, and an unselected shape is its box alone. This is also how a viewer's +selection reads, since a read-only frame paints no grips. + +**A model's work is marked; a person's is not.** `provenance: "model"` earns a `Sparkles` +glyph on the side-panel row, whose accessible name carries the claim in words and whose +tooltip carries the full `model_ref`. That glyph is the *only* provenance signal in the +editor — the canvas label is the class and nothing else. Never colour alone — class colour is +already user data and cannot also mean provenance. **Absence is the human case**: no "manual" +badge, no mark on the common path, because the row a reviewer sees a thousand times is the one +that must stay quiet. `import` provenance is unmarked until there is an importer whose mark +would mean something. + +**Confidence renders on the live suggestion preview and nowhere else in the editor.** The +number tells somebody whether to accept a proposal; once accepted, the shape is a label like +any other and the score is decoration on every subsequent reading of it. So no percentage on a +canvas box, on a panel row, or in a tooltip. It is not discarded — `confidence` and +`model_ref` are stored unchanged — and its home is the batch review loop, where a surface +showing it must also name *what it measures*: a point-prompted mask score and a detection's +prompt affinity are different quantities on different scales and cannot be pooled, thresholded +or sorted together. + +**Where it is shown, it has one spelling, and that is whole percent.** `confidencePercent` in +`frontend/annotator/src/adapters/react/paint.ts` — the same shared-helper rule as `classColor` +above: whoever shows the number imports it rather than respelling it, because two notations +for one quantity is a number that disagrees with itself. Two decimals would claim a precision +a confidence does not have. A `null` confidence reads as absent — never as `0`, never as a low +score. ## Libraries diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index df940d7a..bc93370b 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -128,6 +128,7 @@ import type { } from "react"; import { topmostAnnotationAt } from "../../core/geometry/hitTest"; +import { withinBounds } from "../../core/geometry/primitives"; import { assetTolerances } from "../../core/geometry/tolerance"; import { affordanceAt, viewerAffordanceAt } from "../../core/interaction/affordance"; import { transition } from "../../core/interaction/machine"; @@ -825,6 +826,20 @@ export function AnnotatorCanvas({ // A session with no handler swallows the press rather than falling through, // for the reason the prop's docstring gives. if (suggestion !== null) { + // The stage surround is not the asset. The pane spans the whole viewport + // on purpose (see the input surface's note below), so `point` here can be + // negative or past the edge — which a *drag* wants, because "make the box + // this big" survives leaving the picture, and which a prompt point cannot + // use: there is nothing under the margin for a segmenter to segment. So + // an out-of-frame press is dropped whole rather than clamped onto the + // edge, and dropped **here**, before the host is told: a point the host + // never hears about records no click, sends no request and moves no + // preview, which is one guarantee instead of three. + // + // Asked in asset pixels rather than in screen ones, so zoom and pan are + // already accounted for by `imagePoint` and there is no second transform + // to keep in step with the first. + if (!withinBounds(point, asset)) return; onSuggestPoint?.(point, event.altKey ? "negative" : "positive"); return; } diff --git a/frontend/annotator/src/adapters/react/Shapes.tsx b/frontend/annotator/src/adapters/react/Shapes.tsx index 95069494..4ef8c07e 100644 --- a/frontend/annotator/src/adapters/react/Shapes.tsx +++ b/frontend/annotator/src/adapters/react/Shapes.tsx @@ -64,7 +64,7 @@ import type { PolygonGeometry, PolylineGeometry, } from "../../core/types"; -import { confidencePercent, screenPx } from "./paint"; +import { screenPx } from "./paint"; import type { PaintedAnnotation } from "./paint"; /** Stroke thickness, in screen pixels, for an unselected shape. */ @@ -273,29 +273,16 @@ export function Vertices({ points, color, zoom, hotIndex }: { ); } -/** - * What a label says: the class, and for a model's work what the model claimed. - * - * A suffix rather than a second element, so the shipped label metrics are - * untouched — same size, same weight, same anchor, same lift. The label only - * gets longer, and only on a selected shape, which is the one place it renders - * at all. - * - * A model-produced annotation always carries the separator, because - * `confidence` is optional on one and a shape marked only when the model - * happened to score itself is not a mark a reviewer can trust the absence of. - * `import` provenance reads as a person's for now — the importers that would - * make that distinction mean something are not built. - */ -export function labelText(shape: PaintedAnnotation): string { - if (shape.provenance !== "model") return shape.labelClass; - const score = shape.confidence === null ? "model" : confidencePercent(shape.confidence); - return `${shape.labelClass} · ${score}`; -} - /** * The class name, above the shape. * + * **The class and nothing else.** A confidence is a decision aid for + * accept-or-reject and stops being one the moment a shape is accepted, so the + * number lives on the live suggestion preview and nowhere else in the editor; + * which model produced a stored shape is the side panel's mark. A canvas that + * wrote either would be answering, on every selected shape, a question a + * reviewer asks about one. + * * `y` is the anchor itself — a plain asset coordinate — and the screen-pixel lift * above it rides on the CSS `translate` property instead of being subtracted here. * That is what keeps every one of this element's attributes free of the zoom: @@ -318,7 +305,7 @@ export function ShapeLabel({ shape }: { readonly shape: PaintedAnnotation }): JS translate: "0 var(--vs-label-lift)", }} > - {labelText(shape)} + {shape.labelClass} ); } @@ -352,7 +339,14 @@ export function PolylineShape({ geometry, color, hot, selected }: { } /** - * One committed annotation: its shape, its label, and its grips when selected. + * One committed annotation: its shape, and — while it is selected — its label + * and its grips. + * + * **The label is part of what selection looks like.** A frame carrying forty + * boxes drew forty class names over the picture at all times, which hid the + * asset behind the annotations of it; the panel is the full inventory, and the + * canvas answers "what is *this* one" for the shape somebody picked. It is also + * how a viewer's selection reads, since a read-only frame paints no grips. * * Grips and vertices are drawn only for a selected shape, which mirrors * `resolveTarget` — it looks for a handle or a vertex only among the *selected* @@ -386,7 +380,7 @@ export function AnnotationShape({ shape, zoom, handles }: ShapeProps): JSX.Eleme selected={shape.selected} /> )} - + {shape.selected && } {handles && shape.selected && shape.geometry.type === "bbox" && ( )} diff --git a/frontend/annotator/src/adapters/react/paint.test.ts b/frontend/annotator/src/adapters/react/paint.test.ts index f7f1437b..f142c290 100644 --- a/frontend/annotator/src/adapters/react/paint.test.ts +++ b/frontend/annotator/src/adapters/react/paint.test.ts @@ -11,7 +11,6 @@ import { ASSET, SCHEMA, annotation } from "../../core/state/_sample"; import { createDocument } from "../../core/state/document"; import { EMPTY_SELECTION, selectionOf } from "../../core/state/selection"; import type { Annotation } from "../../core/types"; -import { labelText } from "./Shapes"; import { SUGGESTION_DASH, SUGGESTION_OPACITY, @@ -327,7 +326,15 @@ describe("a pending suggestion, drawn as a proposal (#424)", () => { }); }); -describe("what a model produced, projected so the label can say so", () => { +/** + * Where a model's confidence lives, and where the draw list refuses to carry it. + * + * The preview shows the number because it is what an accept-or-reject decision + * is made on; a committed annotation does not, because by then the decision has + * been made. The projection is what enforces the second half: a renderer cannot + * write a score it was never given. + */ +describe("a committed annotation's confidence is not the canvas's business", () => { function predicted(overrides: Partial = {}): Annotation { return { ...annotation("m1"), @@ -349,45 +356,39 @@ describe("what a model produced, projected so the label can say so", () => { return painted; } - it("carries both fields onto the draw list", () => { - const shape = paintOne(predicted()); - expect(shape.provenance).toBe("model"); - expect(shape.confidence).toBe(0.62); - }); - - it("carries a person's own values too, so the label can tell them apart", () => { - const shape = paintOne(annotation("h1")); - expect(shape.provenance).toBe("human"); - expect(shape.confidence).toBeNull(); - }); - - it("writes the model's score beside the class, at the shipped spelling", () => { - expect(labelText(paintOne(predicted()))).toBe("sign · 62%"); + it("hands the renderer neither the score nor who produced it", () => { + // Absence in the projection, not a rule in the component: a `` that + // has never been given a confidence cannot start writing one. + const shape: Record = { ...paintOne(predicted()) }; + expect("confidence" in shape).toBe(false); + expect("provenance" in shape).toBe(false); }); - it("still marks a model's work when the model reported no score", () => { - // `confidence` is optional on a model-produced annotation, and a mark that - // appeared only when a model happened to score itself is a mark whose - // absence says nothing. - expect(labelText(paintOne(predicted({ confidence: null })))).toBe("sign · model"); + it("paints a model's shape exactly as it paints a person's", () => { + // Every field the two share, and there is now nothing else: the canvas + // cannot tell them apart, which is the point. + const mine = paintOne(annotation("m1")); + const theirs = paintOne(predicted()); + expect(theirs).toEqual({ ...mine, id: theirs.id }); }); - it("leaves a person's label exactly as it shipped", () => { - expect(labelText(paintOne(annotation("h1")))).toBe("sign"); - }); - - it("leaves an imported label alone, having no mark that would mean anything", () => { - const imported = predicted({ provenance: "import", model_ref: null, confidence: null }); - expect(labelText(paintOne(imported))).toBe("sign"); + it("keeps the number on the live preview, which is where the decision is", () => { + // The one surface in the editor that shows it, and `paintSuggestion` is + // what puts it there — see the `#424` block above for the full range. + const asked = withPoint(armed("sign"), [100, 120], "positive"); + const preview = answered(asked, asked.serial, { + geometry: { type: "bbox", x: 10, y: 20, width: 30, height: 40 }, + confidence: 0.62, + modelRef: "facebook/sam2-hiera-base-plus@main", + }); + const declared = SCHEMA.classes.find((one) => one.name === "sign"); + expect(paintSuggestion(preview, declared)?.label).toBe("sign 62%"); }); - it("spells the score the way the suggestion overlay already does", () => { - // One quantity, one spelling. `confidenceLabel` is the canvas's live - // suggestion; `labelText` is the committed annotation; a reader looking at - // both at once must not see two notations for the same number. - expect(labelText(paintOne(predicted({ confidence: 0.87 })))).toContain( - confidencePercent(0.87), - ); - expect(confidenceLabel("sign", 0.87)).toContain(confidencePercent(0.87)); + it("spells it one way, so no second surface can disagree with the first", () => { + // `confidencePercent` is exported for this: whoever shows the number + // imports it. Two `Math.round`s would be the same quantity in two + // notations, which is the defect the shared helper exists to prevent. + expect(confidenceLabel("sign", 0.87)).toBe(`sign ${confidencePercent(0.87)}`); }); }); diff --git a/frontend/annotator/src/adapters/react/paint.ts b/frontend/annotator/src/adapters/react/paint.ts index cd94514f..ed3d9f4d 100644 --- a/frontend/annotator/src/adapters/react/paint.ts +++ b/frontend/annotator/src/adapters/react/paint.ts @@ -53,7 +53,6 @@ import type { Point, PolygonGeometry, PolylineGeometry, - Provenance, } from "../../core/types"; /** A shape whose class the schema declares, ready to draw. */ @@ -66,17 +65,6 @@ export interface PaintedAnnotation { /** Under the pointer, or held by the drag in flight. */ readonly hot: boolean; readonly color: string; - /** - * Who drew it, projected so the label can say so. - * - * Carried rather than derived at the label because the draw list is where a - * renderer's questions are already answered — a `` that reached back - * into the document for provenance would be the one component that needs a - * document, and the only reason for it would be four characters of suffix. - */ - readonly provenance: Provenance; - /** How sure the model was, and `null` for every label a person drew. */ - readonly confidence: number | null; } /** @@ -188,8 +176,6 @@ function painted( selected: selection.has(annotation.id), hot: annotation.id === hotId, color: classColor(declared, annotation.label_class), - provenance: annotation.provenance, - confidence: annotation.confidence, }; } @@ -328,9 +314,17 @@ export function paintSuggestion( * have. A confidence outside `[0, 1]` cannot arrive — the kernel's * `PredictedRegion` refuses one — so nothing is clamped here. * - * Exported for the same reason `classColor` is: the panel shows the same - * quantity as the canvas, and a second spelling of it in `ui-core` would be a - * number that disagrees with itself across two surfaces. + * **One consumer today** — the live suggestion preview, through + * `confidenceLabel`. Exported anyway, for the reason `classColor` is exported: + * this is the spelling, and the surfaces that will show a confidence next are + * outside this package. A second `Math.round` in `ui-core` would be the same + * number disagreeing with itself across two screens, which is the defect the + * shared helper exists to make impossible rather than merely unlikely. + * + * The editor deliberately shows it nowhere else. A confidence helps somebody + * decide whether to accept a proposal; once accepted, the shape is a label like + * any other, and the number would be decoration on every subsequent reading of + * it. */ export function confidencePercent(confidence: number): string { return `${Math.round(confidence * 100)}%`; diff --git a/frontend/annotator/src/core/geometry/primitives.test.ts b/frontend/annotator/src/core/geometry/primitives.test.ts index df788970..8906c2aa 100644 --- a/frontend/annotator/src/core/geometry/primitives.test.ts +++ b/frontend/annotator/src/core/geometry/primitives.test.ts @@ -1,6 +1,6 @@ /** - * The four primitives, and the one claim about `Bounds` a reader should not have - * to take on faith: that an `AssetDescriptor` is already a frame. + * The primitives, and the one claim about `Bounds` a reader should not have to + * take on faith: that an `AssetDescriptor` is already a frame. */ import { describe, expect, it } from "vitest"; @@ -10,6 +10,7 @@ import { clampPoint, closestPointOnSegment, distance, + withinBounds, type Bounds, } from "./primitives"; @@ -58,6 +59,46 @@ describe("a point pushed back inside the frame", () => { }); }); +/** + * The other question about a frame: not *where does this point go* but *was it + * in there at all*. A caller that must not salvage a stray coordinate asks this + * one — see the docstring for why the two exist side by side. + */ +describe("whether a point is inside the frame", () => { + const frame: Bounds = { width: 100, height: 50 }; + + it("says yes for a point in the middle", () => { + expect(withinBounds([40, 20], frame)).toBe(true); + }); + + it("counts the edges and the corners as inside, exactly as the clamp does", () => { + // The same range, stated twice: a point `clampPoint` would leave untouched + // is a point this must call inside, or the last row of pixels becomes a + // place where a press silently stops working. + for (const corner of [[0, 0], [100, 0], [0, 50], [100, 50]] as const) { + expect(withinBounds(corner, frame)).toBe(true); + expect(clampPoint(corner, frame)).toEqual([...corner]); + } + }); + + it("says no past each of the four edges, one axis at a time", () => { + expect(withinBounds([-0.001, 20], frame)).toBe(false); + expect(withinBounds([100.001, 20], frame)).toBe(false); + expect(withinBounds([40, -0.001], frame)).toBe(false); + expect(withinBounds([40, 50.001], frame)).toBe(false); + }); + + it("says no for a coordinate that is not a number", () => { + expect(withinBounds([Number.NaN, 20], frame)).toBe(false); + expect(withinBounds([40, Number.NaN], frame)).toBe(false); + }); + + it("takes an asset descriptor as its frame, like everything else here", () => { + expect(withinBounds([ASSET.width, ASSET.height], ASSET)).toBe(true); + expect(withinBounds([ASSET.width + 1, ASSET.height], ASSET)).toBe(false); + }); +}); + describe("a frame is a width and a height", () => { it("accepts an asset descriptor without conversion", () => { // The assignment is the assertion: if `AssetDescriptor` ever stopped diff --git a/frontend/annotator/src/core/geometry/primitives.ts b/frontend/annotator/src/core/geometry/primitives.ts index 8616672a..ce7e9fa8 100644 --- a/frontend/annotator/src/core/geometry/primitives.ts +++ b/frontend/annotator/src/core/geometry/primitives.ts @@ -79,3 +79,26 @@ export function closestPointOnSegment(p: Point, a: Point, b: Point): Point { export function clampPoint(p: Point, bounds: Bounds): Point { return [clamp(p[0], 0, bounds.width), clamp(p[1], 0, bounds.height)]; } + +/** + * Whether `p` is inside the frame at all — the question `clampPoint` answers by + * moving the point instead. + * + * The two are for different callers, and the difference is whether a stray + * coordinate is *work to be salvaged* or *an instruction that was never given*. + * A drag that left the picture still means "make the box this big", so it + * clamps; a click in the margin around the picture is not a click on anything, + * and clamping it would put a prompt point on the asset's edge that nobody + * placed there. + * + * Inclusive at both ends, matching `clampPoint`'s own range: the last row of + * pixels is part of the asset, and a rule that excluded `width` would make the + * edge a place where a press silently stopped working. + * + * `false` for a non-finite coordinate, which falls out of the comparisons rather + * than being tested for — the module note above says why nothing here guards + * against `NaN`, and here the honest answer for one happens to be "not inside". + */ +export function withinBounds(p: Point, bounds: Bounds): boolean { + return p[0] >= 0 && p[0] <= bounds.width && p[1] >= 0 && p[1] <= bounds.height; +} diff --git a/frontend/annotator/src/index.ts b/frontend/annotator/src/index.ts index 1201a12b..2242f684 100644 --- a/frontend/annotator/src/index.ts +++ b/frontend/annotator/src/index.ts @@ -63,6 +63,7 @@ export { clampPoint, closestPointOnSegment, distance, + withinBounds, type Bounds, } from "./core/geometry/primitives"; export { diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index ccf55fee..2b1b2b5b 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -160,6 +160,32 @@ function schemaOfSize(size: SchemaSize | undefined): typeof SCHEMA { }; } +/** + * A workspace with a segmenter in it, for the one scenario that needs the + * suggest tool to actually work. + * + * Off by default, because the interesting answer for every other test here is + * the empty list — that is the state the tool's explanation panel exists for, + * and it is what a workspace that has never been to the Inference section is in. + */ +const READY_SAM = { + id: "66666666-6666-4666-8666-666666666666", + name: "local sam", + connection_type: "local", + model_id: "facebook/sam2-hiera-base-plus", + model_revision: "main", + device: "cuda", + precision: "fp16", + endpoint_url: null, + setup_state: "ready", + allowed_actions: [], + capabilities: ["point_suggest"], + download: null, + integrity_check: null, + created_at: "2026-08-08T00:00:00Z", + updated_at: "2026-08-08T00:00:00Z", +}; + async function serveApi( page: Page, sent: Request[], @@ -167,6 +193,7 @@ async function serveApi( lifecycle: Lifecycle = openedWorld(), size?: SchemaSize, seeded: readonly Record[] = [], + suggestible = false, ): Promise { const stored: Record[] = [...seeded]; const batchBody = (): Record => ({ @@ -340,7 +367,23 @@ async function serveApi( // here: it is the state the panel's explanation exists for, and it is what a workspace // that has never been to the Inference section is in. if (path === "/inference/connections") { - return route.fulfill({ json: { items: [], total: 0 } }); + const items = suggestible ? [READY_SAM] : []; + return route.fulfill({ json: { items, total: items.length } }); + } + if (path === "/inference/suggest" && request.method() === "POST") { + // `SuggestionOut`: a `model_ref` and a `region` that wraps the geometry + // beside the score. A flatter shape is refused by the generated runtime + // check, which reads as "the server answered something this app does not + // recognise" and looks nothing like a stub bug. + return route.fulfill({ + json: { + model_ref: "facebook/sam2-hiera-base-plus@main", + region: { + geometry: { type: "bbox", x: 100, y: 100, width: 80, height: 60 }, + confidence: 0.91, + }, + }, + }); } return route.fulfill({ status: 500, json: { code: "NO_STUB", message: path } }); }); @@ -353,8 +396,9 @@ async function openJob( lifecycle?: Lifecycle, size?: SchemaSize, seeded?: readonly Record[], + suggestible?: boolean, ): Promise { - await serveApi(page, sent, progress, lifecycle, size, seeded); + await serveApi(page, sent, progress, lifecycle, size, seeded, suggestible); await page.goto(`/jobs/${JOB}`); await page.getByTestId("token-input").fill("a-token"); await page.getByTestId("token-submit").click(); @@ -2722,6 +2766,60 @@ test("the no-connection panel now has somewhere to send you (#424 D6)", async ({ await expect(page.getByTestId("inference-screen")).toBeVisible(); }); +/** + * The stage surround is not the asset — and this claim needs a browser, because + * jsdom has no surround. + * + * The pane spans the whole stage while the picture is fitted inside it with + * padding, so there is a margin around the image that is still the input + * surface. In jsdom every rectangle is zero, so that margin does not exist and a + * component test asserting anything about it would be asserting about nothing. + * Here the two rectangles are measured and the press is put between them. + * + * Run at a **non-default zoom**, because the rule has to be applied in asset + * pixels: one written against screen coordinates would pass at the fitted scale + * and refuse half the picture at any other. + */ +test("a suggest click in the margin around the picture asks nothing, at any zoom", async ({ + page, +}) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + + await page.getByTestId("tool-suggest").click(); + await expect(page.getByTestId("suggest-idle")).toBeVisible(); + + // Off the fitted scale, so nothing below can be true by accident of zoom 1. + await page.getByTestId("zoom-in").click(); + await expect(page.getByTestId("zoom-readout")).not.toHaveText("100%"); + + const asks = (): Request[] => + sent.filter((r) => r.method() === "POST" && r.url().endsWith("/inference/suggest")); + + // The `` is laid out at the asset's own size inside the scaled wrapper, + // so its box on screen *is* the picture's rectangle. + const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; + const pane = (await page.getByTestId("annotator-pane").boundingBox())!; + // The fit leaves room on at least one axis, and this test is meaningless + // without it — so it is asserted rather than assumed. + expect(picture.x).toBeGreaterThan(pane.x + 2); + + // Left of the picture and inside the pane: on the stage, off the asset. + await page.mouse.click((pane.x + picture.x) / 2, picture.y + picture.height / 2); + // Nothing to wait for, so the absence is given a chance to be wrong: a press + // that *did* ask would have its request logged well inside this. + await expect(page.getByTestId("suggest-idle")).toBeVisible(); + expect(asks()).toHaveLength(0); + await expect(page.getByTestId("suggestion-shape")).toHaveCount(0); + + // The same gesture on the picture itself, so the absence above is a rule and + // not a broken fixture. + await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); + + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); + expect(asks()).toHaveLength(1); +}); + /** * The notice surface, measured — which is the only way any of it can be claimed. * diff --git a/frontend/app/e2e/perf.spec.ts b/frontend/app/e2e/perf.spec.ts index 7f580d57..ba9e6412 100644 --- a/frontend/app/e2e/perf.spec.ts +++ b/frontend/app/e2e/perf.spec.ts @@ -107,14 +107,19 @@ test("the benchmark scene is 220 annotations, and the committed layer is one gro // What 220 annotations cost in SVG elements, itemised. A deliberate rendering // change moves these numbers and should; an accidental one — a wrapper added // per shape, a label drawn twice — shows up here rather than in a frame time - // nobody ran. Three elements per annotation is also the number the zoom - // scenario below multiplies by. + // nobody ran. + // + // **No labels, because nothing is selected.** The class label is part of what + // selection looks like, so a frame nobody has picked a shape on draws two + // elements per annotation rather than three: 660 → 440, measured. That is a + // legibility decision that happens to pay here — the `` was the most + // expensive of the three, carrying a stroke, a paint order and a translate. expect(nodes).toEqual({ - all: 660, + all: 440, groups: BENCH_ANNOTATIONS, rects: 200, polygons: 20, - labels: BENCH_ANNOTATIONS, + labels: 0, }); }); diff --git a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx index 0fa591bf..dd73d104 100644 --- a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx +++ b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx @@ -72,7 +72,6 @@ import { annotationsInDrawOrder, classNamed, - confidencePercent, hotkeyForClass, isTaggableClass, randomUuid, @@ -383,7 +382,7 @@ function TagStrip({ } /** - * What a model produced, and how sure it was — on the rows that have one. + * That a model produced this one — on the rows that have one, and nothing more. * * Renders nothing at all for a person's work. That is the whole of the design: * accepting a predicted box is the act that most needs the reviewer to know what @@ -391,6 +390,14 @@ function TagStrip({ * earns no badge. Absence is the human case, so the row a reviewer sees a * thousand times is exactly the row that shipped. * + * **The confidence is deliberately not here.** It is a decision aid for + * accept-or-reject and it stops being one once the shape is accepted; a column + * of percentages down a settled list invites a reviewer to sort by a number + * that means different things per capability — a point-prompted mask score and + * a detection's prompt affinity are not the same quantity. Where the number + * does belong is the batch review loop, and it will be named there. In the + * editor it appears on the live suggestion preview alone. + * * Never colour alone: the glyph carries it, and the accessible name says it in * words, so neither a monochrome screen nor a screen reader depends on the * muted foreground this is tinted with. @@ -408,32 +415,17 @@ function ModelMark({ readonly index: number; }): JSX.Element | null { if (annotation.provenance !== "model") return null; - // `confidencePercent`, not a local `toFixed` — the canvas writes the same - // quantity beside the same annotation, and DESIGN.md's rule for `classColor` - // is the rule here too: ui-core imports the answer, it does not respell it. - const score = annotation.confidence === null ? null : confidencePercent(annotation.confidence); return ( {/* The full reference, which is far too long for the row and is exactly diff --git a/frontend/ui-core/src/annotator/canvasLabel.test.tsx b/frontend/ui-core/src/annotator/canvasLabel.test.tsx new file mode 100644 index 00000000..2c78617f --- /dev/null +++ b/frontend/ui-core/src/annotator/canvasLabel.test.tsx @@ -0,0 +1,156 @@ +/** + * What the canvas writes over a shape, and when. + * + * Two rules, one element. The label renders **only while its shape is + * selected** — a frame of forty boxes drawing forty class names hides the asset + * behind the annotations of it, and the panel is the full inventory. And it + * says **the class and nothing else** — a confidence belongs to the + * accept-or-reject decision on the live preview, and which model produced a + * stored shape is the panel row's mark. + * + * Here rather than in `@visionset/annotator`, which has no DOM to render into: + * its suites are pure by construction, and this is a claim about markup. It is + * jsdom-complete — the presence or absence of a `` node is not something + * a browser knows better — so `AnnotationLayer` is driven directly rather than + * through the page. The label's *metrics* are the browser's business and are + * unchanged by this file. + */ + +import { + AnnotationLayer, + EMPTY_SELECTION, + documentFromWire, + selectionOf, +} from "@visionset/annotator"; +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; +import type { JSX } from "react"; +import type { Selection } from "@visionset/annotator"; + +const SCHEMA = { + project_id: "11111111-1111-4111-8111-111111111111", + version: 1, + classes: [ + { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + ], +}; + +function annotation(id: string, labelClass: string, overrides: Record = {}): unknown { + return { + id, + asset_id: "asset-1", + label_class: labelClass, + schema_version: 1, + geometry: { type: "bbox", x: 10, y: 10, width: 20, height: 20 }, + attributes: {}, + provenance: "human", + model_ref: null, + confidence: null, + job_id: null, + ...overrides, + }; +} + +/** A model's own work: the three fields the accept path stamps together. */ +function predicted(id: string, labelClass: string, confidence: number | null = 0.62): unknown { + return annotation(id, labelClass, { + provenance: "model", + model_ref: "IDEA-Research/grounding-dino-tiny@abc123", + confidence, + }); +} + +function paint(annotations: readonly unknown[], selection: Selection = EMPTY_SELECTION): JSX.Element { + const document = documentFromWire({ + asset: { id: "asset-1", width: 100, height: 100 }, + schema: SCHEMA, + annotations, + }); + return ( + + + + ); +} + +/** Every class name the canvas is currently writing. */ +function labels(): readonly string[] { + return [...screen.getByTestId("annotation-layer").querySelectorAll("text")].map( + (node) => node.textContent ?? "", + ); +} + +describe("the class label is part of what selection looks like", () => { + it("writes nothing over an unselected shape", () => { + render(paint([annotation("a", "vehicle")])); + + // The box is drawn; the name over it is not. + expect(screen.getByTestId("annotation-layer").querySelector("rect")).not.toBeNull(); + expect(labels()).toEqual([]); + }); + + it("writes the class over the shape somebody picked", () => { + render(paint([annotation("a", "vehicle")], selectionOf(["a"]))); + + expect(labels()).toEqual(["vehicle"]); + }); + + it("names only the selection, in a frame carrying several shapes", () => { + render( + paint( + [ + annotation("a", "vehicle"), + annotation("b", "pedestrian"), + annotation("c", "vehicle"), + ], + selectionOf(["b"]), + ), + ); + + // One name on a canvas of three, which is the whole of the rule: the panel + // is the inventory and the canvas answers "what is *this* one". + expect(labels()).toEqual(["pedestrian"]); + }); + + it("names each of several selected shapes", () => { + render( + paint([annotation("a", "vehicle"), annotation("b", "pedestrian")], selectionOf(["a", "b"])), + ); + + expect(labels()).toEqual(["vehicle", "pedestrian"]); + }); +}); + +describe("the label is the class and nothing else", () => { + it("writes no score over a model's shape", () => { + render(paint([predicted("m", "vehicle")], selectionOf(["m"]))); + + // The number aided a decision that has already been made. It is still + // stored, and it still renders on the live suggestion preview. + expect(labels()).toEqual(["vehicle"]); + }); + + it("writes no provenance mark either — the panel row carries that", () => { + render(paint([predicted("m", "vehicle", null)], selectionOf(["m"]))); + + expect(labels()).toEqual(["vehicle"]); + }); + + it("draws a model's selected shape exactly as it draws a person's", () => { + const { container: mine } = render(paint([annotation("h", "vehicle")], selectionOf(["h"]))); + const person = mine.innerHTML; + const { container: theirs } = render(paint([predicted("h", "vehicle")], selectionOf(["h"]))); + + // Same id, same class, same geometry: the only difference in the fixture is + // the three fields the canvas has stopped being given. + expect(theirs.innerHTML).toBe(person); + }); +}); diff --git a/frontend/ui-core/src/annotator/panel.test.tsx b/frontend/ui-core/src/annotator/panel.test.tsx index 8d441940..3e9bd24c 100644 --- a/frontend/ui-core/src/annotator/panel.test.tsx +++ b/frontend/ui-core/src/annotator/panel.test.tsx @@ -486,11 +486,10 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { }; } - it("marks it, and says how sure the model was", () => { + it("marks it — that a model drew it, and no more than that", () => { render(mount(storeWith([predicted("m")]))); expect(screen.getByTestId("object-model-0")).toBeDefined(); - expect(screen.getByTestId("object-confidence-0").textContent).toBe("62%"); }); it("says in words what the glyph says in a picture", () => { @@ -499,26 +498,38 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { render(mount(storeWith([predicted("m")]))); expect(screen.getByTestId("object-model-0").getAttribute("aria-label")).toBe( - "Model-produced by IDEA-Research/grounding-dino-tiny@abc123, confidence 62%", + "Model-produced by IDEA-Research/grounding-dino-tiny@abc123", ); }); + it("shows no confidence anywhere on the row, nor in the name it announces", () => { + // The number is an accept-or-reject aid and this row is past that decision. + // Asserted on the whole row rather than on the element that used to hold + // it, so a percentage reappearing anywhere in it turns this red. + render(mount(storeWith([predicted("m")]))); + + const row = screen.getByTestId("object-row-0"); + expect(row.textContent).not.toContain("%"); + expect(row.textContent).not.toContain("62"); + expect(screen.getByTestId("object-model-0").getAttribute("aria-label")).not.toContain("%"); + }); + it("puts nothing at all on a label a person drew", () => { // The common path stays exactly as it shipped — absence is the human case, // so a reviewer's thousandth row gains no badge and no noise. render(mount(storeWith([annotation("h", "vehicle", "bbox")]))); expect(screen.queryByTestId("object-model-0")).toBeNull(); - expect(screen.queryByTestId("object-confidence-0")).toBeNull(); }); - it("still marks the model's work when no score was recorded", () => { + it("marks the model's work the same whether or not a score was recorded", () => { + // `confidence` is optional on a model-produced annotation, and the mark no + // longer depends on it at all — which is what makes the mark's absence mean + // "a person drew this" rather than "the model did not score itself". render(mount(storeWith([predicted("m", { confidence: null })]))); expect(screen.getByTestId("object-model-0")).toBeDefined(); - // Absent reads as absent — never as a zero, and never as a low score. - expect(screen.queryByTestId("object-confidence-0")).toBeNull(); - expect(screen.getByTestId("object-model-0").textContent).not.toContain("0"); + expect(screen.getByTestId("object-row-0").textContent).not.toContain("0"); }); it("marks nothing for an imported label, which has no model to name", () => { @@ -550,6 +561,7 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { ); expect(screen.queryByTestId("object-model-0")).toBeNull(); - expect(screen.getByTestId("object-confidence-1").textContent).toBe("41%"); + expect(screen.getByTestId("object-model-1")).toBeDefined(); + expect(screen.getByTestId("object-row-1").textContent).not.toContain("41"); }); }); diff --git a/frontend/ui-core/src/annotator/suggestFlow.test.tsx b/frontend/ui-core/src/annotator/suggestFlow.test.tsx index 69b0a84e..5fef5cc2 100644 --- a/frontend/ui-core/src/annotator/suggestFlow.test.tsx +++ b/frontend/ui-core/src/annotator/suggestFlow.test.tsx @@ -268,17 +268,29 @@ async function arm(): Promise { await screen.findByTestId("suggest-panel"); } -/** One press on the canvas. `alt` is D2's negative point. */ -function clickCanvas(alt = false): void { +/** + * One press on the canvas, at a client position. + * + * jsdom's rect is all zeros and the initial view is identity, so a client + * coordinate here **is** an asset coordinate — which is what lets a test place a + * press inside or outside a 640 × 480 frame without any layout. `alt` is D2's + * negative point. + */ +function clickCanvasAt(clientX: number, clientY: number, alt = false): void { fireEvent.pointerDown(screen.getByTestId("annotator-pane"), { button: 0, - clientX: 100, - clientY: 100, + clientX, + clientY, altKey: alt, pointerId: 1, }); } +/** One press well inside the asset. `alt` is D2's negative point. */ +function clickCanvas(alt = false): void { + clickCanvasAt(100, 100, alt); +} + /** Every suggest request that has left, newest last. */ function asks(): readonly Record[] { return sent @@ -488,6 +500,104 @@ describe("a click asks the model", () => { }); }); +/** + * The stage surround is not the asset. + * + * The pane spans the whole viewport on purpose, so a press in the margin around + * the picture reaches the adapter with a coordinate outside the frame. A *drag* + * wants that — "make the box this big" survives leaving the picture. A prompt + * point cannot use it: there is nothing under the margin to segment. + * + * Before this, such a press was a prompt like any other. Measured on the fixture + * below: a click at (900, 700) on a 640 × 480 asset sent + * `positive: [{"x":900,"y":700}]`, painted a dot there, and drew whatever came + * back. + */ +describe("a press outside the asset is not a prompt", () => { + it("sends nothing and shows nothing", async () => { + await open(); + await arm(); + + clickCanvasAt(900, 700); + + // Waited on rather than asserted immediately, so this cannot pass merely by + // reading the log before a request had time to leave. + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + expect(screen.queryByTestId("suggestion-shape")).toBeNull(); + }); + + it("swallows a negative press the same way", async () => { + await open(); + await arm(); + + clickCanvasAt(-5, 200, true); + + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + }); + + it("records no point, so the next real click is still the first one", async () => { + await open(); + await arm(); + clickCanvasAt(900, 700); + clickCanvasAt(320, -40); + + clickCanvas(); + + await waitFor(() => expect(asks()).toHaveLength(1)); + // One point, not three. A dropped press that had quietly accumulated would + // send the model two coordinates nobody clicked on. + expect(asks()[0]["positive"]).toHaveLength(1); + expect(asks()[0]["negative"]).toEqual([]); + }); + + it("leaves a preview that is already showing exactly where it is", async () => { + await open(); + await arm(); + clickCanvas(); + await screen.findByTestId("suggestion-shape"); + + clickCanvasAt(900, 700); + + // Not a refine, and not a discard either: nothing happened at all, so the + // shape a person was about to accept is still there and still acceptable. + expect(screen.getByTestId("suggestion-shape")).toBeTruthy(); + expect(asks()).toHaveLength(1); + }); + + it("counts the asset's own edge as inside", async () => { + await open(); + await arm(); + + // The last row of pixels is part of the picture; a rule that excluded it + // would make the border a place where a press silently stopped working. + clickCanvasAt(640, 480); + + await waitFor(() => expect(asks()).toHaveLength(1)); + }); + + it("asks in asset pixels, so the same screen position changes answer under zoom", async () => { + await open(); + await arm(); + + // Outside at 1x: the asset is 640 wide and this is 700. + clickCanvasAt(700, 100); + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + + // The same screen position, one zoom step in. jsdom's pane measures zero, so + // zooming about its centre leaves the pan at the origin and 700 screen + // pixels is now 560 asset pixels — inside. A bounds check written against + // screen coordinates would still refuse it. + await userEvent.click(screen.getByTestId("zoom-in")); + clickCanvasAt(700, 100); + + await waitFor(() => expect(asks()).toHaveLength(1)); + expect(asks()[0]["positive"]).toEqual([{ x: 560, y: 80 }]); + }); +}); + describe("the preview is outside the document and outside the history", () => { /** * **The mutation test for D4.** Turn a suggestion into a `stage` or an `add`