diff --git a/DESIGN.md b/DESIGN.md index f8fd344..f440c4f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -678,21 +678,18 @@ inside the window, so the delay suppressed nothing and bought a second state to threshold that hides a state needs evidence that the state occurs**, and the place to get that is a real machine rather than an argument about perception. -The worked example is the annotation editor's suggest tool (`docs/ui.md`): a ring at the -click point from the moment the request is dispatched, and past 1.5s a sentence saying that -the first click on a frame is the slow one. Both surfaces reporting that wait read one -clock, in the component that owns the request — two clocks are free to drift, and a card and -a canvas disagreeing about whether something is happening is worse than either alone. Where -that clock is read through React state, the surfaces derive *is a request out* directly -rather than waiting for the clock's own announcement to arrive: an effect's round trip is a -delay like any other, and it is the one that reintroduces the suppression nobody wanted. - -**A moving indicator has a still form, and a person who asked for one gets it.** This is the -product's first `prefers-reduced-motion` handling and the rule it sets is that the -*information survives the preference*: the still form is the same shape at the same weight, -it simply does not move. Removing the indicator instead would answer a request about motion -by withholding a fact, which is not what was asked. Nothing in the interface may be legible -only while it is animating. +The worked example is the annotation editor's suggest tool (`docs/ui.md`): the panel says +`Looking at that…` from the moment the request is dispatched, and past 1.5s adds a sentence +saying that the first click on a frame is the slow one. + +**A wait is reported in one place, and never at the cursor** (#557, superseding #541's +second surface). The suggest tool also drew a ring at the click point and put a busy cursor +on the canvas; both were removed. An indicator sitting on the picture, next to the pointer, +reads as the machine having *seized* rather than as work in progress — it is in the way of +the thing being looked at, and it moves with the hand. The card is out of the way, says the +same thing in words, and is where every other answer about this tool already appears. Two +surfaces reporting one wait also had to share one clock to avoid disagreeing; one surface +has nothing to disagree with. **An indicator is never coral.** The brand's two sites are above, and a spinner is not a third — a thing that appears on every slow click would spend more of the brand than the @@ -1154,9 +1151,17 @@ gallery badges (#55) — and it **already exists, shipped and unit-tested**: and the caller shows a neutral for that case alone. #162: binding such an input to the *stored* colour rendered every derived class grey beside a dot showing the real one. - v1's shape metrics, kept as the reference: stroke width 2, selected 3; vertices render - only while selected, radius 5 (7 when the vertex itself is selected), with a 2px white - outline; the class label renders only while selected, 11px / 700, anchored at the - first vertex, never a pointer target. + **while selected, or while the shape is a suggestion preview**, radius 5 (7 when the + vertex itself is selected), with a 2px white outline; the class label renders only while + selected, 11px / 700, anchored at the first vertex, never a pointer target. + + **The preview is a third visual state, not a shape marked selected** (#557). Selection + carries the panel row, the delete key and the keyboard rules a proposal must not have, so + it gets its own rule rather than borrowing one. Its vertices are up the whole time it is + on screen, undecimated at every detail step, because where precision was gained or lost + *is* what the detail control is about — a live vertex count beside the slider and nothing + on the picture made it a control somebody moved blind. Its outline is dashed and an + accepted annotation's is solid, which is what tells proposed from committed at a glance. **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 diff --git a/docs/inference.md b/docs/inference.md index 8dc7bd1..c0f776f 100644 --- a/docs/inference.md +++ b/docs/inference.md @@ -288,8 +288,8 @@ POST /inference/suggest "contour": [[404.0, 221.0], …] } ], - "applied": {"detail": "balanced", "fill_holes": 0.002, "fragments": "one"}, - "parameters": ["detail", "fill_holes", "fragments"] + "applied": {"detail": "balanced"}, + "parameters": ["detail"] } ``` @@ -311,8 +311,7 @@ get that outline's extent; name a kind that holds no shape and `region` is `null kind your schema would refuse would hand you a suggestion that cannot be accepted. **An empty `regions` is a successful answer with nothing to propose** - a click on empty -background, a model less sure than you asked for, a shape too thin to be a polygon, or settings -that leave nothing. `model_ref` is still there, because it is what an accepted suggestion has to +background, a model less sure than you asked for, or a shape too thin to be a polygon. `model_ref` is still there, because it is what an accepted suggestion has to carry, and so is `parameters`, because a caller that adjusted its way into an empty answer needs the controls to adjust its way back out. @@ -320,7 +319,7 @@ the controls to adjust its way back out. mask; the pieces cut out of it are that same claim seen in parts, and a separate number for each would be precision nobody expressed. -## What happens to the mask, and the three things you can move +## What happens to the mask, and the one thing you can move A segmenter answers with a grid of booleans. Turning that into a polygon or a box is a fixed chain, and the whole of it happens here rather than inside whatever ran the model - so a second @@ -332,60 +331,75 @@ model. 3. **Tracing** the boundary of what is left. 4. **Simplifying** that boundary to a vertex count somebody can edit. -The geometry branch happens after the second step: a polygon class takes steps 3 and 4, a box -class takes the extent of what survived. A box therefore does not move when `detail` does. +The geometry branch happens after the second step: a polygon class takes steps 3 and 4 on the +piece you pointed at, a box class takes one extent over every piece that survived. A box +therefore does not move when `detail` does. | Setting | What it moves | Applies to | | --- | --- | --- | | `detail` | `coarse`, `balanced` or `fine` - how much of the outline survives | polygon | -| `fill_holes` | the widest gap closed, as a share of the piece's area | polygon | -| `fragments` | `one` piece - the one under your points - or `all` of them | polygon and box | -Every one is optional, and omitting all three gives what this route always gave: `balanced`, -a reach of two parts in a thousand, and the piece you pointed at. +It is optional, and omitting it gives what this route always gave: `balanced`. + +**Two settings used to be here and are not** (#557). How wide a gap gets closed and how many +pieces become shapes are still decided, at fixed defaults nobody asks for. As controls they +did nothing at all to the ordinary single clean mask - every position gave an identical +shape - so they read as knobs wired to nothing, and could only be got wrong on the unusual +one. Their value is in the default rather than in the choice. They come back as settings if +a real need for the choice appears. **The tolerance is relative, which is what makes one setting work everywhere.** It is a fraction of the region's own size rather than a pixel count, so it does the same thing to a thing eight pixels across and a thing eight hundred across, and `balanced` keeps a typical object in the 10-40 vertex range. -**`one` means the piece you pointed at, not the biggest one on the frame.** A mask routinely -carries more than one separate piece - a speck of antialiasing along an edge, a reflection, a -scrap of the same colour elsewhere - and which of them you meant is a question only the points can -answer. So the choice is made from the prompt: a point inside a piece picks that piece; several -points inside several pieces pick the largest of *those*, because two positives describe one -object rather than propose two; and a point inside none of them picks the piece nearest to it, -since a mask need not cover the exact pixel you clicked. Negative points never select - they say -what the shape is not, and a piece is chosen before its shape is known. +**Specks are dropped first, and a click never becomes a cleanup job.** A mask routinely carries +more than one separate piece - a scrap of antialiasing along an edge, a reflection, a patch of +the same colour elsewhere - and anything under a twentieth of the largest piece is discarded +before anything else looks at the mask. + +**A polygon is the piece you pointed at, not the biggest one on the frame.** Which of the +survivors you meant is a question only the points can answer, so the choice is made from the +prompt: a point inside a piece picks that piece; several points inside several pieces pick the +largest of *those*, because two positives describe one object rather than propose two; and a +point inside none of them picks the piece nearest to it, since a mask need not cover the exact +pixel you clicked. Negative points never select - they say what the shape is not, and a piece is +chosen before its shape is known. Picking whichever piece happened to own the topmost-leftmost lit pixel would be a different rule and a worse one: that is a fact about where the speckle fell, not about what you asked for. -`all` drops the question and proposes every piece at or above a twentieth of the largest one's -area. The floor is there because one click should not become a cleanup job. - -**`parameters` says which of them apply here**, for the kind of shape your `allowed_geometries` -will produce. A box has no outline, so `detail` and `fill_holes` have nothing to do to one and -are not named. A client renders what this lists and works none of it out for itself. - -**`fill_holes` closes gaps rather than filling enclosed holes**, and the distinction is worth -stating because the name suggests otherwise. Boundary tracing walks a shape's *outer* ring and a -polygon is one ring with no interior, so an enclosed hole is invisible to the answer - filling an -8x8 hole in a 20x20 square moves the mask and leaves the traced outline byte-identical. What the -setting does reach is the notches and bays a segmenter bites out of an edge, which are exactly -what makes an outline ragged. - -**`contour` is the unsimplified outline**, in the asset's own pixels, and it is there so a client -can re-run `detail` without asking again. It is the same points the server reduced, which matters: +**A box is one box over every surviving piece.** A point prompt means *this object*, and a mask +that arrives in several pieces is nearly always one object seen around an occlusion - a railing +across an animal, a post in front of a car. Both alternatives are wrong in exactly that case: +the largest piece alone cuts the object off at the occlusion, and a box per piece annotates one +thing twice. + +**`parameters` says which settings apply here**, for the kind of shape your `allowed_geometries` +will produce. A box has no outline, so `detail` has nothing to do to one and the list comes back +**empty** - which is how a client is told to offer no adjustments at all. A client renders what +this lists and works none of it out for itself. + +**Closing gaps is not filling enclosed holes**, and the distinction is worth stating because the +obvious reading is the other one. Boundary tracing walks a shape's *outer* ring and a polygon is +one ring with no interior, so an enclosed hole is invisible to the answer - filling an 8x8 hole +in a 20x20 square moves the mask and leaves the traced outline byte-identical. What the close +does reach is the notches and bays a segmenter bites out of an edge, which are exactly what +makes an outline ragged. Its reach grows with the piece and stops at a few pixels: past that, a +gap is a feature of the shape rather than an artefact of tracing it. + +**`contour` is the outline the shape was reduced from**, in the asset's own pixels, and it is +there so a client can re-run `detail` without asking again. It is the same points the server +reduced, which matters: simplification is not nested, so a client starting from anything else could not be held to the server's answer. A box carries none, because it is an extent rather than something reduced from anything. -**Accepting a plural answer is all of it or none.** Asking for every piece can propose several -shapes, and they are written together as one entry in the undo history. Accepting part of one is -real and is deliberately not here: it needs a selection the preview does not have. That is planned -growth rather than a gap nobody noticed, and it is tracked as *accepting part of a plural -suggestion* (#548). +**`regions` is a list that holds at most one shape today.** One click asks about one object, and +both geometries now answer with one. The plural shape is kept rather than collapsed because +accepting *part* of a plural proposal is real planned work - it needs a selection the preview +does not have - tracked as *accepting part of a plural suggestion* (#548). Where several are +proposed they are written together as one entry in the undo history. ### Nothing is written, and the first click is the slow one diff --git a/docs/ui.md b/docs/ui.md index 7ca56ac..3af4c6e 100644 --- a/docs/ui.md +++ b/docs/ui.md @@ -504,50 +504,59 @@ Every click sends **all** the points placed so far - the route is stateless - an the answer replaces the preview. The first click on a frame is the slow one, because the model reads the whole image once; refining after it is quick. -**A wait says so where you are looking, from the moment it starts.** A ring appears -around the click you are waiting on, the cursor picks up the busy arrow, and the -panel says `Looking at that…` - all of it on the same frame the request leaves. -Past a second and a half - long enough that this is plausibly the first click on -the frame - the panel adds the sentence explaining why that one is slow. Once the -ring is up it stays a quarter second even if the answer beats it, so a fast answer -cannot leave it on screen for two frames. +**A wait says so on the panel, and nowhere else.** The card reads +`Looking at that…` on the same frame the request leaves. Past a second and a half - +long enough that this is plausibly the first click on the frame - it adds the +sentence explaining why that one is slow. -Refining while an answer is still out is fine and is the ordinary way to work: the -ring moves to the newest click and the shape already on screen stays drawn, so you -keep the best answer so far while the next one is fetched. The panel goes back to -`Looking at that…` for the duration, which is why `Accept` is not offered until the -newer shape arrives. `Esc` takes the whole thing back at once - points, ring and -cursor - without waiting the quarter second out. A refusal clears the ring and -renders as prose on the panel, so nothing is ever left spinning over a request -that is over. +Nothing appears at the cursor or the click point. A ring and a busy cursor used to, +and both were taken away: sitting on the picture beside the pointer, they read as +the machine having seized rather than as work in progress, and they were in the way +of the thing being looked at. -Somebody whose system asks for reduced motion gets the same ring, drawn still. A -preference about motion is not a preference about being told what is happening. +Refining while an answer is still out is fine and is the ordinary way to work: the +shape already on screen stays drawn, so you keep the best answer so far while the +next one is fetched. The panel goes back to `Looking at that…` for the duration, +which is why `Accept` is not offered until the newer shape arrives. `Esc` takes the +whole thing back at once. A refusal replaces the card with prose, so nothing is ever +left saying a request is out when it is over. **The proposal is not an annotation until it is accepted.** It is drawn faintly -with a dashed outline, carries its class and the model's confidence beside it, and -is in neither the document nor the undo history. `Esc` is its undo. Switching -class, switching frames or leaving the page discards it, and nothing is written. +with a dashed outline - solid is what an accepted annotation gets - carries its +class and the model's confidence beside it, and is in neither the document nor the +undo history. `Esc` is its undo. Switching class, switching frames or leaving the +page discards it, and nothing is written. + +**Its vertices are drawn the whole time it is up**, which is what makes the detail +setting something you can see rather than a number that changes. A committed shape +shows its vertices only while it is selected; a proposal is not selected and shows +them anyway, because choosing how much outline to keep is exactly a question about +where the points are. **The shape can be adjusted before it is accepted**, from a section inside the same card - never a second panel over the picture, which would cover the thing -being adjusted. It is closed until you ask for it, because the defaults are right +being adjusted. It is closed until you ask for it, because the default is right most of the time. -Three settings, and which of them appear is the server's answer rather than the -editor's guess (`docs/inference.md`). **Detail** is three steps with a live count -of the vertices beside it, and `[` and `]` move it without opening anything - -that one costs no request at all, because the answer carried the full outline and -the editor re-simplifies it here. **Close gaps** smooths the notches a segmenter -bites out of an edge. **Every separate piece** proposes each part of the mask -rather than only the one you pointed at - which is what it offers otherwise, and -it is chosen from your clicks rather than by size, so a stray speck elsewhere on -the frame never wins. Those last two change what the model's mask -*is*, so they ask again - the frame is already read, so it is quick. - -On a class that stores a box only the last of the three appears, because the -other two change an outline and a box has none. The editor does not know that; -the answer says so. +One setting, and whether it appears is the server's answer rather than the +editor's guess (`docs/inference.md`). **Detail** is a three-position slider - +coarse, balanced, fine - with a label beside it naming the step and what it costs, +`Fine · 41 pts`. `[` and `]` move it without opening anything. Either way it costs +no request at all: the answer carried the outline it was reduced from, and the +editor re-simplifies it here, so the shape and its vertices move under a held key. + +Pressing the slider never takes focus off the canvas, so `[`, `]`, `Esc` and `↵` +keep working while you drag it. Tab still reaches it, for driving it from the +keyboard on purpose. + +On a class that stores a box the section does not appear at all, because detail +changes an outline and a box has none. The editor does not know that; the answer +says so, by naming no settings. + +**Two settings were here and are not.** Closing the gaps in the mask and proposing +every separate piece are still done, at fixed defaults. As controls they did +nothing to an ordinary clean mask - every position gave the same shape - and each +touch of one re-ran the model to produce it. **Adjusting into nothing says so and leaves the controls up.** A setting can empty the proposal - every piece too small, or an outline reduced past being a @@ -555,10 +564,9 @@ shape - and when it does, the card says what happened and every control stays where it was, so the way back is the way you came. The preview never simply vanishes. -**Accepting several pieces is one action and one undo.** Where the answer -proposed more than one shape, `Accept` writes all of them and a single `mod+z` -takes all of them back. Accepting some and not others is planned rather than -present (#548). +**Accepting is one action and one undo.** Where an answer proposes more than one +shape, `Accept` writes all of them and a single `mod+z` takes all of them back. +Accepting some and not others is planned rather than present (#548). **The tool stays armed while you change class.** Arming it is a decision about how to work, and picking the class to work on is the next thing you do - so a class diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index e959af5..9764b70 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -402,15 +402,6 @@ export interface AnnotatorCanvasProps { * which would draw a shape somebody was trying to point at. */ readonly onSuggestPoint?: (point: Point, polarity: Polarity) => void; - /** - * Whether a suggest request has been out long enough to report. - * - * A prop rather than something derived here from `suggestion.status`, because - * the panel reports the same wait and the two have to agree about the threshold. - * One `usePendingIndicator` in the host is one clock; a copy on each side would - * be two, free to drift by however long a render took. - */ - readonly suggestPending?: boolean; } /** What a host can do to the stage. Read the position through `onViewChange`. */ @@ -443,7 +434,6 @@ export function AnnotatorCanvas({ readOnly = false, suggestion = null, onSuggestPoint, - suggestPending = false, }: AnnotatorCanvasProps): JSX.Element { const snapshot = useAnnotatorSnapshot(store); const { asset, schema } = snapshot.document; @@ -1002,16 +992,10 @@ export function AnnotatorCanvas({ inset: 0, overflow: "hidden", touchAction: "none", - // `progress` — arrow *plus* busy, which is the honest pair: the canvas - // is working and still takes input, because a refine click during the - // wait is a supported gesture rather than something to be blocked. - // - // Applied here and not in `affordanceAt`, because `Cursor` answers - // "what would a press do here" off the tool and the shape under the - // pointer, and a request being out is neither. It has nothing to do - // with where the pointer is, which is exactly why it is not an - // affordance. - cursor: suggestPending ? "progress" : affordance.cursor, + // Never a busy cursor while a suggest is out: the panel is the one + // place a wait is reported, and a spinner riding the pointer over the + // picture read as the machine having hung (#557). + cursor: affordance.cursor, }} // (7) The input surface, and the only one. It spans the whole viewport, // so a press in the margin around the picture reaches the machine with the @@ -1103,7 +1087,6 @@ export function AnnotatorCanvas({ asset={asset} suggestions={painted} {...(suggestion === null ? {} : { promptPoints: suggestion.points })} - suggestPending={suggestPending} /> diff --git a/frontend/annotator/src/adapters/react/TransientLayer.tsx b/frontend/annotator/src/adapters/react/TransientLayer.tsx index 590a1c8..2726d5f 100644 --- a/frontend/annotator/src/adapters/react/TransientLayer.tsx +++ b/frontend/annotator/src/adapters/react/TransientLayer.tsx @@ -25,13 +25,7 @@ import type { Target } from "../../core/interaction/target"; import type { InteractionState } from "../../core/interaction/state"; import type { PromptPoint } from "../../core/interaction/suggestion"; import type { AssetDescriptor, Point } from "../../core/types"; -import { useReducedMotion } from "./hooks"; import { - HALO_MAX_PX, - HALO_MIN_PX, - HALO_OPACITY, - HALO_PERIOD, - HALO_STROKE, SUGGESTION_DASH, SUGGESTION_OPACITY, pendingPolygon, @@ -44,6 +38,7 @@ import { HANDLE_PX, STROKE_PX, VERTEX_PX, + Vertices, } from "./Shapes"; export interface TransientLayerProps { @@ -81,15 +76,6 @@ export interface TransientLayerProps { * press look like it had been dropped. */ readonly promptPoints?: readonly PromptPoint[]; - /** - * Whether a suggest request has been out long enough to be worth saying so. - * - * The threshold is not this layer's to decide — `usePendingIndicator` owns it, - * once, in the host that owns the request — so what arrives here is already the - * answer. A warm click never raises it, which is what keeps a 90 ms round trip - * from flashing a halo nobody had time to read. - */ - readonly suggestPending?: boolean; } const DASH = "6 4"; @@ -113,7 +99,6 @@ export function TransientLayer({ asset, suggestions = NO_SUGGESTIONS, promptPoints, - suggestPending = false, }: TransientLayerProps): JSX.Element { const band = rubberBand(state); const pending = pendingPolygon(state); @@ -168,13 +153,6 @@ export function TransientLayer({ )} - {/* After the points, so the ring sits over the marker it is about rather - than under it — and anchored to the newest click, which is the one the - answer being waited on belongs to. */} - {suggestPending && promptPoints !== undefined && promptPoints.length > 0 && ( - - )} - ); @@ -237,6 +215,22 @@ function SuggestedShape({ strokeLinejoin="round" /> )} + {/* + A preview is a **third visual state**, not a shape marked selected. + + `DESIGN.md` has vertices render only on a selected shape, and a proposal + is not one — selection carries the panel row, the delete key and the + keyboard rules a preview must not have. So it gets its own rule: the + vertices are up the whole time the preview is, undecimated at every step, + because where precision was gained or lost *is* what `detail` is about and + a counter alone made it a blind control (#557). + + Polygons only. A box has no vertex list — its corners are grips, and a + preview has nothing to drag. + */} + {geometry.type === "polygon" && ( + + )} {/* `ShapeLabel`'s treatment, at the meta size — the halo is what keeps it readable over a photograph, and the lift rides the CSS custom property the stage publishes so this element carries no zoom of its own. */} @@ -297,61 +291,6 @@ function PromptPoints({ ); } -/** - * A ring around the newest click, while the answer to it is still out. - * - * The panel already says "Looking at that…", and this exists because that card is - * in the top-right corner while the person who clicked is looking at the middle of - * the picture. On a cold start — model load plus inference on the CPU — the wait - * runs past a second, and a silent canvas is how *working* comes to look like - * *broken*. - * - * **SMIL rather than CSS**, and it is the boundary deciding rather than taste: - * this package ships no stylesheet at all, so there is nowhere for a `@keyframes` - * to live that would not also be a thing an embedder has to load. `` - * needs nothing but the SVG it is already inside. - * - * The pulse expands and fades to nothing, so its peak is the same opacity as the - * still ring a person who asked for reduced motion gets — the moving version is - * never the louder one. - */ -function SuggestHalo({ at, zoom }: { readonly at: Point; readonly zoom: number }): JSX.Element { - const reduced = useReducedMotion(); - const min = screenPx(HALO_MIN_PX, zoom); - const max = screenPx(HALO_MAX_PX, zoom); - return ( - - {!reduced && ( - <> - - - - )} - - ); -} - /** * A shape mid-session: the vertices placed, the rubber band to the cursor, and — * for a polygon — the ring around vertex zero that says where a click would close diff --git a/frontend/annotator/src/adapters/react/hooks.ts b/frontend/annotator/src/adapters/react/hooks.ts index 280262a..cbb6071 100644 --- a/frontend/annotator/src/adapters/react/hooks.ts +++ b/frontend/annotator/src/adapters/react/hooks.ts @@ -118,38 +118,3 @@ export function usePendingIndicator(active: boolean): PendingIndicatorState { const cancel = useCallback(() => indicator.cancel(), [indicator]); return { shown: active || phase.shown, escalated: phase.escalated, cancel }; } - -/** The query, named once so the hook and its documentation cannot disagree. */ -const REDUCED_MOTION = "(prefers-reduced-motion: reduce)"; - -/** Whether the host has one, and whether it says to stop moving things. */ -function askedForStillness(): boolean { - // `typeof` rather than a truthiness check, and it is load-bearing: this package - // is unit-tested under **node**, and `ui-core` renders it under jsdom, and - // neither implements `matchMedia`. A bare read would throw in both. - return typeof matchMedia === "function" && matchMedia(REDUCED_MOTION).matches; -} - -/** - * The one accessibility preference this renderer reads. - * - * The engine cannot: `matchMedia` is a browser global, and `eslint.config.js` - * bans every one of those under `src/core/**`. That is the boundary working — - * whether an indicator pulses is a fact about a screen, not about an annotation. - */ -export function useReducedMotion(): boolean { - const [reduced, setReduced] = useState(askedForStillness); - - useEffect(() => { - if (typeof matchMedia !== "function") return; - const query = matchMedia(REDUCED_MOTION); - const read = (): void => setReduced(query.matches); - // Read once on attach as well: the preference can have moved between the - // lazy initializer and here, and a listener alone would never hear about it. - read(); - query.addEventListener("change", read); - return () => query.removeEventListener("change", read); - }, []); - - return reduced; -} diff --git a/frontend/annotator/src/adapters/react/index.ts b/frontend/annotator/src/adapters/react/index.ts index 45fbec1..d16c38e 100644 --- a/frontend/annotator/src/adapters/react/index.ts +++ b/frontend/annotator/src/adapters/react/index.ts @@ -16,7 +16,6 @@ export { useAnnotatorSnapshot, useAnnotatorStore, usePendingIndicator, - useReducedMotion, type PendingIndicatorState, } from "./hooks"; export { @@ -42,11 +41,6 @@ export { Vertices, } from "./Shapes"; export { - HALO_MAX_PX, - HALO_MIN_PX, - HALO_OPACITY, - HALO_PERIOD, - HALO_STROKE, SUGGESTION_DASH, SUGGESTION_OPACITY, classColor, diff --git a/frontend/annotator/src/adapters/react/paint.test.ts b/frontend/annotator/src/adapters/react/paint.test.ts index e587c97..f97192d 100644 --- a/frontend/annotator/src/adapters/react/paint.test.ts +++ b/frontend/annotator/src/adapters/react/paint.test.ts @@ -50,7 +50,7 @@ function answerOf(...suggestions: readonly Suggestion[]): Answer { modelRef: MODEL_REF, confidence: suggestions[0]?.confidence ?? null, suggestions, - parameters: ["detail", "fill_holes", "fragments"], + parameters: ["detail"], }; } diff --git a/frontend/annotator/src/adapters/react/paint.ts b/frontend/annotator/src/adapters/react/paint.ts index cf56fa3..45de160 100644 --- a/frontend/annotator/src/adapters/react/paint.ts +++ b/frontend/annotator/src/adapters/react/paint.ts @@ -260,49 +260,6 @@ export const SUGGESTION_OPACITY = 0.6; /** The preview's stroke pattern — see `SUGGESTION_OPACITY`. */ export const SUGGESTION_DASH = "10 6"; -/** - * The waiting halo, in the package's own vocabulary rather than the product's. - * - * `@visionset/annotator` ships headless — no Tailwind, no design tokens, no - * stylesheet — which is the whole of its claim to be embeddable, so a ring drawn - * here cannot name a semantic colour the way a `ui-core` component would. It is a - * neutral white at low opacity for the same reason the crosshair is white: it has - * to read over a photograph nobody chose, and a hue would compete with the class - * colour beside it, which is the one colour on this canvas that carries meaning. - * - * It is deliberately **not** the brand: the product spends coral in two places, - * and a spinner is not a third. - * - * The two radii are the pulse. `screenPx` divides both by zoom at the call site, - * so the halo is the same size on screen at every magnification — the rule every - * transient in this package follows. - */ -export const HALO_STROKE = "#ffffff"; - -/** - * Where the ring starts each cycle, in screen pixels. - * - * A prompt marker is drawn at `VERTEX_PX + 1`, so this clears it: the halo is a - * ring *around* the click, never a disc over it, and the point a person placed - * has to stay legible while they wait for the answer to it. - */ -export const HALO_MIN_PX = 9; - -/** Where it ends, in screen pixels. See `HALO_MIN_PX`. */ -export const HALO_MAX_PX = 18; - -/** One breath. Slow enough to read as waiting rather than as an alarm. */ -export const HALO_PERIOD = "1.4s"; - -/** - * The ring's opacity: the pulse's peak, and the still ring's fixed value. - * - * One number for both, because they are one answer to one question — *how loud - * may this be* — and the pulse fading from it to nothing is what makes the moving - * version no louder than the still one. - */ -export const HALO_OPACITY = 0.6; - /** A pending suggestion, ready to draw. */ export interface PaintedSuggestion { /** Never a tag or a path: the two kinds `SUGGESTIBLE_GEOMETRY_TYPES` names. */ @@ -337,10 +294,11 @@ export interface PaintedSuggestion { * must stay on screen while the answer to the click that placed them is still * coming back. * - * **A list, because `fragments` can answer with every piece of a mask.** One - * click can propose several shapes, and they are drawn the same way and accepted - * together; the label rides on each, because a reader looking at one shape should - * not have to find another to learn what class it is. + * **A list, though an answer carries at most one shape today.** The plural shape + * is kept because accepting part of a plural proposal is tracked work (#548); + * they are drawn the same way and accepted together, and the label rides on each, + * because a reader looking at one shape should not have to find another to learn + * what class it is. */ export function paintSuggestions( state: SuggestionState, diff --git a/frontend/annotator/src/core/interaction/suggestion.test.ts b/frontend/annotator/src/core/interaction/suggestion.test.ts index 5a75a9c..31cc8a5 100644 --- a/frontend/annotator/src/core/interaction/suggestion.test.ts +++ b/frontend/annotator/src/core/interaction/suggestion.test.ts @@ -15,13 +15,12 @@ import { AnnotatorStore } from "../state/store"; import { annotationsInDrawOrder, createDocument } from "../state/document"; import type { AnnotationDocument } from "../state/document"; import { addAnnotationCommand } from "../state/commands"; +import { DETAIL_STEPS, polygonAt } from "../geometry/simplify"; import type { AnnotationSchema, AssetDescriptor, Geometry, LabelClass, Point } from "../types"; import type { Answer } from "./suggestion"; import { - DEFAULT_ADJUSTMENTS, vertexCount, withDetail, - withMaskAdjustment, SUGGESTIBLE_GEOMETRY_TYPES, acceptedAnnotations, allowedGeometriesFor, @@ -100,7 +99,7 @@ function answerOf(...suggestions: readonly Suggestion[]): Answer { modelRef: MODEL_REF, confidence: suggestions[0]?.confidence ?? null, suggestions, - parameters: ["detail", "fill_holes", "fragments"], + parameters: ["detail"], }; } @@ -524,46 +523,20 @@ describe("adjusting the vertex density", () => { expect(vertexCount(showing(proposal(A_BOX)))).toBe(0); expect(vertexCount(withContour())).toBe(vertexCount(withContour())); }); -}); - -describe("adjusting what the mask itself is", () => { - it("asks again, because the client never had the pixels", () => { - // Unlike `detail`, these two change what is traced rather than how much of - // the trace survives — and a mask is the one thing that does not travel. - const before = showing(); - const after = withMaskAdjustment(before, { fragments: "all" }); - expect(after.status).toBe("asking"); - expect(after.serial).toBe(before.serial + 1); - expect(after.adjustments.fragments).toBe("all"); - }); - - it("keeps the previews up while the answer is in flight", () => { - // `withPoint`'s rule: what is drawn is still the best answer anyone has. - const after = withMaskAdjustment(showing(), { fillHoles: 0 }); - expect(after.suggestions).not.toEqual([]); - }); - - it("returns the state by identity when nothing actually moved", () => { - const state = showing(); - expect(withMaskAdjustment(state, { fragments: state.adjustments.fragments })).toBe(state); - }); - - it("records the setting without asking when there is nothing to ask about", () => { - // No points placed yet, so there is no gesture to re-send. - const armedOnly = armed("car"); - const after = withMaskAdjustment(armedOnly, { fragments: "all" }); - expect(after.status).toBe("idle"); - expect(after.serial).toBe(armedOnly.serial); - expect(after.adjustments.fragments).toBe("all"); - }); - - it("starts a session on the kernel's own defaults", () => { - expect(armed("car").adjustments).toEqual(DEFAULT_ADJUSTMENTS); - }); - it("carries adjustments into a fresh session, so a choice survives the frame", () => { - const chosen = { ...DEFAULT_ADJUSTMENTS, detail: "coarse" as const }; - expect(armed("car", chosen).adjustments).toEqual(chosen); + it("a step can never lose a shape, because only a zero-area outline is refused", () => { + // Reported in planning as a defect — a coarser step dropping a shape and + // taking its contour with it, so a finer step could not bring it back. It is + // not reachable, and this is the measurement rather than the argument: + // `polygonAt` refuses only a contour with no area, and whether points are + // collinear does not depend on the tolerance. So the three steps agree about + // which shapes exist and differ only in how many vertices each spends (#557). + const collinear: readonly Point[] = [[0, 0], [5, 0], [10, 0]]; + const curved = RING; + for (const step of DETAIL_STEPS) { + expect(polygonAt(collinear, step)).toBeNull(); + expect(polygonAt(curved, step)).not.toBeNull(); + } }); }); @@ -574,9 +547,11 @@ describe("what the answer declares", () => { modelRef: MODEL_REF, confidence: 0.5, suggestions: [proposal()], - parameters: ["fragments"], + parameters: [], }); - expect(boxy.parameters).toEqual(["fragments"]); + // A box class declares nothing, and the panel renders no section for it — + // which this file is not allowed to work out for itself (#557). + expect(boxy.parameters).toEqual([]); }); it("keeps the controls on an answer with nothing in it", () => { @@ -587,9 +562,9 @@ describe("what the answer declares", () => { modelRef: MODEL_REF, confidence: null, suggestions: [], - parameters: ["detail", "fill_holes", "fragments"], + parameters: ["detail"], }); expect(empty.status).toBe("none"); - expect(empty.parameters).toEqual(["detail", "fill_holes", "fragments"]); + expect(empty.parameters).toEqual(["detail"]); }); }); diff --git a/frontend/annotator/src/core/interaction/suggestion.ts b/frontend/annotator/src/core/interaction/suggestion.ts index 4847410..d2d40cc 100644 --- a/frontend/annotator/src/core/interaction/suggestion.ts +++ b/frontend/annotator/src/core/interaction/suggestion.ts @@ -200,7 +200,7 @@ export interface Suggestion { /** The model that proposed it, carried onto the annotation if it is accepted. */ readonly modelRef: string; /** - * The unsimplified outline this shape was reduced from, empty for a box. + * The outline this shape was reduced from, empty for a box. * * What makes {@link withDetail} arithmetic rather than a round trip. It is the * *same* points the server reduced, which matters because Douglas-Peucker is @@ -210,11 +210,8 @@ export interface Suggestion { readonly contour: readonly Point[]; } -/** How many of a mask's separate pieces become shapes. The wire's `fragments`. */ -export type Fragments = "one" | "all"; - /** A setting the server says has some effect here. The wire's `parameters`. */ -export type SuggestParameter = "detail" | "fill_holes" | "fragments"; +export type SuggestParameter = "detail"; /** What an answer carries back, beside the shapes themselves. */ export interface Answer { @@ -246,25 +243,21 @@ export interface Answer { */ export type SuggestionStatus = "idle" | "asking" | "shown" | "none" | "refused"; -/** The three settings, as they stand right now. Sent on every ask. */ +/** The one setting, as it stands right now. Sent on every ask. */ export interface Adjustments { readonly detail: Detail; - readonly fillHoles: number; - readonly fragments: Fragments; } /** * What a session starts with, and what the server means by "nothing was sent". * - * The numbers are the kernel's own defaults. They are restated here because this - * package has no HTTP and cannot read them from an answer that has not arrived — - * and `simplify.test.ts` holds `EPSILON` to the kernel's table, which is the half + * The value is the kernel's own default. It is restated here because this package + * has no HTTP and cannot read it from an answer that has not arrived — and + * `simplify.test.ts` holds `EPSILON` to the kernel's table, which is the half * that could silently differ. */ export const DEFAULT_ADJUSTMENTS: Adjustments = { detail: "balanced", - fillHoles: 0.002, - fragments: "one", }; /** The whole of a suggest session. `null`, in a host, is a tool that is not armed. */ @@ -283,14 +276,14 @@ export interface SuggestionState { /** * The previews, when there are any. Kept across `asking` so they do not flicker. * - * Plural because `fragments` can answer with every piece of a mask. Accepting - * is all of them or none, in one history entry — see - * {@link acceptedAnnotations}. + * Plural though today an answer carries at most one, because accepting part of + * a plural proposal is tracked work (#548). Accepting is all of them or none, + * in one history entry — see {@link acceptedAnnotations}. */ readonly suggestions: readonly Suggestion[]; - /** Where the three settings stand. Sent on every ask, echoed by every answer. */ + /** Where the setting stands. Sent on every ask, echoed by every answer. */ readonly adjustments: Adjustments; - /** Which of them the server says apply here. Empty until an answer arrives. */ + /** Which settings the server says apply here. Empty until an answer arrives. */ readonly parameters: readonly SuggestParameter[]; /** What the server refused with, in prose. Non-null only while `refused`. */ readonly refusal: string | null; @@ -467,9 +460,12 @@ export function isAcceptable(state: SuggestionState): boolean { * A step that is already set returns the state **by identity**, so a host can * fold this through unconditionally without a render. * - * A shape that simplifies away to fewer than three points is dropped, and losing - * every shape lands on `none` rather than on an empty `shown` — a status that - * claims a preview nobody can see. + * **A step can never lose a shape, and that was measured rather than assumed.** + * `polygonAt` answers `null` only for a contour with no area — three or more + * collinear points — and collinearity is a property of the contour rather than of + * the tolerance, so a shape drawable at one step is drawable at all three. The + * empty branch below is the type's, not a reachable state: a zero-area outline is + * dropped by the server before it is ever sent (#557). */ export function withDetail(state: SuggestionState, detail: Detail): SuggestionState { if (detail === state.adjustments.detail) return state; @@ -491,33 +487,6 @@ function resimplified(one: Suggestion, detail: Detail): Suggestion | null { return { ...one, geometry: { ...one.geometry, type: "polygon", points } as Geometry }; } -/** - * A different mask, which only the server can produce. - * - * `fill_holes` and `fragments` change the pixels the shape is traced from, and - * the client never had those — so unlike {@link withDetail} this cannot be - * answered locally. It records the setting and bumps the serial, which is - * `withPoint`'s shape: the host sees `asking` and sends the same accumulated - * points again with the new value. - * - * The previews stay up while the answer is in flight, for `withPoint`'s reason: - * what is drawn is still the best answer anyone has. - */ -export function withMaskAdjustment( - state: SuggestionState, - adjustment: Partial>, -): SuggestionState { - const adjustments = { ...state.adjustments, ...adjustment }; - if ( - adjustments.fillHoles === state.adjustments.fillHoles && - adjustments.fragments === state.adjustments.fragments - ) { - return state; - } - if (state.points.length === 0) return { ...state, adjustments }; - return { ...state, adjustments, status: "asking", refusal: null, serial: state.serial + 1 }; -} - /** How many vertices the preview is currently spending. What the counter reads. */ export function vertexCount(state: SuggestionState): number { return state.suggestions.reduce( diff --git a/frontend/annotator/src/index.ts b/frontend/annotator/src/index.ts index 0a9faaa..1187b3b 100644 --- a/frontend/annotator/src/index.ts +++ b/frontend/annotator/src/index.ts @@ -169,11 +169,9 @@ export { vertexCount, withClass, withDetail, - withMaskAdjustment, withPoint, type Adjustments, type Answer, - type Fragments, type Polarity, type Prompt, type PromptPoint, @@ -299,11 +297,6 @@ export { PolygonShape, SELECTED_STROKE_PX, ESCALATE_MS, - HALO_MAX_PX, - HALO_MIN_PX, - HALO_OPACITY, - HALO_PERIOD, - HALO_STROKE, MIN_VISIBLE_MS, STROKE_PX, SUGGESTION_DASH, @@ -329,7 +322,6 @@ export { useAnnotatorSnapshot, useAnnotatorStore, usePendingIndicator, - useReducedMotion, type AnnotationLayerProps, type AnnotatorCanvasProps, type AnnotatorView, diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 2518cba..1ebd9c9 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -387,9 +387,10 @@ async function serveApi( contour: [], }, ], - applied: { detail: "balanced", fill_holes: 0.002, fragments: "one" }, - // A box class, so the wire names only the setting that moves a box. - parameters: ["fragments"], + applied: { detail: "balanced" }, + // A box class, so the wire names no settings at all — which is how the + // editor is told to render no adjustments section (#557). + parameters: [], }, }); } @@ -2999,24 +3000,18 @@ test("saving leaves the viewport exactly where it was", async ({ page }) => { }); /** - * The in-flight indicator, which only a browser can be asked about. + * Where a wait is reported, and where it must not be. * - * Three of the four claims below are unreachable from jsdom. A cursor is a - * computed style on a laid-out element; `prefers-reduced-motion` is a media query - * with no implementation there; and where the halo sits is a coordinate, which - * every rectangle being zero makes meaningless. The remaining thresholds — the - * visibility floor and the escalation — are not retested here; they are unit- - * tested on fake time in the annotator's `pending.test.ts`, and a wall-clock - * assertion on a shared runner fails for reasons nobody chose. + * jsdom cannot answer either half: a cursor is a computed style on a laid-out + * element, and "nothing is drawn near the click" is a claim about coordinates + * that every rectangle being zero makes meaningless. * * The request is **held open** rather than delayed by a sleep. `e2e_discipline` * bans fixed waits and is right to: a sleep is a coin toss against any real * timing, while a route that has genuinely not answered keeps the wait true for * as long as the assertions need it. */ -test("a suggest request that is out says so at the click and on the cursor", async ({ - page, -}) => { +test("a suggest request that is out says so on the panel and nowhere else", async ({ page }) => { const sent: Request[] = []; await openJob(page, sent, undefined, undefined, undefined, undefined, true); @@ -3038,8 +3033,8 @@ test("a suggest request that is out says so at the click and on the cursor", asy contour: [], }, ], - applied: { detail: "balanced", fill_holes: 0.002, fragments: "one" }, - parameters: ["fragments"], + applied: { detail: "balanced" }, + parameters: [], }, }); }); @@ -3050,64 +3045,26 @@ test("a suggest request that is out says so at the click and on the cursor", asy const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); - const halo = page.getByTestId("suggest-halo"); - await expect(halo).toBeVisible(); - await expect(halo).toHaveAttribute("data-motion", "pulsing"); - - // Arrow *and* busy: the canvas is working and still takes a refine click. - await expect(page.getByTestId("annotator-pane")).toHaveCSS("cursor", "progress"); - - // The halo is on the click, not on the picture's origin — the claim jsdom's - // zero rectangles cannot carry. - const dot = page.getByTestId("prompt-points").locator("circle").last(); - await expect(halo).toHaveAttribute("cx", (await dot.getAttribute("cx"))!); - await expect(halo).toHaveAttribute("cy", (await dot.getAttribute("cy"))!); - - // The panel reports the same wait, off the same clock, and has not yet earned - // the sentence about a cold start. + // The card is the report, and it is up on the frame the request left. await expect(page.getByTestId("suggest-asking")).toBeVisible(); await expect(page.getByTestId("suggest-cold-start")).toHaveCount(0); - answer(); - - await expect(page.getByTestId("suggestion-shape")).toBeVisible(); - await expect(halo).toHaveCount(0); + // The mutation test for the removal: restore the ring or the busy cursor and + // one of these turns red (#557). + await expect(page.getByTestId("suggest-halo")).toHaveCount(0); await expect(page.getByTestId("annotator-pane")).not.toHaveCSS("cursor", "progress"); -}); - -test("the halo holds still for somebody who asked not to be moved", async ({ page }) => { - await page.emulateMedia({ reducedMotion: "reduce" }); - - const sent: Request[] = []; - await openJob(page, sent, undefined, undefined, undefined, undefined, true); - - let answer = (): void => {}; - const held = new Promise((resolve) => { - answer = resolve; - }); - await page.route("**/inference/suggest", async (route) => { - await held; - await route.fulfill({ status: 200, json: { model_ref: "m@1", region: null } }); - }); - await page.getByTestId("tool-suggest").click(); - await expect(page.getByTestId("suggest-idle")).toBeVisible(); - - const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; - await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); - - const halo = page.getByTestId("suggest-halo"); - await expect(halo).toHaveAttribute("data-motion", "still"); - // The ring is still drawn — a preference about motion is not a preference - // about being told what is happening. It simply does not move. - await expect(halo).toBeVisible(); - await expect(halo.locator("animate")).toHaveCount(0); + // The clicks themselves are still drawn — those are what makes a refine + // legible, and they are not an indicator. + await expect(page.getByTestId("prompt-points")).toBeVisible(); answer(); - await expect(halo).toHaveCount(0); + + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); + await expect(page.getByTestId("annotator-pane")).not.toHaveCSS("cursor", "progress"); }); -test("escape takes the halo back without waiting out its visibility floor", async ({ page }) => { +test("escape takes the wait back while the request is still out", async ({ page }) => { const sent: Request[] = []; await openJob(page, sent, undefined, undefined, undefined, undefined, true); @@ -3125,15 +3082,14 @@ test("escape takes the halo back without waiting out its visibility floor", asyn const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); - await expect(page.getByTestId("suggest-halo")).toBeVisible(); + await expect(page.getByTestId("suggest-asking")).toBeVisible(); await page.keyboard.press("Escape"); - // Down with the points, and the cursor with it — the request is still out, so - // nothing but the explicit cancel could have done this. - await expect(page.getByTestId("suggest-halo")).toHaveCount(0); + // Down with the points — the request is still out, so nothing but the explicit + // cancel could have done this. + await expect(page.getByTestId("suggest-asking")).toHaveCount(0); await expect(page.getByTestId("prompt-points")).toHaveCount(0); - await expect(page.getByTestId("annotator-pane")).not.toHaveCSS("cursor", "progress"); answer(); }); @@ -3142,11 +3098,12 @@ test("escape takes the halo back without waiting out its visibility floor", asyn * The adjustments, in a real browser. * * jsdom can say the section renders. What it cannot say is that a bracket - * reaches it from the keyboard, that the counter changes without a request - * leaving, or that `Esc` closes this before it clears the gesture — all three - * are about a live document with focus in it. + * reaches it from the keyboard, that the shape changes without a request + * leaving, that pressing a control leaves the keyboard working, or that a press + * on the card never reaches the picture underneath — all of which are about a + * live document with focus and hit-testing in it. */ -test("a box class is offered only the setting that moves a box", async ({ page }) => { +test("a box class is offered no adjustments at all", async ({ page }) => { const sent: Request[] = []; await openJob(page, sent, undefined, undefined, undefined, undefined, true); @@ -3155,37 +3112,47 @@ test("a box class is offered only the setting that moves a box", async ({ page } await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); await expect(page.getByTestId("suggestion-shape")).toBeVisible(); - await page.getByTestId("suggest-adjust-open").click(); - - // The stub declares `["fragments"]`, which is what the kernel declares for a - // box — and the editor renders exactly that. No condition in the app mentions - // a box at all. - await expect(page.getByTestId("suggest-fragments")).toBeVisible(); - await expect(page.getByTestId("suggest-detail-balanced")).toHaveCount(0); - await expect(page.getByTestId("suggest-fill-holes")).toHaveCount(0); + // The stub declares `[]`, which is what the kernel declares for a box — and the + // editor renders exactly that. No condition in the app mentions a box at all. + await expect(page.getByTestId("suggest-adjust-open")).toHaveCount(0); + await expect(page.getByTestId("suggest-adjustments")).toHaveCount(0); + await expect(page.getByTestId("suggest-detail")).toHaveCount(0); }); -test("a polygon class steps its detail from the keyboard, with no request", async ({ page }) => { - const sent: Request[] = []; - await openJob(page, sent, undefined, undefined, undefined, undefined, true); +/** A traced ring big enough that the three steps genuinely differ. */ +const RING = Array.from({ length: 64 }, (_, index) => { + const angle = (index / 64) * 2 * Math.PI; + return [Math.round(160 + 90 * Math.cos(angle)), Math.round(160 + 90 * Math.sin(angle))]; +}); - // A traced ring big enough that the three steps genuinely differ: a rectangle - // is four corners at every setting and would report a dead control as working. - const ring = Array.from({ length: 64 }, (_, index) => { - const angle = (index / 64) * 2 * Math.PI; - return [Math.round(160 + 90 * Math.cos(angle)), Math.round(160 + 90 * Math.sin(angle))]; - }); +/** A polygon answer over that ring, with `detail` declared as the one setting. */ +async function servePolygonSuggestion(page: Page): Promise { await page.route("**/inference/suggest", async (route) => route.fulfill({ json: { model_ref: "facebook/sam2-hiera-base-plus@main", confidence: 0.9, - regions: [{ geometry: { type: "polygon", points: ring }, contour: ring }], - applied: { detail: "balanced", fill_holes: 0.002, fragments: "one" }, - parameters: ["detail", "fill_holes", "fragments"], + regions: [{ geometry: { type: "polygon", points: RING }, contour: RING }], + applied: { detail: "balanced" }, + parameters: ["detail"], }, }), ); +} + +/** The vertex count the canvas is actually drawing, read off the polygon itself. */ +async function drawnVertices(page: Page): Promise { + const points = (await page.getByTestId("suggestion-shape").getAttribute("points")) ?? ""; + return points.trim().split(/\s+/).filter(Boolean).length; +} + +const asks = (sent: readonly Request[]): number => + sent.filter((one) => one.url().includes("/inference/suggest")).length; + +test("a polygon class steps its detail from the keyboard, with no request", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + await servePolygonSuggestion(page); await page.getByTestId("tool-suggest").click(); const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; @@ -3193,46 +3160,36 @@ test("a polygon class steps its detail from the keyboard, with no request", asyn await expect(page.getByTestId("suggestion-shape")).toBeVisible(); // The bracket first, with nothing opened — which is the whole point of it. - // The shape on the canvas is what has to move, so that is what is measured; - // the counter is a second reading of the same fact, checked below. - const shape = page.getByTestId("suggestion-shape"); - const vertices = async (): Promise => - ((await shape.getAttribute("points")) ?? "").trim().split(/\s+/).filter(Boolean).length; - const before = sent.filter((one) => one.url().includes("/inference/suggest")).length; + // The shape on the canvas is what has to move, so that is what is measured. + const before = asks(sent); await page.keyboard.press("["); - const coarse = await vertices(); + const coarse = await drawnVertices(page); // The claim that only a real request log can settle: no round trip. - expect(sent.filter((one) => one.url().includes("/inference/suggest")).length).toBe(before); + expect(asks(sent)).toBe(before); await page.keyboard.press("]"); await page.keyboard.press("]"); - const fine = await vertices(); + const fine = await drawnVertices(page); expect(fine).toBeGreaterThan(coarse); - expect(sent.filter((one) => one.url().includes("/inference/suggest")).length).toBe(before); + expect(asks(sent)).toBe(before); // And it stops at the end rather than wrapping round to the coarsest. await page.keyboard.press("]"); - expect(await vertices()).toBe(fine); + expect(await drawnVertices(page)).toBe(fine); await page.keyboard.press("["); await page.keyboard.press("["); - // Opening the section must not switch the keyboard off, which is what a control - // taking focus would silently do. await page.getByTestId("suggest-adjust-open").click(); - const counter = page.getByTestId("suggest-vertex-count"); - await expect(counter).toHaveText(`${coarse} pts`); - await expect(page.getByTestId("suggest-detail-coarse")).toHaveAttribute("aria-pressed", "true"); + await expect(page.getByTestId("suggest-detail-label")).toHaveText(`Coarse · ${coarse} pts`); + await expect(page.getByTestId("suggest-detail")).toHaveValue("0"); // Opening the section must not switch the keyboard off, which is what a control // taking focus would silently do — and does, in a browser, where jsdom has no // focus to move and would report this working. await page.keyboard.press("]"); - await expect(page.getByTestId("suggest-detail-balanced")).toHaveAttribute( - "aria-pressed", - "true", - ); + await expect(page.getByTestId("suggest-detail")).toHaveValue("1"); // Escape closes the adjustments and stops there: the points and the shape are // both still on screen, and the second press is what takes them. @@ -3244,3 +3201,71 @@ test("a polygon class steps its detail from the keyboard, with no request", asyn await expect(page.getByTestId("suggestion-shape")).toHaveCount(0); await expect(page.getByTestId("suggest-idle")).toBeVisible(); }); + +test("the preview draws its vertices, and a committed shape does not", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + await servePolygonSuggestion(page); + + await page.getByTestId("tool-suggest").click(); + const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; + await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); + + const preview = page.getByTestId("suggestion-preview"); + await expect(preview.getByTestId("suggestion-shape")).toBeVisible(); + + // Dashed, and carrying one dot per vertex. Without the dots the detail control + // moves a number and nothing anybody can see (#557). + await expect(preview.locator("polygon")).toHaveAttribute("stroke-dasharray", "10 6"); + const drawn = await drawnVertices(page); + expect(drawn).toBeGreaterThan(3); + await expect(preview.locator("circle")).toHaveCount(drawn); + + // The set follows the detail, with no request — the same fact the counter + // reports, read off the canvas instead. + await page.keyboard.press("["); + await expect(preview.locator("circle")).toHaveCount(await drawnVertices(page)); + + // Accept it, and it becomes an ordinary shape: solid, and no vertices until it + // is selected. That is the contrast the preview state exists to make. + await page.keyboard.press("Enter"); + await expect(page.getByTestId("suggestion-preview")).toHaveCount(0); + const committed = page.getByTestId("annotator-canvas").locator("g[data-annotation-id]"); + await expect(committed).toHaveCount(1); + await expect(committed.locator("polygon")).not.toHaveAttribute("stroke-dasharray", "10 6"); +}); + +test("a press on the suggest panel never reaches the picture underneath", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + await servePolygonSuggestion(page); + + await page.getByTestId("tool-suggest").click(); + const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; + await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); + + const seeded = await page.getByTestId("prompt-points").locator("circle").count(); + const before = asks(sent); + + // Every interactive control on the card, in turn. None of them may fire a + // suggest or move the seed point — the card sits over the picture, so a press + // that fell through would place a prompt point where somebody was aiming at a + // button (#557). + await page.getByTestId("suggest-adjust-open").click(); + await expect(page.getByTestId("suggest-adjustments")).toBeVisible(); + + const slider = page.getByTestId("suggest-detail"); + const box = (await slider.boundingBox())!; + await page.mouse.click(box.x + 2, box.y + box.height / 2); + await slider.click(); + + expect(asks(sent)).toBe(before); + await expect(page.getByTestId("prompt-points").locator("circle")).toHaveCount(seeded); + + // And the keyboard still belongs to the canvas after all of it, which is the + // half a click that merely *took focus* would break. + await page.keyboard.press("]"); + await expect(page.getByTestId("suggest-adjustments")).toBeVisible(); + expect(asks(sent)).toBe(before); +}); diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index 09f2b34..c02c34a 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -114,14 +114,12 @@ import { usePendingIndicator, withClass, withDetail, - withMaskAdjustment, withPoint, type Answer, type AnnotatorStore, type AnnotatorView, type Clipboard, type Detail, - type Fragments, type Point, type Polarity, type Suggestion, @@ -1021,40 +1019,6 @@ function Workspace({ return true; } - /** A setting only the server can honour: record it and ask again. */ - function adjustMask(adjustment: { fillHoles?: number; fragments?: Fragments }): void { - if (session === null || connection === null) return; - const declared = store.document.schema.classes.find( - (candidate) => candidate.name === session.labelClass, - ); - if (declared === undefined) return; - const next = withMaskAdjustment(session, adjustment); - if (next === session) return; - setSession(next); - if (next.status !== "asking") return; - const asked = next.serial; - const prompt = promptOf(next); - suggestRegion.mutate( - { - projectId, - assetId: asset.id, - connectionId: connection.id, - positive: prompt.positive, - negative: prompt.negative, - allowedGeometries: allowedGeometriesFor(declared), - adjustments: next.adjustments, - }, - { - onSuccess: (answer) => { - setSession((live) => (live === null ? live : answered(live, asked, readAnswer(answer)))); - }, - onError: (error: unknown) => { - setSession((live) => (live === null ? live : refused(live, asked, refusalProse(error)))); - }, - }, - ); - } - /** * Escape, in three layers: close the adjustments, clear what is pending, then * put the tool away. @@ -2449,7 +2413,6 @@ function Workspace({ // The halo and the busy cursor. Keyed to `diverting` for the same // reason the session is: a parked tool has nothing in flight, so a // halo over one would be reporting a wait nobody started. - suggestPending={diverting !== null && pending.shown} /> )} @@ -2630,7 +2593,6 @@ function Workspace({ adjusting={adjusting} onAdjusting={setAdjusting} onDetail={applyDetail} - onMaskAdjustment={adjustMask} // Off the same clock the halo is drawn from, which is what lets // the card and the canvas be read as one report of one wait // rather than as two. The card's own appearance follows the diff --git a/frontend/ui-core/src/annotator/SuggestPanel.tsx b/frontend/ui-core/src/annotator/SuggestPanel.tsx index f2a00f3..986ae1c 100644 --- a/frontend/ui-core/src/annotator/SuggestPanel.tsx +++ b/frontend/ui-core/src/annotator/SuggestPanel.tsx @@ -62,13 +62,11 @@ import { hasPending, type SuggestionState, type Detail, - type Fragments, } from "@visionset/annotator"; import { Check, Loader2, Sparkles, TriangleAlert, X } from "lucide-react"; import type { JSX, ReactNode } from "react"; import { EditorNotice } from "./EditorNotice"; -import { cn } from "../lib/cn"; import { Button } from "../primitives/Button"; import { Select, @@ -127,16 +125,14 @@ export interface SuggestPanelProps { readonly onAdjusting?: (open: boolean) => void; /** A step of vertex density, applied without a request. */ readonly onDetail?: (detail: Detail) => void; - /** A setting only the server can honour, which is why it is a separate door. */ - readonly onMaskAdjustment?: (adjustment: { fillHoles?: number; fragments?: Fragments }) => void; /** * Whether the wait has lasted long enough to be worth explaining. * * The cold-start sentence used to ride along with the spinner on every click, * including the fast ones it is not about. It is true of the first click on a * frame, so it waits for a wait that is plausibly that one. Decided by the - * host's `usePendingIndicator`, which the canvas reads too, so the sentence and - * the halo cannot disagree about how long this has been going. + * host's `usePendingIndicator` — which is now the *only* thing that clock is + * for, since the canvas stopped reporting the wait at all (#557). */ readonly pendingEscalated?: boolean; } @@ -205,7 +201,6 @@ export function SuggestPanel({ adjusting, onAdjusting, onDetail, - onMaskAdjustment, pendingEscalated = false, }: SuggestPanelProps): JSX.Element { /* @@ -348,7 +343,6 @@ export function SuggestPanel({ open={adjusting === true} {...(onAdjusting === undefined ? {} : { onOpen: onAdjusting })} {...(onDetail === undefined ? {} : { onDetail })} - {...(onMaskAdjustment === undefined ? {} : { onMaskAdjustment })} /> ); @@ -392,7 +386,6 @@ export function SuggestPanel({ open={adjusting === true} {...(onAdjusting === undefined ? {} : { onOpen: onAdjusting })} {...(onDetail === undefined ? {} : { onDetail })} - {...(onMaskAdjustment === undefined ? {} : { onMaskAdjustment })} /> ); @@ -515,9 +508,11 @@ function Chip({ children }: { readonly children: ReactNode }): JSX.Element { * would stop stepping, and `Esc` would stop being the preview's undo, both with * nothing on screen to say why. Found in a browser: jsdom has no focus to move. * - * On the two controls whose whole effect is on the canvas, and not on the slider - * or the checkbox, which are ordinary form controls somebody may want to reach - * with the keyboard and operate there. + * On every control here, the slider included: all of them act on the canvas, so + * none of them has any business holding focus. `mousedown` rather than the whole + * control, so Tab still reaches the slider for somebody who wants to drive it + * from the keyboard — that is a deliberate arrival, not a side effect of pointing + * at it. */ function keepFocusOnCanvas(event: { preventDefault: () => void }): void { event.preventDefault(); @@ -533,23 +528,21 @@ function keepFocusOnCanvas(event: { preventDefault: () => void }): void { * touches anything else — the nearest thing to hand is the thing a press undoes. * * **What renders is what the server declared, and nothing is worked out here.** - * `session.parameters` comes off the answer; a box class gets `fragments` alone - * because that is what the kernel's table says applies to a box, not because - * this file knows anything about boxes. Adding a fourth setting is a kernel - * change and a row below, and no condition in between. + * `session.parameters` comes off the answer; a box class declares nothing at all + * and so gets no section, because that is what the kernel's table says applies to + * a box, not because this file knows anything about boxes. Adding a second + * setting is a kernel change and a row below, and no condition in between. */ function Adjustments({ session, open, onOpen, onDetail, - onMaskAdjustment, }: { readonly session: SuggestionState; readonly open: boolean; readonly onOpen?: (open: boolean) => void; readonly onDetail?: (detail: Detail) => void; - readonly onMaskAdjustment?: (adjustment: { fillHoles?: number; fragments?: Fragments }) => void; }): JSX.Element | null { // A host that cannot honour the controls renders none, and a kind with nothing // to adjust is told so by the wire rather than guessed at here. @@ -569,75 +562,64 @@ function Adjustments({ ); } - const { detail, fillHoles, fragments } = session.adjustments; + const { detail } = session.adjustments; + const step = DETAIL_STEPS.indexOf(detail); return ( -
+
{session.parameters.includes("detail") && onDetail !== undefined && ( -
+
Detail -
- {DETAIL_STEPS.map((step) => ( - - ))} +
+ {/* + A slider, because three words in a row did not read as pressable and + gave no feedback about what had moved (#557). Three stops rather than + a continuous range: `Detail` is three steps, and a range that landed + between them would be a position the server has no answer for. + + A native `input[type=range]` and not a primitive, because there is no + slider primitive in this package and one control does not earn one. + It comes keyboard-operable and `focus-visible` for nothing. + */} + onDetail(DETAIL_STEPS[Number(event.target.value)] ?? detail)} + /> {/* - Beside the control it is about, because it is the price of the - setting rather than a fact about the shape. Tabular figures so the - number does not shift the row as it changes under a held key. + Step and count in one label, because they are one fact: what this + position costs. Tabular figures so the number does not shift the row + as it changes under a held key. */} - {vertexCount(session)} pts + {labelFor(detail)} · {vertexCount(session)} pts [ ]
)} - {session.parameters.includes("fill_holes") && onMaskAdjustment !== undefined && ( - - )} - {session.parameters.includes("fragments") && onMaskAdjustment !== undefined && ( - - )}
); } + +/** Sentence case, from the wire's own lowercase vocabulary. */ +function labelFor(detail: Detail): string { + return detail.charAt(0).toUpperCase() + detail.slice(1); +} diff --git a/frontend/ui-core/src/annotator/suggestFlow.test.tsx b/frontend/ui-core/src/annotator/suggestFlow.test.tsx index ebd40a5..28fb8e4 100644 --- a/frontend/ui-core/src/annotator/suggestFlow.test.tsx +++ b/frontend/ui-core/src/annotator/suggestFlow.test.tsx @@ -215,10 +215,10 @@ beforeEach(() => { contour: [], }, ], - applied: { detail: "balanced", fill_holes: 0.002, fragments: "one" }, - // Declared as the server declares it for a box class: `fragments` alone. - // The panel renders what this names and works none of it out itself. - parameters: ["fragments"], + applied: { detail: "balanced" }, + // Declared as the server declares it for a box class: nothing at all, so the + // panel renders no adjustments. It works none of that out for itself (#557). + parameters: [], }; suggestRefusal = null; suggestHold = null; @@ -625,176 +625,76 @@ describe("a press outside the asset is not a prompt", () => { }); }); -describe("what the wait looks like while it is happening", () => { +describe("what the wait looks like, and where it is not reported", () => { /* - These are about the *wiring* — that the host's one clock reaches both the card - and the canvas — and not about the thresholds themselves, which are unit-tested - on fake time in `adapters/react/pending.test.ts`. Real timers here, because the - request is real: the route is held open, so the wait is genuine rather than - simulated, and `findBy*` does the waiting that a fixed sleep would guess at. - - jsdom's `matchMedia` stub answers `matches: true` to every query, so the halo - renders in its reduced-motion form throughout. That is asserted rather than - worked around: a stub that lies in one direction should be visible in what the - test claims. + The panel is the only surface that says a request is out (#557). A ring at the + click point and a busy cursor were both removed: sitting on the picture next + to the pointer, they read as the machine having hung rather than as work in + progress, and the card already says so in words. + + Real timers, because the request is real: the route is held open, so the wait + is genuine rather than simulated, and `findBy*` does the waiting that a fixed + sleep would guess at. */ - it("puts a halo at the click and a busy cursor on the canvas once the wait is real", async () => { + it("says so on the card and puts nothing at all on the canvas", async () => { const gate = held(); await open(); await arm(); clickCanvas(); - const halo = await screen.findByTestId("suggest-halo"); - expect(halo.getAttribute("data-motion")).toBe("still"); - expect(screen.getByTestId("annotator-pane").style.cursor).toBe("progress"); - - gate.release(); - - await waitFor(() => expect(screen.queryByTestId("suggest-halo")).toBeNull()); - expect(screen.getByTestId("annotator-pane").style.cursor).not.toBe("progress"); - }); - - it("is up on the first render after the click, with no clock advanced", async () => { - // The load-bearing case for the delay's removal, and it is deliberately - // synchronous — no `findBy`, no `waitFor`, nothing that would let a timer or - // a second render slip in underneath. A show delay of any length, including - // one frame's worth, turns this red. - const gate = held(); - await open(); - await arm(); - - clickCanvas(); - - expect(screen.getByTestId("suggest-halo")).toBeTruthy(); - expect(screen.getByTestId("suggest-asking")).toBeTruthy(); - expect(screen.getByTestId("annotator-pane").style.cursor).toBe("progress"); - - gate.release(); - await waitFor(() => expect(screen.queryByTestId("suggest-halo")).toBeNull()); - }); - - it("comes up on a fast answer too, and goes away on its own", async () => { - // Nothing held, so the stub answers about as fast as anything can, and the - // halo is still up on the click — which is the delay's removal seen from the - // case the delay existed for. - // - // What is deliberately **not** asserted here is the floor holding the halo - // past the answer. That is real behaviour and it is pinned in the machine's - // own unit tests, but its rendered consequence is not observable from this - // harness: a jsdom round trip through react-query routinely runs past 250ms - // — an attempt at exactly this assertion measured 273ms and failed on a - // floor that had honestly expired — and the alternative, driving the hook - // under fake timers, does not deliver the effect-driven state update at all. - // A racy assertion here would be worse than the gap it papers over. - await open(); - await arm(); - clickCanvas(); - - expect(screen.getByTestId("suggest-halo")).toBeTruthy(); - - await screen.findByTestId("suggestion-shape"); - await waitFor(() => expect(screen.queryByTestId("suggest-halo")).toBeNull()); + await screen.findByTestId("suggest-asking"); + // The mutation test for the removal: restore either affordance and one of + // these turns red. + expect(screen.queryByTestId("suggest-halo")).toBeNull(); expect(screen.getByTestId("annotator-pane").style.cursor).not.toBe("progress"); - }); - - it("reports the wait in the card and on the canvas off the same clock", async () => { - const gate = held(); - await open(); - await arm(); - clickCanvas(); - - await screen.findByTestId("suggest-halo"); - expect(screen.getByTestId("suggest-asking")).toBeTruthy(); - // The cold-start sentence is a second threshold away and has not been earned. - expect(screen.queryByTestId("suggest-cold-start")).toBeNull(); gate.release(); - await waitFor(() => expect(screen.queryByTestId("suggest-asking")).toBeNull()); + await screen.findByTestId("suggest-shown"); + expect(screen.queryByTestId("suggest-halo")).toBeNull(); }); - it("takes the halo back on Escape without waiting out the visibility floor", async () => { + it("is up on the first render after the click, with no clock advanced", async () => { + // The load-bearing case for the delay's removal: the card is the report, so + // it has to be there on the frame the request leaves. const gate = held(); await open(); await arm(); clickCanvas(); - await screen.findByTestId("suggest-halo"); - - await userEvent.keyboard("{Escape}"); - - // Synchronous: `discardSuggestion` cancels the indicator itself rather than - // letting the status drop do it, because the ordinary path honours the floor - // and a take-back that lingers a quarter second is not one. - expect(screen.queryByTestId("suggest-halo")).toBeNull(); - expect(screen.getByTestId("annotator-pane").style.cursor).not.toBe("progress"); + await screen.findByTestId("suggest-asking"); gate.release(); + await screen.findByTestId("suggest-shown"); }); it("keeps the shape drawn through a refine, while the card reports the new ask", async () => { - // The canvas and the panel deliberately say different things here, and both - // are true: the shape is the best answer anyone has until a better one - // arrives, and the card is about the request that is out. What must not - // happen is the shape blanking — that is `paintSuggestion` testing what the - // session holds rather than what its status is. await open(); await arm(); clickCanvas(); await screen.findByTestId("suggestion-shape"); - const gate = held(); - clickCanvas(); - - await waitFor(() => expect(screen.getByTestId("suggest-asking")).toBeTruthy()); - expect(screen.getByTestId("suggestion-shape")).toBeTruthy(); - expect(screen.queryByTestId("suggest-accept")).toBeNull(); - - gate.release(); - await waitFor(() => - expect((screen.getByTestId("suggest-accept") as HTMLButtonElement).disabled).toBe(false), - ); - }); - - it("anchors the halo on the newest click, which is the one being waited for", async () => { - await open(); - await arm(); - clickCanvasAt(100, 100); - await screen.findByTestId("suggestion-shape"); - const gate = held(); clickCanvasAt(140, 160); - - const halo = await screen.findByTestId("suggest-halo"); - const dots = screen.getByTestId("prompt-points").children; - const newest = dots[dots.length - 1]!; - expect(halo.getAttribute("cx")).toBe(newest.getAttribute("cx")); - expect(halo.getAttribute("cy")).toBe(newest.getAttribute("cy")); + await screen.findByTestId("suggest-asking"); + // The best answer anyone has stays on screen while the next one is fetched. + expect(screen.getByTestId("suggestion-shape")).toBeTruthy(); gate.release(); + await screen.findByTestId("suggest-shown"); }); - it("clears the indicator when the answer is a refusal", async () => { - // A refusal is an answer. The one thing this must not do is leave a halo - // pulsing over a request that is over — the panel carries the sentence. + it("clears the report when the answer is a refusal", async () => { const gate = held(); - suggestRefusal = { - status: 409, - code: "INFERENCE_CONNECTION_NOT_RUNNABLE", - message: "Install the local-inference extra to run this model.", - }; await open(); await arm(); clickCanvas(); - await screen.findByTestId("suggest-halo"); + await screen.findByTestId("suggest-asking"); + suggestRefusal = { status: 422, code: "UNSUPPORTED_PROMPT", message: "not a segmenter" }; gate.release(); await screen.findByTestId("suggest-refusal"); - // `waitFor`, not an immediate read: a refusal *is* an answer, so it goes out - // through the ordinary path and pays the visibility floor like any other. The - // claim is that it goes, not that it goes on the same tick. - await waitFor(() => expect(screen.queryByTestId("suggest-halo")).toBeNull()); - expect(screen.getByTestId("annotator-pane").style.cursor).not.toBe("progress"); + expect(screen.queryByTestId("suggest-asking")).toBeNull(); }); }); diff --git a/frontend/ui-core/src/annotator/suggestPanel.test.tsx b/frontend/ui-core/src/annotator/suggestPanel.test.tsx index b592d9c..1c75687 100644 --- a/frontend/ui-core/src/annotator/suggestPanel.test.tsx +++ b/frontend/ui-core/src/annotator/suggestPanel.test.tsx @@ -9,7 +9,7 @@ * never a dead button. */ -import { cleanup, render, screen } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { userEvent } from "@testing-library/user-event"; import { describe, expect, it, vi } from "vitest"; import type { JSX } from "react"; @@ -35,7 +35,7 @@ function answerOf(...suggestions: readonly Suggestion[]): Answer { modelRef: MODEL_REF, confidence: suggestions[0]?.confidence ?? null, suggestions, - parameters: ["detail", "fill_holes", "fragments"], + parameters: ["detail"], }; } @@ -453,7 +453,7 @@ describe("the adjustments, which are a section and never a popup", () => { contour: [[0, 0], [10, 0], [10, 10], [0, 10]], }, ], - parameters: ["detail", "fill_holes", "fragments"], + parameters: ["detail"], }; } @@ -468,28 +468,26 @@ describe("the adjustments, which are a section and never a popup", () => { expect(screen.queryByTestId("suggest-adjustments")).toBeNull(); }); - it("renders exactly the parameters the server declared, in its order", () => { + it("renders exactly the parameters the server declared, and nothing else", () => { render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), - onDetail: vi.fn(), onMaskAdjustment: vi.fn() })); - expect(screen.getByTestId("suggest-detail-balanced")).toBeTruthy(); - expect(screen.getByTestId("suggest-fill-holes")).toBeTruthy(); - expect(screen.getByTestId("suggest-fragments")).toBeTruthy(); + onDetail: vi.fn() })); + expect(screen.getByTestId("suggest-detail")).toBeTruthy(); + // The two that were here and are not (#557). A control wired to nothing on + // the ordinary mask is worse than no control. + expect(screen.queryByTestId("suggest-fill-holes")).toBeNull(); + expect(screen.queryByTestId("suggest-fragments")).toBeNull(); }); - it("offers a box class only what moves a box, because the wire says only that", () => { + it("offers a box class no section at all, because the wire declared nothing", () => { // The whole of the rule: no condition in this file mentions a box. Declare - // `fill_holes` for a box in the kernel's table and this test goes red there. + // `detail` for a box in the kernel's table and this test goes red there. const session = asked(); - const boxy = answered(session, session.serial, { - ...polygonAnswer(), - parameters: ["fragments"], - }); - render(mount({ session: boxy, adjusting: true, onAdjusting: vi.fn(), - onDetail: vi.fn(), onMaskAdjustment: vi.fn() })); + const boxy = answered(session, session.serial, { ...polygonAnswer(), parameters: [] }); + render(mount({ session: boxy, adjusting: true, onAdjusting: vi.fn(), onDetail: vi.fn() })); - expect(screen.getByTestId("suggest-fragments")).toBeTruthy(); - expect(screen.queryByTestId("suggest-detail-balanced")).toBeNull(); - expect(screen.queryByTestId("suggest-fill-holes")).toBeNull(); + expect(screen.queryByTestId("suggest-adjustments")).toBeNull(); + expect(screen.queryByTestId("suggest-adjust-open")).toBeNull(); + expect(screen.queryByTestId("suggest-detail")).toBeNull(); }); it("renders nothing at all where the server declared no parameters", () => { @@ -501,28 +499,39 @@ describe("the adjustments, which are a section and never a popup", () => { expect(screen.queryByTestId("suggest-adjust-open")).toBeNull(); }); - it("says what the current setting costs, beside the control that sets it", () => { + it("names the step and what it costs in one label, beside the control", () => { render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), - onDetail: vi.fn(), onMaskAdjustment: vi.fn() })); - expect(screen.getByTestId("suggest-vertex-count").textContent).toBe("4 pts"); + onDetail: vi.fn() })); + expect(screen.getByTestId("suggest-detail-label").textContent).toBe("Balanced · 4 pts"); }); - it("reports a step through the door that needs no request", async () => { - const onDetail = vi.fn(); + it("puts the slider on the step the session is holding", () => { render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), - onDetail, onMaskAdjustment: vi.fn() })); + onDetail: vi.fn() })); + const slider = screen.getByTestId("suggest-detail") as HTMLInputElement; + expect(slider.value).toBe("1"); + expect(slider.min).toBe("0"); + expect(slider.max).toBe("2"); + }); - await userEvent.click(screen.getByTestId("suggest-detail-coarse")); + it("reports a step through the door that needs no request", () => { + const onDetail = vi.fn(); + render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), onDetail })); + + fireEvent.change(screen.getByTestId("suggest-detail"), { target: { value: "0" } }); expect(onDetail).toHaveBeenCalledWith("coarse"); + fireEvent.change(screen.getByTestId("suggest-detail"), { target: { value: "2" } }); + expect(onDetail).toHaveBeenCalledWith("fine"); }); - it("reports a mask setting through the other door, which asks again", async () => { - const onMaskAdjustment = vi.fn(); + it("does not let a press on the slider take focus off the canvas", () => { + // Every chord in the editor is a keydown on the annotator's own root, so a + // control that took focus would switch `[`, `]`, Esc and Enter off with + // nothing on screen to say why (#557). render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), - onDetail: vi.fn(), onMaskAdjustment })); - - await userEvent.click(screen.getByTestId("suggest-fragments")); - expect(onMaskAdjustment).toHaveBeenCalledWith({ fragments: "all" }); + onDetail: vi.fn() })); + const press = fireEvent.mouseDown(screen.getByTestId("suggest-detail")); + expect(press).toBe(false); }); it("keeps the controls operable on an answer with nothing in it", () => { @@ -534,11 +543,10 @@ describe("the adjustments, which are a section and never a popup", () => { ...polygonAnswer(), suggestions: [], }); - render(mount({ session: empty, adjusting: true, onAdjusting: vi.fn(), - onDetail: vi.fn(), onMaskAdjustment: vi.fn() })); + render(mount({ session: empty, adjusting: true, onAdjusting: vi.fn(), onDetail: vi.fn() })); expect(screen.getByTestId("suggest-none")).toBeTruthy(); expect(screen.getByTestId("suggest-adjustments")).toBeTruthy(); - expect(screen.getByTestId("suggest-detail-balanced")).toBeTruthy(); + expect(screen.getByTestId("suggest-detail")).toBeTruthy(); }); }); diff --git a/frontend/ui-core/src/data/inferenceQueries.ts b/frontend/ui-core/src/data/inferenceQueries.ts index 4b266f9..a66879a 100644 --- a/frontend/ui-core/src/data/inferenceQueries.ts +++ b/frontend/ui-core/src/data/inferenceQueries.ts @@ -46,7 +46,6 @@ import type { Adjustments, Detail, - Fragments, GeometryType, SuggestParameter, } from "@visionset/annotator"; @@ -106,8 +105,6 @@ export interface SuggestionOut { readonly regions: readonly SuggestedRegion[]; readonly applied: { readonly detail: Detail; - readonly fill_holes: number; - readonly fragments: Fragments; }; /** * Which settings have any effect on the kind of shape this class holds. @@ -447,8 +444,6 @@ export function useSuggestRegion() { negative: input.negative.map(([x, y]) => ({ x, y })), allowed_geometries: [...input.allowedGeometries], detail: input.adjustments.detail, - fill_holes: input.adjustments.fillHoles, - fragments: input.adjustments.fragments, } as never, }), checkSuggestRegion, diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 5c28267..ddd0bf5 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -977,28 +977,29 @@ export interface paths { * * **`allowed_geometries` is the caller's schema, not a preference.** The answer * is produced in one of the kinds named or not at all: a class that admits - * polygons gets outlines, a class that admits only boxes gets their extents, - * and a class that admits neither gets no regions. Answering in a kind the - * schema would refuse would produce a suggestion that cannot be accepted. - * - * **The three parameters shape the mask, and none of them reaches the model.** - * `detail` decides how much of an outline survives simplification, `fill_holes` - * how wide a gap in the mask is closed before it is traced, and `fragments` - * whether the piece under the click is the answer or every piece worth - * proposing. Each is optional and each has a default, so a caller that sends - * none of them gets what this route always gave. - * - * **`parameters` says which of them apply here**, for the kind of shape this - * request will come back in — `detail` and `fill_holes` change an outline and a - * box has none — and it is present even when there is nothing to propose, so + * polygons gets the outline of the piece under the click, a class that admits + * only boxes gets one box over every piece the mask kept, and a class that + * admits neither gets no regions. Answering in a kind the schema would refuse + * would produce a suggestion that cannot be accepted. + * + * **`detail` is the one setting, and it does not reach the model.** It decides + * how much of an outline survives simplification. It is optional and defaults + * to `balanced`, which is what every suggestion used before there was a choice. + * Closing the small gaps in a mask and dropping its noise specks still happen, + * at fixed defaults nobody asks for. + * + * **`parameters` says which settings apply here**, for the kind of shape this + * request will come back in. It is empty for a box class — `detail` changes an + * outline and a box has none — which is how a client is told to render no + * adjustments at all. It is present even when there is nothing to propose, so * somebody who adjusted their way into an empty answer can adjust their way * back out. A client renders what this names and works none of it out itself. * - * **`contour` on each region is the unsimplified outline.** It is what lets a - * client re-run `detail` locally rather than asking again, and it is the *same* - * points this route reduced — simplification is not nested, so a client - * starting from anything else could not be held to the same answer. A box - * carries none, because there is nothing it was reduced from. + * **`contour` on each region is the outline the shape was reduced from.** It is + * what lets a client re-run `detail` locally rather than asking again, and it + * is the *same* points this route reduced — simplification is not nested, so a + * client starting from anything else could not be held to the same answer. A + * box carries none, because there is nothing it was reduced from. * * **Every point must be on the asset**, positive and negative alike — `x` in * `[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own @@ -2264,9 +2265,6 @@ export interface components { */ AppliedParameters: { detail: components["schemas"]["Detail"]; - /** Fill Holes */ - fill_holes: number; - fragments: components["schemas"]["Fragments"]; }; /** * AssetAction @@ -3104,16 +3102,6 @@ export interface components { /** Total */ total: number; }; - /** - * Fragments - * @description How many of a mask's separate pieces become shapes. - * - * `one` is the piece your points are on, not the biggest piece on the frame: - * which of them you meant is a question only the prompt can answer. `all` - * proposes every piece big enough to be worth proposing. - * @enum {string} - */ - Fragments: "one" | "all"; /** * GeometryType * @description Every geometry the domain can address. @@ -3870,7 +3858,7 @@ export interface components { * @description A setting that shapes a suggestion. Order is display order. * @enum {string} */ - SuggestParameter: "detail" | "fill_holes" | "fragments"; + SuggestParameter: "detail"; /** * SuggestPoint * @description One click, in the asset's own pixel coordinates. @@ -3916,13 +3904,6 @@ export interface components { connection_id: string; /** @default balanced */ detail: components["schemas"]["Detail"]; - /** - * Fill Holes - * @default 0.002 - */ - fill_holes: number; - /** @default one */ - fragments: components["schemas"]["Fragments"]; /** Negative */ negative?: components["schemas"]["SuggestPoint"][]; /** Positive */ @@ -3953,7 +3934,7 @@ export interface components { * `regions` is empty when there is no suggestion, and that is an ordinary * answer rather than an error: a click can land on sky, the model can be less * sure than the caller asked for, the shape found can be one this class cannot - * hold, and the parameters as set can leave nothing. A 404 or a 409 for any of + * hold, and the detail as set can leave nothing. A 404 or a 409 for any of * those would be telling the caller they did something wrong when they did not. * * `model_ref` is echoed on every answer, including the empty one, because it @@ -3965,8 +3946,10 @@ export interface components { * * `parameters` names which settings have any effect on the kind of shape this * request will come back in, so a client renders exactly those and works none - * of it out for itself. It is present on an empty answer too, which is what - * lets somebody who adjusted their way into nothing adjust their way back out. + * of it out for itself. It is empty for a box class, which is how a client is + * told to render no adjustments at all. It is present on an empty answer too, + * which is what lets somebody who adjusted their way into nothing adjust their + * way back out. */ SuggestionOut: { applied: components["schemas"]["AppliedParameters"]; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 3b0acf5..fa6c338 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -290,14 +290,11 @@ export const checkSplitAssignmentOut: Check = export const checkDetail: Check = /*#__PURE__*/ oneOf(["coarse", "balanced", "fine"] as const); -export const checkFragments: Check = - /*#__PURE__*/ oneOf(["one", "all"] as const); - export const checkAppliedParameters: Check = - /*#__PURE__*/ object({ "detail": [true, checkDetail], "fill_holes": [true, isNumber], "fragments": [true, checkFragments] } as const); + /*#__PURE__*/ object({ "detail": [true, checkDetail] } as const); export const checkSuggestParameter: Check = - /*#__PURE__*/ oneOf(["detail", "fill_holes", "fragments"] as const); + /*#__PURE__*/ oneOf(["detail"] as const); export const checkBboxGeometry: Check = /*#__PURE__*/ object({ "height": [true, isNumber], "type": [true, lit("bbox")], "width": [true, isNumber], "x": [true, isNumber], "y": [true, isNumber] } as const); diff --git a/openapi.json b/openapi.json index 85c6202..bee8dc9 100644 --- a/openapi.json +++ b/openapi.json @@ -428,19 +428,10 @@ "properties": { "detail": { "$ref": "#/components/schemas/Detail" - }, - "fill_holes": { - "title": "Fill Holes", - "type": "number" - }, - "fragments": { - "$ref": "#/components/schemas/Fragments" } }, "required": [ - "detail", - "fill_holes", - "fragments" + "detail" ], "title": "AppliedParameters", "type": "object" @@ -2347,15 +2338,6 @@ "title": "FormatPage", "type": "object" }, - "Fragments": { - "description": "How many of a mask's separate pieces become shapes.\n\n`one` is the piece your points are on, not the biggest piece on the frame:\nwhich of them you meant is a question only the prompt can answer. `all`\nproposes every piece big enough to be worth proposing.", - "enum": [ - "one", - "all" - ], - "title": "Fragments", - "type": "string" - }, "GeometryType": { "description": "Every geometry the domain can address.\n\n3D values exist today even though unimplemented: the domain never assumes\n\"image\" anywhere \u2014 that is the Physical AI roadmap encoded as a type.", "enum": [ @@ -3815,9 +3797,7 @@ "SuggestParameter": { "description": "A setting that shapes a suggestion. Order is display order.", "enum": [ - "detail", - "fill_holes", - "fragments" + "detail" ], "title": "SuggestParameter", "type": "string" @@ -3868,17 +3848,6 @@ "$ref": "#/components/schemas/Detail", "default": "balanced" }, - "fill_holes": { - "default": 0.002, - "maximum": 1.0, - "minimum": 0.0, - "title": "Fill Holes", - "type": "number" - }, - "fragments": { - "$ref": "#/components/schemas/Fragments", - "default": "one" - }, "negative": { "items": { "$ref": "#/components/schemas/SuggestPoint" @@ -3965,7 +3934,7 @@ "type": "object" }, "SuggestionOut": { - "description": "What the model proposes, or an honest nothing.\n\n`regions` is empty when there is no suggestion, and that is an ordinary\nanswer rather than an error: a click can land on sky, the model can be less\nsure than the caller asked for, the shape found can be one this class cannot\nhold, and the parameters as set can leave nothing. A 404 or a 409 for any of\nthose would be telling the caller they did something wrong when they did not.\n\n`model_ref` is echoed on every answer, including the empty one, because it\nis what an accepted suggestion has to carry into its annotation \u2014 and a\ncaller that had to remember which connection it asked would be keeping a\nsecond copy of something the response can simply state. `confidence` is the\nsame: one number for the answer, because the model scored one mask and the\npieces cut out of it are that same claim seen in parts.\n\n`parameters` names which settings have any effect on the kind of shape this\nrequest will come back in, so a client renders exactly those and works none\nof it out for itself. It is present on an empty answer too, which is what\nlets somebody who adjusted their way into nothing adjust their way back out.", + "description": "What the model proposes, or an honest nothing.\n\n`regions` is empty when there is no suggestion, and that is an ordinary\nanswer rather than an error: a click can land on sky, the model can be less\nsure than the caller asked for, the shape found can be one this class cannot\nhold, and the detail as set can leave nothing. A 404 or a 409 for any of\nthose would be telling the caller they did something wrong when they did not.\n\n`model_ref` is echoed on every answer, including the empty one, because it\nis what an accepted suggestion has to carry into its annotation \u2014 and a\ncaller that had to remember which connection it asked would be keeping a\nsecond copy of something the response can simply state. `confidence` is the\nsame: one number for the answer, because the model scored one mask and the\npieces cut out of it are that same claim seen in parts.\n\n`parameters` names which settings have any effect on the kind of shape this\nrequest will come back in, so a client renders exactly those and works none\nof it out for itself. It is empty for a box class, which is how a client is\ntold to render no adjustments at all. It is present on an empty answer too,\nwhich is what lets somebody who adjusted their way into nothing adjust their\nway back out.", "properties": { "applied": { "$ref": "#/components/schemas/AppliedParameters" @@ -7335,7 +7304,7 @@ }, "/inference/suggest": { "post": { - "description": "Propose a shape for the thing under those points.\n\nThe server side of the editor's suggest gesture. One asset, one prompt set,\none answer \u2014 batch prediction is a separate path and is not this one.\n\n**Nothing is written and nothing is remembered.** A suggestion is a proposal:\naccepting it is a later, ordinary annotation write carrying `provenance:\nmodel`, this response's `model_ref`, and its `confidence`. Discarding it\ncosts a request that already finished. The only thing that outlives the call\nis a cached image embedding, which is an optimisation rather than a record \u2014\nso the same points sent twice answer the same way, and a restart changes\nnothing but the latency of the first click.\n\n**The first click on an asset is the slow one.** A segmenter reads the whole\nimage once and then answers any number of clicks from that reading almost for\nfree, which is what makes refining by adding points practical. Sending the\naccumulated points \u2014 rather than a diff \u2014 is what keeps this stateless.\n\n**`allowed_geometries` is the caller's schema, not a preference.** The answer\nis produced in one of the kinds named or not at all: a class that admits\npolygons gets outlines, a class that admits only boxes gets their extents,\nand a class that admits neither gets no regions. Answering in a kind the\nschema would refuse would produce a suggestion that cannot be accepted.\n\n**The three parameters shape the mask, and none of them reaches the model.**\n`detail` decides how much of an outline survives simplification, `fill_holes`\nhow wide a gap in the mask is closed before it is traced, and `fragments`\nwhether the piece under the click is the answer or every piece worth\nproposing. Each is optional and each has a default, so a caller that sends\nnone of them gets what this route always gave.\n\n**`parameters` says which of them apply here**, for the kind of shape this\nrequest will come back in \u2014 `detail` and `fill_holes` change an outline and a\nbox has none \u2014 and it is present even when there is nothing to propose, so\nsomebody who adjusted their way into an empty answer can adjust their way\nback out. A client renders what this names and works none of it out itself.\n\n**`contour` on each region is the unsimplified outline.** It is what lets a\nclient re-run `detail` locally rather than asking again, and it is the *same*\npoints this route reduced \u2014 simplification is not nested, so a client\nstarting from anything else could not be held to the same answer. A box\ncarries none, because there is nothing it was reduced from.\n\n**Every point must be on the asset**, positive and negative alike \u2014 `x` in\n`[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own\npixel frame. One point off the picture refuses the whole request with 422\n`PROMPT_POINT_OUT_OF_BOUNDS` rather than being dropped, because a gesture\nwith a point removed is a different gesture. Nothing is clamped: a\ncoordinate outside the frame is not a place on the image, and answering\nabout the nearest edge instead would return a mask, and a confidence, for a\nquestion nobody asked.\n\nAn empty `regions` is a successful answer with nothing to propose. Refusals\nare reserved for things the caller can act on: an unknown project, asset or\nconnection is 404; a connection whose weights are not here yet, or whose kind\nthis build cannot run, is 409 and names what to do; a connection whose model\nanswers words rather than places is 422, as is a prompt point off the asset.", + "description": "Propose a shape for the thing under those points.\n\nThe server side of the editor's suggest gesture. One asset, one prompt set,\none answer \u2014 batch prediction is a separate path and is not this one.\n\n**Nothing is written and nothing is remembered.** A suggestion is a proposal:\naccepting it is a later, ordinary annotation write carrying `provenance:\nmodel`, this response's `model_ref`, and its `confidence`. Discarding it\ncosts a request that already finished. The only thing that outlives the call\nis a cached image embedding, which is an optimisation rather than a record \u2014\nso the same points sent twice answer the same way, and a restart changes\nnothing but the latency of the first click.\n\n**The first click on an asset is the slow one.** A segmenter reads the whole\nimage once and then answers any number of clicks from that reading almost for\nfree, which is what makes refining by adding points practical. Sending the\naccumulated points \u2014 rather than a diff \u2014 is what keeps this stateless.\n\n**`allowed_geometries` is the caller's schema, not a preference.** The answer\nis produced in one of the kinds named or not at all: a class that admits\npolygons gets the outline of the piece under the click, a class that admits\nonly boxes gets one box over every piece the mask kept, and a class that\nadmits neither gets no regions. Answering in a kind the schema would refuse\nwould produce a suggestion that cannot be accepted.\n\n**`detail` is the one setting, and it does not reach the model.** It decides\nhow much of an outline survives simplification. It is optional and defaults\nto `balanced`, which is what every suggestion used before there was a choice.\nClosing the small gaps in a mask and dropping its noise specks still happen,\nat fixed defaults nobody asks for.\n\n**`parameters` says which settings apply here**, for the kind of shape this\nrequest will come back in. It is empty for a box class \u2014 `detail` changes an\noutline and a box has none \u2014 which is how a client is told to render no\nadjustments at all. It is present even when there is nothing to propose, so\nsomebody who adjusted their way into an empty answer can adjust their way\nback out. A client renders what this names and works none of it out itself.\n\n**`contour` on each region is the outline the shape was reduced from.** It is\nwhat lets a client re-run `detail` locally rather than asking again, and it\nis the *same* points this route reduced \u2014 simplification is not nested, so a\nclient starting from anything else could not be held to the same answer. A\nbox carries none, because there is nothing it was reduced from.\n\n**Every point must be on the asset**, positive and negative alike \u2014 `x` in\n`[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own\npixel frame. One point off the picture refuses the whole request with 422\n`PROMPT_POINT_OUT_OF_BOUNDS` rather than being dropped, because a gesture\nwith a point removed is a different gesture. Nothing is clamped: a\ncoordinate outside the frame is not a place on the image, and answering\nabout the nearest edge instead would return a mask, and a confidence, for a\nquestion nobody asked.\n\nAn empty `regions` is a successful answer with nothing to propose. Refusals\nare reserved for things the caller can act on: an unknown project, asset or\nconnection is 404; a connection whose weights are not here yet, or whose kind\nthis build cannot run, is 409 and names what to do; a connection whose model\nanswers words rather than places is 422, as is a prompt point off the asset.", "operationId": "suggest_region", "requestBody": { "content": { diff --git a/src/visionset/inference/masks.py b/src/visionset/inference/masks.py index 7abeb8b..1e096cd 100644 --- a/src/visionset/inference/masks.py +++ b/src/visionset/inference/masks.py @@ -17,15 +17,20 @@ **The pipeline is fixed and its order is not configurable.** -1. :func:`components` — which pieces of the mask survive. -2. :func:`filled` — the gaps in them narrower than a reach, closed or kept. +1. :func:`components` — which pieces of the mask survive the noise filter. +2. :func:`filled` — the gaps in them narrower than a reach, closed. 3. :func:`contour` — the boundary of what is left. 4. :func:`polygon_at` — that boundary, reduced to a vertex count somebody can edit. -The geometry branch happens after step 2: a polygon class takes steps 3 and 4, a -box class takes the filled piece's extent. **A box therefore does not depend on -``detail``**, which is what "applies to polygon only" means once it is code -rather than a table. +The geometry branch happens after step 2: a polygon class takes steps 3 and 4 on +the piece the prompt points at, a box class takes one extent over *every* +surviving piece. **A box therefore does not depend on ``detail``**, which is what +"applies to polygon only" means once it is code rather than a table. + +**Only one of these is a question anybody is asked.** The reach of the close and +the noise floor are fixed here, because on the ordinary single clean piece every +setting of either produced the same shape — controls wired to nothing (#557). +``detail`` is the one that moves something a person can see. **Which shape is produced is the caller's schema decision, not this module's guess.** :func:`shapes_from` takes the geometry kinds the active class actually @@ -61,11 +66,8 @@ class allowing only boxes gets extents, and a class allowing neither is not from visionset.kernel.domain import ( DEFAULT_DETAIL, - DEFAULT_FILL_HOLES, - DEFAULT_FRAGMENTS, BboxGeometry, Detail, - Fragments, Geometry, GeometryType, Mask, @@ -102,14 +104,43 @@ class allowing only boxes gets extents, and a class allowing neither is not """ MINIMUM_FRAGMENT_SHARE: Final = 0.05 -"""How big a piece has to be, against the biggest one, to be worth proposing. - -Only consulted when the caller asked for every piece. A segmenter's mask -routinely carries specks a twentieth the size of the thing that was clicked — -antialiasing along an edge, a reflection, a scrap of the same colour across the -frame — and proposing each of them as its own annotation turns one click into a -cleanup job. Relative to the largest piece rather than to the frame, so it means -the same thing on a mask covering everything and a mask covering a corner. +"""How big a piece has to be, against the biggest one, to survive the noise filter. + +A segmenter's mask routinely carries specks a twentieth the size of the thing +that was clicked — antialiasing along an edge, a reflection, a scrap of the same +colour across the frame. None of them is ever the answer to a click, so they are +dropped before anything else looks at the mask. Relative to the largest piece +rather than to the frame, so it means the same thing on a mask covering +everything and a mask covering a corner. + +Applied unconditionally, and that is a decision rather than a simplification +(#557): as a setting it did nothing on the ordinary single clean piece and could +only be got wrong on the unusual one. +""" + +CLOSING_REACH: Final = 0.002 +"""The largest gap closed, as a share of the piece's own lit area. + +A share rather than a pixel count, so it means the same thing on a thing fifty +pixels across and a thing five hundred across. Two parts in a thousand puts the +reach at about one pixel on an object fifty across, two on one a hundred across +and four on one two hundred across — the scale of the notches and bays a +segmenter leaves along an edge, and well under anything somebody would call a +feature of the shape. + +Fixed rather than asked for (#557). It lives here rather than in the domain +because it is a number about *this* pipeline, the way :data:`EPSILON` is. +""" + +MAXIMUM_CLOSING_RADIUS: Final = 6 +"""However large the piece, the reach stops here. + +Two reasons that agree. A gap wider than a few pixels is a feature of the shape +rather than an artefact of tracing it, so a reach that keeps growing with the +object eventually closes a real bay somebody wanted; and the close costs a pass +per unit of radius, on the path somebody is waiting on after a click — an +unbounded reach worked out at 22 on a 4K frame, which is 44 passes to bridge +gaps that were never there. """ @@ -334,16 +365,20 @@ def _cropped(label: int, found: Sequence[tuple[int, int, int]], labels: Sequence return Piece(x=left, y=top, mask=grid) -def components( - mask: Mask, *, fragments: Fragments = DEFAULT_FRAGMENTS, at: Sequence[Point] = () -) -> list[Piece]: +def components(mask: Mask, *, at: Sequence[Point] = ()) -> list[Piece]: """Step 1 — the pieces of the mask worth turning into shapes. - ``ONE`` is the piece the prompt points at, per :func:`_pointed_at`. ``ALL`` - is every piece at or above :data:`MINIMUM_FRAGMENT_SHARE` of the largest, - ordered biggest first so that a panel showing several proposals leads with - the one most likely to be the thing that was clicked; ties keep reading - order, so the result is stable for a given mask. + Everything below :data:`MINIMUM_FRAGMENT_SHARE` of the largest piece is + dropped as noise, first and unconditionally. What survives is ordered with + the piece the prompt points at (:func:`_pointed_at`) at the head and the rest + biggest-first behind it, ties in reading order, so the answer is stable for a + given mask. + + **That one ordering is the whole difference between the two geometries.** A + polygon takes the head of the list, because a click asks about one object; a + box takes the union of all of it, because a mask arriving in several pieces + is nearly always one object seen around an occlusion. Neither branch needs a + setting to say which it wants. An empty mask answers with no pieces, which is an ordinary answer and not an error — the click landed on sky. @@ -354,15 +389,12 @@ def components( labels = _components(found) size = _areas(found, labels) - if fragments is Fragments.ONE: - chosen = [_pointed_at(found, labels, size, at)] - else: - floor = max(size.values()) * MINIMUM_FRAGMENT_SHARE - chosen = sorted( - (label for label, area in size.items() if area >= floor), - key=lambda label: (-size[label], label), - ) - return [_cropped(label, found, labels) for label in chosen] + floor = max(size.values()) * MINIMUM_FRAGMENT_SHARE + survived = {label for label, area in size.items() if area >= floor} + kept = [(run, label) for run, label in zip(found, labels, strict=True) if label in survived] + first = _pointed_at([run for run, _ in kept], [label for _, label in kept], size, at) + rest = sorted(survived - {first}, key=lambda label: (-size[label], label)) + return [_cropped(label, found, labels) for label in (first, *rest)] def _bits(mask: Mask, *, pad: int) -> tuple[list[int], int]: @@ -433,23 +465,22 @@ def _met(rows: Sequence[int]) -> int: return met -def closing_radius(mask: Mask, *, fill_holes: float) -> int: - """How far to reach, for a piece of this size and that share. +def closing_radius(mask: Mask) -> int: + """How far to reach, for a piece of this size. - The share names the largest *hole* to close, and a hole of area ``a`` needs a - reach of about ``sqrt(a) / 2`` to be bridged — so the radius comes from the - piece's own lit area rather than from a pixel count, and the setting means - the same thing on a thing fifty pixels across and a thing five hundred + :data:`CLOSING_REACH` names the largest *hole* to close, and a hole of area + ``a`` needs a reach of about ``sqrt(a) / 2`` to be bridged — so the radius + comes from the piece's own lit area rather than from a pixel count, and it + means the same thing on a thing fifty pixels across and a thing five hundred across. Rounded down, so the smallest shapes get no closing at all rather - than one that would swallow a feature. + than one that would swallow a feature, and capped at + :data:`MAXIMUM_CLOSING_RADIUS` at the other end. """ - if fill_holes <= 0.0: - return 0 lit = sum(last - first + 1 for _, first, last in runs(mask)) - return int((lit * fill_holes) ** 0.5 / 2) + return min(int((lit * CLOSING_REACH) ** 0.5 / 2), MAXIMUM_CLOSING_RADIUS) -def filled(mask: Mask, *, fill_holes: float = DEFAULT_FILL_HOLES) -> Mask: +def filled(mask: Mask) -> Mask: """Step 2 — close the small gaps in a piece, wherever they are. A morphological close: grow the shape, then shrink it back by the same @@ -466,14 +497,11 @@ def filled(mask: Mask, *, fill_holes: float = DEFAULT_FILL_HOLES) -> Mask: bites out of an edge, the notch where two strokes almost meet, the pinhole at a corner. - ``0.0`` closes nothing and is a legitimate request rather than a disabled - feature: a mask of foliage is mostly gaps and every one of them is real. - Returns the mask unchanged — the same object — when the reach works out at nothing or the shape has no gap that narrow, which is the common case and saves rebuilding a grid to say so. """ - radius = closing_radius(mask, fill_holes=fill_holes) + radius = closing_radius(mask) if radius < 1: return mask before, width = _bits(mask, pad=radius) @@ -689,23 +717,52 @@ def target_kind(allowed: Sequence[GeometryType]) -> GeometryType | None: return None +def _union_of(pieces: Sequence[Piece]) -> BboxGeometry | None: + """One box over every piece, or ``None`` if none of them holds anything. + + The answer to a point prompt is *this object*, and a mask that arrives in + several pieces is nearly always one object seen around an occlusion — a + railing across an animal, a post in front of a car. Both of the alternatives + are wrong in exactly that case: the largest piece alone cuts the object off + at the occlusion, and a box per piece annotates one thing twice (#557). + """ + boxes = [box for box in (_boxed(piece) for piece in pieces) if box is not None] + if not boxes: + return None + left = min(box.x for box in boxes) + top = min(box.y for box in boxes) + right = max(box.x + box.width for box in boxes) + bottom = max(box.y + box.height for box in boxes) + return BboxGeometry(x=left, y=top, width=right - left, height=bottom - top) + + def shapes_from( mask: Mask, *, allowed: Sequence[GeometryType], detail: Detail = DEFAULT_DETAIL, - fill_holes: float = DEFAULT_FILL_HOLES, - fragments: Fragments = DEFAULT_FRAGMENTS, at: Sequence[Point] = (), ) -> list[Shaped]: """The whole pipeline: a mask and a class's geometries in, proposals out. The four steps in their fixed order, with the geometry branch after the - second. A class that admits polygons gets outlines; one that admits only - boxes gets extents, measured off the filled piece rather than off a - simplified outline's corners — which is what keeps ``detail`` from quietly + first. A class that admits polygons gets the outline of the piece the prompt + points at; one that admits only boxes gets a single box over every piece that + survived the noise filter, measured off the mask's own extent rather than off + a simplified outline's corners — which is what keeps ``detail`` from quietly moving a box. + **The branch is before the close, and the close is paid for once.** A close + only ever adds pixels whose whole neighbourhood was already reachable, so it + cannot push an edge outward and a box is the same box either way + (``test_closing_never_moves_the_extent``). So a box skips it entirely, and a + polygon runs it on the one piece it is about to trace — rather than on every + piece that survived, which is work thrown away for all but one of them. + + **A list, though today it holds at most one.** The plural shape is kept + because accepting part of a plural proposal is tracked work (#548) and + because an empty list is how "nothing to propose" is already said. + **Nothing is ever widened.** A class admitting neither kind gets an empty list, and a piece too thin to be a polygon is dropped rather than demoted to a box: answering in a kind the caller did not ask for is how a suggestion @@ -715,16 +772,16 @@ def shapes_from( if kind is None: return [] - shaped: list[Shaped] = [] - for piece in components(mask, fragments=fragments, at=at): - whole = Piece(x=piece.x, y=piece.y, mask=filled(piece.mask, fill_holes=fill_holes)) - if kind is GeometryType.POLYGON: - traced = _shifted(contour(whole.mask), piece=whole) - polygon = polygon_at(traced, detail=detail) - if polygon is not None: - shaped.append(Shaped(geometry=polygon, contour=tuple(traced))) - continue - box = _boxed(whole) - if box is not None: - shaped.append(Shaped(geometry=box)) - return shaped + pieces = components(mask, at=at) + if not pieces: + return [] + + if kind is GeometryType.BBOX: + box = _union_of(pieces) + return [] if box is None else [Shaped(geometry=box)] + + pointed = pieces[0] + whole = Piece(x=pointed.x, y=pointed.y, mask=filled(pointed.mask)) + traced = _shifted(contour(whole.mask), piece=whole) + polygon = polygon_at(traced, detail=detail) + return [] if polygon is None else [Shaped(geometry=polygon, contour=tuple(traced))] diff --git a/src/visionset/inference/suggestions.py b/src/visionset/inference/suggestions.py index 6a2372c..9fa977b 100644 --- a/src/visionset/inference/suggestions.py +++ b/src/visionset/inference/suggestions.py @@ -33,10 +33,7 @@ from visionset.inference.providers import ProviderPool, resident from visionset.kernel.domain import ( DEFAULT_DETAIL, - DEFAULT_FILL_HOLES, - DEFAULT_FRAGMENTS, Detail, - Fragments, GeometryType, PointPrompt, PredictionRequest, @@ -91,8 +88,6 @@ def suggest( prompt: PointPrompt, allowed: tuple[GeometryType, ...], detail: Detail = DEFAULT_DETAIL, - fill_holes: float = DEFAULT_FILL_HOLES, - fragments: Fragments = DEFAULT_FRAGMENTS, minimum_confidence: float = 0.0, pool: ProviderPool | None = None, ) -> Suggestion: @@ -106,7 +101,7 @@ def suggest( An empty ``shapes`` is a real answer and not a failure: the model was asked about a patch of sky, or was not sure enough, or the shape it found cannot be - expressed in the kinds this class admits, or the parameters as set leave + expressed in the kinds this class admits, or the detail as set leaves nothing. Every one of those is "no suggestion", and none of them is an error somebody made. @@ -167,14 +162,7 @@ def suggest( segment = answer.segments[0] at: tuple[Point, ...] = tuple(prompt.positive) - shapes = shapes_from( - segment.mask, - allowed=allowed, - detail=detail, - fill_holes=fill_holes, - fragments=fragments, - at=at, - ) + shapes = shapes_from(segment.mask, allowed=allowed, detail=detail, at=at) return Suggestion( model_ref=answer.model_ref, shapes=tuple(shapes), diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index a815dba..eed0e31 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -182,11 +182,8 @@ ) from visionset.kernel.domain.suggestion import ( DEFAULT_DETAIL, - DEFAULT_FILL_HOLES, - DEFAULT_FRAGMENTS, PARAMETER_APPLIES_TO, Detail, - Fragments, SuggestParameter, suggest_parameters, ) @@ -367,12 +364,9 @@ "Mask", "SegmentedMask", "Detail", - "Fragments", "SuggestParameter", "PARAMETER_APPLIES_TO", "DEFAULT_DETAIL", - "DEFAULT_FILL_HOLES", - "DEFAULT_FRAGMENTS", "suggest_parameters", "ActivityEntry", "ActivityKind", diff --git a/src/visionset/kernel/domain/suggestion.py b/src/visionset/kernel/domain/suggestion.py index b630f7e..751102c 100644 --- a/src/visionset/kernel/domain/suggestion.py +++ b/src/visionset/kernel/domain/suggestion.py @@ -1,11 +1,12 @@ -# usage: from visionset.kernel.domain import Detail, Fragments, suggest_parameters +# usage: from visionset.kernel.domain import Detail, suggest_parameters """How a model's mask becomes a shape, said out loud so a client can offer it. A segmenter answers a click with a grid of booleans, and turning that grid into a polygon or a box is a chain of choices: which blobs to keep, whether to close the -holes inside them, how much of the traced outline survives. Those choices used to -be made once, invisibly, by whatever ran the model. This module is the vocabulary -for making them askable. +holes inside them, how much of the traced outline survives. Most of those choices +are made once, with a fixed default, by the pipeline — and one of them is worth +putting in front of the person looking at the proposal. This module is the +vocabulary for the one. **The names are this domain's, not an imaging library's.** ``detail`` is a question about the shape somebody is going to edit; ``epsilon`` is a parameter of @@ -13,12 +14,20 @@ would publish an implementation as a contract and make a second implementation a breaking change. -**Applicability is declared, never derived.** ``detail`` and ``fill_holes`` are -about an outline, so they mean nothing for a class that stores a box; a client -that worked that out for itself would be the hand-mirrored table -``capabilities.py`` exists to prevent, and it would drift the first time a -parameter changed hands. So :data:`PARAMETER_APPLIES_TO` states it once and -:func:`suggest_parameters` is the only reader. +**Applicability is declared, never derived.** ``detail`` is about an outline, so +it means nothing for a class that stores a box; a client that worked that out for +itself would be the hand-mirrored table ``capabilities.py`` exists to prevent, and +it would drift the first time a parameter changed hands. So +:data:`PARAMETER_APPLIES_TO` states it once and :func:`suggest_parameters` is the +only reader. + +**Two settings were here and are not.** Closing the gaps in a mask and dropping +its noise specks are still done, at fixed defaults that live beside the pipeline +in ``visionset.inference.masks``. They stopped being askable because on an +ordinary single clean piece every position of either gave the same shape, so they +read as controls wired to nothing (#557). Their value is in the default rather +than in the choice; they come back as parameters if a real need for the choice +appears. Pure, and in the domain rather than beside the code that computes the pipeline, on ``capabilities.py``'s terms: a question about domain values, answered from a @@ -58,77 +67,31 @@ class Detail(StrEnum): FINE = "fine" -# ``ONE`` rather than ``LARGEST``, because the blob it selects is the one the -# prompt points at rather than the biggest one on the frame. That distinction is -# the whole of the gesture: a point-prompted segmenter is asked about a *place*, -# and the blob owning the topmost-leftmost lit pixel is a fact about where -# speckle fell. Naming this ``largest`` would describe a tie-break as though it -# were the rule. -class Fragments(StrEnum): - """How many of a mask's separate pieces become shapes. - - `one` is the piece your points are on, not the biggest piece on the frame: - which of them you meant is a question only the prompt can answer. `all` - proposes every piece big enough to be worth proposing. - """ - - #: The piece the prompt points at, and nothing else. - ONE = "one" - #: Every piece big enough to be worth proposing. - ALL = "all" - - # One member per parameter, and the table below owes every one of them a row. class SuggestParameter(StrEnum): """A setting that shapes a suggestion. Order is display order.""" DETAIL = "detail" - FILL_HOLES = "fill_holes" - FRAGMENTS = "fragments" DEFAULT_DETAIL: Final = Detail.BALANCED """What a caller that says nothing gets, and what every suggestion got before.""" -DEFAULT_FRAGMENTS: Final = Fragments.ONE -"""One shape per click, which is what a click asks for.""" - - -DEFAULT_FILL_HOLES: Final = 0.002 -"""The largest gap closed by default, as a share of the piece's area. - -A share rather than a pixel count, so it means the same thing on a thing fifty -pixels across and a thing five hundred across. Two parts in a thousand puts the -reach at about one pixel on an object fifty across, two on one a hundred across -and four on one two hundred across — the scale of the notches and bays a -segmenter leaves along an edge, and well under anything somebody would call a -feature of the shape. - -Zero closes nothing, which is a legitimate thing to ask for and not a disabled -feature: a mask of foliage is mostly gaps, and every one of them is real. -""" - - PARAMETER_APPLIES_TO: Final[Mapping[SuggestParameter, frozenset[GeometryType]]] = { SuggestParameter.DETAIL: frozenset({GeometryType.POLYGON}), - SuggestParameter.FILL_HOLES: frozenset({GeometryType.POLYGON}), - SuggestParameter.FRAGMENTS: frozenset({GeometryType.POLYGON, GeometryType.BBOX}), } """Which geometries each parameter has any effect on. -``detail`` and ``fill_holes`` are about an *outline*: one decides how many -vertices it keeps and the other decides whether it detours into every notch -along the way. A box has no outline to spend either on — closing a gap cannot -move an extent outward, and a vertex budget has nothing to buy — so offering -them for a box class would be offering controls that do nothing. - -``fragments`` applies to both, because it decides how many shapes there are -before any of them has a kind. +``detail`` is about an *outline*: it decides how many vertices that outline keeps. +A box has no outline to spend a vertex budget on, so offering it for a box class +would be offering a control that does nothing. A box class therefore declares no +parameters at all, and a client shows it no adjustments — which is a rendering +rule the client reads rather than one it works out (#557). **A parameter missing from this mapping is a test failure, not a default.** ``test_every_parameter_declares_the_geometries_it_applies_to`` sweeps -``SuggestParameter`` against these keys, so a fourth parameter arrives with its +``SuggestParameter`` against these keys, so a second parameter arrives with its applicability stated or it does not arrive. The alternative — treating an absent row as "applies to everything" — is the one that ships a control nobody can use and nobody notices. @@ -140,8 +103,9 @@ def suggest_parameters(geometry: GeometryType) -> tuple[SuggestParameter, ...]: The only reader of :data:`PARAMETER_APPLIES_TO`, so that a surface deciding what to render never re-derives the rule. A geometry no parameter applies to - answers empty, which is honest rather than exceptional: a class that holds no - shape gets no suggestion to adjust in the first place. + answers empty, which is honest rather than exceptional: it is how a box class + is told there is nothing here to adjust, and a class that holds no shape gets + no suggestion to adjust in the first place. """ return tuple( parameter for parameter in SuggestParameter if geometry in PARAMETER_APPLIES_TO[parameter] diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 054f37c..b4b6a71 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -48,8 +48,6 @@ from visionset.inference import capabilities_of from visionset.kernel.domain import ( DEFAULT_DETAIL, - DEFAULT_FILL_HOLES, - DEFAULT_FRAGMENTS, ActivityEntry, ActivityKind, Annotation, @@ -84,7 +82,6 @@ Detail, DownloadSize, ExportCompatibility, - Fragments, Geometry, GeometryType, ImageFormat, @@ -1938,21 +1935,19 @@ class SuggestRequest(BaseModel): #: How much of an outline survives simplification. Omitted means `balanced`, #: which is what every suggestion used before there was a choice. detail: Detail = DEFAULT_DETAIL - #: The largest gap closed in the model's mask, as a share of the piece's own - #: area. Zero closes nothing. - fill_holes: float = Field(DEFAULT_FILL_HOLES, ge=0.0, le=1.0) - #: Whether the piece under the click is the answer, or every piece big enough - #: to be worth proposing. - fragments: Fragments = DEFAULT_FRAGMENTS class SuggestedRegion(BaseModel): """One proposed shape, and the contour it was reduced from.""" geometry: Geometry - #: The unsimplified outline, in the asset's own pixels — what lets a client - #: re-run `detail` locally instead of asking again. Empty for a box, which is - #: an extent rather than something reduced from anything. + #: The outline the shape was reduced from, in the asset's own pixels — what + #: lets a client re-run `detail` locally instead of asking again. Already + #: reduced once at the half-pixel floor, which is what makes the client's + #: answer and the server's provably the same: simplification is not nested, + #: so both have to start from identical points. + #: Empty for a box, which is an extent rather than something reduced from + #: anything. #: #: Required rather than defaulted: it is on every answer, and a field a #: client has to check for is one it will eventually forget to check for. @@ -1963,8 +1958,6 @@ class AppliedParameters(BaseModel): """The parameter values this answer was actually produced with.""" detail: Detail - fill_holes: float - fragments: Fragments class SuggestionOut(BaseModel): @@ -1973,7 +1966,7 @@ class SuggestionOut(BaseModel): `regions` is empty when there is no suggestion, and that is an ordinary answer rather than an error: a click can land on sky, the model can be less sure than the caller asked for, the shape found can be one this class cannot - hold, and the parameters as set can leave nothing. A 404 or a 409 for any of + hold, and the detail as set can leave nothing. A 404 or a 409 for any of those would be telling the caller they did something wrong when they did not. `model_ref` is echoed on every answer, including the empty one, because it @@ -1985,8 +1978,10 @@ class SuggestionOut(BaseModel): `parameters` names which settings have any effect on the kind of shape this request will come back in, so a client renders exactly those and works none - of it out for itself. It is present on an empty answer too, which is what - lets somebody who adjusted their way into nothing adjust their way back out. + of it out for itself. It is empty for a box class, which is how a client is + told to render no adjustments at all. It is present on an empty answer too, + which is what lets somebody who adjusted their way into nothing adjust their + way back out. """ model_ref: str diff --git a/src/visionset/server/routes/inference.py b/src/visionset/server/routes/inference.py index 8c47d0f..7785f46 100644 --- a/src/visionset/server/routes/inference.py +++ b/src/visionset/server/routes/inference.py @@ -318,28 +318,29 @@ def suggest_region(workspace: WorkspaceDep, body: SuggestRequest) -> SuggestionO **`allowed_geometries` is the caller's schema, not a preference.** The answer is produced in one of the kinds named or not at all: a class that admits - polygons gets outlines, a class that admits only boxes gets their extents, - and a class that admits neither gets no regions. Answering in a kind the - schema would refuse would produce a suggestion that cannot be accepted. - - **The three parameters shape the mask, and none of them reaches the model.** - `detail` decides how much of an outline survives simplification, `fill_holes` - how wide a gap in the mask is closed before it is traced, and `fragments` - whether the piece under the click is the answer or every piece worth - proposing. Each is optional and each has a default, so a caller that sends - none of them gets what this route always gave. - - **`parameters` says which of them apply here**, for the kind of shape this - request will come back in — `detail` and `fill_holes` change an outline and a - box has none — and it is present even when there is nothing to propose, so + polygons gets the outline of the piece under the click, a class that admits + only boxes gets one box over every piece the mask kept, and a class that + admits neither gets no regions. Answering in a kind the schema would refuse + would produce a suggestion that cannot be accepted. + + **`detail` is the one setting, and it does not reach the model.** It decides + how much of an outline survives simplification. It is optional and defaults + to `balanced`, which is what every suggestion used before there was a choice. + Closing the small gaps in a mask and dropping its noise specks still happen, + at fixed defaults nobody asks for. + + **`parameters` says which settings apply here**, for the kind of shape this + request will come back in. It is empty for a box class — `detail` changes an + outline and a box has none — which is how a client is told to render no + adjustments at all. It is present even when there is nothing to propose, so somebody who adjusted their way into an empty answer can adjust their way back out. A client renders what this names and works none of it out itself. - **`contour` on each region is the unsimplified outline.** It is what lets a - client re-run `detail` locally rather than asking again, and it is the *same* - points this route reduced — simplification is not nested, so a client - starting from anything else could not be held to the same answer. A box - carries none, because there is nothing it was reduced from. + **`contour` on each region is the outline the shape was reduced from.** It is + what lets a client re-run `detail` locally rather than asking again, and it + is the *same* points this route reduced — simplification is not nested, so a + client starting from anything else could not be held to the same answer. A + box carries none, because there is nothing it was reduced from. **Every point must be on the asset**, positive and negative alike — `x` in `[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own @@ -368,8 +369,6 @@ def suggest_region(workspace: WorkspaceDep, body: SuggestRequest) -> SuggestionO prompt=prompt, allowed=tuple(body.allowed_geometries), detail=body.detail, - fill_holes=body.fill_holes, - fragments=body.fragments, ) return SuggestionOut( model_ref=answer.model_ref, @@ -378,9 +377,7 @@ def suggest_region(workspace: WorkspaceDep, body: SuggestRequest) -> SuggestionO SuggestedRegion(geometry=shape.geometry, contour=list(shape.contour)) for shape in answer.shapes ], - applied=AppliedParameters( - detail=body.detail, fill_holes=body.fill_holes, fragments=body.fragments - ), + applied=AppliedParameters(detail=body.detail), parameters=list(answer.parameters), ) diff --git a/tests/inference/test_masks.py b/tests/inference/test_masks.py index b36888e..414b1bf 100644 --- a/tests/inference/test_masks.py +++ b/tests/inference/test_masks.py @@ -15,7 +15,9 @@ import pytest +from visionset.inference import masks from visionset.inference.masks import ( + MAXIMUM_CLOSING_RADIUS, MINIMUM_FRAGMENT_SHARE, MINIMUM_TOLERANCE, bbox_from, @@ -32,7 +34,6 @@ from visionset.kernel.domain import ( BboxGeometry, Detail, - Fragments, GeometryType, PolygonGeometry, ) @@ -118,7 +119,7 @@ def islands() -> list[list[int]]: def test_one_piece_is_the_piece_under_the_click() -> None: """The first symptom this rule was written for: a shape from somewhere nobody clicked.""" - pieces = components(speckled(), fragments=Fragments.ONE, at=[(3.0, 5.0)]) + pieces = components(speckled(), at=[(3.0, 5.0)]) assert len(pieces) == 1 assert (pieces[0].x, pieces[0].y) == (1, 3) assert lit(list(pieces[0].mask)) == 36, "the 36-px object, not the 1-px speck" @@ -140,25 +141,37 @@ def test_several_points_spanning_pieces_prefer_the_largest_of_them() -> None: assert (pieces[0].x, pieces[0].y) == (1, 3), "the 36-px object beats the 1-px speck" -def test_without_a_point_the_topmost_piece_still_wins() -> None: - """The no-point call is unchanged — nothing outside a point prompt has an opinion.""" +def test_without_a_point_the_noise_filter_still_answers_with_the_object() -> None: + """Nothing outside a point prompt has an opinion — but the speck is gone first. + + The topmost-leftmost rule only ever ran over pieces that survived, and a 1-px + speck against a 36-px object does not. What used to need a click to beat the + speck no longer needs one (#557). + """ pieces = components(speckled()) - assert (pieces[0].x, pieces[0].y) == (9, 0) + assert len(pieces) == 1 + assert (pieces[0].x, pieces[0].y) == (1, 3) -def test_every_piece_is_offered_when_every_piece_was_asked_for() -> None: - pieces = components(islands(), fragments=Fragments.ALL) +def test_every_surviving_piece_comes_back_biggest_first_behind_the_pointed_at_one() -> None: + pieces = components(islands()) assert [lit(list(piece.mask)) for piece in pieces] == [25, 16], "biggest first" assert [(piece.x, piece.y) for piece in pieces] == [(1, 2), (14, 3)] -def test_a_speck_is_dropped_even_when_every_piece_was_asked_for() -> None: +def test_the_piece_under_the_click_leads_even_when_it_is_not_the_biggest() -> None: + """The ordering the two geometries both read: head for a polygon, all of it for a box.""" + pieces = components(islands(), at=[(15.0, 4.0)]) + assert [lit(list(piece.mask)) for piece in pieces] == [16, 25] + + +def test_a_speck_is_dropped_before_anything_else_looks_at_the_mask() -> None: """One click must not become a cleanup job. - The speck is 1 px against a 36-px object — far under the share — so asking - for everything still answers with the thing that was clicked. + The speck is 1 px against a 36-px object — far under the share — so it never + reaches a shape, whichever geometry asked. """ - pieces = components(speckled(), fragments=Fragments.ALL) + pieces = components(speckled()) assert len(pieces) == 1 assert lit(list(pieces[0].mask)) == 36 @@ -166,19 +179,23 @@ def test_a_speck_is_dropped_even_when_every_piece_was_asked_for() -> None: def test_a_piece_is_kept_when_it_clears_the_share() -> None: """The other side of the same rule, so the threshold is not merely "drop small things".""" assert 25 * MINIMUM_FRAGMENT_SHARE <= 16 - assert len(components(islands(), fragments=Fragments.ALL)) == 2 + assert len(components(islands())) == 2 def test_an_empty_mask_has_no_pieces() -> None: assert components(empty(), at=[(5.0, 5.0)]) == [] - assert components(empty(), fragments=Fragments.ALL) == [] + assert components(empty()) == [] # --- step 2: closing the gaps -------------------------------------------------- -def notched(depth: int, size: int = 13) -> list[list[bool]]: - """A solid square with a one-row bite ``depth`` pixels deep, cut in from the right.""" +def notched(depth: int, size: int = 64) -> list[list[bool]]: + """A solid square with a one-row bite ``depth`` pixels deep, cut in from the right. + + 64 on a side by default, because the reach is a share of the piece's own area + and a 13-px square is under the floor where it works out at nothing. + """ mask = [[True] * size for _ in range(size)] for x in range(size - depth, size): mask[size // 2][x] = False @@ -193,43 +210,53 @@ def test_a_narrow_notch_is_closed_and_the_outline_gets_simpler() -> None: comes back the shape somebody meant. """ mask = notched(2) - after = filled(mask, fill_holes=0.05) + after = filled(mask) assert lit([list(row) for row in after]) > lit(mask) assert len(contour(after)) < len(contour(mask)) def test_a_bay_wider_than_the_reach_is_kept() -> None: """Concavity at that scale is shape rather than noise, and it survives untouched.""" - mask = notched(2, size=13) - for y in (5, 6, 7): - for x in (10, 11, 12): + mask = [[True] * 64 for _ in range(64)] + for y in range(28, 36): + for x in range(56, 64): mask[y][x] = False - assert filled(mask, fill_holes=0.05) is mask + assert filled(mask) is mask def test_closing_never_moves_the_extent() -> None: - """Why `fill_holes` is declared for a polygon and not for a box. + """Why the close is declared for a polygon and does nothing to a box. A close only ever adds pixels whose whole neighbourhood was already reachable, so it cannot push an edge outward. The applicability table says the same thing; this is the behaviour under it. """ mask = notched(2) - assert bbox_from(filled(mask, fill_holes=0.05)) == bbox_from(mask) + assert bbox_from(filled(mask)) == bbox_from(mask) def test_the_reach_scales_with_the_piece_rather_than_the_frame() -> None: - """One setting, every size — the property an absolute pixel count cannot have.""" - small = closing_radius(rect(0, 0, 39, 39, width=40, height=40), fill_holes=0.002) - large = closing_radius(rect(0, 0, 199, 199, width=200, height=200), fill_holes=0.002) + """One default, every size — the property an absolute pixel count cannot have.""" + small = closing_radius(rect(0, 0, 63, 63, width=64, height=64)) + large = closing_radius(rect(0, 0, 199, 199, width=200, height=200)) assert small < large -def test_closing_nothing_is_a_request_the_pipeline_honours() -> None: - """A mask of foliage is mostly gaps and every one of them is real.""" - mask = notched(2) - assert filled(mask, fill_holes=0.0) is mask - assert closing_radius(mask, fill_holes=0.0) == 0 +def test_the_reach_stops_at_the_cap_however_large_the_piece() -> None: + """A gap wider than a few pixels is shape, and the pass costs a step per unit. + + Uncapped this worked out at 22 on a 4K frame — 44 bitset passes on the path + somebody is waiting on after a click, to bridge gaps that were never there. + """ + huge = closing_radius(rect(0, 0, 799, 799, width=800, height=800)) + assert huge == MAXIMUM_CLOSING_RADIUS + + +def test_a_piece_too_small_to_have_artefacts_is_closed_not_at_all() -> None: + """Rounded down, so the smallest shapes keep every feature they have.""" + tiny = rect(0, 0, 11, 11, width=12, height=12) + assert closing_radius(tiny) == 0 + assert filled(tiny) is tiny def test_an_enclosed_hole_is_closed_in_the_mask_and_invisible_in_the_shape() -> None: @@ -242,23 +269,27 @@ def test_an_enclosed_hole_is_closed_in_the_mask_and_invisible_in_the_shape() -> pixels to 400 and left the traced outline byte-identical. """ piece = components(holed(2))[0] - after = filled(piece.mask, fill_holes=0.02) - assert lit(list(piece.mask)) == 400 - 4 - assert lit([list(row) for row in after]) == 400 + after = filled(piece.mask) + assert lit(list(piece.mask)) == 64 * 64 - 4 + assert lit([list(row) for row in after]) == 64 * 64 assert outline(piece.mask) == outline(after), "the hole was never on the outer ring" def holed(hole: int) -> list[list[bool]]: - """A 20x20 square with a centred square hole ``hole`` pixels on a side.""" - low = 10 - hole // 2 + """A 64x64 square with a centred square hole ``hole`` pixels on a side. + + 64 rather than 20 for :func:`notched`'s reason: the reach is a share of the + piece's area, so a small square is under the floor where it reaches nothing. + """ + low = 32 - hole // 2 return [ [ - 5 <= x <= 24 - and 5 <= y <= 24 + 5 <= x <= 68 + and 5 <= y <= 68 and not (low <= x - 5 < low + hole and low <= y - 5 < low + hole) - for x in range(40) + for x in range(80) ] - for y in range(40) + for y in range(80) ] @@ -407,25 +438,65 @@ def test_a_piece_too_thin_to_be_a_polygon_is_dropped_rather_than_demoted() -> No assert shapes_from(rect(10, 10, 11, 10), allowed=BOTH) == [] -def test_every_island_becomes_its_own_shape() -> None: - shaped = shapes_from(islands(), allowed=POLYGON_ONLY, fragments=Fragments.ALL) - assert len(shaped) == 2 - assert [s.geometry.type for s in shaped] == [GeometryType.POLYGON, GeometryType.POLYGON] +def test_a_polygon_is_the_piece_that_was_clicked_and_only_that_piece() -> None: + """A click asks about one object, so a polygon class gets one outline.""" + shaped = shapes_from(islands(), allowed=POLYGON_ONLY, at=[(3.0, 4.0)]) + assert len(shaped) == 1 + assert shaped[0].geometry.type is GeometryType.POLYGON + + +def test_a_box_is_one_union_over_every_surviving_piece() -> None: + """Decision 4: an occluded object is one thing, however many pieces it arrives in. + + `islands()` is 25 px at (1,2) and 16 px at (14,3) — a railing's worth apart. + Largest-only would cut the object off at the occlusion; a box per piece would + annotate it twice. One box covers both. + """ + shaped = shapes_from(islands(), allowed=BOX_ONLY, at=[(3.0, 4.0)]) + assert len(shaped) == 1 + assert shaped[0].geometry == BboxGeometry(x=1.0, y=2.0, width=17.0, height=5.0) -def test_one_fragment_is_still_the_default() -> None: - assert len(shapes_from(islands(), allowed=POLYGON_ONLY, at=[(3.0, 4.0)])) == 1 +def test_the_union_leaves_out_the_specks_the_noise_filter_dropped() -> None: + """The other half of the same rule: a union of everything would follow speckle. + + `speckled()` is a 36-px object at (1,3) and a 1-px speck at (9,0). The speck + is under the share, so the box stops at the object rather than stretching to + the corner of the frame. + """ + shaped = shapes_from(speckled(), allowed=BOX_ONLY, at=[(3.0, 5.0)]) + assert shaped[0].geometry == BboxGeometry(x=1.0, y=3.0, width=6.0, height=6.0) + + +def test_a_box_never_pays_for_the_close_at_all(monkeypatch: pytest.MonkeyPatch) -> None: + """A performance rule with no behavioural signature, so it is asserted structurally. + + A close only ever adds pixels whose neighbourhood was already reachable, so + it cannot move an extent — which is exactly why skipping it for a box is safe + and exactly why no assertion about the *box* could ever notice the skip. So + the assertion is that the step is not reached: at 4K it was twelve bitset + passes per click, thrown away (#557). + """ + monkeypatch.setattr(masks, "filled", lambda mask: pytest.fail("a box class ran the close")) + shaped = shapes_from(islands(), allowed=BOX_ONLY, at=[(3.0, 4.0)]) + + assert shaped[0].geometry == BboxGeometry(x=1.0, y=2.0, width=17.0, height=5.0) def test_closing_a_notch_reaches_the_polygon_and_leaves_the_box_alone() -> None: - """Both halves at once, because each is how the other's mistake stays hidden.""" - ragged = shapes_from(notched(2, size=41), allowed=POLYGON_ONLY, fill_holes=0.0) - smoothed = shapes_from(notched(2, size=41), allowed=POLYGON_ONLY, fill_holes=0.005) + """Both halves at once, because each is how the other's mistake stays hidden. + + The reach is fixed now, so the comparison is against a piece small enough to + fall under it rather than against a setting of zero. + """ + ragged = shapes_from(notched(2, size=40), allowed=POLYGON_ONLY) + smoothed = shapes_from(notched(2, size=64), allowed=POLYGON_ONLY) + assert closing_radius(notched(2, size=40)) == 0, "under the reach: nothing bridged" + assert closing_radius(notched(2, size=64)) >= 1, "over it: the notch is bridged" assert len(ragged[0].contour) > len(smoothed[0].contour) - assert ( - shapes_from(notched(2, size=41), allowed=BOX_ONLY, fill_holes=0.005)[0].geometry - == shapes_from(notched(2, size=41), allowed=BOX_ONLY, fill_holes=0.0)[0].geometry - ) + assert shapes_from(notched(2, size=64), allowed=BOX_ONLY)[0].geometry == shapes_from( + notched(2, size=40), allowed=BOX_ONLY + )[0].geometry.model_copy(update={"width": 64.0, "height": 64.0}) def test_a_mask_with_nothing_in_it_proposes_nothing() -> None: diff --git a/tests/kernel/test_suggestion_parameters.py b/tests/kernel/test_suggestion_parameters.py index 188eae3..8ff7208 100644 --- a/tests/kernel/test_suggestion_parameters.py +++ b/tests/kernel/test_suggestion_parameters.py @@ -7,7 +7,7 @@ or, under a kinder default, appear on a box class where it does nothing. So the first test here is the one that matters. It sweeps `SuggestParameter` -itself, so a fourth parameter arrives with its applicability stated or the suite +itself, so a second parameter arrives with its applicability stated or the suite goes red naming it. """ @@ -15,11 +15,8 @@ from visionset.kernel.domain import ( DEFAULT_DETAIL, - DEFAULT_FILL_HOLES, - DEFAULT_FRAGMENTS, PARAMETER_APPLIES_TO, Detail, - Fragments, GeometryType, SuggestParameter, suggest_parameters, @@ -52,17 +49,14 @@ def test_a_declared_geometry_is_one_the_domain_actually_stores( def test_a_polygon_is_offered_every_parameter() -> None: - assert suggest_parameters(GeometryType.POLYGON) == ( - SuggestParameter.DETAIL, - SuggestParameter.FILL_HOLES, - SuggestParameter.FRAGMENTS, - ) + assert suggest_parameters(GeometryType.POLYGON) == (SuggestParameter.DETAIL,) -def test_a_box_is_offered_only_the_one_that_moves_it() -> None: - # `detail` and `fill_holes` change an outline, and a box has none: its extent - # is the same whichever way either is set. - assert suggest_parameters(GeometryType.BBOX) == (SuggestParameter.FRAGMENTS,) +def test_a_box_is_offered_nothing_at_all() -> None: + # `detail` changes an outline and a box has none, so a box class declares no + # parameters — which is what tells a client to render no adjustments rather + # than an empty section (#557). + assert suggest_parameters(GeometryType.BBOX) == () def test_a_kind_that_holds_no_shape_is_offered_nothing() -> None: @@ -77,10 +71,5 @@ def test_the_reader_answers_in_declaration_order() -> None: assert list(offered) == [p for p in SuggestParameter if p in offered] -def test_the_defaults_are_members_of_their_own_vocabularies() -> None: +def test_the_default_is_a_member_of_its_own_vocabulary() -> None: assert DEFAULT_DETAIL in Detail - assert DEFAULT_FRAGMENTS in Fragments - - -def test_the_default_hole_share_is_a_share() -> None: - assert 0.0 <= DEFAULT_FILL_HOLES <= 1.0 diff --git a/tests/server/test_suggest.py b/tests/server/test_suggest.py index 28e9e8f..d00c642 100644 --- a/tests/server/test_suggest.py +++ b/tests/server/test_suggest.py @@ -573,7 +573,7 @@ def test_a_request_that_sends_no_parameters_gets_the_defaults_back( body = ask(client, project=project, asset=asset, connection=connection).json() - assert body["applied"] == {"detail": "balanced", "fill_holes": 0.002, "fragments": "one"} + assert body["applied"] == {"detail": "balanced"} def test_the_answer_echoes_the_parameters_it_was_given( @@ -588,14 +588,12 @@ def test_the_answer_echoes_the_parameters_it_was_given( asset=asset, connection=connection, detail="fine", - fill_holes=0.0, - fragments="all", ).json() - assert body["applied"] == {"detail": "fine", "fill_holes": 0.0, "fragments": "all"} + assert body["applied"] == {"detail": "fine"} -def test_a_polygon_class_is_told_every_parameter_applies( +def test_a_polygon_class_is_told_the_one_parameter_applies( client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] ) -> None: connection = a_connection(client) @@ -603,15 +601,16 @@ def test_a_polygon_class_is_told_every_parameter_applies( body = ask(client, project=project, asset=asset, connection=connection).json() - assert body["parameters"] == ["detail", "fill_holes", "fragments"] + assert body["parameters"] == ["detail"] -def test_a_box_class_is_told_only_fragments_applies( +def test_a_box_class_is_told_nothing_applies( client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] ) -> None: """What the editor renders on a box class, and the whole of why it renders it. - A client works none of this out. Remove `fill_holes` from the polygon row of + A client works none of this out: an empty list is what tells it to render no + adjustments at all. Remove `detail` from the polygon row of `PARAMETER_APPLIES_TO` and the assertion above goes red; declare it for a box and this one does. """ @@ -620,7 +619,7 @@ def test_a_box_class_is_told_only_fragments_applies( body = ask(client, project=project, asset=asset, connection=connection, allowed=["bbox"]).json() - assert body["parameters"] == ["fragments"] + assert body["parameters"] == [] def test_an_answer_with_nothing_in_it_still_carries_its_controls( @@ -635,7 +634,7 @@ def test_an_answer_with_nothing_in_it_still_carries_its_controls( connection = a_connection(client) asset = an_asset(client, runner, project, tmp_path) - body = ask(client, project=project, asset=asset, connection=connection, fragments="all").json() + body = ask(client, project=project, asset=asset, connection=connection).json() empty = ask( client, project=project, @@ -690,34 +689,58 @@ def test_a_coarser_setting_comes_back_with_no_more_vertices( assert counts[0] < counts[1] < counts[2], counts -def test_every_piece_comes_back_when_every_piece_was_asked_for( +def test_a_split_mask_is_one_polygon_and_one_union_box( client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - """`fragments` over HTTP, and the plural answer acceptance is all-or-nothing over.""" + """Decision 4 over HTTP: a click asks about one object, however it arrived. + + A polygon class gets the piece under the click; a box class gets one box over + both, because a mask in two pieces is nearly always one object seen around an + occlusion. + """ scripted(monkeypatch, two_blocks()) connection = a_connection(client) asset = an_asset(client, runner, project, tmp_path) - one = ask(client, project=project, asset=asset, connection=connection).json() - every = ask(client, project=project, asset=asset, connection=connection, fragments="all").json() + outlined = ask(client, project=project, asset=asset, connection=connection).json() + boxed = ask( + client, project=project, asset=asset, connection=connection, allowed=["bbox"] + ).json() - assert len(one["regions"]) == 1 - assert len(every["regions"]) == 2 + assert len(outlined["regions"]) == 1 + assert len(boxed["regions"]) == 1 + box = boxed["regions"][0]["geometry"] + outline = outlined["regions"][0]["geometry"]["points"] + assert box["width"] > max(x for x, _ in outline) - min(x for x, _ in outline), ( + "the box spans both pieces, so it is wider than the piece the outline traced" + ) -def test_a_share_outside_nought_to_one_is_refused_by_the_schema( +def test_a_setting_the_request_no_longer_takes_is_refused( client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] ) -> None: + """`extra="forbid"`, doing the work of the deprecation nobody has to write. + + A client still sending the two settings that came out (#557) is told plainly + rather than having them silently ignored. + """ connection = a_connection(client) asset = an_asset(client, runner, project, tmp_path) - answer = ask(client, project=project, asset=asset, connection=connection, fill_holes=1.5) - - assert answer.status_code == 422 + assert ( + ask(client, project=project, asset=asset, connection=connection, fill_holes=0.5).status_code + == 422 + ) + assert ( + ask( + client, project=project, asset=asset, connection=connection, fragments="all" + ).status_code + == 422 + ) def test_a_detail_step_the_vocabulary_does_not_have_is_refused(