From 5ef3a60644a895d5300cd0bf3b3265f5230550f2 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 19:46:45 -0700 Subject: [PATCH 01/17] feat(kernel): a label class accepts a set of geometries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `LabelClass.geometry` becomes `geometries`, a non-empty deduplicated tuple kept in one sorted order. A class labelled as a box on some frames and as a polygon on others is one class; splitting it in two made every consumer downstream re-unify them, and COCO's own docstring already recorded the cost. The write gate in `AnnotationService._validate` becomes a membership test in that class's own set — still not the version's union, which is wider. `schema_diff` answers the module's governing question per geometry, the shape the `select` options rule already had: one added is additive, one removed is destructive. So widening a class is an ordinary save and narrowing stays behind the flag. No migration. `annotation_schema.classes` is a JSON column, so a `model_validator(mode='before')` on `LabelClass` reading the retired singular key covers stored schemas and stored release manifests alike — and MCP, which takes the domain model directly. `LabelClassBody` deliberately does not read it: a REST client sending it is better told so than silently reinterpreted. `MANIFEST_VERSION` moves to 2, since `Manifest.classes` carries these verbatim. Documents already published keep their bytes and their hashes and still load, so every existing release stays verifiable. The export report is now one row per (class, geometry). Keyed by class alone it carried one verdict for a class YOLO answers twice — writing the boxes whole and reducing the polygons — and would have misdescribed half its own output. No exporter changed: every one of them already branches per annotation. cf. #584 --- docs/mcp-walkthrough.md | 4 +- docs/schemas.md | 8 +- docs/tutorial.md | 6 +- examples/cli_end_to_end.sh | 2 +- examples/http_end_to_end.py | 2 +- examples/ingest_end_to_end.py | 2 +- examples/mcp_end_to_end.py | 4 +- examples/sdk_end_to_end.py | 6 +- examples/thirty_minute_flow.py | 4 +- frontend/ui-core/src/generated/api.ts | 5 +- frontend/ui-core/src/generated/checks.ts | 2 +- openapi.json | 13 ++- scripts/export_wire_fixtures.py | 13 ++- src/visionset/cli/schemas.py | 10 +- src/visionset/kernel/domain/release.py | 18 ++- src/visionset/kernel/domain/schema.py | 48 +++++++- src/visionset/kernel/domain/schema_diff.py | 26 +++-- src/visionset/kernel/errors.py | 10 +- .../kernel/services/annotation_service.py | 13 ++- .../kernel/services/release_service.py | 43 +++++--- .../kernel/services/schema_service.py | 25 +++-- src/visionset/mcp/schemas.py | 18 +-- src/visionset/server/models.py | 14 ++- src/visionset/wire/__init__.py | 4 +- tests/cli/_flow.py | 2 +- tests/cli/test_full_cycle.py | 2 +- tests/cli/test_schema_commands.py | 8 +- tests/fixtures/samples.py | 5 +- tests/fixtures/wire_annotations.json | 17 ++- tests/formats/test_coco.py | 3 +- tests/formats/test_coco_smoke.py | 6 +- tests/formats/test_report_agreement.py | 85 +++++++++++++- tests/formats/test_yolo.py | 8 +- tests/formats/test_yolo_smoke.py | 6 +- tests/kernel/test_annotation_service.py | 82 ++++++++++++-- tests/kernel/test_batch_service.py | 4 +- tests/kernel/test_capabilities.py | 2 +- tests/kernel/test_concurrency.py | 2 +- tests/kernel/test_concurrent_membership.py | 2 +- tests/kernel/test_dataset_service.py | 4 +- tests/kernel/test_events.py | 2 +- tests/kernel/test_geometry.py | 22 ++-- tests/kernel/test_ingest_service.py | 2 +- tests/kernel/test_job_service.py | 2 +- tests/kernel/test_metadata_store.py | 4 +- tests/kernel/test_project_service.py | 4 +- tests/kernel/test_release.py | 4 +- tests/kernel/test_release_service.py | 8 +- tests/kernel/test_schema_diff.py | 23 +++- tests/kernel/test_schema_service.py | 104 ++++++++++++++++-- tests/kernel/test_summary_service.py | 2 +- tests/kernel/test_trunk_supersession.py | 6 +- tests/mcp/_flow.py | 4 +- tests/mcp/test_agent_walk.py | 4 +- tests/mcp/test_batch_tools.py | 6 +- tests/mcp/test_release_tools.py | 2 +- tests/mcp/test_schema_tools.py | 20 ++-- tests/mcp/test_tool_errors.py | 6 +- tests/server/_flow.py | 6 +- tests/server/test_batches.py | 4 +- tests/server/test_external_client.py | 2 +- tests/server/test_releases.py | 3 +- tests/server/test_schemas.py | 10 +- tests/server/test_wire_fixtures.py | 2 +- tests/server/test_wire_models.py | 11 +- 65 files changed, 594 insertions(+), 207 deletions(-) diff --git a/docs/mcp-walkthrough.md b/docs/mcp-walkthrough.md index a23c3e83..22d9676f 100644 --- a/docs/mcp-walkthrough.md +++ b/docs/mcp-walkthrough.md @@ -56,8 +56,8 @@ tool that reads the dataset on its own: `get_project` already carries `dataset_i ``` create_schema_version project="road-signs" classes=[ - {"name": "sign", "geometry": "bbox"}, - {"name": "empty-road", "geometry": "classification_tag"}] + {"name": "sign", "geometries": ["bbox"]}, + {"name": "empty-road", "geometries": ["classification_tag"]}] -> {"version": 1, ...} ``` diff --git a/docs/schemas.md b/docs/schemas.md index b957615d..085b9b2a 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -19,13 +19,13 @@ with WorkspaceService.open("./road-signs") as workspace: sign = LabelClass( name="sign", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), attributes=[ Attribute(name="occluded", kind="boolean", default=False), Attribute(name="weather", kind="select", options=["dry", "wet"]), ], ) - lane = LabelClass(name="lane", geometry=GeometryType.POLYGON) + lane = LabelClass(name="lane", geometries=(GeometryType.POLYGON,)) published = schemas.create_version(project.id, [sign, lane]) published.published.version # 1 @@ -199,7 +199,7 @@ not damage. second edit, and `create_version` refuses anything outside it: ```python -LabelClass(name="road", geometry=GeometryType.MASK) # constructs fine +LabelClass(name="road", geometries=(GeometryType.MASK,)) # constructs fine schemas.create_version(project.id, [that]) # UnsupportedGeometry ``` @@ -386,7 +386,7 @@ The file is **JSON**, and it is byte-for-byte the same document "classes": [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "color": "#ff0000", "attributes": [ {"name": "occluded", "kind": "boolean", "required": false, "default": false} diff --git a/docs/tutorial.md b/docs/tutorial.md index 98c6e025..4bd43728 100644 --- a/docs/tutorial.md +++ b/docs/tutorial.md @@ -42,9 +42,9 @@ takes. ```json { "classes": [ - { "name": "vehicle", "geometry": "bbox", "color": "#eb5a47" }, - { "name": "sign", "geometry": "bbox", "color": "#2a9d8f" }, - { "name": "lane", "geometry": "polygon", "color": "#f4a261" } + { "name": "vehicle", "geometries": ["bbox"], "color": "#eb5a47" }, + { "name": "sign", "geometries": ["bbox"], "color": "#2a9d8f" }, + { "name": "lane", "geometries": ["polygon"], "color": "#f4a261" } ] } ``` diff --git a/examples/cli_end_to_end.sh b/examples/cli_end_to_end.sh index fd1834aa..9b21d69b 100755 --- a/examples/cli_end_to_end.sh +++ b/examples/cli_end_to_end.sh @@ -60,7 +60,7 @@ cat > "$DEST/schema.json" <<'JSON' "classes": [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "color": "#ff0000", "attributes": [{"name": "occluded", "kind": "boolean", "default": false}] } diff --git a/examples/http_end_to_end.py b/examples/http_end_to_end.py index 3b7e3788..25f2fd95 100644 --- a/examples/http_end_to_end.py +++ b/examples/http_end_to_end.py @@ -326,7 +326,7 @@ def _walk(client: Client, base_url: str, downloads: Path) -> Summary: "classes": [ { "name": "nodule", - "geometry": "bbox", + "geometries": ["bbox"], "attributes": [{"name": "malignant", "kind": "boolean", "required": True}], } ] diff --git a/examples/ingest_end_to_end.py b/examples/ingest_end_to_end.py index 523f254c..8b7a2d6b 100644 --- a/examples/ingest_end_to_end.py +++ b/examples/ingest_end_to_end.py @@ -98,7 +98,7 @@ #: approval pins the active version forever — but this example writes no labels, #: so the contract only has to exist. CLASSES: tuple[LabelClass, ...] = ( - LabelClass(name="vehicle", geometry=GeometryType.BBOX, color="#2a9d8f"), + LabelClass(name="vehicle", geometries=(GeometryType.BBOX,), color="#2a9d8f"), ) diff --git a/examples/mcp_end_to_end.py b/examples/mcp_end_to_end.py index 3b267d7d..10a364d7 100644 --- a/examples/mcp_end_to_end.py +++ b/examples/mcp_end_to_end.py @@ -234,8 +234,8 @@ async def tool(name: str, /, **arguments: Any) -> CallToolResult: "create_schema_version", project=PROJECT, classes=[ - {"name": "sign", "geometry": "bbox"}, - {"name": "empty-road", "geometry": "classification_tag"}, + {"name": "sign", "geometries": ["bbox"]}, + {"name": "empty-road", "geometries": ["classification_tag"]}, ], ) ) diff --git a/examples/sdk_end_to_end.py b/examples/sdk_end_to_end.py index d5897642..d0448f67 100644 --- a/examples/sdk_end_to_end.py +++ b/examples/sdk_end_to_end.py @@ -86,7 +86,7 @@ CLASSES: tuple[LabelClass, ...] = ( LabelClass( name="stop-sign", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), color="#d62828", attributes=( # The one required attribute: an annotation without it is refused by @@ -103,10 +103,10 @@ Attribute(name="damaged", kind="boolean", default=False), ), ), - LabelClass(name="lane-marking", geometry=GeometryType.POLYGON, color="#f4a261"), + LabelClass(name="lane-marking", geometries=(GeometryType.POLYGON,), color="#f4a261"), LabelClass( name="weather", - geometry=GeometryType.CLASSIFICATION_TAG, + geometries=(GeometryType.CLASSIFICATION_TAG,), color="#264653", attributes=(Attribute(name="condition", kind="select", options=("clear", "rain", "fog")),), ), diff --git a/examples/thirty_minute_flow.py b/examples/thirty_minute_flow.py index e2a0e788..65748fa5 100644 --- a/examples/thirty_minute_flow.py +++ b/examples/thirty_minute_flow.py @@ -94,8 +94,8 @@ BOX_COUNT = 50 CLASSES: tuple[LabelClass, ...] = ( - LabelClass(name="vehicle", geometry=GeometryType.BBOX, color="#eb5a47"), - LabelClass(name="sign", geometry=GeometryType.BBOX, color="#2a9d8f"), + LabelClass(name="vehicle", geometries=(GeometryType.BBOX,), color="#eb5a47"), + LabelClass(name="sign", geometries=(GeometryType.BBOX,), color="#2a9d8f"), ) SPLIT = SplitRecipe(train=0.7, val=0.15, test=0.15, seed=42) diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index dd134b16..5355bdbf 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -3381,7 +3381,7 @@ export interface components { JsonValue: unknown; /** * LabelClassBody - * @description One labelable class, bound to a geometry. + * @description One labelable class, and the geometries an annotation of it may carry. */ LabelClassBody: { /** @@ -3391,7 +3391,8 @@ export interface components { attributes: components["schemas"]["AttributeBody"][]; /** Color */ color?: string | null; - geometry: components["schemas"]["GeometryType"]; + /** Geometries */ + geometries: components["schemas"]["GeometryType"][]; /** Name */ name: string; }; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index f2d93a66..aae76068 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -264,7 +264,7 @@ export const checkAttributeBody: Check = /*#__PURE__*/ object({ "default": [false, either([isBoolean, isNumber, isString, isNull] as const)], "kind": [true, oneOf(["string", "number", "boolean", "select"] as const)], "name": [true, isString], "options": [false, either([arrayOf(isString), isNull] as const)], "required": [true, isBoolean] } as const); export const checkLabelClassBody: Check = - /*#__PURE__*/ object({ "attributes": [true, arrayOf(checkAttributeBody)], "color": [false, either([isString, isNull] as const)], "geometry": [true, checkGeometryType], "name": [true, isString] } as const); + /*#__PURE__*/ object({ "attributes": [true, arrayOf(checkAttributeBody)], "color": [false, either([isString, isNull] as const)], "geometries": [true, arrayOf(checkGeometryType)], "name": [true, isString] } as const); export const checkSchemaProvenance: Check = /*#__PURE__*/ oneOf(["curated", "annotation"] as const); diff --git a/openapi.json b/openapi.json index 592478e7..7c1e87ec 100644 --- a/openapi.json +++ b/openapi.json @@ -2757,7 +2757,7 @@ "JsonValue": {}, "LabelClassBody": { "additionalProperties": false, - "description": "One labelable class, bound to a geometry.", + "description": "One labelable class, and the geometries an annotation of it may carry.", "properties": { "attributes": { "default": [], @@ -2778,8 +2778,13 @@ ], "title": "Color" }, - "geometry": { - "$ref": "#/components/schemas/GeometryType" + "geometries": { + "items": { + "$ref": "#/components/schemas/GeometryType" + }, + "minItems": 1, + "title": "Geometries", + "type": "array" }, "name": { "title": "Name", @@ -2788,7 +2793,7 @@ }, "required": [ "name", - "geometry" + "geometries" ], "title": "LabelClassBody", "type": "object" diff --git a/scripts/export_wire_fixtures.py b/scripts/export_wire_fixtures.py index da45baa0..4e0f9d66 100644 --- a/scripts/export_wire_fixtures.py +++ b/scripts/export_wire_fixtures.py @@ -87,7 +87,12 @@ def _schema() -> AnnotationSchema: committed and diffed, so a clock in it would make every regeneration a change. """ populated = SCHEMA_VERSION.classes[0] - assert populated.geometry is GeometryType.BBOX, "the sample class is the bbox one" + # Two geometries, so the mirror's parser is exercised on a real set rather + # than on a list that happens to hold one — a reader dropping everything + # after the first element would pass against a singleton everywhere. + assert populated.geometries == (GeometryType.BBOX, GeometryType.POLYGON), ( + "the sample class is the multi-geometry one" + ) return AnnotationSchema( project_id=_PROJECT_ID, version=SCHEMA_VERSION.version, @@ -98,17 +103,17 @@ def _schema() -> AnnotationSchema: populated, # No colour, no attributes. A renderer must choose its own colour for # this one, and the parser must accept the keys being absent. - LabelClass(name="lane", geometry=GeometryType.POLYGON), + LabelClass(name="lane", geometries=(GeometryType.POLYGON,)), # A geometry a class can declare with no drawing tool behind it is a # fact about the annotator rather than about the wire — so it appears # here exactly like the other three. - LabelClass(name="centerline", geometry=GeometryType.POLYLINE, color="#eb5a47"), + LabelClass(name="centerline", geometries=(GeometryType.POLYLINE,), color="#eb5a47"), # An attribute with every optional at its default: not required, no # options, no default. `select` is the only kind that may carry # options, so this is the other side of `populated`'s attribute. LabelClass( name="weather", - geometry=GeometryType.CLASSIFICATION_TAG, + geometries=(GeometryType.CLASSIFICATION_TAG,), color="#00ff00", attributes=(Attribute(name="note", kind="string"),), ), diff --git a/src/visionset/cli/schemas.py b/src/visionset/cli/schemas.py index 8c01106e..c35898bf 100644 --- a/src/visionset/cli/schemas.py +++ b/src/visionset/cli/schemas.py @@ -4,11 +4,17 @@ The document is **JSON**, read with the standard library, and it is byte-for-byte the same document ``POST /projects/{id}/schema/versions`` takes:: - {"classes": [{"name": "sign", "geometry": "bbox", "color": "#ff0000", + {"classes": [{"name": "sign", "geometries": ["bbox", "polygon"], + "color": "#ff0000", "attributes": [{"name": "occluded", "kind": "boolean", "required": true, "options": null, "default": false}]}]} +``geometries`` is a set: a class labeled as a box on some frames and as a polygon +on others is one class. A document written before that was plural may still spell +it ``"geometry": "bbox"``, and ``LabelClass`` reads one — so an old schema file +still applies. + No YAML, and the reason is not taste: a second file format means a runtime dependency in every wheel, a second parser to keep honest, and two shapes that can disagree — while the surface a schema file has to interoperate with, the REST @@ -152,7 +158,7 @@ def schema_list( ( str(v.version), str(len(v.classes)), - ",".join(sorted({c.geometry.value for c in v.classes})), + ",".join(sorted({g.value for c in v.classes for g in c.geometries})), ) for v in versions ], diff --git a/src/visionset/kernel/domain/release.py b/src/visionset/kernel/domain/release.py index 3d8ec76e..7c286310 100644 --- a/src/visionset/kernel/domain/release.py +++ b/src/visionset/kernel/domain/release.py @@ -65,9 +65,14 @@ #: ``DatasetChange.operation`` makes, where forward-compatibility wins because a #: log is advisory. Here the document is hash-pinned evidence, and #: half-understanding one is worse than refusing it. This field is what lets the -#: refusal say "format 2, and this build reads 1" rather than complain about an +#: refusal say "format 3, and this build reads 2" rather than complain about an #: unrecognised key. -MANIFEST_VERSION: int = 1 +#: +#: **2** since #584, which made ``LabelClass.geometries`` plural and so changed +#: the shape of ``classes`` here. A version-1 document still loads — ``LabelClass`` +#: reads the old singular key — so every release published before that stays +#: verifiable, with its bytes and its hash untouched. +MANIFEST_VERSION: int = 2 class ManifestAnnotation(BaseModel): @@ -437,7 +442,14 @@ class ClassExportStatus(StrEnum): class ClassCompatibility(BaseModel): - """One class of a release, judged against one format's declared capabilities. + """One class and one of its geometries, judged against one format. + + A class accepts a *set* of geometries, and a format's answer can differ + across that set — a class holding boxes and polygons is, to a boxes-only + format, one half written whole and one half reduced. So the row is per + ``(label_class, geometry)`` and a class contributes as many rows as it has + geometries. One row per class could only report one of the two answers, and + would describe the output wrongly whichever it picked. Per class rather than per annotation, and the counts are what make it useful: "polygon is unsupported" is a fact about the schema, while "polygon is diff --git a/src/visionset/kernel/domain/schema.py b/src/visionset/kernel/domain/schema.py index e43804f0..a5ca5223 100644 --- a/src/visionset/kernel/domain/schema.py +++ b/src/visionset/kernel/domain/schema.py @@ -170,15 +170,53 @@ def rejects(self, value: AttributeValue) -> str | None: class LabelClass(BaseModel): - """One labelable class in a schema, bound to a geometry type.""" + """One labelable class in a schema, and the geometries it may be drawn as. + + ``geometries`` is a set in meaning and a **sorted tuple** in representation. + A class labelled as a box on some frames and as a polygon on others is one + class; splitting it in two would make every consumer downstream re-unify + them. Which of the allowed shapes a given label carries is the annotation's + own business, and ``AnnotationService`` tests it for membership here. + + Sorted, deduplicated, and a tuple rather than a ``frozenset``, because + ``release.canonical_bytes`` hashes ``model_dump(mode='json')``: a set's + iteration order is not stable across processes, so a set-valued field would + make a release hash irreproducible. Sorting also means the order carries no + meaning and nobody can read one into it — unlike ``AnnotationSchema.classes``, + which is authored. + """ model_config = ConfigDict(frozen=True, extra="forbid") name: str - geometry: GeometryType + geometries: tuple[GeometryType, ...] = Field(min_length=1) color: str | None = None attributes: tuple[Attribute, ...] = () + @model_validator(mode="before") + @classmethod + def _one_geometry_is_a_set_of_one(cls, data: object) -> object: + """Read a pre-#584 document, which spelled this ``geometry`` and singular. + + The single back-compatibility point, and it serves three readers at once: + schema rows (``annotation_schema.classes`` is a JSON column), release + manifests (``Manifest.classes`` carries these verbatim, and an existing + release must stay verifiable), and MCP, which takes this model directly + as a tool parameter — so an agent written against either spelling works. + + Deliberately *not* mirrored on ``LabelClassBody``: a REST client sending + the old key should get a clean refusal naming the field, not a silent + reinterpretation of what it asked for. + """ + if not isinstance(data, Mapping) or "geometry" not in data: + return data + if "geometries" in data: + # A document carrying both is not one this ever wrote. Leave it be and + # let ``extra='forbid'`` refuse the stray key, which names it. + return data + rest = {key: value for key, value in data.items() if key != "geometry"} + return rest | {"geometries": (data["geometry"],)} + @field_validator("name") @classmethod def _named(cls, value: str) -> str: @@ -188,6 +226,12 @@ def _named(cls, value: str) -> str: raise ValueError("a class name must contain at least one non-blank character") return stripped + @field_validator("geometries") + @classmethod + def _a_set_in_a_fixed_order(cls, value: tuple[GeometryType, ...]) -> tuple[GeometryType, ...]: + """Deduplicated and sorted, so two ways of writing one set are one value.""" + return tuple(sorted(set(value), key=lambda geometry: geometry.value)) + @model_validator(mode="after") def _attribute_names_unique(self) -> LabelClass: names = [attribute.name.casefold() for attribute in self.attributes] diff --git a/src/visionset/kernel/domain/schema_diff.py b/src/visionset/kernel/domain/schema_diff.py index d8b5fec2..541d53fd 100644 --- a/src/visionset/kernel/domain/schema_diff.py +++ b/src/visionset/kernel/domain/schema_diff.py @@ -7,9 +7,9 @@ - **Additive** — it does. New classes, new optional attributes, a wider ``select``. Nothing already labeled stops meaning what it meant. -- **Destructive** — it does not. Removing a class, changing its geometry, adding - a required attribute, narrowing a ``select``. Existing annotations are left - referring to something the contract no longer describes. +- **Destructive** — it does not. Removing a class, taking a geometry away from + one, adding a required attribute, narrowing a ``select``. Existing annotations + are left referring to something the contract no longer describes. Matching is by **exact class name and exact attribute name**. That makes a rename read as a removal plus an addition, which looks lossy until you remember that @@ -162,14 +162,24 @@ def _class_changes(before: LabelClass, after: LabelClass) -> Iterator[SchemaChan ``color`` is deliberately absent: it is how a class is drawn, not what it means, and reporting it would put cosmetic edits behind a destructive gate. """ - if before.geometry is not after.geometry: + # The same shape as a ``select``'s options below, for the same reason: a class + # that gains a geometry invalidates nothing already drawn, and one that loses + # a geometry orphans every annotation carrying it. Answering the module's one + # question per geometry is what makes widening an ordinary save — the whole + # point of a class holding a set — while narrowing stays behind the flag. + old_geometries = set(before.geometries) + new_geometries = set(after.geometries) + for geometry in sorted(new_geometries - old_geometries): + yield SchemaChange( + kind=ChangeKind.ADDITIVE, + label_class=after.name, + detail=f"geometry {geometry.value!r} added to class {after.name!r}", + ) + for geometry in sorted(old_geometries - new_geometries): yield SchemaChange( kind=ChangeKind.DESTRUCTIVE, label_class=after.name, - detail=( - f"class {after.name!r} changed geometry from {before.geometry.value!r} " - f"to {after.geometry.value!r}" - ), + detail=f"geometry {geometry.value!r} removed from class {after.name!r}", ) old = {attribute.name: attribute for attribute in before.attributes} diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index 77967732..ebfd4847 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -585,12 +585,12 @@ class LabelClassNotInSchema(InvalidAnnotation): class DisallowedGeometry(InvalidAnnotation): - """The annotation's geometry is not the one its class is bound to. + """The annotation's geometry is not one its class accepts. - A ``LabelClass`` declares a single ``geometry``, so this is an equality - test, not a membership one. ``SchemaService.allowed_geometries`` is the - union across a version's classes — the right answer to "what may this - project draw?" and the wrong one here, where a polygon under a bbox class + A ``LabelClass`` declares a set of ``geometries``, so this is membership in + **that class's** set. ``SchemaService.allowed_geometries`` is the union + across a version's classes — the right answer to "what may this project + draw?" and the wrong one here, where a polygon under a boxes-only class would sail through. """ diff --git a/src/visionset/kernel/services/annotation_service.py b/src/visionset/kernel/services/annotation_service.py index 38096701..b86a1688 100644 --- a/src/visionset/kernel/services/annotation_service.py +++ b/src/visionset/kernel/services/annotation_service.py @@ -495,10 +495,10 @@ def _validate(annotation: Annotation, schema: AnnotationSchema) -> None: a remove plus an add there, and what makes ``LabelClass.name`` stored stripped here. - The geometry rule is per-class equality, not membership: a ``LabelClass`` - declares one ``geometry``. ``SchemaService.allowed_geometries`` is the union - across a version's classes, which answers "what may this project draw?" and - would happily let a polygon through under a bbox class. + The geometry rule is membership in **this class's** set. That is not the same + test as ``SchemaService.allowed_geometries``, which is the union across a + version's classes: it answers "what may this project draw?" and would happily + let a polygon through under a class that only accepts boxes. Pure, and given the schema rather than reading one, so the whole rule can be exercised without a workspace. @@ -511,9 +511,10 @@ def _validate(annotation: Annotation, schema: AnnotationSchema) -> None: f"which declares {known}" ) - if annotation.geometry.type != label_class.geometry: + if annotation.geometry.type not in label_class.geometries: + allowed = ", ".join(geometry.value for geometry in label_class.geometries) raise DisallowedGeometry( - f"class {label_class.name!r} is a {label_class.geometry.value} in schema version " + f"class {label_class.name!r} accepts {allowed} in schema version " f"{schema.version}, but this annotation carries a {annotation.geometry.type.value}" ) diff --git a/src/visionset/kernel/services/release_service.py b/src/visionset/kernel/services/release_service.py index 74088876..c3121f92 100644 --- a/src/visionset/kernel/services/release_service.py +++ b/src/visionset/kernel/services/release_service.py @@ -767,11 +767,14 @@ def _compatibility(release: Release, manifest: Manifest, exporter: Exporter) -> :meth:`ReleaseService.check_export` and :meth:`ReleaseService.export` — cannot disagree about what a release contains. - **Three outcomes, not two.** Each class is written whole, written reduced, or - not written, read off the format's two declared geometry sets. With only a - boolean, this would count a converted polygon as an absent one while the YOLO - and VOC exporters wrote it as a box — the report and the output disagreeing - about the same annotations, neither wrong on its own terms. + **Three outcomes, not two.** Each class *and geometry* is written whole, + written reduced, or not written, read off the format's two declared geometry + sets. With only a boolean, this would count a converted polygon as an absent + one while the YOLO and VOC exporters wrote it as a box — the report and the + output disagreeing about the same annotations, neither wrong on its own terms. + Per geometry rather than per class for the same kind of reason: a class + accepting both boxes and polygons has two answers under a boxes-only format, + and a row that carried one of them would misdescribe the other. ``excluded_annotations`` counts what disappears and ``degraded_annotations`` counts what survives coarser; ``compatible`` is false for either, so nothing about consent moved. @@ -792,24 +795,30 @@ def _compatibility(release: Release, manifest: Manifest, exporter: Exporter) -> cannot open would have to become a field on ``ManifestAsset``, behind a ``MANIFEST_VERSION`` bump, which is its own decision. """ - per_class: dict[str, tuple[GeometryType, int, set[UUID]]] = {} + # Keyed by class *and* geometry: a class accepting both boxes and polygons + # gets one row per shape, because a boxes-only format writes one whole and + # reduces the other and a single row could only say one of those. Seeded from + # the declared classes so a class nobody used still appears, at zero. + per_shape: dict[tuple[str, GeometryType], tuple[int, set[UUID]]] = {} for declared in manifest.classes: - per_class[declared.name] = (declared.geometry, 0, set()) + for geometry in declared.geometries: + per_shape[(declared.name, geometry)] = (0, set()) counts = {status: 0 for status in ClassExportStatus} touched: dict[ClassExportStatus, set[UUID]] = {status: set() for status in ClassExportStatus} for asset in manifest.assets: for annotation in asset.annotations: - geometry, count, assets = per_class.get( - annotation.label_class, - # A class the manifest's own `classes` does not declare cannot - # happen — `SchemaChangeWouldOrphan` refuses to remove a class - # annotations depend on — but a report that dropped a label it - # could not place would be silently wrong, so it is placed by the - # geometry it actually carries. - (GeometryType(annotation.geometry.type), 0, set()), + geometry = GeometryType(annotation.geometry.type) + # A shape the manifest's own `classes` does not declare cannot happen + # — `SchemaChangeWouldOrphan` refuses to remove a class annotations + # depend on, and taking a geometry away from one is destructive for + # the same reason — but a report that dropped a label it could not + # place would be silently wrong, so it is placed by what it carries. + count, assets = per_shape.get((annotation.label_class, geometry), (0, set())) + per_shape[(annotation.label_class, geometry)] = ( + count + 1, + assets | {asset.asset_id}, ) - per_class[annotation.label_class] = (geometry, count + 1, assets | {asset.asset_id}) status = _status_of(geometry, exporter) counts[status] += 1 touched[status].add(asset.asset_id) @@ -823,7 +832,7 @@ def _compatibility(release: Release, manifest: Manifest, exporter: Exporter) -> assets=len(assets), reason=_reason_for(_status_of(geometry, exporter), geometry, exporter), ) - for name, (geometry, count, assets) in per_class.items() + for (name, geometry), (count, assets) in per_shape.items() ) dropped = counts[ClassExportStatus.DROPPED] diff --git a/src/visionset/kernel/services/schema_service.py b/src/visionset/kernel/services/schema_service.py index 0b25ce0e..ff4abae9 100644 --- a/src/visionset/kernel/services/schema_service.py +++ b/src/visionset/kernel/services/schema_service.py @@ -119,19 +119,23 @@ def list_versions(self, project_id: UUID) -> list[AnnotationSchema]: def allowed_geometries( self, project_id: UUID, version: int | None = None ) -> frozenset[GeometryType]: - """Which geometries a version permits: the ones its classes are bound to. + """Which geometries a version permits: the union across its classes. - Derived rather than stored, so it cannot disagree with the classes. This - is the set an annotation's ``geometry.type`` is membership-tested against - — the discriminator's values *are* ``GeometryType`` members, so no - translation sits in between. + Derived rather than stored, so it cannot disagree with the classes. It + answers "what may this project draw?", and it is deliberately **not** the + test a write goes through: an annotation is judged against its own + class's ``geometries``, which this union is wider than as soon as two + classes accept different shapes. ``AnnotationService._validate`` owns + that narrower test. Raises: ProjectNotFound: no such project in this workspace. SchemaNotFound: no such version, or no schema at all. """ schema = self.get_active(project_id) if version is None else self.get(project_id, version) - return frozenset(label_class.geometry for label_class in schema.classes) + return frozenset( + geometry for label_class in schema.classes for geometry in label_class.geometries + ) # --- comparing --------------------------------------------------------- @@ -451,13 +455,18 @@ def _require_coherent(classes: Sequence[LabelClass]) -> None: to everybody except the code. """ unsupported = sorted( - {c.geometry.value for c in classes if c.geometry not in IMPLEMENTED_GEOMETRIES} + { + geometry.value + for c in classes + for geometry in c.geometries + if geometry not in IMPLEMENTED_GEOMETRIES + } ) if unsupported: supported = ", ".join(sorted(geometry.value for geometry in IMPLEMENTED_GEOMETRIES)) raise UnsupportedGeometry( f"no geometry implementation for {', '.join(repr(g) for g in unsupported)}; " - f"a class can only use one of {supported}" + f"a class can only use {supported}" ) seen: dict[str, str] = {} diff --git a/src/visionset/mcp/schemas.py b/src/visionset/mcp/schemas.py index 5aded4fa..29e10c99 100644 --- a/src/visionset/mcp/schemas.py +++ b/src/visionset/mcp/schemas.py @@ -11,7 +11,7 @@ here would throw that away and add a second definition to keep in step. **The one wart it inherits, stated rather than hidden**: a discriminated union's -tag carries a default in the domain — ``LabelClass.geometry`` does not, but +tag carries a default in the domain — ``LabelClass.geometries`` does not, but ``Geometry`` and ``Partition`` do — so the generated schema shows ``type`` as optional while pydantic needs it in the input dict to pick a variant. The REST surface fixes that by dropping the defaults from its own bodies; here it is @@ -124,9 +124,10 @@ def preview_schema_change(project: ProjectRef, classes: ClassesParam) -> dict[st only way to find out that a change is destructive without attempting it. `diff.is_destructive` true means the proposal narrows the contract: a class or - an attribute is gone, or a geometry moved. `diff.destructive_classes` names - them, and applying it then needs `allow_destructive=true`. Adding classes or - optional attributes is additive and needs no flag. + an attribute is gone, or a class lost one of its geometries. + `diff.destructive_classes` names them, and applying it then needs + `allow_destructive=true`. Adding classes, optional attributes, or another + geometry to a class is additive and needs no flag. **`is_refused` is the answer no flag changes.** True means annotations already exist under a class this proposal drops, so `create_schema_version` refuses @@ -140,9 +141,10 @@ def preview_schema_change(project: ProjectRef, classes: ClassesParam) -> dict[st the publish, in which case the publish refuses and that refusal is the authoritative one. - Each entry of `classes` must carry `geometry` as one of the declared geometry - types; matching against the current version is by exact class name, so - renaming a class reads here as one removal plus one addition. + Each entry of `classes` must carry `geometries`, a non-empty list of declared + geometry types — a class may be labeled as more than one shape. Matching + against the current version is by exact class name, so renaming a class reads + here as one removal plus one addition. """ with opened_workspace() as workspace: resolved = resolve_project(workspace, project) @@ -200,7 +202,7 @@ def create_schema_version( It is stored verbatim and can never be edited, so write it as a record rather than as a note to yourself. Omitting it is legal. - Three refusals to expect. A class bound to a geometry VisionSet has not + Three refusals to expect. A class naming a geometry VisionSet has not implemented is rejected outright. A narrowing change is rejected until you pass `allow_destructive=true`. And a narrowing change that would orphan annotations already written under an affected class is rejected with **no** diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 8579eba2..c3d32a5d 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -295,7 +295,7 @@ def of(cls, attribute: Attribute) -> Self: # Request and response, for the reason above ``AttributeBody``. class LabelClassBody(BaseModel): - """One labelable class, bound to a geometry.""" + """One labelable class, and the geometries an annotation of it may carry.""" model_config = ConfigDict(extra="forbid") @@ -305,7 +305,13 @@ class LabelClassBody(BaseModel): # UNSUPPORTED_GEOMETRY from ``SchemaService``. Narrowing the enum here would # be a second list to keep in step with ``IMPLEMENTED_GEOMETRIES`` — which is # derived off the ``Geometry`` union precisely so no second list exists. - geometry: GeometryType + # + # A response always carries this sorted and deduplicated, because the domain + # does; a request need not, and gets it back canonicalised. The old singular + # ``geometry`` key is deliberately *not* accepted here, although ``LabelClass`` + # itself reads one — a stored document has to keep loading, while a client + # sending the retired spelling is better told so than silently reinterpreted. + geometries: tuple[GeometryType, ...] = Field(min_length=1) color: str | None = None attributes: tuple[AttributeBody, ...] = () @@ -318,7 +324,7 @@ def _the_domain_accepts_it(self) -> Self: def to_domain(self) -> LabelClass: return LabelClass( name=self.name, - geometry=self.geometry, + geometries=self.geometries, color=self.color, attributes=tuple(attribute.to_domain() for attribute in self.attributes), ) @@ -327,7 +333,7 @@ def to_domain(self) -> LabelClass: def of(cls, label_class: LabelClass) -> Self: return cls( name=label_class.name, - geometry=label_class.geometry, + geometries=label_class.geometries, color=label_class.color, attributes=tuple(AttributeBody.of(a) for a in label_class.attributes), ) diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index fa5303fb..e0695da0 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -156,7 +156,9 @@ def label_class(value: LabelClass) -> dict[str, Any]: """One class of a schema version. Also the *input* shape ``schema apply`` reads.""" return { "name": value.name, - "geometry": value.geometry.value, + # Already sorted and deduplicated by the domain, so the list is one + # value rather than one of several spellings of it. + "geometries": [geometry.value for geometry in value.geometries], "color": value.color, "attributes": [attribute(a) for a in value.attributes], } diff --git a/tests/cli/_flow.py b/tests/cli/_flow.py index ee892596..0437ac52 100644 --- a/tests/cli/_flow.py +++ b/tests/cli/_flow.py @@ -32,7 +32,7 @@ "classes": [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "color": "#ff0000", "attributes": [{"name": "occluded", "kind": "boolean", "default": False}], } diff --git a/tests/cli/test_full_cycle.py b/tests/cli/test_full_cycle.py index dc0e2a6e..7e52dcda 100644 --- a/tests/cli/test_full_cycle.py +++ b/tests/cli/test_full_cycle.py @@ -29,7 +29,7 @@ "classes": [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "attributes": [{"name": "occluded", "kind": "boolean"}], } ] diff --git a/tests/cli/test_schema_commands.py b/tests/cli/test_schema_commands.py index 52416a6a..1a014e27 100644 --- a/tests/cli/test_schema_commands.py +++ b/tests/cli/test_schema_commands.py @@ -139,7 +139,7 @@ def test_a_select_with_no_options_exits_two_in_the_domains_words( [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "attributes": [{"name": "condition", "kind": "select"}], } ], @@ -155,7 +155,7 @@ def test_a_select_with_no_options_exits_two_in_the_domains_words( def test_a_blank_class_name_exits_two_and_says_where(root: Path, tmp_path: Path) -> None: - path = _document(tmp_path, [{"name": " ", "geometry": "bbox"}]) + path = _document(tmp_path, [{"name": " ", "geometries": ["bbox"]}]) result = run(root, "schema", "apply", str(path), "-p", "road-signs") assert result.exit_code == 2, result.output assert "classes.0.name" in usage_error(result) @@ -164,7 +164,7 @@ def test_a_blank_class_name_exits_two_and_says_where(root: Path, tmp_path: Path) def test_an_unimplemented_geometry_exits_one(root: Path, tmp_path: Path) -> None: # ``mask`` is a legal ``GeometryType`` member, so the document parses; it is # the *service* that refuses it. A domain refusal, therefore exit 1. - path = _document(tmp_path, [{"name": "road", "geometry": "mask"}]) + path = _document(tmp_path, [{"name": "road", "geometries": ["mask"]}]) result = run(root, "schema", "apply", str(path), "-p", "road-signs") assert result.exit_code == 1, result.output assert "Error:" in result.stderr @@ -175,7 +175,7 @@ def test_an_unimplemented_geometry_exits_one(root: Path, tmp_path: Path) -> None def test_removing_a_class_exits_one_until_the_flag(root: Path, tmp_path: Path) -> None: ok(root, "schema", "apply", str(schema_file(tmp_path)), "-p", "road-signs") - narrowed = _document(tmp_path, [{"name": "lane", "geometry": "bbox"}]) + narrowed = _document(tmp_path, [{"name": "lane", "geometries": ["bbox"]}]) refused = run(root, "schema", "apply", str(narrowed), "-p", "road-signs") assert refused.exit_code == 1, refused.output assert ok(root, "schema", "apply", str(narrowed), "-p", "road-signs", "--allow-destructive") diff --git a/tests/fixtures/samples.py b/tests/fixtures/samples.py index 89e90a0b..2544d355 100644 --- a/tests/fixtures/samples.py +++ b/tests/fixtures/samples.py @@ -77,7 +77,10 @@ classes=( LabelClass( name="sign", - geometry=GeometryType.BBOX, + # Two, so the round-trip gate exercises a set rather than a list that + # happens to hold one thing — a projection that dropped every element + # after the first would pass against a singleton. + geometries=(GeometryType.BBOX, GeometryType.POLYGON), color="#ff0000", attributes=( Attribute( diff --git a/tests/fixtures/wire_annotations.json b/tests/fixtures/wire_annotations.json index be2c3fb4..ddb8b2d0 100644 --- a/tests/fixtures/wire_annotations.json +++ b/tests/fixtures/wire_annotations.json @@ -167,19 +167,26 @@ } ], "color": "#ff0000", - "geometry": "bbox", + "geometries": [ + "bbox", + "polygon" + ], "name": "sign" }, { "attributes": [], "color": null, - "geometry": "polygon", + "geometries": [ + "polygon" + ], "name": "lane" }, { "attributes": [], "color": "#eb5a47", - "geometry": "polyline", + "geometries": [ + "polyline" + ], "name": "centerline" }, { @@ -193,7 +200,9 @@ } ], "color": "#00ff00", - "geometry": "classification_tag", + "geometries": [ + "classification_tag" + ], "name": "weather" } ], diff --git a/tests/formats/test_coco.py b/tests/formats/test_coco.py index 22919661..5e3f9d43 100644 --- a/tests/formats/test_coco.py +++ b/tests/formats/test_coco.py @@ -25,6 +25,7 @@ from visionset.formats.coco import ANNOTATIONS_DIRNAME, CocoExporter from visionset.kernel import ExportSourceUnreadable, LossyExportNotConsented from visionset.kernel.domain import ( + MANIFEST_VERSION, Annotation, ClassificationGeometry, PolygonGeometry, @@ -117,7 +118,7 @@ def test_the_info_block_names_the_release_it_was_cut_from(tmp_path: Path) -> Non # The important one: it names the exact frozen document, so an export can # be traced back to a release that can be re-verified. "manifest_hash": release.manifest_hash, - "manifest_version": 1, + "manifest_version": MANIFEST_VERSION, "schema_version": 1, } diff --git a/tests/formats/test_coco_smoke.py b/tests/formats/test_coco_smoke.py index b0bddb8d..600e0fb4 100644 --- a/tests/formats/test_coco_smoke.py +++ b/tests/formats/test_coco_smoke.py @@ -63,9 +63,9 @@ ) CLASSES = ( - LabelClass(name="sign", geometry=GeometryType.BBOX), - LabelClass(name="lane", geometry=GeometryType.POLYGON), - LabelClass(name="weather", geometry=GeometryType.CLASSIFICATION_TAG), + LabelClass(name="sign", geometries=(GeometryType.BBOX,)), + LabelClass(name="lane", geometries=(GeometryType.POLYGON,)), + LabelClass(name="weather", geometries=(GeometryType.CLASSIFICATION_TAG,)), ) IMAGE_SIZE = (64, 48) diff --git a/tests/formats/test_report_agreement.py b/tests/formats/test_report_agreement.py index aa09f90d..d19c2b9c 100644 --- a/tests/formats/test_report_agreement.py +++ b/tests/formats/test_report_agreement.py @@ -350,7 +350,7 @@ def _polyline(points: list[tuple[float, float]] | None = None) -> Annotation: 2: [_box(x=1, y=1, width=8, height=8), _tag()], } -LANE_CLASSES = (*CLASSES, LabelClass(name="centerline", geometry=GeometryType.POLYLINE)) +LANE_CLASSES = (*CLASSES, LabelClass(name="centerline", geometries=(GeometryType.POLYLINE,))) def _tusimple_lanes(root: Path) -> int: @@ -488,3 +488,86 @@ def test_the_three_general_formats_declare_polyline_truthfully( assert lane.status is ClassExportStatus.DROPPED assert lane.annotations == 3 assert written["centerline"] == 0 + + +# --- one class, two shapes ---------------------------------------------------- + +#: A single class labelled both ways, which is what #584 made expressible. YOLO +#: writes the boxes whole and reduces the polygons; COCO carries both intact. +MIXED_CLASSES = (LabelClass(name="sign", geometries=(GeometryType.BBOX, GeometryType.POLYGON)),) + +#: Two boxes and one polygon, all under ``sign``, on two assets. +MIXED_DRAWING: dict[int, list[Annotation]] = { + 0: [_box(x=8, y=6, width=20, height=22), _polygon()], + 1: [_box(x=2, y=2, width=10, height=10)], +} + + +@pytest.fixture +def mixed(tmp_path: Path) -> Fixture: + fixture = Fixture(tmp_path, classes=MIXED_CLASSES) + fixture.label( + { + position: [one.model_copy(update={"label_class": "sign"}) for one in drawn] + for position, drawn in MIXED_DRAWING.items() + } + ) + return fixture + + +@pytest.mark.parametrize("format_name", sorted(COUNTERS)) +def test_a_class_labelled_two_ways_gets_a_report_row_for_each( + tmp_path: Path, mixed: Fixture, format_name: str +) -> None: + """The defect a per-class report cannot express, and the reason it is per shape. + + Under YOLO one class here is two different answers at once — the boxes are + written whole, the polygons are written as their bounding box and lose their + shape. A report with one row per class could carry only one of those verdicts, + and would describe half its own output wrongly whichever it picked. + + The counts are read off the artifact, like every other test in this file: the + rows a format wrote under the class name must equal the sum of the rows it did + not report as dropped. + """ + release_id = mixed.publish() + dest = tmp_path / f"out-{format_name}" + report = _export(mixed, release_id, _installed()[format_name], dest) + written = COUNTERS[format_name](dest) + mixed.close() + + rows = [one for one in report.classes if one.label_class == "sign"] + assert {one.geometry for one in rows} == {GeometryType.BBOX, GeometryType.POLYGON} + assert sum(one.annotations for one in rows) == 3 + + carried = sum(one.annotations for one in rows if one.status is not ClassExportStatus.DROPPED) + assert written["sign"] == carried, ( + f"{format_name} reports {[(one.geometry.value, one.status.value) for one in rows]} " + f"and wrote {written['sign']} row(s)" + ) + + +def test_yolo_splits_one_mixed_class_into_a_whole_half_and_a_degraded_half( + tmp_path: Path, mixed: Fixture +) -> None: + """The verdicts themselves, named — the parametrized test above only compares counts. + + Written against YOLO specifically because it is the format whose two answers + differ: ``supported_geometries`` is ``{bbox}`` and ``degraded_geometries`` is + ``{polygon}``, so one class produces one of each. COCO carries both and would + make the assertion vacuous. + """ + release_id = mixed.publish() + report = _export(mixed, release_id, _installed()["yolo"], tmp_path / "out") + mixed.close() + + verdicts = {one.geometry: one.status for one in report.classes if one.label_class == "sign"} + assert verdicts == { + GeometryType.BBOX: ClassExportStatus.SUPPORTED, + GeometryType.POLYGON: ClassExportStatus.DEGRADED, + } + # Nothing is lost, so consent is still asked for — a degraded export is not a + # compatible one, which is the call `_compatibility` already made. + assert report.excluded_annotations == 0 + assert report.degraded_annotations == 1 + assert report.compatible is False diff --git a/tests/formats/test_yolo.py b/tests/formats/test_yolo.py index f228a38f..f102e9a1 100644 --- a/tests/formats/test_yolo.py +++ b/tests/formats/test_yolo.py @@ -54,9 +54,9 @@ #: happened to pass under sorting would fail here — which is the whole point of #: The "classes come from the frozen schema" rule. CLASSES = ( - LabelClass(name="sign", geometry=GeometryType.BBOX), - LabelClass(name="lane", geometry=GeometryType.POLYGON), - LabelClass(name="weather", geometry=GeometryType.CLASSIFICATION_TAG), + LabelClass(name="sign", geometries=(GeometryType.BBOX,)), + LabelClass(name="lane", geometries=(GeometryType.POLYGON,)), + LabelClass(name="weather", geometries=(GeometryType.CLASSIFICATION_TAG,)), ) IMAGE_SIZE = (64, 48) @@ -199,7 +199,7 @@ def test_a_class_name_that_would_break_yaml_is_quoted(tmp_path: Path) -> None: fixture = Fixture(tmp_path) fixture.schemas.create_version( fixture.project.id, - [*CLASSES, LabelClass(name='odd: "name"', geometry=GeometryType.BBOX)], + [*CLASSES, LabelClass(name='odd: "name"', geometries=(GeometryType.BBOX,))], ) fixture.label({}) out = fixture.export(fixture.publish(), tmp_path / "out") diff --git a/tests/formats/test_yolo_smoke.py b/tests/formats/test_yolo_smoke.py index 3a92347d..3186b922 100644 --- a/tests/formats/test_yolo_smoke.py +++ b/tests/formats/test_yolo_smoke.py @@ -72,9 +72,9 @@ #: point: this asserts ultralytics reads back the schema's order and not the #: alphabet's. CLASSES = ( - LabelClass(name="sign", geometry=GeometryType.BBOX), - LabelClass(name="lane", geometry=GeometryType.POLYGON), - LabelClass(name="weather", geometry=GeometryType.CLASSIFICATION_TAG), + LabelClass(name="sign", geometries=(GeometryType.BBOX,)), + LabelClass(name="lane", geometries=(GeometryType.POLYGON,)), + LabelClass(name="weather", geometries=(GeometryType.CLASSIFICATION_TAG,)), ) IMAGE_SIZE = (64, 48) diff --git a/tests/kernel/test_annotation_service.py b/tests/kernel/test_annotation_service.py index 7debda12..131870aa 100644 --- a/tests/kernel/test_annotation_service.py +++ b/tests/kernel/test_annotation_service.py @@ -6,6 +6,7 @@ from __future__ import annotations +from collections.abc import Sequence from io import BytesIO from pathlib import Path from typing import Any @@ -56,16 +57,16 @@ SIGN = LabelClass( name="sign", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), attributes=( Attribute(name="occluded", kind="boolean", required=True), Attribute(name="weather", kind="select", options=("dry", "wet")), ), ) -LANE = LabelClass(name="lane", geometry=GeometryType.POLYGON) +LANE = LabelClass(name="lane", geometries=(GeometryType.POLYGON,)) KIOSK = LabelClass( name="kiosk", - geometry=GeometryType.CLASSIFICATION_TAG, + geometries=(GeometryType.CLASSIFICATION_TAG,), attributes=( Attribute(name="operator", kind="string"), Attribute(name="height", kind="number"), @@ -74,7 +75,7 @@ ), ) #: A class the project only learns about in schema version 2. -GHOST = LabelClass(name="ghost", geometry=GeometryType.BBOX) +GHOST = LabelClass(name="ghost", geometries=(GeometryType.BBOX,)) UNANNOTATED = AssetProgress.UNANNOTATED ANNOTATED = AssetProgress.ANNOTATED @@ -117,14 +118,21 @@ def _box(asset_id: UUID, **overrides: Any) -> Annotation: class Fixture: """A workspace with one three-asset batch, ready to be approved and worked.""" - def __init__(self, tmp_path: Path, name: str = "ws", *, assets: int = 3) -> None: + def __init__( + self, + tmp_path: Path, + name: str = "ws", + *, + assets: int = 3, + classes: Sequence[LabelClass] = (SIGN, LANE, KIOSK), + ) -> None: self.workspace = WorkspaceService.init(tmp_path / name) self.batches = BatchService(self.workspace) self.jobs = JobService(self.workspace) self.schemas = SchemaService(self.workspace) self.annotations = AnnotationService(self.workspace) self.project = ProjectService(self.workspace).create(f"{name}-project") - self.schemas.create_version(self.project.id, [SIGN, LANE, KIOSK]) + self.schemas.create_version(self.project.id, list(classes)) self.assets = [self._asset(f"{name}-{index}") for index in range(assets)] self.batch = self.batches.create(self.project.id, "first", self.assets) @@ -244,7 +252,7 @@ def test_an_unknown_job_or_asset_is_refused_on_read(tmp_path: Path) -> None: pytest.param( {"label_class": "lane", "attributes": {}}, DisallowedGeometry, - "is a polygon .* carries a bbox", + "accepts polygon .* carries a bbox", id="geometry-the-class-did-not-declare", ), pytest.param( @@ -292,7 +300,7 @@ def test_an_annotation_the_pinned_version_rejects_is_not_stored( def test_the_geometry_rule_is_per_class_not_the_versions_union(tmp_path: Path) -> None: - """The version allows polygons — but not under a class bound to bboxes.""" + """The version allows polygons — but not under a class that accepts only bboxes.""" fixture = Fixture(tmp_path) job = fixture.working() assert fixture.schemas.allowed_geometries(fixture.project.id) >= { @@ -300,7 +308,7 @@ def test_the_geometry_rule_is_per_class_not_the_versions_union(tmp_path: Path) - GeometryType.POLYGON, } - with pytest.raises(DisallowedGeometry, match="is a bbox .* carries a polygon"): + with pytest.raises(DisallowedGeometry, match="accepts bbox .* carries a polygon"): fixture.annotations.add( job.id, [ @@ -313,6 +321,55 @@ def test_the_geometry_rule_is_per_class_not_the_versions_union(tmp_path: Path) - fixture.close() +#: One class, two shapes. The whole point of #584: a sign photographed close up is +#: worth outlining and one at the end of the street is worth boxing, and they are +#: the same class. +BOTH = LabelClass( + name="sign", + geometries=(GeometryType.BBOX, GeometryType.POLYGON), + attributes=(Attribute(name="occluded", kind="boolean", required=True),), +) + + +def test_a_class_accepting_two_geometries_accepts_either_of_them(tmp_path: Path) -> None: + """Both, in one call, under one class — which is the feature. + + Written as one `add` rather than two so the all-or-nothing write is exercised + too: a gate that admitted the box and refused the polygon would store neither + and this would fail on the count rather than on the refusal. + """ + fixture = Fixture(tmp_path, classes=(BOTH,)) + job = fixture.working() + + stored = fixture.annotations.add( + job.id, + [ + _box(fixture.assets[0]), + _box( + fixture.assets[0], + geometry=PolygonGeometry(points=[(0.0, 0.0), (1.0, 0.0), (1.0, 1.0)]), + ), + ], + ) + + assert [one.geometry.type for one in stored] == [GeometryType.BBOX, GeometryType.POLYGON] + fixture.close() + + +def test_a_class_accepting_two_geometries_still_refuses_a_third(tmp_path: Path) -> None: + """Membership, not "anything goes" — the half a widened gate loses silently.""" + fixture = Fixture(tmp_path, classes=(BOTH,)) + job = fixture.working() + + with pytest.raises(DisallowedGeometry, match="accepts bbox, polygon"): + fixture.annotations.add( + job.id, + [_box(fixture.assets[0], geometry=ClassificationGeometry(), attributes={})], + ) + assert fixture.annotations.for_asset(job.id, fixture.assets[0]) == [] + fixture.close() + + def test_an_optional_attribute_may_simply_be_absent(tmp_path: Path) -> None: """`required` and `default` are independent — nothing is filled in for you.""" fixture = Fixture(tmp_path) @@ -983,7 +1040,12 @@ def test_an_update_cannot_collide_with_a_tag_already_there(tmp_path: Path) -> No fixture = Fixture(tmp_path) fixture.schemas.create_version( fixture.project.id, - [SIGN, LANE, KIOSK, LabelClass(name="booth", geometry=GeometryType.CLASSIFICATION_TAG)], + [ + SIGN, + LANE, + KIOSK, + LabelClass(name="booth", geometries=(GeometryType.CLASSIFICATION_TAG,)), + ], ) job = fixture.working() asset_id = fixture.assets[0] diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index b860ccbd..0d10fae3 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -57,8 +57,8 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) -LANE = LabelClass(name="lane", geometry=GeometryType.POLYGON) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) +LANE = LabelClass(name="lane", geometries=(GeometryType.POLYGON,)) class Fixture: diff --git a/tests/kernel/test_capabilities.py b/tests/kernel/test_capabilities.py index 1734d06a..920ffd17 100644 --- a/tests/kernel/test_capabilities.py +++ b/tests/kernel/test_capabilities.py @@ -93,7 +93,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) UNANNOTATED = AssetProgress.UNANNOTATED ANNOTATED = AssetProgress.ANNOTATED diff --git a/tests/kernel/test_concurrency.py b/tests/kernel/test_concurrency.py index a65ee315..3ae2bd88 100644 --- a/tests/kernel/test_concurrency.py +++ b/tests/kernel/test_concurrency.py @@ -274,7 +274,7 @@ def _open_job(root: Path) -> tuple[UUID, list[UUID]]: try: project = ProjectService(workspace).create("p") SchemaService(workspace).create_version( - project.id, [LabelClass(name="sign", geometry=GeometryType.BBOX)] + project.id, [LabelClass(name="sign", geometries=(GeometryType.BBOX,))] ) assets = [] for seed in ("a", "b", "c"): diff --git a/tests/kernel/test_concurrent_membership.py b/tests/kernel/test_concurrent_membership.py index 6555d20d..eedace02 100644 --- a/tests/kernel/test_concurrent_membership.py +++ b/tests/kernel/test_concurrent_membership.py @@ -50,7 +50,7 @@ def __init__(self, tmp_path: Path) -> None: self.workspace = WorkspaceService.init(self.root) self.project = ProjectService(self.workspace).create("membership") SchemaService(self.workspace).create_version( - self.project.id, [LabelClass(name="sign", geometry=GeometryType.BBOX)] + self.project.id, [LabelClass(name="sign", geometries=(GeometryType.BBOX,))] ) self.assets = [self._asset(index) for index in range(4)] self.batch = BatchService(self.workspace).create(self.project.id, "draft", self.assets[:2]) diff --git a/tests/kernel/test_dataset_service.py b/tests/kernel/test_dataset_service.py index a0f30d70..42ed2df2 100644 --- a/tests/kernel/test_dataset_service.py +++ b/tests/kernel/test_dataset_service.py @@ -47,7 +47,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) UNANNOTATED = AssetProgress.UNANNOTATED ANNOTATED = AssetProgress.ANNOTATED @@ -691,7 +691,7 @@ def test_per_class_counts_come_back_in_class_name_order(tmp_path: Path) -> None: fixture = Fixture(tmp_path) fixture.schemas.create_version( fixture.project.id, - [SIGN, LabelClass(name="alpha", geometry=GeometryType.BBOX)], + [SIGN, LabelClass(name="alpha", geometries=(GeometryType.BBOX,))], allow_destructive=True, ) (job,) = fixture.working() diff --git a/tests/kernel/test_events.py b/tests/kernel/test_events.py index c91f0459..352cfe37 100644 --- a/tests/kernel/test_events.py +++ b/tests/kernel/test_events.py @@ -57,7 +57,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) #: One of every event, for the sweeps below. Checked against the class tree by #: ``test_every_event_has_a_sample``, which is what stops this drifting. diff --git a/tests/kernel/test_geometry.py b/tests/kernel/test_geometry.py index 5cc0a3e2..311a03f4 100644 --- a/tests/kernel/test_geometry.py +++ b/tests/kernel/test_geometry.py @@ -177,17 +177,23 @@ def test_discriminator_values_are_geometry_type_members() -> None: def test_geometry_type_is_comparable_to_a_label_class_without_translation() -> None: - # This is the check AnnotationService performs, and it is per class: a LabelClass - # declares one geometry, so the rule is equality against `LabelClass.geometry`, not - # membership in `SchemaService.allowed_geometries` (which is the union across a - # version's classes, and would let a polygon through under a bbox class). The union - # is designed so either comparison needs no adapter layer. - label_class = LabelClass(name="car", geometry=GeometryType.BBOX) + # This is the check AnnotationService performs, and it is per class: the rule + # is membership in *this class's* `geometries`, not in + # `SchemaService.allowed_geometries` (which is the union across a version's + # classes, and would let a polygon through under a boxes-only class). The + # discriminator's values are `GeometryType` members, so neither comparison + # needs an adapter layer. + label_class = LabelClass(name="car", geometries=(GeometryType.BBOX, GeometryType.POLYGON)) annotation = _annotation(BboxGeometry(x=1.0, y=2.0, width=10.0, height=20.0)) - assert annotation.geometry.type == label_class.geometry + assert annotation.geometry.type in label_class.geometries + + # The second member of the set, so the test would notice a membership check + # that had quietly collapsed back into equality against the first. + polygon = _annotation(PolygonGeometry(points=[(0.0, 0.0), (1.0, 0.0), (1.0, 1.0)])) + assert polygon.geometry.type in label_class.geometries tagged = _annotation(ClassificationGeometry()) - assert tagged.geometry.type != label_class.geometry + assert tagged.geometry.type not in label_class.geometries def test_implemented_geometries_names_exactly_the_variants_of_the_union() -> None: diff --git a/tests/kernel/test_ingest_service.py b/tests/kernel/test_ingest_service.py index c81fc0e8..d93cf771 100644 --- a/tests/kernel/test_ingest_service.py +++ b/tests/kernel/test_ingest_service.py @@ -256,7 +256,7 @@ def assets(self) -> list[Asset]: def freeze(self, batch_id: UUID) -> None: """Approve the batch, creating the schema version approval has to pin.""" SchemaService(self.workspace).create_version( - self.project.id, [LabelClass(name="thing", geometry=GeometryType.BBOX)] + self.project.id, [LabelClass(name="thing", geometries=(GeometryType.BBOX,))] ) self.batches.approve(batch_id) diff --git a/tests/kernel/test_job_service.py b/tests/kernel/test_job_service.py index f99eb9a4..42fffa15 100644 --- a/tests/kernel/test_job_service.py +++ b/tests/kernel/test_job_service.py @@ -46,7 +46,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) UNANNOTATED = AssetProgress.UNANNOTATED ANNOTATED = AssetProgress.ANNOTATED diff --git a/tests/kernel/test_metadata_store.py b/tests/kernel/test_metadata_store.py index 6ce169d0..cee8fb06 100644 --- a/tests/kernel/test_metadata_store.py +++ b/tests/kernel/test_metadata_store.py @@ -131,7 +131,7 @@ def _seed(uow: UnitOfWork) -> list[tuple[str, UUID]]: classes=[ LabelClass( name="car", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), color="#ff0000", attributes=[Attribute(name="occluded", kind="boolean", required=True)], ) @@ -373,7 +373,7 @@ def test_schema_classes_and_attributes_round_trip(tmp_path: Path) -> None: schema = uow.schemas.get(_seed(uow)[6][1]) assert schema is not None label_class = schema.classes[0] - assert label_class.geometry is GeometryType.BBOX + assert label_class.geometries == (GeometryType.BBOX,) assert label_class.attributes[0] == Attribute( name="occluded", kind="boolean", required=True ) diff --git a/tests/kernel/test_project_service.py b/tests/kernel/test_project_service.py index 80a19a0e..ff6716a1 100644 --- a/tests/kernel/test_project_service.py +++ b/tests/kernel/test_project_service.py @@ -65,7 +65,7 @@ def _populate(workspace: WorkspaceService, project_id: UUID, dataset_id: UUID) - AnnotationSchema( project_id=project_id, version=1, - classes=[LabelClass(name="sign", geometry=GeometryType.BBOX)], + classes=[LabelClass(name="sign", geometries=(GeometryType.BBOX,))], ) ) uow.sources.add( @@ -539,7 +539,7 @@ def _schema(workspace: WorkspaceService, project_id: UUID, *names: str) -> None: AnnotationSchema( project_id=project_id, version=1, - classes=[LabelClass(name=name, geometry=GeometryType.BBOX) for name in names], + classes=[LabelClass(name=name, geometries=(GeometryType.BBOX,)) for name in names], ) ) diff --git a/tests/kernel/test_release.py b/tests/kernel/test_release.py index 740df0c2..42f6f3bb 100644 --- a/tests/kernel/test_release.py +++ b/tests/kernel/test_release.py @@ -29,7 +29,7 @@ sha256_hex, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) BOX = BboxGeometry(x=1.0, y=2.0, width=3.0, height=4.0) @@ -137,7 +137,7 @@ def test_attribute_values_are_ordered_by_key_and_not_by_insertion() -> None: def test_the_bytes_are_utf_eight_with_no_incidental_whitespace() -> None: manifest = Manifest( - schema_version=1, classes=(LabelClass(name="señal", geometry=SIGN.geometry),) + schema_version=1, classes=(LabelClass(name="señal", geometries=SIGN.geometries),) ) raw = canonical_bytes(manifest) assert b"se\xc3\xb1al" in raw diff --git a/tests/kernel/test_release_service.py b/tests/kernel/test_release_service.py index 1c7c5ed4..c1f6c7d5 100644 --- a/tests/kernel/test_release_service.py +++ b/tests/kernel/test_release_service.py @@ -62,7 +62,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) RECIPE = SplitRecipe(train=0.6, val=0.2, test=0.2, seed=42) @@ -243,7 +243,7 @@ def test_the_manifest_pins_the_schema_version_in_force_when_it_was_published( fixture = Fixture(tmp_path) dataset_id = fixture.ready() fixture.schemas.create_version( - fixture.project.id, [SIGN, LabelClass(name="lane", geometry=GeometryType.POLYGON)] + fixture.project.id, [SIGN, LabelClass(name="lane", geometries=(GeometryType.POLYGON,))] ) release = fixture.releases.publish(dataset_id, "v1") @@ -943,7 +943,7 @@ def _polygon(asset_id: UUID) -> Annotation: def _mixed(fixture: Fixture) -> UUID: """A release holding boxes on every asset and polygons on the first two.""" fixture.schemas.create_version( - fixture.project.id, [SIGN, LabelClass(name="lane", geometry=GeometryType.POLYGON)] + fixture.project.id, [SIGN, LabelClass(name="lane", geometries=(GeometryType.POLYGON,))] ) batch = fixture.batches.create(fixture.project.id, "mixed", fixture.asset_ids) fixture.batches.approve(batch.id) @@ -1007,7 +1007,7 @@ def test_a_class_nobody_used_excludes_nothing_however_unsupported(tmp_path: Path """ fixture = Fixture(tmp_path) fixture.schemas.create_version( - fixture.project.id, [SIGN, LabelClass(name="lane", geometry=GeometryType.POLYGON)] + fixture.project.id, [SIGN, LabelClass(name="lane", geometries=(GeometryType.POLYGON,))] ) fixture.promote() release = fixture.releases.publish(fixture.dataset_id, "v1") diff --git a/tests/kernel/test_schema_diff.py b/tests/kernel/test_schema_diff.py index 2b1b0cb2..e5b5d51f 100644 --- a/tests/kernel/test_schema_diff.py +++ b/tests/kernel/test_schema_diff.py @@ -21,7 +21,7 @@ def _class(name: str = "sign", **overrides: object) -> LabelClass: - return LabelClass(name=name, **{"geometry": GeometryType.BBOX, **overrides}) # type: ignore[arg-type] + return LabelClass(name=name, **{"geometries": (GeometryType.BBOX,), **overrides}) # type: ignore[arg-type] def _with(*attributes: Attribute) -> LabelClass: @@ -37,7 +37,7 @@ def _with(*attributes: Attribute) -> LabelClass: ( "first version is all additive", (), - (SIGN, _class("lane", geometry=GeometryType.POLYGON)), + (SIGN, _class("lane", geometries=(GeometryType.POLYGON,))), {(ADDITIVE, "sign", None), (ADDITIVE, "lane", None)}, ), ("class added", (SIGN,), (SIGN, _class("lane")), {(ADDITIVE, "lane", None)}), @@ -55,9 +55,24 @@ def _with(*attributes: Attribute) -> LabelClass: {(ADDITIVE, "Sign", None), (DESTRUCTIVE, "sign", None)}, ), ( - "class geometry changed", + # A geometry set moves in two independent directions, and only one of + # them can orphan a label — so the swap below is *both* at once, and the + # two rows beneath it are each direction on its own. + "class geometry swapped is a removal plus an addition", + (SIGN,), + (_class(geometries=(GeometryType.POLYGON,)),), + {(ADDITIVE, "sign", None), (DESTRUCTIVE, "sign", None)}, + ), + ( + "class gains a geometry", + (SIGN,), + (_class(geometries=(GeometryType.BBOX, GeometryType.POLYGON)),), + {(ADDITIVE, "sign", None)}, + ), + ( + "class loses a geometry", + (_class(geometries=(GeometryType.BBOX, GeometryType.POLYGON)),), (SIGN,), - (_class(geometry=GeometryType.POLYGON),), {(DESTRUCTIVE, "sign", None)}, ), ("class color changed is not a change", (SIGN,), (_class(color="#ff0000"),), set()), diff --git a/tests/kernel/test_schema_service.py b/tests/kernel/test_schema_service.py index b258b5b9..46ea8954 100644 --- a/tests/kernel/test_schema_service.py +++ b/tests/kernel/test_schema_service.py @@ -47,13 +47,13 @@ from visionset.kernel.ports import UnitOfWork from visionset.kernel.services import ProjectService, SchemaService, WorkspaceService -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) -LANE = LabelClass(name="lane", geometry=GeometryType.POLYGON) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) +LANE = LabelClass(name="lane", geometries=(GeometryType.POLYGON,)) #: One class using every attribute kind, so the round-trip test covers them all. RICH = LabelClass( name="vehicle", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), color="#3355ff", attributes=( Attribute(name="note", kind="string", default="none"), @@ -337,7 +337,7 @@ class to everybody except the code.""" project = projects.create("signs") with pytest.raises(InvalidSchema, match="unique within a version"): schemas.create_version( - project.id, [SIGN, LabelClass(name=duplicate, geometry=GeometryType.POLYGON)] + project.id, [SIGN, LabelClass(name=duplicate, geometries=(GeometryType.POLYGON,))] ) assert schemas.list_versions(project.id) == [] workspace.close() @@ -354,7 +354,7 @@ def test_a_class_bound_to_an_unimplemented_geometry_is_refused( workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") with pytest.raises(UnsupportedGeometry, match="no geometry implementation"): - schemas.create_version(project.id, [LabelClass(name="thing", geometry=geometry)]) + schemas.create_version(project.id, [LabelClass(name="thing", geometries=(geometry,))]) assert schemas.list_versions(project.id) == [] workspace.close() @@ -364,9 +364,9 @@ def test_every_implemented_geometry_is_accepted(tmp_path: Path, geometry: Geomet workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") schema = schemas.create_version( - project.id, [LabelClass(name="thing", geometry=geometry)] + project.id, [LabelClass(name="thing", geometries=(geometry,))] ).published - assert schema.classes[0].geometry is geometry + assert schema.classes[0].geometries == (geometry,) workspace.close() @@ -375,7 +375,9 @@ def test_an_unsupported_geometry_is_reported_as_an_invalid_schema(tmp_path: Path workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") with pytest.raises(InvalidSchema): - schemas.create_version(project.id, [LabelClass(name="road", geometry=GeometryType.MASK)]) + schemas.create_version( + project.id, [LabelClass(name="road", geometries=(GeometryType.MASK,))] + ) workspace.close() @@ -413,7 +415,7 @@ def test_one_class_cannot_carry_two_attributes_with_the_same_name() -> None: with pytest.raises(ValidationError, match="same name"): LabelClass( name="sign", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), attributes=( Attribute(name="weather", kind="string"), Attribute(name="WEATHER", kind="boolean"), @@ -561,7 +563,7 @@ def test_a_renamed_class_is_refused_like_a_removed_one(tmp_path: Path) -> None: with pytest.raises(SchemaChangeWouldOrphan, match="'sign'"): schemas.create_version( project.id, - [LabelClass(name="signal", geometry=GeometryType.BBOX)], + [LabelClass(name="signal", geometries=(GeometryType.BBOX,))], allow_destructive=True, ) workspace.close() @@ -916,3 +918,85 @@ def test_each_version_carries_its_own_provenance(tmp_path: Path) -> None: None, ] workspace.close() + + +def test_a_schema_row_written_before_geometries_were_plural_still_loads( + tmp_path: Path, +) -> None: + """The one back-compatibility point, exercised against a real stored row. + + ``annotation_schema.classes`` is a JSON column, so #584 needed no migration — + which means nothing rewrote the documents already on disk, and every one of + them spells the field ``geometry`` and singular. The rewrite lives in + ``LabelClass``'s own before-validator, and this is the only test that can see + it working on the shape it exists for: a document this build never wrote. + + Rewritten through the store rather than through a service for the reason the + provenance test above gives — no service can produce this state, and that is + the point. + """ + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + schemas.create_version( + project.id, + [LabelClass(name="sign", geometries=(GeometryType.BBOX, GeometryType.POLYGON))], + ) + workspace.close() + + store = SqliteMetadataStore(tmp_path / "ws" / "visionset.db") + with store.engine.begin() as connection: + connection.execute( + text("update annotation_schema set classes = :classes"), + {"classes": '[{"name": "sign", "geometry": "bbox", "color": null, "attributes": []}]'}, + ) + store.close() + + reopened = WorkspaceService.open(tmp_path / "ws") + (loaded,) = SchemaService(reopened).get(project.id, 1).classes + assert loaded.geometries == (GeometryType.BBOX,) + reopened.close() + + +def test_the_old_and_new_spellings_of_one_class_are_the_same_value(tmp_path: Path) -> None: + """Equal, not merely both loadable — so the rewrite cannot drift into a second shape.""" + old = LabelClass.model_validate({"name": "sign", "geometry": "bbox"}) + assert old == LabelClass(name="sign", geometries=(GeometryType.BBOX,)) + + +def test_a_class_carrying_both_spellings_is_refused(tmp_path: Path) -> None: + """No build ever wrote one, so guessing which the author meant would be guessing.""" + with pytest.raises(ValidationError, match="geometry"): + LabelClass.model_validate({"name": "sign", "geometry": "bbox", "geometries": ["polygon"]}) + + +def test_a_geometry_set_is_stored_deduplicated_and_in_one_order(tmp_path: Path) -> None: + """Two spellings of one set are one value, which is what keeps a release hash stable. + + ``release.canonical_bytes`` dumps this field straight into the document it + hashes, so an order that depended on how the caller happened to type the set + would make two identical schemas produce two different manifests. + """ + workspace, projects, schemas = _services(tmp_path) + project = projects.create("signs") + written = schemas.create_version( + project.id, + [ + LabelClass( + name="sign", + geometries=( + GeometryType.POLYGON, + GeometryType.BBOX, + GeometryType.POLYGON, + ), + ) + ], + ) + + assert written.classes[0].geometries == (GeometryType.BBOX, GeometryType.POLYGON) + workspace.close() + + +def test_a_class_must_accept_at_least_one_geometry() -> None: + """A class nobody could ever label is refused where it is written, not later.""" + with pytest.raises(ValidationError, match="at least 1 item"): + LabelClass(name="sign", geometries=()) diff --git a/tests/kernel/test_summary_service.py b/tests/kernel/test_summary_service.py index 4093a543..fc8ef676 100644 --- a/tests/kernel/test_summary_service.py +++ b/tests/kernel/test_summary_service.py @@ -46,7 +46,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) class Fixture: diff --git a/tests/kernel/test_trunk_supersession.py b/tests/kernel/test_trunk_supersession.py index 72d68461..e86a9f75 100644 --- a/tests/kernel/test_trunk_supersession.py +++ b/tests/kernel/test_trunk_supersession.py @@ -56,7 +56,7 @@ WorkspaceService, ) -SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) +SIGN = LabelClass(name="sign", geometries=(GeometryType.BBOX,)) UNANNOTATED = AssetProgress.UNANNOTATED ANNOTATED = AssetProgress.ANNOTATED @@ -291,7 +291,7 @@ def test_a_correction_replaces_the_assets_labels_rather_than_adding_a_round( first, _ = fixture.assets fixture.schemas.create_version( fixture.project.id, - [SIGN, LabelClass(name="lamp", geometry=GeometryType.BBOX)], + [SIGN, LabelClass(name="lamp", geometries=(GeometryType.BBOX,))], ) parent_job = fixture.open_batch("first", [first]) fixture.annotations.add(parent_job.id, [_box(first)]) @@ -395,7 +395,7 @@ def test_whichever_batch_wrote_last_is_what_the_trunk_projects( first, _ = fixture.assets fixture.schemas.create_version( fixture.project.id, - [SIGN, LabelClass(name="lamp", geometry=GeometryType.BBOX)], + [SIGN, LabelClass(name="lamp", geometries=(GeometryType.BBOX,))], ) parent_job = fixture.open_batch("first", [first]) fixture.annotations.add(parent_job.id, [_box(first)]) diff --git a/tests/mcp/_flow.py b/tests/mcp/_flow.py index 79d86b0d..d4c2691c 100644 --- a/tests/mcp/_flow.py +++ b/tests/mcp/_flow.py @@ -46,7 +46,7 @@ SCHEMA_CLASSES: list[dict[str, Any]] = [ { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "color": "#ff0000", "attributes": [{"name": "occluded", "kind": "boolean", "default": False}], } @@ -54,7 +54,7 @@ """The smallest schema that is not trivial: one class, one optional attribute.""" #: The lane class, for the suites that write one. Not in ``SCHEMA_CLASSES``. -CENTERLINE: dict[str, Any] = {"name": "centerline", "geometry": "polyline"} +CENTERLINE: dict[str, Any] = {"name": "centerline", "geometries": ["polyline"]} BBOX: dict[str, Any] = {"type": "bbox", "x": 1.0, "y": 2.0, "width": 8.0, "height": 6.0} """A box that fits inside the fixtures' 64x48 images. ``type`` is always spelled out.""" diff --git a/tests/mcp/test_agent_walk.py b/tests/mcp/test_agent_walk.py index ae202764..03ab5f34 100644 --- a/tests/mcp/test_agent_walk.py +++ b/tests/mcp/test_agent_walk.py @@ -55,8 +55,8 @@ def test_an_agent_can_take_a_folder_of_images_to_an_exported_release( "create_schema_version", project="road-signs", classes=[ - {"name": "sign", "geometry": "bbox"}, - {"name": "empty-road", "geometry": "classification_tag"}, + {"name": "sign", "geometries": ["bbox"]}, + {"name": "empty-road", "geometries": ["classification_tag"]}, ], ) ) diff --git a/tests/mcp/test_batch_tools.py b/tests/mcp/test_batch_tools.py index 2cc21ad8..4a40c726 100644 --- a/tests/mcp/test_batch_tools.py +++ b/tests/mcp/test_batch_tools.py @@ -231,7 +231,7 @@ def test_repinning_after_that_is_a_no_op_rather_than_an_error( call( "create_schema_version", project=project, - classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometry": "bbox"}], + classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometries": ["bbox"]}], ) ) @@ -246,7 +246,7 @@ def test_a_narrowing_repin_names_the_flag_that_retries_it( call( "create_schema_version", project=project, - classes=[{"name": "crossing", "geometry": "bbox"}], + classes=[{"name": "crossing", "geometries": ["bbox"]}], allow_destructive=True, ) ) @@ -270,7 +270,7 @@ def test_a_repin_that_would_orphan_this_batchs_labels_offers_no_retry( call( "create_schema_version", project=project, - classes=[{"name": "crossing", "geometry": "bbox"}], + classes=[{"name": "crossing", "geometries": ["bbox"]}], allow_destructive=True, ) ) diff --git a/tests/mcp/test_release_tools.py b/tests/mcp/test_release_tools.py index 4dc896f2..9715c3e9 100644 --- a/tests/mcp/test_release_tools.py +++ b/tests/mcp/test_release_tools.py @@ -64,7 +64,7 @@ def test_a_class_nobody_used_does_not_appear_in_the_stats( # drop its `occluded` attribute, which is a narrowing change and needs # `allow_destructive` — a good demonstration of why `create_schema_version` # says "a class left out is a class removed". - classes=[*SCHEMA_CLASSES, {"name": "pedestrian", "geometry": "bbox"}], + classes=[*SCHEMA_CLASSES, {"name": "pedestrian", "geometries": ["bbox"]}], ) ) stats = payload(call("dataset_stats", project=named)) diff --git a/tests/mcp/test_schema_tools.py b/tests/mcp/test_schema_tools.py index 36fe8094..27c6d3eb 100644 --- a/tests/mcp/test_schema_tools.py +++ b/tests/mcp/test_schema_tools.py @@ -13,8 +13,8 @@ import pytest from tests.mcp._flow import SCHEMA_CLASSES, call, error, payload, project, schema, tool_schemas -CAR_ONLY: list[dict[str, Any]] = [{"name": "car", "geometry": "bbox"}] -BOTH: list[dict[str, Any]] = [*SCHEMA_CLASSES, {"name": "car", "geometry": "bbox"}] +CAR_ONLY: list[dict[str, Any]] = [{"name": "car", "geometries": ["bbox"]}] +BOTH: list[dict[str, Any]] = [*SCHEMA_CLASSES, {"name": "car", "geometries": ["bbox"]}] def test_a_new_project_has_no_schema_at_all( @@ -138,7 +138,11 @@ def test_a_class_bound_to_an_unimplemented_geometry_is_refused( ) -> None: named = project(monkeypatch, tmp_path) refusal = error( - call("create_schema_version", project=named, classes=[{"name": "lane", "geometry": "mask"}]) + call( + "create_schema_version", + project=named, + classes=[{"name": "lane", "geometries": ["mask"]}], + ) ) assert "mask" in refusal["message"] @@ -153,7 +157,7 @@ def test_the_domain_refuses_a_malformed_class_before_the_body_runs( # the same split the API makes between 422 and 409. named = project(monkeypatch, tmp_path) result = call( - "create_schema_version", project=named, classes=[{"name": " ", "geometry": "bbox"}] + "create_schema_version", project=named, classes=[{"name": " ", "geometries": ["bbox"]}] ) assert result.is_error assert "classes.0.name" in result.content[0].text @@ -180,7 +184,7 @@ def test_a_version_carries_the_description_the_agent_wrote( call( "create_schema_version", project=named, - classes=[{"name": "sign", "geometry": "bbox"}], + classes=[{"name": "sign", "geometries": ["bbox"]}], description="the first contract", ) ) @@ -202,7 +206,7 @@ def test_a_version_created_without_one_reports_null_rather_than_omitting_it( call( "create_schema_version", project=named, - classes=[{"name": "sign", "geometry": "bbox"}], + classes=[{"name": "sign", "geometries": ["bbox"]}], ) ) @@ -223,7 +227,7 @@ def test_comparing_two_versions_classifies_what_changed( call( "create_schema_version", project=named, - classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometry": "bbox"}], + classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometries": ["bbox"]}], ) ) @@ -244,7 +248,7 @@ def test_a_narrowing_comparison_names_what_would_break( call( "create_schema_version", project=named, - classes=[{"name": "crossing", "geometry": "bbox"}], + classes=[{"name": "crossing", "geometries": ["bbox"]}], allow_destructive=True, ) ) diff --git a/tests/mcp/test_tool_errors.py b/tests/mcp/test_tool_errors.py index b8fe58ca..e3cb99d9 100644 --- a/tests/mcp/test_tool_errors.py +++ b/tests/mcp/test_tool_errors.py @@ -109,7 +109,11 @@ def test_allow_destructive_is_the_retry_word_for_narrowing_a_contract( ) -> None: named = schema(monkeypatch, tmp_path) refusal = error( - call("create_schema_version", project=named, classes=[{"name": "car", "geometry": "bbox"}]) + call( + "create_schema_version", + project=named, + classes=[{"name": "car", "geometries": ["bbox"]}], + ) ) assert refusal["retry_with"] == "allow_destructive" diff --git a/tests/server/_flow.py b/tests/server/_flow.py index c134aba5..f3cd5aff 100644 --- a/tests/server/_flow.py +++ b/tests/server/_flow.py @@ -23,14 +23,14 @@ #: payload able to be wrong in an interesting way. SIGN: Final[dict[str, Any]] = { "name": "sign", - "geometry": "bbox", + "geometries": ["bbox"], "attributes": [{"name": "occluded", "kind": "boolean", "required": True}], } -LANE: Final[dict[str, Any]] = {"name": "lane", "geometry": "polygon"} +LANE: Final[dict[str, Any]] = {"name": "lane", "geometries": ["polygon"]} #: The lane geometry. NOT in the default schema: four tests elsewhere count the #: classes `project_with_schema` declares, so a suite that needs a lane passes #: `classes=[SIGN, LANE, CENTERLINE]` rather than widening what everyone gets. -CENTERLINE: Final[dict[str, Any]] = {"name": "centerline", "geometry": "polyline"} +CENTERLINE: Final[dict[str, Any]] = {"name": "centerline", "geometries": ["polyline"]} def image_parts(tmp_path: Path, count: int) -> list[tuple[str, tuple[str, bytes, str]]]: diff --git a/tests/server/test_batches.py b/tests/server/test_batches.py index bdad86ab..70a984cb 100644 --- a/tests/server/test_batches.py +++ b/tests/server/test_batches.py @@ -464,7 +464,7 @@ def test_a_class_added_after_approval_reaches_the_batch_with_no_second_call( """ approved(client, ingested) - response = new_version(client, project, SIGN, LANE, {"name": "crossing", "geometry": "bbox"}) + response = new_version(client, project, SIGN, LANE, {"name": "crossing", "geometries": ["bbox"]}) assert response.status_code == 201 body = response.json() @@ -566,7 +566,7 @@ def test_a_completed_batchs_pin_is_history( client: TestClient, tmp_path: Path, runner: InlineDispatcher ) -> None: project, batch_id = annotated_batch(client, runner, tmp_path, images=2) - new_version(client, project, SIGN, LANE, {"name": "crossing", "geometry": "bbox"}) + new_version(client, project, SIGN, LANE, {"name": "crossing", "geometries": ["bbox"]}) response = client.post(f"/batches/{batch_id}/repin") diff --git a/tests/server/test_external_client.py b/tests/server/test_external_client.py index 567a72f4..0d5194ab 100644 --- a/tests/server/test_external_client.py +++ b/tests/server/test_external_client.py @@ -79,7 +79,7 @@ def test_an_external_client_drives_the_cycle_from_ingest_to_an_exported_release( "classes": [ { "name": "nodule", - "geometry": "bbox", + "geometries": ["bbox"], "attributes": [{"name": "malignant", "kind": "boolean", "required": True}], } ] diff --git a/tests/server/test_releases.py b/tests/server/test_releases.py index bf87ba0f..95e60444 100644 --- a/tests/server/test_releases.py +++ b/tests/server/test_releases.py @@ -35,6 +35,7 @@ from tests.server._flow import dataset_of, promoted_dataset from tests.server._jobs import InlineDispatcher +from visionset.kernel.domain import MANIFEST_VERSION from visionset.kernel.services.release_service import EXPORT_REPORT_FILENAME RECIPE = {"train": 0.6, "val": 0.2, "test": 0.2, "seed": 42} @@ -208,7 +209,7 @@ def test_the_manifest_is_served_as_json_and_parses(client: TestClient, release: assert response.headers["content-type"].startswith("application/json") document = response.json() - assert document["manifest_version"] == 1 + assert document["manifest_version"] == MANIFEST_VERSION assert document["schema_version"] == 1 assert len(document["assets"]) == 3 diff --git a/tests/server/test_schemas.py b/tests/server/test_schemas.py index 6861e7a6..7a562e75 100644 --- a/tests/server/test_schemas.py +++ b/tests/server/test_schemas.py @@ -48,7 +48,7 @@ def version_of(response: Any) -> Any: def a_class(name: str = "sign", **overrides: Any) -> dict[str, Any]: - return {"name": name, "geometry": "bbox", **overrides} + return {"name": name, "geometries": ["bbox"], **overrides} # --- the empty start --------------------------------------------------------- @@ -87,7 +87,7 @@ def test_creating_the_first_version_answers_201_and_numbers_it_1( def test_the_next_version_is_numbered_one_higher(client: TestClient, project: str) -> None: post_version(client, project, a_class()) - response = post_version(client, project, a_class(), a_class("lane", geometry="polygon")) + response = post_version(client, project, a_class(), a_class("lane", geometries=("polygon",))) assert response.status_code == 201 assert version_of(response)["version"] == 2 @@ -193,7 +193,7 @@ def test_a_class_with_an_unimplemented_geometry_is_422_unsupported_geometry( The wire model keeps all eight members deliberately, so naming one gets this rather than "not a valid enumeration member". """ - response = post_version(client, project, a_class(geometry="mask")) + response = post_version(client, project, a_class(geometries=("mask",))) assert response.status_code == 422 assert response.json()["code"] == "UNSUPPORTED_GEOMETRY" @@ -202,7 +202,7 @@ def test_a_class_with_an_unimplemented_geometry_is_422_unsupported_geometry( def test_a_geometry_outside_the_enum_is_422_validation_error( client: TestClient, project: str ) -> None: - response = post_version(client, project, a_class(geometry="hexagon")) + response = post_version(client, project, a_class(geometries=("hexagon",))) assert response.status_code == 422 assert response.json()["code"] == "VALIDATION_ERROR" @@ -275,7 +275,7 @@ def test_attributes_and_colors_survive_the_round_trip(client: TestClient, projec body = client.get(f"/projects/{project}/schema").json() assert body["classes"] == [ - {"name": "sign", "geometry": "bbox", "color": "#ff0000", "attributes": [attribute]} + {"name": "sign", "geometries": ["bbox"], "color": "#ff0000", "attributes": [attribute]} ] diff --git a/tests/server/test_wire_fixtures.py b/tests/server/test_wire_fixtures.py index 0411e15a..497aa0e5 100644 --- a/tests/server/test_wire_fixtures.py +++ b/tests/server/test_wire_fixtures.py @@ -95,7 +95,7 @@ def test_the_schema_declares_one_class_per_carryable_geometry() -> None: how two spellings of a contract start. """ payload = committed() - declared = {c["geometry"] for c in payload["schema"]["classes"]} + declared = {g for c in payload["schema"]["classes"] for g in c["geometries"]} assert declared == set(payload["implemented_geometry_types"]) diff --git a/tests/server/test_wire_models.py b/tests/server/test_wire_models.py index 794bb17c..1219eac9 100644 --- a/tests/server/test_wire_models.py +++ b/tests/server/test_wire_models.py @@ -71,14 +71,17 @@ def test_the_wire_attribute_kinds_are_the_domains_own_four() -> None: def test_the_wire_geometry_is_the_domains_own_enum() -> None: """Reused rather than restated, so the eight members cannot drift apart.""" - assert LabelClassBody.model_fields["geometry"].annotation is GeometryType + assert get_args(LabelClassBody.model_fields["geometries"].annotation) == ( + GeometryType, + Ellipsis, + ) def test_a_label_class_round_trips_through_the_domain_and_back() -> None: """`of` and `to_domain` are inverses, so nothing is lost on the way out.""" original = LabelClassBody( name="sign", - geometry=GeometryType.BBOX, + geometries=(GeometryType.BBOX,), color="#ff0000", attributes=( AttributeBody( @@ -96,7 +99,7 @@ def test_a_label_class_round_trips_through_the_domain_and_back() -> None: def test_a_domain_label_class_survives_being_published() -> None: """The other direction: a stored class comes back identical.""" - label_class = LabelClass(name="lane", geometry=GeometryType.POLYGON) + label_class = LabelClass(name="lane", geometries=(GeometryType.POLYGON,)) assert LabelClassBody.of(label_class).to_domain() == label_class @@ -108,7 +111,7 @@ def test_a_wire_label_class_is_refused_by_the_domains_own_rules() -> None: and a malformed payload is answering 500 again. """ with pytest.raises(ValueError, match="at least one non-blank character"): - LabelClassBody(name=" ", geometry=GeometryType.BBOX) + LabelClassBody(name=" ", geometries=(GeometryType.BBOX,)) with pytest.raises(ValueError, match="needs at least one option"): AttributeBody(name="weather", kind="select") From ef86a5274c0556b90f4d85937c002d0337eb8044 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 19:46:53 -0700 Subject: [PATCH 02/17] feat(annotator): a tool is resolved against the class's set, not derived from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `LabelClass.geometries` mirrors the wire, and `toolFor` gains the tool the host currently holds: it keeps that tool when the class accepts it and falls to the class's first drawable geometry when it does not. **An active tool the selected class forbids is unrepresentable**, because one function decides and it never returns one. `InputHost` gains `activeTool`. Without it `activate-class` compares two class defaults rather than two resolved tools, and misses a real move: a host drawing polygons under a both-shapes class that switches to a boxes-only one does change tool, and a polygon in flight has to be cancelled. `isTaggableClass` and `drawableGeometries` stop being each other's negation — a class may accept a tag and a shape — so `classAction` sends `toggle-tag` only when the class draws nothing. Folding a drawable class into it would tag the asset where somebody pressing a class digit meant to arm it. `allowedGeometriesFor` filters instead of wrapping a scalar, which its own docstring had predicted was the only change a set would need. cf. #584 --- .../src/adapters/react/AnnotatorCanvas.tsx | 20 ++++- .../src/adapters/react/visibility.test.ts | 2 +- frontend/annotator/src/core/input/_palette.ts | 18 +++-- frontend/annotator/src/core/input/bindings.ts | 21 ++++-- .../annotator/src/core/input/runAction.ts | 24 +++++- .../src/core/interaction/clipboard.test.ts | 8 +- .../src/core/interaction/draft.test.ts | 4 +- .../src/core/interaction/suggestion.test.ts | 4 +- .../src/core/interaction/suggestion.ts | 13 ++-- .../src/core/interaction/tags.test.ts | 12 +-- .../annotator/src/core/interaction/tags.ts | 14 ++-- .../annotator/src/core/interaction/tool.ts | 73 +++++++++++++------ frontend/annotator/src/core/state/_sample.ts | 6 +- .../annotator/src/core/state/document.test.ts | 8 +- frontend/annotator/src/core/types.ts | 15 ++-- frontend/annotator/src/core/wire.test.ts | 35 ++++++--- frontend/annotator/src/core/wire.ts | 33 ++++++--- frontend/annotator/src/index.ts | 2 +- 18 files changed, 202 insertions(+), 110 deletions(-) diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index 5c352fd7..420fa738 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -257,6 +257,16 @@ export interface AnnotatorCanvasProps { readonly imageSrc: string; /** The class a drawing gesture will carry. `null` is select mode. */ readonly activeClass: string | null; + /** + * Which of the active class's geometries to draw, when it accepts several. + * + * Optional, unlike `InputHost.activeTool` which it feeds, and the asymmetry is + * deliberate: omitting it is a host saying *no preference*, which resolves to + * the class's first geometry and is exactly the behaviour before a class could + * accept more than one. A host with no tool strip has nothing to say here, and + * making it write `activeTool={null}` would be ceremony rather than a decision. + */ + readonly activeTool?: Tool | null; /** Core reads the active class back and never stores it — `InputHost`'s rule. */ readonly onActivateClass: (labelClass: string | null) => void; /** The committed document, after every change. Not called on mount. */ @@ -468,6 +478,7 @@ export function AnnotatorCanvas({ store, imageSrc, activeClass, + activeTool = null, onActivateClass, onAnnotationsChange, onSelectionChange, @@ -564,7 +575,7 @@ export function AnnotatorCanvas({ setView(next); }, []); - const tool: Tool = toolFor(snapshot.document, activeClass); + const tool: Tool = toolFor(snapshot.document, activeClass, activeTool); const tolerances = assetTolerances(view.zoom); // `defaultRegistry` rather than the fold spelled out here: the help sheet lists @@ -584,7 +595,7 @@ export function AnnotatorCanvas({ // catching every click over it. document: withoutHidden(store.document, hiddenNow.current), selection: store.selection, - tool: toolFor(store.document, activeClass), + tool: toolFor(store.document, activeClass, activeTool), tolerances: assetTolerances(viewNow.current.zoom), labelClass: activeClass, mint, @@ -593,7 +604,7 @@ export function AnnotatorCanvas({ setInteraction(turn.state); runEffects(store, turn.effects); }, - [store, activeClass, mint], + [store, activeClass, activeTool, mint], ); /** @@ -777,6 +788,7 @@ export function AnnotatorCanvas({ const host: InputHost = { activeClass, + activeTool, activateClass: onActivateClass, run: (name) => { if (name === RESET_ZOOM) { @@ -932,7 +944,7 @@ export function AnnotatorCanvas({ // Keep the palette effect above from firing a second, redundant `tool-changed` // once the host's state catches up: this path already told the machine. if (action.kind === "activate-class") { - toolNow.current = toolFor(store.document, action.labelClass); + toolNow.current = toolFor(store.document, action.labelClass, activeTool); } } diff --git a/frontend/annotator/src/adapters/react/visibility.test.ts b/frontend/annotator/src/adapters/react/visibility.test.ts index ce9b6677..523ef238 100644 --- a/frontend/annotator/src/adapters/react/visibility.test.ts +++ b/frontend/annotator/src/adapters/react/visibility.test.ts @@ -12,7 +12,7 @@ const WIRE = { schema: { project_id: "11111111-1111-4111-8111-111111111111", version: 1, - classes: [{ name: "box", geometry: "bbox", color: null, attributes: [] }], + classes: [{ name: "box", geometries: ["bbox"], color: null, attributes: [] }], }, annotations: [ { diff --git a/frontend/annotator/src/core/input/_palette.ts b/frontend/annotator/src/core/input/_palette.ts index 4e50bbbb..8f786bfc 100644 --- a/frontend/annotator/src/core/input/_palette.ts +++ b/frontend/annotator/src/core/input/_palette.ts @@ -58,35 +58,35 @@ export const PALETTE_ASSET: AssetDescriptor = { id: "asset-46", width: 800, heig /** Digit 1. */ export const SIGN: LabelClass = { name: "sign", - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], }; /** Digit 2. */ export const LANE: LabelClass = { name: "lane", - geometry: "polygon", + geometries: ["polygon"], color: null, attributes: [], }; /** Digit 3 — tagged, never drawn. */ export const WEATHER: LabelClass = { name: "weather", - geometry: "classification_tag", + geometries: ["classification_tag"], color: null, attributes: [], }; /** Digit 4 — a lane. Drawable, since `polyline` has a tool. */ export const RAIL: LabelClass = { name: "rail", - geometry: "polyline", + geometries: ["polyline"], color: null, attributes: [], }; /** Digit 5 — a second bbox class, so "same tool" has a witness. */ export const STOP: LabelClass = { name: "stop", - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], }; @@ -103,7 +103,7 @@ export const STOP: LabelClass = { */ export const POSE: LabelClass = { name: "pose", - geometry: "keypoints", + geometries: ["keypoints"], color: null, attributes: [], }; @@ -135,7 +135,7 @@ export function wideSchema(count: number): AnnotationSchema { ...PALETTE_SCHEMA, classes: Array.from({ length: count }, (_unused, index) => ({ name: `c${index + 1}`, - geometry: "bbox" as const, + geometries: ["bbox"] as const, color: null, attributes: [], })), @@ -218,6 +218,10 @@ export function recordingHost( get activeClass(): string | null { return current; }, + // No preference, so `toolFor` falls to each class's first geometry — the + // behaviour these tests were written against. A case about a host holding a + // tool sets it on the returned object. + activeTool: null, activateClass(labelClass: string | null): void { current = labelClass; activated.push(labelClass); diff --git a/frontend/annotator/src/core/input/bindings.ts b/frontend/annotator/src/core/input/bindings.ts index 36de632d..dfc8e7da 100644 --- a/frontend/annotator/src/core/input/bindings.ts +++ b/frontend/annotator/src/core/input/bindings.ts @@ -122,6 +122,7 @@ */ import { isTaggableClass } from "../interaction/tags"; +import { drawableGeometries } from "../interaction/tool"; import type { AnnotationSchema } from "../types"; import { FOCUS_CLASS_FIELD, @@ -208,18 +209,26 @@ export const DEFAULT_BINDINGS: readonly Binding[] = [ /** * What pressing this class's key should do, or `null` if the schema forgot it. * - * A tag class toggles; every other declared class becomes active, including one - * declaring a geometry no annotation can carry — `runAction.ts` and the palette - * handle that between them, and silently skipping it here would renumber the - * digits. Exported so a hand-written override names a class the same way - * `classHotkeys` does, rather than guessing which kind to write. + * A class that can only be tagged toggles; every other declared class becomes + * active, including one declaring a geometry no annotation can carry — + * `runAction.ts` and the palette handle that between them, and silently skipping + * it here would renumber the digits. Exported so a hand-written override names a + * class the same way `classHotkeys` does, rather than guessing which kind to + * write. + * + * **A class accepting a tag *and* a shape arms rather than toggles**, and the + * order of the two tests is the whole rule. Since #584 the two are no longer + * exclusive, and folding a drawable class into `toggle-tag` would be the exact + * bug the split kinds exist to prevent: a toggle changes no tool, so a digit + * pressed mid-draw would silently tag the asset instead of arming the class — + * and arming is what somebody pressing a class digit on a canvas means. */ export function classAction(schema: AnnotationSchema, labelClass: string): Action | null { // `classNamed` is this lookup at the document level; a schema is all that is // needed here, and taking one is what keeps a registry memoizable. const declared = schema.classes.find((candidate) => candidate.name === labelClass); if (declared === undefined) return null; - return isTaggableClass(declared) + return isTaggableClass(declared) && drawableGeometries(declared).length === 0 ? { kind: "toggle-tag", labelClass } : { kind: "activate-class", labelClass }; } diff --git a/frontend/annotator/src/core/input/runAction.ts b/frontend/annotator/src/core/input/runAction.ts index 20515eca..77f99a74 100644 --- a/frontend/annotator/src/core/input/runAction.ts +++ b/frontend/annotator/src/core/input/runAction.ts @@ -118,6 +118,7 @@ import { copiedEntries, pastedAnnotations } from "../interaction/clipboard"; import type { Clipboard } from "../interaction/clipboard"; import { toggleTagCommand } from "../interaction/tags"; import { toolFor } from "../interaction/tool"; +import type { Tool } from "../interaction/tool"; import { addAnnotationCommand, composeCommands, @@ -131,15 +132,27 @@ import type { Action, SentEvent } from "./actions"; /** * The capabilities core does not have. * - * All three members are required. An optional one would make "I declined" and "I + * All four members are required. An optional one would make "I declined" and "I * forgot" the same program, with the compiler blessing the second; a host with no * zoom writes `run: () => false` in one line, which is honest. That is this * package's posture everywhere — `tagCommand` answers `null`, `store.discard` - * answers `false`, `drawableGeometry` answers `null`: a refusal is always a value. + * answers `false`, `drawableGeometries` answers `[]`: a refusal is always a value. */ export interface InputHost { /** The class a drawing gesture will carry. `null` is select mode. */ readonly activeClass: string | null; + /** + * The tool the host would rather keep, when the class it is holding allows it. + * + * Required since #584, when a class started accepting a set of geometries and + * `toolFor` stopped being a function of the class alone. Without it, this + * module computes the tool before and after a class change from the class's + * *first* geometry, and so misses a real move: a host drawing polygons under a + * both-shapes class that switches to a boxes-only one really does change tool, + * and a polygon in flight has to be cancelled. `null` means no preference, + * which is what a host that never lets the user pick a tool writes. + */ + readonly activeTool: Tool | null; /** Make this the active class. Core reads it back; it never stores it. */ activateClass(labelClass: string | null): void; /** Anything core cannot do — a zoom, a help sheet. Answers whether it did. */ @@ -275,8 +288,11 @@ export function runAction(action: Action, context: ActionContext): ActionOutcome ) { return UNCHANGED; } - const before = toolFor(document, host.activeClass); - const after = toolFor(document, action.labelClass); + // Both readings take the host's preference, so the comparison is between + // the tool that *was* resolved and the one that will be — not between two + // class defaults, which would agree while the real tool moved. + const before = toolFor(document, host.activeClass, host.activeTool); + const after = toolFor(document, action.labelClass, host.activeTool); host.activateClass(action.labelClass); if (before === after) return CHANGED; return { changed: true, events: [{ type: "tool-changed" }] }; diff --git a/frontend/annotator/src/core/interaction/clipboard.test.ts b/frontend/annotator/src/core/interaction/clipboard.test.ts index 7da1e7f6..f1a68619 100644 --- a/frontend/annotator/src/core/interaction/clipboard.test.ts +++ b/frontend/annotator/src/core/interaction/clipboard.test.ts @@ -30,10 +30,10 @@ const SCHEMA: AnnotationSchema = { created_at: "2026-08-06T00:00:00Z", provenance: null, classes: [ - { name: "sign", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, - { name: "centerline", geometry: "polyline", color: null, attributes: [] }, - { name: "weather", geometry: "classification_tag", color: null, attributes: [] }, + { name: "sign", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, + { name: "centerline", geometries: ["polyline"], color: null, attributes: [] }, + { name: "weather", geometries: ["classification_tag"], color: null, attributes: [] }, ], }; diff --git a/frontend/annotator/src/core/interaction/draft.test.ts b/frontend/annotator/src/core/interaction/draft.test.ts index f7753bcc..ebbe99ed 100644 --- a/frontend/annotator/src/core/interaction/draft.test.ts +++ b/frontend/annotator/src/core/interaction/draft.test.ts @@ -26,7 +26,7 @@ const BOX: Geometry = { type: "bbox", x: 10, y: 20, width: 30, height: 40 }; /** Every attribute kind, one with a default and one without, on one class. */ const SIGN: LabelClass = { name: "sign", - geometry: "bbox", + geometries: ["bbox"], color: "#ff0000", attributes: [ { name: "occluded", kind: "boolean", required: false, options: null, default: false }, @@ -45,7 +45,7 @@ const SIGN: LabelClass = { }; /** No attributes at all — the ordinary case, and what `_sample.ts` uses. */ -const BARE: LabelClass = { name: "bare", geometry: "bbox", color: null, attributes: [] }; +const BARE: LabelClass = { name: "bare", geometries: ["bbox"], color: null, attributes: [] }; const SCHEMA: AnnotationSchema = { project_id: "project-7", diff --git a/frontend/annotator/src/core/interaction/suggestion.test.ts b/frontend/annotator/src/core/interaction/suggestion.test.ts index 31cc8a57..02af2ba6 100644 --- a/frontend/annotator/src/core/interaction/suggestion.test.ts +++ b/frontend/annotator/src/core/interaction/suggestion.test.ts @@ -43,8 +43,8 @@ import type { Suggestion, SuggestionState } from "./suggestion"; const ASSET: AssetDescriptor = { id: "asset-424", width: 800, height: 600 }; -function classOf(name: string, geometry: LabelClass["geometry"]): LabelClass { - return { name, geometry, color: null, attributes: [] }; +function classOf(name: string, ...geometries: LabelClass["geometries"]): LabelClass { + return { name, geometries, color: null, attributes: [] }; } const CAR = classOf("car", "bbox"); diff --git a/frontend/annotator/src/core/interaction/suggestion.ts b/frontend/annotator/src/core/interaction/suggestion.ts index d2d40cc7..f66d5ce4 100644 --- a/frontend/annotator/src/core/interaction/suggestion.ts +++ b/frontend/annotator/src/core/interaction/suggestion.ts @@ -116,17 +116,16 @@ export type SuggestibleGeometryType = (typeof SUGGESTIBLE_GEOMETRY_TYPES)[number /** Whether a class can hold anything a segmenter is able to propose. */ export function isSuggestibleClass(labelClass: LabelClass): boolean { - return (SUGGESTIBLE_GEOMETRY_TYPES as readonly string[]).includes(labelClass.geometry); + return allowedGeometriesFor(labelClass).length > 0; } /** * The kinds the answer may come back in, for the class a suggestion will carry. * - * A **list of one** for every class this build has, because `LabelClass.geometry` - * is singular — `types.ts`: *"`geometry` is singular, and that is the rule an - * annotator is built around"*. It is still a list, because that is the shape the - * route takes and because the day a class declares a set, this function is the - * only thing that changes. + * The intersection of what the class accepts and what a segmenter can propose, + * so a class taking boxes and polygons asks for both and lets the provider pick. + * Ordered by `SUGGESTIBLE_GEOMETRY_TYPES` rather than by the class, so two + * classes offering the same pair ask the route the same question. * * Empty for a class that can hold neither, which is the same fact * `isSuggestibleClass` reports and the reason the tool is not offered there. @@ -134,7 +133,7 @@ export function isSuggestibleClass(labelClass: LabelClass): boolean { export function allowedGeometriesFor( labelClass: LabelClass, ): readonly SuggestibleGeometryType[] { - return isSuggestibleClass(labelClass) ? [labelClass.geometry as SuggestibleGeometryType] : []; + return SUGGESTIBLE_GEOMETRY_TYPES.filter((kind) => labelClass.geometries.includes(kind)); } /** diff --git a/frontend/annotator/src/core/interaction/tags.test.ts b/frontend/annotator/src/core/interaction/tags.test.ts index c111c511..7dbc29a4 100644 --- a/frontend/annotator/src/core/interaction/tags.test.ts +++ b/frontend/annotator/src/core/interaction/tags.test.ts @@ -58,7 +58,7 @@ const ASSET: AssetDescriptor = { id: "asset-7", width: 640, height: 480 }; /** A tag class carrying attributes, so the draft's seeding is visible here too. */ const WEATHER: LabelClass = { name: "weather", - geometry: "classification_tag", + geometries: ["classification_tag"], color: "#00a0ff", attributes: [ { name: "heavy", kind: "boolean", required: false, options: null, default: false }, @@ -69,15 +69,15 @@ const WEATHER: LabelClass = { /** A second tag class, bare — the "multiple classes tag one asset" other half. */ const NIGHT: LabelClass = { name: "night", - geometry: "classification_tag", + geometries: ["classification_tag"], color: null, attributes: [], }; -const SIGN: LabelClass = { name: "sign", geometry: "bbox", color: null, attributes: [] }; -const LANE: LabelClass = { name: "lane", geometry: "polygon", color: null, attributes: [] }; +const SIGN: LabelClass = { name: "sign", geometries: ["bbox"], color: null, attributes: [] }; +const LANE: LabelClass = { name: "lane", geometries: ["polygon"], color: null, attributes: [] }; /** Declarable in a schema, never carryable by an annotation. Not taggable either. */ -const RAIL: LabelClass = { name: "rail", geometry: "polyline", color: null, attributes: [] }; +const RAIL: LabelClass = { name: "rail", geometries: ["polyline"], color: null, attributes: [] }; const SCHEMA: AnnotationSchema = { project_id: "project-7", @@ -160,7 +160,7 @@ describe("which classes can be tagged", () => { // Reads the vocabulary rather than restating it, so a ninth kernel geometry // arriving in `GEOMETRY_TYPES` cannot silently become taggable. const taggable = GEOMETRY_TYPES.filter((geometry) => - isTaggableClass({ name: "x", geometry, color: null, attributes: [] }), + isTaggableClass({ name: "x", geometries: [geometry], color: null, attributes: [] }), ); expect(taggable).toEqual(["classification_tag"]); }); diff --git a/frontend/annotator/src/core/interaction/tags.ts b/frontend/annotator/src/core/interaction/tags.ts index 80e81166..ae23d71a 100644 --- a/frontend/annotator/src/core/interaction/tags.ts +++ b/frontend/annotator/src/core/interaction/tags.ts @@ -133,17 +133,21 @@ import type { Annotation, LabelClass } from "../types"; import { draftAnnotation } from "./draft"; /** - * Whether this class is tagged rather than drawn. + * Whether this class can be tagged — that is, whether it accepts a tag at all. * - * `drawableGeometry`'s missing half: that one answers `null` for a tag class and - * for a `polyline` class alike, so a palette holding only it cannot tell "usable, + * `drawableGeometries`' missing half: that one answers `[]` for a tag class and + * for a `mask` class alike, so a palette holding only it cannot tell "usable, * just not on the canvas" from "not usable here at all". Takes a `LabelClass` - * rather than a name, to match `drawableGeometry` — a palette iterating + * rather than a name, to match `drawableGeometries` — a palette iterating * `schema.classes` already holds one, and a caller holding only a name uses * `tagCommand`, which resolves it internally. + * + * The two are no longer exclusive: a class accepting both a tag and a box is + * taggable *and* drawable, so a caller must ask both questions rather than + * treating one as the negation of the other. */ export function isTaggableClass(labelClass: LabelClass): boolean { - return labelClass.geometry === "classification_tag"; + return labelClass.geometries.includes("classification_tag"); } /** Whether this annotation is a tag carrying this class. Geometry first. */ diff --git a/frontend/annotator/src/core/interaction/tool.ts b/frontend/annotator/src/core/interaction/tool.ts index e051b4b4..7f7f3524 100644 --- a/frontend/annotator/src/core/interaction/tool.ts +++ b/frontend/annotator/src/core/interaction/tool.ts @@ -1,18 +1,26 @@ /** - * Which tool is active — derived from the class the user is holding, never - * stored. + * Which tool is active — resolved against the class the user is holding, never + * stored here. * - * `types.ts` states the rule this file implements: *"`geometry` is singular, and - * that is the rule an annotator is built around: picking a class picks a tool."* + * ## A class accepts a set, so the class alone no longer answers * - * ## Derived, because v1 needed two mechanisms to keep a stored one honest + * Until #584 a class was bound to one geometry and the tool was a pure function + * of the class. A class accepting both boxes and polygons has no single answer, + * so `toolFor` takes what the host currently has active and *resolves*: it keeps + * that tool when the class accepts it, and otherwise falls to the class's first + * drawable geometry. The fallback is the whole guarantee — **an active tool the + * selected class forbids is unrepresentable**, because there is one function + * that decides and it never returns one. + * + * ## Resolved rather than stored, because v1 needed two mechanisms otherwise * * v1 held `activeTool` as its own state and then had to defend the invariant * twice: `ensureToolAllowed` refused a tool outside the project's list, and a * `useEffect` re-forced the tool whenever the task changed underneath it. Both * exist only because `activeTool` and the available geometries were two facts - * free to disagree. Derivation makes the disagreement unrepresentable, and it - * deletes both mechanisms. + * free to disagree. Resolving through one function keeps that disagreement + * unrepresentable, and still deletes both mechanisms: the host's preference is an + * *input* to the answer, never the answer. * * It also removes v1's strangest behaviour: clicking any annotation body while a * drawing tool was active called `ensureToolAllowed("select")`, so the canvas @@ -22,9 +30,9 @@ * `if (activeTool !== "select") return;` guard on every start-move handler, kept, * minus the escape hatch. * - * ## The active class itself is the HOST's, and stays there + * ## The active class — and now the preferred tool — are the HOST's * - * Nothing in `core/` stores it. It arrives as `InteractionContext.labelClass` on + * Nothing in `core/` stores either. The class arrives as `InteractionContext.labelClass` on * every turn, and `finishDrawing` stamps it onto the annotation the gesture * produced, which is how a drawn shape gets its class. * Moving it into `AnnotatorStore` was considered and declined: the @@ -55,12 +63,16 @@ * split `types.ts` keeps. `polyline` is not in this list, because it has a tool; * the ones left are the ones with no `Geometry` variant to carry. * - * `drawableGeometry` is exported separately so a class palette can distinguish + * `drawableGeometries` is exported separately so a class palette can distinguish * 3 and 4 from 1 and 2 and say "this class cannot be drawn here" rather than * silently behaving like select. Telling the user is a panel's job; conflating - * the four would take the information away from it. It answers `null` for 3 and + * the four would take the information away from it. It answers `[]` for 3 and * 4 alike, which is why `tags.ts` exports `isTaggableClass`: the two together - * split "tagged instead of drawn" from "not usable here at all". + * split "tagged instead of drawn" from "not usable here at all". It is also what + * a tool strip filters itself by once a class is selected. + * + * A class may accept a tag *and* a shape, so 3 is no longer exclusive with the + * rest: `classification_tag` simply contributes nothing to the drawable list. */ import { classNamed } from "../state/document"; @@ -70,23 +82,33 @@ import type { LabelClass } from "../types"; /** The four modes the canvas has. Three draw; one edits what is already there. */ export type Tool = "select" | "bbox" | "polygon" | "polyline"; +/** The tools that draw, in the order a strip filtered by one class offers them. */ +const DRAWING_TOOLS = ["bbox", "polygon", "polyline"] as const satisfies readonly Tool[]; + +type DrawingTool = (typeof DRAWING_TOOLS)[number]; + /** - * The geometry this class draws, or `null` when it draws nothing. + * The geometries of this class that can actually be drawn, possibly none. * - * `null` covers both a tag (no coordinates) and a geometry the wire declares but - * no annotation may carry. + * Empty covers a class that is only a tag (no coordinates) and one whose every + * geometry the wire declares but no annotation may carry. Order is + * `DRAWING_TOOLS`', not the class's, so two classes offering the same shapes + * offer them in the same order and the fallback below is stable. */ -export function drawableGeometry( - labelClass: LabelClass, -): "bbox" | "polygon" | "polyline" | null { - if (labelClass.geometry === "bbox") return "bbox"; - if (labelClass.geometry === "polygon") return "polygon"; - if (labelClass.geometry === "polyline") return "polyline"; - return null; +export function drawableGeometries(labelClass: LabelClass): readonly DrawingTool[] { + return DRAWING_TOOLS.filter((tool) => labelClass.geometries.includes(tool)); } /** - * The tool the active class implies. `select` for all four causes above. + * The tool the active class permits, preferring the one the host already holds. + * + * `select` for all four causes above. Otherwise `preferred` when the class + * accepts it, and the class's first drawable geometry when it does not — which + * is what stops a class switch from stranding a tool the new class forbids. + * + * `preferred` is consulted only when it draws: `select` is expressed by having + * no active class (which is what `v` does), so honouring it here would make two + * spellings of one state and leave a class armed that nothing could draw with. * * Takes the document rather than a `LabelClass` so a caller holding only the * name — which is what a palette selection is — does not have to resolve it @@ -95,9 +117,12 @@ export function drawableGeometry( export function toolFor( document: AnnotationDocument, activeClass: string | null, + preferred: Tool | null = null, ): Tool { if (activeClass === null) return "select"; const declared = classNamed(document, activeClass); if (declared === undefined) return "select"; - return drawableGeometry(declared) ?? "select"; + const drawable = drawableGeometries(declared); + if (drawable.length === 0) return "select"; + return drawable.find((tool) => tool === preferred) ?? drawable[0]; } diff --git a/frontend/annotator/src/core/state/_sample.ts b/frontend/annotator/src/core/state/_sample.ts index 1c604b94..c51e9b7e 100644 --- a/frontend/annotator/src/core/state/_sample.ts +++ b/frontend/annotator/src/core/state/_sample.ts @@ -22,12 +22,12 @@ export const SCHEMA: AnnotationSchema = { project_id: "project-1", version: 1, classes: [ - { name: "sign", geometry: "bbox", color: "#ff0000", attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, + { name: "sign", geometries: ["bbox"], color: "#ff0000", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, // The path tool needs a class to draw with, and the whole point of `toolFor` is // that a class is the only way to pick one. Declared last so the two before it // keep their positions — a class palette's hotkeys are its rows in order. - { name: "path", geometry: "polyline", color: null, attributes: [] }, + { name: "path", geometries: ["polyline"], color: null, attributes: [] }, ], description: null, created_at: null, diff --git a/frontend/annotator/src/core/state/document.test.ts b/frontend/annotator/src/core/state/document.test.ts index 53f632c7..1d1299c8 100644 --- a/frontend/annotator/src/core/state/document.test.ts +++ b/frontend/annotator/src/core/state/document.test.ts @@ -30,8 +30,8 @@ const SCHEMA: AnnotationSchema = { project_id: "project-1", version: 3, classes: [ - { name: "sign", geometry: "bbox", color: "#ff0000", attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, + { name: "sign", geometries: ["bbox"], color: "#ff0000", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, ], description: null, created_at: null, @@ -170,7 +170,7 @@ describe("immutability, which is what makes undo a pointer swap", () => { describe("the schema, which is looked up and not enforced", () => { it("finds a class by its exact name", () => { const document = documentOf("a"); - expect(classNamed(document, "sign")?.geometry).toBe("bbox"); + expect(classNamed(document, "sign")?.geometries).toEqual(["bbox"]); expect(classNamed(document, "lane")?.color).toBeNull(); }); @@ -235,7 +235,7 @@ describe("built from the wire, and back again", () => { // could not load its own round-trip fixture. This is that argument, executed. const document = documentFromWire(wire); const disagreeing = annotationsInDrawOrder(document).filter( - (a) => classNamed(document, a.label_class)?.geometry !== a.geometry.type, + (a) => !classNamed(document, a.label_class)?.geometries.includes(a.geometry.type), ); expect(disagreeing.length).toBeGreaterThan(0); }); diff --git a/frontend/annotator/src/core/types.ts b/frontend/annotator/src/core/types.ts index 65575d7b..50f1cc55 100644 --- a/frontend/annotator/src/core/types.ts +++ b/frontend/annotator/src/core/types.ts @@ -206,21 +206,22 @@ export interface Attribute { } /** - * One labelable class, bound to **one** geometry. Mirrors `LabelClassBody`. + * One labelable class and the geometries it accepts. Mirrors `LabelClassBody`. * - * `geometry` is singular, and that is the rule an annotator is built around: - * picking a class picks a tool. `color` is the kernel's own field — a renderer - * choosing its own palette when it is null is a rendering decision, not a - * document one. + * `geometries` is a **set**, non-empty, and the wire sends it sorted. Picking a + * class therefore no longer picks a tool: it constrains which tools are on + * offer, and `toolFor` resolves the rest against what the host has active. + * `color` is the kernel's own field — a renderer choosing its own palette when + * it is null is a rendering decision, not a document one. * - * `geometry` is a `GeometryType`, all eight, not just the carryable four. A + * The members are `GeometryType`, all eight, not just the carryable four. A * schema may legally declare `mask`; an annotation may not carry one. Keeping the * wide type here is what lets a class list load intact and the refusal happen * where a user can be told about it. */ export interface LabelClass { readonly name: string; - readonly geometry: GeometryType; + readonly geometries: readonly GeometryType[]; readonly color: string | null; readonly attributes: readonly Attribute[]; } diff --git a/frontend/annotator/src/core/wire.test.ts b/frontend/annotator/src/core/wire.test.ts index f2661aaa..331dacf4 100644 --- a/frontend/annotator/src/core/wire.test.ts +++ b/frontend/annotator/src/core/wire.test.ts @@ -177,7 +177,7 @@ describe("rule 4: an input-only mirror survives a server that grew a field", () const schema = parseSchema({ project_id: "p", version: 3, - classes: [{ name: "sign", geometry: "bbox" }], + classes: [{ name: "sign", geometries: ["bbox"] }], description: "why", created_at: "2026-08-02T12:00:00Z", published_by: "someone in a later release", @@ -189,7 +189,7 @@ describe("rule 4: an input-only mirror survives a server that grew a field", () it("parses a label class and an attribute carrying one too", () => { const labelClass = parseLabelClass({ name: "sign", - geometry: "bbox", + geometries: ["bbox"], shortcut_key: "s", }); expect(labelClass.name).toBe("sign"); @@ -215,7 +215,7 @@ describe("rule 4: an input-only mirror survives a server that grew a field", () // server failing to send something the parser reads, which no amount of // version skew excuses. expect(() => parseSchema({ version: 3, classes: [] })).toThrow(/missing project_id/); - expect(() => parseLabelClass({ name: "sign" })).toThrow(/missing geometry/); + expect(() => parseLabelClass({ name: "sign" })).toThrow(/missing geometries/); }); it("does not extend that tolerance to the annotation path", () => { @@ -260,7 +260,7 @@ describe("the attribute vocabulary", () => { describe("parsing the schema the kernel produced", () => { it("parses it, and finds a class per carryable geometry", () => { const schema = parseSchema(fixture.schema); - expect(schema.classes.map((c) => c.geometry).sort()).toEqual([ + expect([...new Set(schema.classes.flatMap((c) => c.geometries))].sort()).toEqual([ ...fixture.implemented_geometry_types, ]); }); @@ -328,10 +328,10 @@ describe("parsing the schema the kernel produced", () => { it("applies the wire's own defaults when an optional key is absent", () => { // Rule 4: the schema is input-only, so absence is legal here where it is not // for an annotation. A host assembling a class by hand writes two fields. - const parsed = parseLabelClass({ name: "sign", geometry: "bbox" }); + const parsed = parseLabelClass({ name: "sign", geometries: ["bbox"] }); expect(parsed).toEqual({ name: "sign", - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], }); @@ -343,7 +343,7 @@ describe("parsing the schema the kernel produced", () => { // the class comes back with the default colour instead of an error. That is // the price of surviving a server one version ahead, and `types.ts` is what // catches the typo for anyone compiling against this package. - const parsed = parseLabelClass({ name: "sign", geometry: "bbox", colour: "#fff" }); + const parsed = parseLabelClass({ name: "sign", geometries: ["bbox"], colour: "#fff" }); expect(parsed.name).toBe("sign"); expect(parsed).not.toHaveProperty("colour"); }); @@ -351,21 +351,34 @@ describe("parsing the schema the kernel produced", () => { it("accepts a class declaring a geometry no annotation can carry", () => { // Eight names, three models. A schema may legally say `polyline`; refusing it // here would make one such class cost the whole class list. - const parsed = parseLabelClass({ name: "lane", geometry: "polyline" }); - expect(parsed.geometry).toBe("polyline"); + const parsed = parseLabelClass({ name: "lane", geometries: ["polyline"] }); + expect(parsed.geometries).toEqual(["polyline"]); }); it("refuses a geometry that is not in the vocabulary at all", () => { - expect(() => parseLabelClass({ name: "lane", geometry: "squiggle" })).toThrow( + expect(() => parseLabelClass({ name: "lane", geometries: ["squiggle"] })).toThrow( /not a GeometryType/, ); + // Every member is checked, not just the first — a loop that returned after + // one would let the bad half of a mixed class through. + expect(() => parseLabelClass({ name: "lane", geometries: ["bbox", "squiggle"] })).toThrow( + /not a GeometryType/, + ); + }); + + it("refuses a class that accepts nothing", () => { + // The kernel cannot write one, and every reader downstream — `toolFor` + // included — assumes a class has at least one shape. + expect(() => parseLabelClass({ name: "lane", geometries: [] })).toThrow( + /declares no geometries/, + ); }); it("refuses an attribute kind the domain does not have", () => { expect(() => parseLabelClass({ name: "sign", - geometry: "bbox", + geometries: ["bbox"], attributes: [{ name: "note", kind: "text" }], }), ).toThrow(/kind "text" is not one of/); diff --git a/frontend/annotator/src/core/wire.ts b/frontend/annotator/src/core/wire.ts index 787f6056..821bb0be 100644 --- a/frontend/annotator/src/core/wire.ts +++ b/frontend/annotator/src/core/wire.ts @@ -148,7 +148,7 @@ export const ANNOTATION_UPDATE_KEYS = Object.keys( // same way, and a second list would only be somewhere for the two to drift. // What each parser actually reads is `types.ts`, which is the mirror. const ATTRIBUTE_REQUIRED_KEYS = ["name", "kind"] as const; -const LABEL_CLASS_REQUIRED_KEYS = ["name", "geometry"] as const; +const LABEL_CLASS_REQUIRED_KEYS = ["name", "geometries"] as const; const SCHEMA_REQUIRED_KEYS = ["project_id", "version", "classes"] as const; // A projection: it names the three fields it wants of the eleven an asset // carries. Rule 4 is what makes that unremarkable rather than a special case. @@ -427,10 +427,14 @@ export function parseAttribute(value: unknown): Attribute { /** * One labelable class. * - * `geometry` is validated against the **eight**, not the four: declaring `mask` - * is legal in a schema and refused at the annotation. Narrowing here would make a - * whole class list unloadable because of one class nobody was going to draw - * with. + * Each member of `geometries` is validated against the **eight**, not the four: + * declaring `mask` is legal in a schema and refused at the annotation. Narrowing + * here would make a whole class list unloadable because of one class nobody was + * going to draw with. + * + * An empty list is refused. The kernel cannot produce one, so a class carrying + * one is a document this does not understand — and every reader downstream + * assumes a class has at least one shape, `toolFor` included. */ export function parseLabelClass(value: unknown): LabelClass { if (!isRecord(value)) { @@ -439,12 +443,17 @@ export function parseLabelClass(value: unknown): LabelClass { allowUndeclaredKeys(value, LABEL_CLASS_REQUIRED_KEYS, "label class"); const name = requireString(value["name"], "label class name"); - const geometry = requireString(value["geometry"], `class ${name} geometry`); - if (!(GEOMETRY_TYPES as readonly string[]).includes(geometry)) { - throw new WireFormatError( - `class ${name} declares geometry ${JSON.stringify(geometry)}, which is not a GeometryType — ` + - `expected one of ${GEOMETRY_TYPES.join(", ")}`, - ); + const geometries = requireStringArray(value["geometries"], `class ${name} geometries`); + if (geometries.length === 0) { + throw new WireFormatError(`class ${name} declares no geometries; a class accepts at least one`); + } + for (const geometry of geometries) { + if (!(GEOMETRY_TYPES as readonly string[]).includes(geometry)) { + throw new WireFormatError( + `class ${name} declares geometry ${JSON.stringify(geometry)}, which is not a GeometryType — ` + + `expected one of ${GEOMETRY_TYPES.join(", ")}`, + ); + } } const attributes = value["attributes"]; @@ -453,7 +462,7 @@ export function parseLabelClass(value: unknown): LabelClass { } return { name, - geometry: geometry as LabelClass["geometry"], + geometries: geometries as LabelClass["geometries"], color: value["color"] === undefined ? null diff --git a/frontend/annotator/src/index.ts b/frontend/annotator/src/index.ts index f80ed92b..f8f89887 100644 --- a/frontend/annotator/src/index.ts +++ b/frontend/annotator/src/index.ts @@ -130,7 +130,7 @@ export { type PointerButton, } from "./core/interaction/events"; export { NO_EFFECTS, type Effect, type EffectKind } from "./core/interaction/effects"; -export { drawableGeometry, toolFor, type Tool } from "./core/interaction/tool"; +export { drawableGeometries, toolFor, type Tool } from "./core/interaction/tool"; export { NO_TARGET, nearestInsertion, From 4a951ad7924ebb20b19528ae144f96e197fb9d6d Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 19:47:39 -0700 Subject: [PATCH 03/17] test(annotator): the tool resolution rule a geometry set made necessary cf. #584 --- .../src/core/interaction/tool.test.ts | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 frontend/annotator/src/core/interaction/tool.test.ts diff --git a/frontend/annotator/src/core/interaction/tool.test.ts b/frontend/annotator/src/core/interaction/tool.test.ts new file mode 100644 index 00000000..cd4c8980 --- /dev/null +++ b/frontend/annotator/src/core/interaction/tool.test.ts @@ -0,0 +1,116 @@ +/** + * Resolving a tool against a class that accepts a set of geometries. + * + * Before #584 a class was bound to one geometry and `toolFor` was a pure function + * of the class, so there was nothing here to test that `draft.test.ts` and the + * palette tests did not already cover between them. A set makes the class an + * insufficient answer, and the resolution rule it needs instead is the one thing + * standing between a class switch and a stranded tool. + * + * Fixtures are inline, for the reason `draft.test.ts` gives for its own: the + * schema *is* the subject, so a reader chasing a failure must be able to see what + * each class accepts without opening a second file. + */ + +import { describe, expect, it } from "vitest"; + +import { createDocument } from "../state/document"; +import type { AnnotationDocument } from "../state/document"; +import type { AnnotationSchema, AssetDescriptor, LabelClass } from "../types"; +import { drawableGeometries, toolFor } from "./tool"; + +const ASSET: AssetDescriptor = { id: "asset-584", width: 640, height: 480 }; + +function classOf(name: string, ...geometries: LabelClass["geometries"]): LabelClass { + return { name, geometries, color: null, attributes: [] }; +} + +/** A box, a box-or-polygon, a pure tag, a tag that is also drawable, and a mask. */ +const CLASSES = [ + classOf("box-only", "bbox"), + classOf("either", "bbox", "polygon"), + classOf("tag-only", "classification_tag"), + classOf("tag-and-box", "bbox", "classification_tag"), + classOf("unbuildable", "mask"), +]; + +const SCHEMA: AnnotationSchema = { + project_id: "project-584", + version: 1, + classes: CLASSES, + description: null, + created_at: null, + provenance: null, +}; + +const DOCUMENT: AnnotationDocument = createDocument(ASSET, SCHEMA); + +describe("which of a class's geometries can be drawn", () => { + it("lists only the ones with a tool behind them", () => { + expect(drawableGeometries(classOf("x", "bbox", "polygon"))).toEqual(["bbox", "polygon"]); + // `classification_tag` has no canvas gesture and `mask` has no model at all, + // so both drop out — and the class is still drawable through what is left. + expect(drawableGeometries(classOf("x", "mask", "polyline", "classification_tag"))).toEqual([ + "polyline", + ]); + }); + + it("is empty for a class that draws nothing, whichever reason", () => { + expect(drawableGeometries(classOf("x", "classification_tag"))).toEqual([]); + expect(drawableGeometries(classOf("x", "mask"))).toEqual([]); + }); + + it("answers in one order, whatever order the class was written in", () => { + // Two classes offering the same shapes must offer them in the same order, or + // `toolFor`'s fallback would depend on how somebody happened to type the set. + expect(drawableGeometries(classOf("x", "polygon", "bbox"))).toEqual( + drawableGeometries(classOf("y", "bbox", "polygon")), + ); + }); +}); + +describe("resolving the tool", () => { + it("keeps the tool the host holds when the class accepts it", () => { + expect(toolFor(DOCUMENT, "either", "polygon")).toBe("polygon"); + expect(toolFor(DOCUMENT, "either", "bbox")).toBe("bbox"); + }); + + it("falls to the class's first drawable geometry when the class forbids it", () => { + // The guarantee: switching class never strands a tool. A host holding + // `polygon` that moves to a boxes-only class draws boxes, and does not keep + // an active tool nothing on the canvas would answer. + expect(toolFor(DOCUMENT, "box-only", "polygon")).toBe("bbox"); + expect(toolFor(DOCUMENT, "box-only", "polyline")).toBe("bbox"); + }); + + it("takes the class's first drawable geometry when the host has no preference", () => { + // The behaviour before a class could accept more than one, kept as the + // default so a host with no tool strip is unaffected. + expect(toolFor(DOCUMENT, "either", null)).toBe("bbox"); + expect(toolFor(DOCUMENT, "either")).toBe("bbox"); + }); + + it("ignores a preference for select, which is spelled by having no class", () => { + // Otherwise there would be two ways to say the same thing and one of them + // would leave a class armed that nothing could draw with. + expect(toolFor(DOCUMENT, "either", "select")).toBe("bbox"); + }); + + it("answers select for a class that draws nothing, preference or not", () => { + for (const preferred of ["bbox", "polygon", null] as const) { + expect(toolFor(DOCUMENT, "tag-only", preferred)).toBe("select"); + expect(toolFor(DOCUMENT, "unbuildable", preferred)).toBe("select"); + } + }); + + it("draws with a class that is both taggable and drawable", () => { + // The two stopped being each other's negation, so a class accepting a tag + // *and* a box is not a tag class: it has a tool, and the tag is a panel's. + expect(toolFor(DOCUMENT, "tag-and-box", null)).toBe("bbox"); + }); + + it("answers select with no class and with a class the schema never declared", () => { + expect(toolFor(DOCUMENT, null, "polygon")).toBe("select"); + expect(toolFor(DOCUMENT, "ghost", "polygon")).toBe("select"); + }); +}); From 6a2b1894923a7c14c21af94c1b989c642c0ef846 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 19:49:51 -0700 Subject: [PATCH 04/17] test(annotator): a class that is both taggable and drawable arms rather than tags The one rule the mutation battery found nothing watching: no class could be both before #584, so `classAction`'s two tests had never been ordered against each other. `PALETTE` gains a seventh row that is a tag and a box at once. cf. #584 --- frontend/annotator/src/core/input/_palette.ts | 16 +++++++++++++++- .../annotator/src/core/input/bindings.test.ts | 15 ++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/frontend/annotator/src/core/input/_palette.ts b/frontend/annotator/src/core/input/_palette.ts index 8f786bfc..a34a5d8d 100644 --- a/frontend/annotator/src/core/input/_palette.ts +++ b/frontend/annotator/src/core/input/_palette.ts @@ -108,7 +108,21 @@ export const POSE: LabelClass = { attributes: [], }; -export const PALETTE: readonly LabelClass[] = [SIGN, LANE, WEATHER, RAIL, STOP, POSE]; +/** + * Digit 7 — a tag *and* a box, which #584 made expressible. + * + * The two questions stopped being each other's negation, so this is the class + * that tells `isTaggableClass` apart from "not drawable": a digit pressed on it + * must arm it, not tag the asset. Appended, for `POSE`'s reason. + */ +export const KIOSK: LabelClass = { + name: "kiosk", + geometries: ["bbox", "classification_tag"], + color: null, + attributes: [], +}; + +export const PALETTE: readonly LabelClass[] = [SIGN, LANE, WEATHER, RAIL, STOP, POSE, KIOSK]; export const PALETTE_SCHEMA: AnnotationSchema = { project_id: "project-46", diff --git a/frontend/annotator/src/core/input/bindings.test.ts b/frontend/annotator/src/core/input/bindings.test.ts index 86fbe56c..e9ff67e9 100644 --- a/frontend/annotator/src/core/input/bindings.test.ts +++ b/frontend/annotator/src/core/input/bindings.test.ts @@ -217,6 +217,17 @@ describe("classAction", () => { }); }); + it("arms a class that accepts a tag and a shape, rather than tagging with it", () => { + // The two are no longer exclusive, so the order of the tests inside + // `classAction` is the rule: taggable *and* drawable arms. A toggle changes + // no tool, so folding this into `toggle-tag` would silently tag the asset + // where somebody pressing a class digit on a canvas meant to arm the class. + expect(classAction(PALETTE_SCHEMA, "kiosk")).toEqual({ + kind: "activate-class", + labelClass: "kiosk", + }); + }); + it("refuses a class the schema does not declare", () => { expect(classAction(PALETTE_SCHEMA, "unicorn")).toBeNull(); expect(classAction(EMPTY_SCHEMA, "sign")).toBeNull(); @@ -234,12 +245,14 @@ describe("classHotkeys", () => { // A class no annotation can carry still gets its digit: arming it is legal, // and the palette is where "this cannot be drawn here" is said (`tool.ts`). { chord: "6", action: { kind: "activate-class", labelClass: "pose" } }, + // Taggable and drawable at once: it arms, because it has a tool. + { chord: "7", action: { kind: "activate-class", labelClass: "kiosk" } }, ]); }); it("does not filter, so a tag class occupies its own row rather than shifting the rest", () => { const bound = classHotkeys(PALETTE_SCHEMA); - expect(bound.map((binding) => binding.chord)).toEqual(["1", "2", "3", "4", "5", "6"]); + expect(bound.map((binding) => binding.chord)).toEqual(["1", "2", "3", "4", "5", "6", "7"]); expect(bound).toHaveLength(PALETTE.length); }); From cc4824acff21d86f4f2e082aec9dbef4cf8294f3 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 20:05:04 -0700 Subject: [PATCH 05/17] feat(ui): a class's geometries are a checkbox group, and a name that exists is an offer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema editor's single-select becomes a checkbox group under the same category headings, using the native input this form already uses for an attribute's `required` flag — no new dependency, no new primitive, and what a class accepts is readable without opening anything. The last ticked box does not come off and carries why. **A defect the new test caught, worth stating:** the first draft refused that last box with `preventDefault()` on the input's click. React synthesises a checkbox's `onChange` from the same native click, so cancelling the click does not cancel the change — the class went to an empty set while the tick stayed on screen, a control lying about what it had just done. The refusal now lives where the value is computed, which cannot come apart. **The rescue flow.** A name the published version already declares stops being a red box and becomes an offer: the alert says what the class accepts today and what publishing would add, and the primary reads `Add polygon to sign`. It carries the **existing** class's colour and attributes, so a form opened to make a new class cannot quietly wipe what the old one declared. The refusal that remains is a name typed twice in one sitting, which has nothing to offer because both entries are being written now. `composeVersion` replaces a same-named class **in place** rather than appending: two classes with one name is what `create_version` refuses outright, and appending would also renumber the digit hotkeys, which are positions in the authored order. The tool strip narrows to the held class's own geometries, and the page holds the preferred tool beside the drawing class at job scope, for the query-key reason the drawing class is already there. cf. #584 --- .../ui-core/src/annotator/AddClassDialog.tsx | 142 ++++++++++++++---- .../ui-core/src/annotator/AnnotationPage.tsx | 39 ++++- .../ui-core/src/annotator/ClassRegion.tsx | 3 +- .../ui-core/src/annotator/ReassignMenu.tsx | 13 +- .../ui-core/src/annotator/ToolPalette.tsx | 122 +++++++++++---- .../ui-core/src/annotator/addClass.test.ts | 46 +++++- .../src/annotator/addClassDialog.test.tsx | 92 ++++++++++-- .../src/annotator/addClassProvenance.test.tsx | 2 +- .../src/annotator/canvasLabel.test.tsx | 4 +- .../src/annotator/canvasReassign.test.tsx | 10 +- .../src/annotator/classRegion.test.tsx | 2 +- .../src/annotator/drawingClass.test.tsx | 4 +- .../src/annotator/editorNotice.test.tsx | 2 +- .../src/annotator/frameGallery.test.tsx | 2 +- .../ui-core/src/annotator/jobQueries.test.ts | 4 +- frontend/ui-core/src/annotator/panel.test.tsx | 18 ++- .../ui-core/src/annotator/pinBadge.test.tsx | 2 +- .../src/annotator/suggestFlow.test.tsx | 6 +- .../src/annotator/toolPalette.test.tsx | 20 +-- .../ui-core/src/annotator/topBar.test.tsx | 21 ++- frontend/ui-core/src/data/geometryCategory.ts | 21 +++ frontend/ui-core/src/palette.test.ts | 6 +- frontend/ui-core/src/patterns/ClassFields.tsx | 128 +++++++++++----- .../ui-core/src/screens/OverviewPanel.tsx | 2 +- .../ui-core/src/screens/ProjectScreen.tsx | 7 +- frontend/ui-core/src/screens/SchemaEditor.tsx | 7 +- .../ui-core/src/screens/schemaDraft.test.tsx | 8 +- frontend/ui-core/src/screens/screens.test.tsx | 61 +++++--- 28 files changed, 599 insertions(+), 195 deletions(-) diff --git a/frontend/ui-core/src/annotator/AddClassDialog.tsx b/frontend/ui-core/src/annotator/AddClassDialog.tsx index 3e548439..7b848138 100644 --- a/frontend/ui-core/src/annotator/AddClassDialog.tsx +++ b/frontend/ui-core/src/annotator/AddClassDialog.tsx @@ -84,12 +84,13 @@ import { DialogTitle, } from "../primitives/Dialog"; import { Input, Label } from "../primitives/Input"; +import { formatGeometries } from "../data/geometryCategory"; import { ClassFields } from "../patterns/ClassFields"; import type { LabelClassBody, SchemaVersion } from "../screens/queries"; /** A fresh class, in the shape the wire takes. */ function blank(): LabelClassBody { - return { name: "", geometry: "bbox", color: null, attributes: [] }; + return { name: "", geometries: ["bbox"], color: null, attributes: [] }; } /** @@ -175,7 +176,41 @@ export async function runAddClass(steps: { readonly note: string; }): Promise { await steps.save(); - await steps.publish([...steps.activeClasses, ...steps.added], steps.note); + await steps.publish(composeVersion(steps.activeClasses, steps.added), steps.note); +} + +/** Case-insensitively, because that is how `create_version` compares class names. */ +function sameName(one: string, other: string): boolean { + return one.toLowerCase() === other.toLowerCase(); +} + +/** + * The whole contract the next version declares: the active classes, with this + * sitting's written **into** them. + * + * A name already in the active version **replaces its entry in place** rather + * than being appended, and both halves of that matter. Appending would publish + * two classes with one name, which `create_version` refuses outright — so the + * rescue flow (widening an existing class rather than making a second one) + * would fail at the API with a 422 that named nothing the user did. And + * replacing *in place* rather than at the end keeps the authored class order, + * which is not cosmetic: the class list renders in it and the digit hotkeys are + * positions in it, so appending would silently renumber somebody's keyboard. + * + * Exported for its own test: it is the one piece of this chain that composes + * rather than sequences, and the order it preserves is invisible from outside. + */ +export function composeVersion( + activeClasses: readonly LabelClassBody[], + added: readonly LabelClassBody[], +): readonly LabelClassBody[] { + const updated = activeClasses.map( + (existing) => added.find((one) => sameName(one.name, existing.name)) ?? existing, + ); + const fresh = added.filter( + (one) => !activeClasses.some((existing) => sameName(existing.name, one.name)), + ); + return [...updated, ...fresh]; } export interface AddClassDialogProps { @@ -240,16 +275,43 @@ export function AddClassDialog({ const name = declared.name.trim(); const failure = error === null || error === undefined ? null : asApiError(error); - // Case-insensitively, because `create_version` refuses a collision that way — - // mirroring the API's rule rather than inventing a second one. The session is - // checked alongside the active version, because two classes in one press go - // into one contract and `create_version` judges that contract as a whole: a - // collision inside the session is refused by exactly the same rule, and - // discovering it from a 409 after the save would be the worst place to learn it. - const collides = (candidate: string): boolean => - (active?.classes.some((entry) => entry.name.toLowerCase() === candidate.toLowerCase()) ?? - false) || session.some((entry) => entry.name.toLowerCase() === candidate.toLowerCase()); - const taken = name !== "" && collides(name); + + // Case-insensitively throughout, because `create_version` compares names that + // way — mirroring the API's rule rather than inventing a second one. Checked + // here rather than learned from the 422 afterwards, because the dialog needs + // the answer *before* the press to know what the press will do. + // + // **The two collisions are different questions and only one of them is a + // refusal.** A name already in the published version is a class that exists, + // and wanting to draw it as another shape is a thing somebody legitimately + // wants — so it becomes an offer to widen that class. A name typed twice in + // this sitting is a mistake with nothing to offer: both entries are being + // written now, and merging them would be guessing which of the two the user + // meant. + /** The published class this name lands on, if there is one. */ + const existing = + name === "" ? undefined : active?.classes.find((entry) => sameName(entry.name, name)); + const inSession = name !== "" && session.some((entry) => sameName(entry.name, name)); + /** What this form would add to that class. Empty when it asks for nothing new. */ + const widening = + existing === undefined + ? [] + : declared.geometries.filter((geometry) => !existing.geometries.includes(geometry)); + const taken = inSession || (existing !== undefined && widening.length === 0); + + /** + * What this form publishes: a new class, or the existing one widened. + * + * The widening carries the **existing** class's colour and attributes, not the + * form's. This is an update to a class that already has both, and the form was + * opened to make a *new* one — so publishing its blank colour and empty + * attribute list would quietly wipe what the class already declared, which is + * not what "add polygon to it" says. Only the geometries move. + */ + const formEntry: LabelClassBody = + existing === undefined + ? { ...declared, name } + : { ...existing, geometries: [...existing.geometries, ...widening] }; /** * What pressing the primary publishes: the session, plus whatever is in the @@ -261,9 +323,7 @@ export function AddClassDialog({ * the implementation has two places to look. */ const readyForm = name !== "" && !taken; - const publishing: readonly LabelClassBody[] = readyForm - ? [...session, { ...declared, name }] - : session; + const publishing: readonly LabelClassBody[] = readyForm ? [...session, formEntry] : session; const description = touched ? note : defaultNote(publishing.map((entry) => entry.name)); function reset(): void { @@ -374,7 +434,9 @@ export function AddClassDialog({ }} /> {entry.name} - {entry.geometry} + + {formatGeometries(entry.geometries)} + )} diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index f652eba9..7f6d2bc8 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -125,6 +125,7 @@ import { type Polarity, type Suggestion, type SuggestionState, + type Tool, type Viewport, } from "@visionset/annotator"; import { AnnotatorStore as Store } from "@visionset/annotator"; @@ -543,6 +544,22 @@ function JobScreen({ */ const [activeClass, setActiveClass] = useState(null); + /** + * Which of the held class's shapes to draw, when it accepts more than one. + * + * Beside `activeClass` and at the same scope, deliberately: they are one + * decision read two ways, and a preference kept at a scope those query keys + * could move would be lost by a mutation with nothing on screen to say so — + * the `ui-capabilities` rule the drawing class already lives under. + * + * A *preference*, never the answer. `toolFor` resolves it against what the + * class actually accepts and falls back when it cannot be honoured, so this + * being stale is harmless and an active tool the class forbids is + * unrepresentable. `null` means no preference, which is the state a schema of + * one-shape classes never leaves. + */ + const [activeTool, setActiveTool] = useState(null); + /** * The suggest tool's vertex density, held here for `activeClass`'s reason. * @@ -635,9 +652,11 @@ function JobScreen({ counts={progress.data ?? null} clipboard={clipboard} activeClass={activeClass} + activeTool={activeTool} detail={detail} onDetail={setDetail} onActivateClass={activateClass} + onActivateTool={setActiveTool} onNavigate={setChosen} {...(onConfigureInference === undefined ? {} : { onConfigureInference })} {...(onOpenGallery === undefined @@ -714,7 +733,10 @@ interface WorkspaceProps { readonly onDetail: (detail: Detail) => void; /** Also `JobScreen`'s, and for a sharper reason — see the note where it is declared. */ readonly activeClass: string | null; + /** `JobScreen`'s too, and at that scope for the same reason the class is. */ + readonly activeTool: Tool | null; readonly onActivateClass: (labelClass: string | null) => void; + readonly onActivateTool: (tool: Tool | null) => void; readonly onNavigate: (index: number) => void; readonly onOpenGallery?: () => void; /** Where to set up a model connection, if the host has such a screen. */ @@ -761,7 +783,9 @@ function Workspace({ detail, onDetail: setDetail, activeClass, + activeTool, onActivateClass: armClass, + onActivateTool, onNavigate, onOpenGallery, onConfigureInference, @@ -2425,6 +2449,7 @@ function Workspace({ // greyed-out toolbar does not stop a drag from drawing a box. readOnly={readOnly} activeClass={readOnly ? null : activeClass} + activeTool={activeTool} onActivateClass={activateClass} onViewChange={setView} hiddenIds={hiddenIds} @@ -2480,10 +2505,12 @@ function Workspace({ would mean the engine shipping chrome, and putting it outside the stage would mean it was not floating over the picture. - `toolFor` is read here rather than held: the tool is derived from the - active class and never stored (`core/interaction/tool.ts`), and a second - copy on this page would be the pair v1 spent two mechanisms keeping in - step. + `toolFor` is read here rather than held: the tool is *resolved* from + the active class and the preference beside it and never stored + (`core/interaction/tool.ts`), and a second copy of the answer on this + page would be the pair v1 spent two mechanisms keeping in step. What + this page does hold is the preference, which is an input to that + function rather than a second copy of its output. */} {/* A viewer gets the strip, carrying the hand and the shortcut sheet and @@ -2500,8 +2527,10 @@ function Workspace({ readOnly={readOnly} hand={{ active: handTool, onToggle: () => setHandTool((on) => !on) }} schema={store.document.schema} - tool={toolFor(store.document, activeClass)} + tool={toolFor(store.document, activeClass, activeTool)} + activeClass={activeClass} onActivateClass={activateClass} + onActivateTool={onActivateTool} onToggleHelp={() => setHelpOpen((open) => !open)} // Empty, unlike the class field's create row: `+` means "I want a // class", not a particular one, and carrying the previous diff --git a/frontend/ui-core/src/annotator/ClassRegion.tsx b/frontend/ui-core/src/annotator/ClassRegion.tsx index d1c28d2d..abf29cfb 100644 --- a/frontend/ui-core/src/annotator/ClassRegion.tsx +++ b/frontend/ui-core/src/annotator/ClassRegion.tsx @@ -44,6 +44,7 @@ import { hotkeyForClass, type AnnotationSchema, type LabelClass } from "@visions import { Plus } from "lucide-react"; import { useState, type JSX, type RefObject } from "react"; +import { formatGeometries } from "../data/geometryCategory"; import { classColor } from "../palette"; import { Button } from "../primitives/Button"; import { Input } from "../primitives/Input"; @@ -246,7 +247,7 @@ function ClassRow({ ): void { @@ -147,7 +152,11 @@ export function ReassignMenu({ ) ) : ( - needs a {declared.geometry} + {/* Named in full rather than "does not take a {geometry}": the + question somebody has is what this class *does* take, and a + refusal that only repeats what they already selected answers + nothing. */} + needs {formatGeometries(declared.geometries)} )} diff --git a/frontend/ui-core/src/annotator/ToolPalette.tsx b/frontend/ui-core/src/annotator/ToolPalette.tsx index 3e817cd6..ea97c9cb 100644 --- a/frontend/ui-core/src/annotator/ToolPalette.tsx +++ b/frontend/ui-core/src/annotator/ToolPalette.tsx @@ -85,7 +85,7 @@ */ import { - drawableGeometry, + drawableGeometries, hotkeyForClass, schemaCanSuggest, type AnnotationSchema, @@ -117,7 +117,7 @@ import { Tooltip, TooltipContent, TooltipTrigger } from "../primitives/Menu"; * that is a property worth more than the twelve lines it costs. * * The record is typed on `string` rather than on the geometry union so an entry - * can name a geometry `drawableGeometry` has never heard of, which is the case it + * can name a geometry `drawableGeometries` never returns, which is the case it * exists for. */ const PENDING_TOOLS: Readonly> = {}; @@ -125,7 +125,7 @@ const PENDING_TOOLS: Readonly> = {}; /** * What each drawing tool is called on the strip. * - * Total over what `drawableGeometry` can answer, so a fourth geometry gaining a + * Total over what `drawableGeometries` can answer, so a fourth geometry gaining a * tool cannot reach the strip unnamed — which is what the ternary this replaced * would have let it do, silently reading "Polygon". */ @@ -160,47 +160,99 @@ interface ToolChoice { * A geometry is represented by the **first** class declaring it, in authored * order, which is the same order `classHotkeys` binds the digit row in. Nothing * here dedupes by class: two bbox classes are one bbox tool. + * + * **`activeClass` narrows it.** With a class selected the strip offers only that + * class's own geometries, because those are the only shapes a gesture could + * produce — a bbox button that armed a different class the moment it was pressed + * would answer "what can I draw here?" with something about somewhere else. With + * none selected it is the union, which is what the strip has always shown and is + * still the right answer to "what does this project label?". + * + * A class is only narrowed *to* when it can be drawn: selecting a pure tag class + * leaves the full union rather than emptying the strip, since the tag lives in a + * panel and the strip would otherwise vanish for a reason nothing on it explains. */ -export function toolChoices(schema: AnnotationSchema): readonly ToolChoice[] { +export function toolChoices( + schema: AnnotationSchema, + activeClass: string | null = null, +): readonly ToolChoice[] { + const selected = schema.classes.find((declared) => declared.name === activeClass); + const narrowed = + selected !== undefined && drawableGeometries(selected).length > 0 ? selected : undefined; + const offered = narrowed === undefined ? schema.classes : [narrowed]; + const choices: ToolChoice[] = [ { tool: "select", label: "Select", labelClass: null, hotkey: "V", unavailable: null }, ]; - for (const declared of schema.classes) { - const geometry = drawableGeometry(declared); - if (geometry === null) continue; - if (choices.some((choice) => choice.tool === geometry)) continue; - choices.push({ - tool: geometry, - label: TOOL_LABELS[geometry], - labelClass: declared.name, - hotkey: hotkeyForClass(schema, declared.name) ?? "—", - unavailable: null, - }); + for (const declared of offered) { + for (const geometry of drawableGeometries(declared)) { + if (choices.some((choice) => choice.tool === geometry)) continue; + choices.push({ + tool: geometry, + label: TOOL_LABELS[geometry], + labelClass: declared.name, + hotkey: hotkeyForClass(schema, declared.name) ?? "—", + unavailable: null, + }); + } } // After the usable tools, never interleaved: the strip reads top to bottom as // "what you can do", and a disabled control in the middle of that list reads as // a broken one rather than as a coming one. + // + // Read off `schema.classes` rather than `offered`, deliberately: a geometry with + // no tool is a fact about the *project*, and hiding it while a class is selected + // would make the explanation come and go with the selection. for (const declared of schema.classes) { - const pending = PENDING_TOOLS[declared.geometry]; - if (pending === undefined) continue; - if (choices.some((choice) => choice.tool === declared.geometry)) continue; - choices.push({ - tool: declared.geometry, - label: declared.geometry, - // No class to activate, because there is no tool to activate it for. - labelClass: null, - hotkey: "—", - unavailable: pending, - }); + for (const geometry of declared.geometries) { + const pending = PENDING_TOOLS[geometry]; + if (pending === undefined) continue; + if (choices.some((choice) => choice.tool === geometry)) continue; + choices.push({ + tool: geometry, + label: geometry, + // No class to activate, because there is no tool to activate it for. + labelClass: null, + hotkey: "—", + unavailable: pending, + }); + } } return choices; } +/** Whether the held class can produce this shape. `false` when it holds none. */ +function accepts( + schema: AnnotationSchema, + activeClass: string | null, + tool: ToolChoice["tool"], +): boolean { + const declared = schema.classes.find((one) => one.name === activeClass); + return declared !== undefined && drawableGeometries(declared).some((one) => one === tool); +} + export interface ToolPaletteProps { readonly schema: AnnotationSchema; /** What `toolFor` currently answers. Reported, never stored here. */ readonly tool: Tool; + /** + * The class the strip is narrowed to, or `null` for the schema's whole union. + * + * The strip answers *what can I draw here*, and once a class accepts a set of + * geometries the honest answer depends on which class is held. + */ + readonly activeClass: string | null; readonly onActivateClass: (labelClass: string | null) => void; + /** + * Prefer this shape, among the ones the held class accepts. + * + * Separate from `onActivateClass` because pressing a tool now means two + * different things depending on the class: within a class that accepts the + * shape it is only a change of shape, and the class must **not** move — a strip + * that re-armed the geometry's first declaring class would silently retarget + * somebody's labels to a different class than the one they had selected. + */ + readonly onActivateTool: (tool: Tool | null) => void; readonly onToggleHelp: () => void; /** * The suggest tool, or absent where the host cannot serve one. @@ -307,7 +359,9 @@ export interface ToolPaletteProps { export function ToolPalette({ schema, tool, + activeClass, onActivateClass, + onActivateTool, onToggleHelp, onAddClass, history, @@ -334,7 +388,7 @@ export function ToolPalette({ className="absolute left-3 top-3 flex w-12 flex-col items-center gap-1 rounded-xl border border-border bg-muted p-2 shadow-lg" > {!readOnly && - toolChoices(schema).map((choice) => ( + toolChoices(schema, activeClass).map((choice) => ( { if (choice.unavailable !== null) return; - if (tool !== choice.tool) onActivateClass(choice.labelClass); - else if (hand.active) hand.onToggle(); + if (tool === choice.tool) { + if (hand.active) hand.onToggle(); + return; + } + // The shape always. The class only when the one being held cannot + // produce that shape — otherwise this is a change of tool inside + // one class, and moving the class would be the retarget the + // `onActivateTool` docstring warns about. + onActivateTool(choice.tool === "select" ? null : (choice.tool as Tool)); + if (!accepts(schema, activeClass, choice.tool)) { + onActivateClass(choice.labelClass); + } }} > diff --git a/frontend/ui-core/src/annotator/addClass.test.ts b/frontend/ui-core/src/annotator/addClass.test.ts index d9fae7e2..436399d5 100644 --- a/frontend/ui-core/src/annotator/addClass.test.ts +++ b/frontend/ui-core/src/annotator/addClass.test.ts @@ -11,12 +11,12 @@ import { describe, expect, it, vi } from "vitest"; -import { defaultNote, runAddClass } from "./AddClassDialog"; +import { composeVersion, defaultNote, runAddClass } from "./AddClassDialog"; import type { LabelClassBody } from "../screens/queries"; -const SIGN: LabelClassBody = { name: "sign", geometry: "bbox", color: null, attributes: [] }; -const LANE: LabelClassBody = { name: "lane", geometry: "polygon", color: null, attributes: [] }; -const NEW: LabelClassBody = { name: "crossing", geometry: "bbox", color: "#eb5a47", attributes: [] }; +const SIGN: LabelClassBody = { name: "sign", geometries: ["bbox"], color: null, attributes: [] }; +const LANE: LabelClassBody = { name: "lane", geometries: ["polygon"], color: null, attributes: [] }; +const NEW: LabelClassBody = { name: "crossing", geometries: ["bbox"], color: "#eb5a47", attributes: [] }; /** Two recorders writing into one list, so the order is a single assertion. */ function recorders(overrides: Partial Promise>> = {}) { @@ -202,3 +202,41 @@ describe("what the chain is given", () => { * true: a completed batch keeps its version, and somebody publishing from inside * one should be told that before they press. */ +describe("composing the version a sitting publishes", () => { + const WIDENED: LabelClassBody = { + name: "sign", + geometries: ["bbox", "polygon"], + color: null, + attributes: [], + }; + + it("appends a class the active version does not have", () => { + expect(composeVersion([SIGN, LANE], [NEW])).toEqual([SIGN, LANE, NEW]); + }); + + it("replaces a class of the same name rather than adding a second", () => { + // Two classes with one name is what `create_version` refuses outright, so an + // append here would turn the widening flow into a 422 naming nothing the user + // did. + expect(composeVersion([SIGN, LANE], [WIDENED])).toEqual([WIDENED, LANE]); + }); + + it("replaces in place, so the authored class order does not move", () => { + // Not cosmetic: the class list renders in this order and the digit hotkeys + // are positions in it, so appending the widened class would silently + // renumber somebody's keyboard. + expect(composeVersion([SIGN, LANE], [WIDENED]).map((one) => one.name)).toEqual([ + "sign", + "lane", + ]); + }); + + it("matches the name the way the API does, ignoring case", () => { + const shouted: LabelClassBody = { ...WIDENED, name: "SIGN" }; + expect(composeVersion([SIGN, LANE], [shouted])).toEqual([shouted, LANE]); + }); + + it("does both at once, for a sitting that widens one class and adds another", () => { + expect(composeVersion([SIGN, LANE], [WIDENED, NEW])).toEqual([WIDENED, LANE, NEW]); + }); +}); diff --git a/frontend/ui-core/src/annotator/addClassDialog.test.tsx b/frontend/ui-core/src/annotator/addClassDialog.test.tsx index aec32501..915462cd 100644 --- a/frontend/ui-core/src/annotator/addClassDialog.test.tsx +++ b/frontend/ui-core/src/annotator/addClassDialog.test.tsx @@ -24,8 +24,8 @@ const ACTIVE = { project_id: "11111111-1111-4111-8111-111111111111", version: 3, classes: [ - { name: "sign", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, + { name: "sign", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, ], description: null, created_at: null, @@ -58,16 +58,59 @@ describe("what the dialog refuses before it asks", () => { expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", false); }); - it("will not submit a name the active version already declares, ignoring case", async () => { - // `create_version` refuses a collision case-insensitively, so this mirrors the - // API's rule rather than inventing a second one — and it explains beside the - // field instead of failing after a round trip. - render(mount()); + it("offers to widen a class the active version already declares, ignoring case", async () => { + // A name that exists is not a refusal. `create_version` compares names + // case-insensitively, so "SIGN" lands on "sign" — and what somebody typing it + // wants is almost always to draw that class as a shape it does not have yet. + const onSubmit = vi.fn(); + render(mount({ onSubmit })); await userEvent.type(screen.getByTestId("class-name-new"), "SIGN"); + // The form starts on `bbox`, which "sign" already accepts, so there is + // nothing to add yet and *that* is the refusal — with the remedy named. + expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", true); + expect(screen.getByText(/adds nothing to it/)).toBeTruthy(); + + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); + + const offer = screen.getByTestId("widen-offer"); + expect(offer.textContent).toContain("“sign” already exists"); + expect(offer.textContent).toContain("declares it as bbox"); + const submit = screen.getByTestId("add-class-submit"); + expect(submit).toHaveProperty("disabled", false); + // The button says what it does, rather than "Add class". + expect(submit.textContent).toContain("Add polygon to “sign”"); + }); + + it("widens the existing class rather than writing a second one", async () => { + const onSubmit = vi.fn(); + render(mount({ onSubmit })); + + await userEvent.type(screen.getByTestId("class-name-new"), "SIGN"); + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); + await userEvent.click(screen.getByTestId("add-class-submit")); + + // The **existing** class's own name and colour, widened — not the form's. The + // form was opened to make a new class, so publishing its blank colour would + // quietly wipe what "sign" already declared. + expect(onSubmit).toHaveBeenCalledWith( + [{ name: "sign", geometries: ["bbox", "polygon"], color: null, attributes: [] }], + expect.anything(), + ); + }); + + it("still refuses a name typed twice in one sitting, which has nothing to offer", async () => { + // The other collision, and the one that stays a refusal: both entries are + // being written now, so merging them would be guessing which was meant. + render(mount()); + + await userEvent.type(screen.getByTestId("class-name-new"), "crossing"); + await userEvent.click(screen.getByTestId("add-another")); + await userEvent.type(screen.getByTestId("class-name-new"), "CROSSING"); expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", true); - expect(screen.getByText(/already declares a class/)).toBeTruthy(); + expect(screen.getByText(/already added a class/)).toBeTruthy(); + expect(screen.queryByTestId("widen-offer")).toBeNull(); }); it("will not submit before the active version has loaded", async () => { @@ -97,7 +140,7 @@ describe("what it submits", () => { await userEvent.click(screen.getByTestId("add-class-submit")); expect(submit).toHaveBeenCalledWith( - [expect.objectContaining({ name: "crossing", geometry: "bbox" })], + [expect.objectContaining({ name: "crossing", geometries: ["bbox"] })], 'Added class "crossing" from the annotation view', ); }); @@ -144,12 +187,11 @@ describe("what it submits", () => { it("groups the geometries under their category, the same as the Schema tab", async () => { render(mount()); - await userEvent.click(screen.getByTestId("class-geometry-new")); - + // No press: the boxes are already on the page. See the Schema tab's twin. const basic = screen.getByTestId("geometry-category-Basic Computer Vision"); const robotics = screen.getByTestId("geometry-category-Robotics and AD"); const membersOf = (label: HTMLElement): string[] => - [...(label.parentElement?.querySelectorAll('[role="option"]') ?? [])].map( + [...(label.parentElement?.querySelectorAll("label") ?? [])].map( (option) => option.textContent ?? "", ); @@ -162,17 +204,35 @@ describe("what it submits", () => { * `toolFor` reads to decide which tool a hotkey arms, so a picker that grouped * its options and stopped writing one would break drawing rather than layout. */ - it("still writes the picked geometry onto the class it will publish", async () => { + it("writes every geometry ticked onto the class it will publish", async () => { const onSubmit = vi.fn(); render(mount({ onSubmit })); await userEvent.type(screen.getByTestId("class-name-new"), "centre-line"); - await userEvent.click(screen.getByTestId("class-geometry-new")); - await userEvent.click(screen.getByRole("option", { name: "polyline" })); + // Tick the second before clearing the first, which is also the only order + // the control allows: a class never passes through accepting nothing. + await userEvent.click(screen.getByTestId("class-geometry-new-polyline")); + await userEvent.click(screen.getByTestId("class-geometry-new-bbox")); + await userEvent.click(screen.getByTestId("add-class-submit")); + + expect(onSubmit).toHaveBeenCalledWith( + [expect.objectContaining({ name: "centre-line", geometries: ["polyline"] })], + expect.anything(), + ); + }); + + it("publishes a class accepting two shapes, which is what a set is for", async () => { + const onSubmit = vi.fn(); + render(mount({ onSubmit })); + + // A name the active version does not hold: "sign" would land on the widening + // path below, which is a different test. + await userEvent.type(screen.getByTestId("class-name-new"), "kerb"); + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); await userEvent.click(screen.getByTestId("add-class-submit")); expect(onSubmit).toHaveBeenCalledWith( - [expect.objectContaining({ name: "centre-line", geometry: "polyline" })], + [expect.objectContaining({ name: "kerb", geometries: ["bbox", "polygon"] })], expect.anything(), ); }); diff --git a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx index 46432863..a2bdcb40 100644 --- a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx +++ b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx @@ -37,7 +37,7 @@ const ASSET = "44444444-4444-4444-8444-444444444444"; const SCHEMA = { project_id: PROJECT, version: 1, - classes: [{ name: "sign", geometry: "bbox", color: null, attributes: [] }], + classes: [{ name: "sign", geometries: ["bbox"], color: null, attributes: [] }], description: null, created_at: null, provenance: "curated", diff --git a/frontend/ui-core/src/annotator/canvasLabel.test.tsx b/frontend/ui-core/src/annotator/canvasLabel.test.tsx index 2c78617f..db4c2e49 100644 --- a/frontend/ui-core/src/annotator/canvasLabel.test.tsx +++ b/frontend/ui-core/src/annotator/canvasLabel.test.tsx @@ -31,8 +31,8 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/canvasReassign.test.tsx b/frontend/ui-core/src/annotator/canvasReassign.test.tsx index 9eab027f..ec65d235 100644 --- a/frontend/ui-core/src/annotator/canvasReassign.test.tsx +++ b/frontend/ui-core/src/annotator/canvasReassign.test.tsx @@ -28,10 +28,10 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, ], }; @@ -168,7 +168,7 @@ describe("what the picker does", () => { "true", ); } - expect(screen.getByTestId("canvas-reclass-lane").textContent).toContain("needs a polygon"); + expect(screen.getByTestId("canvas-reclass-lane").textContent).toContain("needs polygon"); }); it("checks the class the shape already carries", () => { diff --git a/frontend/ui-core/src/annotator/classRegion.test.tsx b/frontend/ui-core/src/annotator/classRegion.test.tsx index 91b7094a..713662f8 100644 --- a/frontend/ui-core/src/annotator/classRegion.test.tsx +++ b/frontend/ui-core/src/annotator/classRegion.test.tsx @@ -33,7 +33,7 @@ function schemaOf(n: number): AnnotationSchema { provenance: "curated", classes: Array.from({ length: n }, (_unused, index) => ({ name: `class-${index + 1}`, - geometry: index % 2 === 0 ? "bbox" : "polygon", + geometries: index % 2 === 0 ? ["bbox"] : ["polygon"], color: null, attributes: [], })), diff --git a/frontend/ui-core/src/annotator/drawingClass.test.tsx b/frontend/ui-core/src/annotator/drawingClass.test.tsx index f08e9264..68cc5ea5 100644 --- a/frontend/ui-core/src/annotator/drawingClass.test.tsx +++ b/frontend/ui-core/src/annotator/drawingClass.test.tsx @@ -39,8 +39,8 @@ const SCHEMA = { project_id: PROJECT, version: 1, classes: [ - { name: "sign", geometry: "bbox", color: null, attributes: [] }, - { name: "vehicle", geometry: "bbox", color: null, attributes: [] }, + { name: "sign", geometries: ["bbox"], color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: null, attributes: [] }, ], description: null, created_at: null, diff --git a/frontend/ui-core/src/annotator/editorNotice.test.tsx b/frontend/ui-core/src/annotator/editorNotice.test.tsx index ab4e5a74..713c6e6e 100644 --- a/frontend/ui-core/src/annotator/editorNotice.test.tsx +++ b/frontend/ui-core/src/annotator/editorNotice.test.tsx @@ -40,7 +40,7 @@ const SCHEMA = { description: null, created_at: null, provenance: "curated", - classes: [{ name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }], }; /** The batch's state, which decides whether the page tries to open it. */ diff --git a/frontend/ui-core/src/annotator/frameGallery.test.tsx b/frontend/ui-core/src/annotator/frameGallery.test.tsx index 502aeb4a..302ee31a 100644 --- a/frontend/ui-core/src/annotator/frameGallery.test.tsx +++ b/frontend/ui-core/src/annotator/frameGallery.test.tsx @@ -43,7 +43,7 @@ const SCHEMA = { description: null, created_at: null, provenance: "curated", - classes: [{ name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }], }; type Progress = "unannotated" | "annotated" | "skipped" | "review_pending" | "accepted"; diff --git a/frontend/ui-core/src/annotator/jobQueries.test.ts b/frontend/ui-core/src/annotator/jobQueries.test.ts index 76108902..5483fbd7 100644 --- a/frontend/ui-core/src/annotator/jobQueries.test.ts +++ b/frontend/ui-core/src/annotator/jobQueries.test.ts @@ -23,8 +23,8 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 2, classes: [ - { name: "vehicle", geometry: "bbox", color: null, attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: null, attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/panel.test.tsx b/frontend/ui-core/src/annotator/panel.test.tsx index 3e9bd24c..a4dc5218 100644 --- a/frontend/ui-core/src/annotator/panel.test.tsx +++ b/frontend/ui-core/src/annotator/panel.test.tsx @@ -24,18 +24,20 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, - { name: "centerline", geometry: "polyline", color: "#eb5a47", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, + { name: "centerline", geometries: ["polyline"], color: "#eb5a47", attributes: [] }, ], }; /** The same schema with its one tag class removed — the strip's absent case. */ const UNTAGGABLE_SCHEMA = { ...SCHEMA, - classes: SCHEMA.classes.filter((declared) => declared.geometry !== "classification_tag"), + classes: SCHEMA.classes.filter( + (declared) => !declared.geometries.includes("classification_tag"), + ), }; function annotation( @@ -313,8 +315,8 @@ describe("reassigning a class from a row", () => { for (const name of ["lane", "centerline", "daytime"]) { expect(screen.getByTestId(`reclass-0-${name}`).getAttribute("aria-disabled")).toBe("true"); } - expect(screen.getByTestId("reclass-0-lane").textContent).toContain("needs a polygon"); - expect(screen.getByTestId("reclass-0-centerline").textContent).toContain("needs a polyline"); + expect(screen.getByTestId("reclass-0-lane").textContent).toContain("needs polygon"); + expect(screen.getByTestId("reclass-0-centerline").textContent).toContain("needs polyline"); }); it("will not reassign to a class the kernel would refuse", async () => { diff --git a/frontend/ui-core/src/annotator/pinBadge.test.tsx b/frontend/ui-core/src/annotator/pinBadge.test.tsx index 08520de5..f4b987a8 100644 --- a/frontend/ui-core/src/annotator/pinBadge.test.tsx +++ b/frontend/ui-core/src/annotator/pinBadge.test.tsx @@ -35,7 +35,7 @@ const ASSET = "44444444-4444-4444-8444-444444444444"; const PINNED = { project_id: PROJECT, version: 1, - classes: [{ name: "sign", geometry: "bbox", color: null, attributes: [] }], + classes: [{ name: "sign", geometries: ["bbox"], color: null, attributes: [] }], description: null, created_at: null, provenance: "curated", diff --git a/frontend/ui-core/src/annotator/suggestFlow.test.tsx b/frontend/ui-core/src/annotator/suggestFlow.test.tsx index 28fb8e4d..aee0faf4 100644 --- a/frontend/ui-core/src/annotator/suggestFlow.test.tsx +++ b/frontend/ui-core/src/annotator/suggestFlow.test.tsx @@ -49,13 +49,13 @@ const SCHEMA = { created_at: null, provenance: "curated", classes: [ - { name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }, - { name: "lane-area", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }, + { name: "lane-area", geometries: ["polygon"], color: null, attributes: [] }, // Drawable, and not suggestible: a mask narrows to a region and a lane is an // open path. It is what parks the tool, and it is a `polyline` rather // than a tag on purpose — a class that can still be drawn on is the case where // a parked tool swallowing presses would be a bug rather than a nuisance. - { name: "lane", geometry: "polyline", color: null, attributes: [] }, + { name: "lane", geometries: ["polyline"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/toolPalette.test.tsx b/frontend/ui-core/src/annotator/toolPalette.test.tsx index 1e4d4d0f..eac56842 100644 --- a/frontend/ui-core/src/annotator/toolPalette.test.tsx +++ b/frontend/ui-core/src/annotator/toolPalette.test.tsx @@ -24,11 +24,11 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, - { name: "kerb", geometry: "polyline", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, ], } as unknown as Parameters[0]; @@ -40,7 +40,9 @@ function mount( { // nobody declared would be a roadmap, not a tool strip. const noLanes = { ...SCHEMA, - classes: SCHEMA.classes.filter((declared) => declared.geometry !== "polyline"), + classes: SCHEMA.classes.filter((declared) => !declared.geometries.includes("polyline")), } as typeof SCHEMA; render(mount({ schema: noLanes })); @@ -145,7 +147,7 @@ describe("the tools a schema can reach", () => { it("shows only select when no class draws anything", () => { const tagsOnly = { ...SCHEMA, - classes: [{ name: "daytime", geometry: "classification_tag", color: null, attributes: [] }], + classes: [{ name: "daytime", geometries: ["classification_tag"], color: null, attributes: [] }], } as unknown as typeof SCHEMA; render(mount({ schema: tagsOnly })); @@ -277,8 +279,8 @@ describe("the suggest tool (#424)", () => { const tagsOnly = { ...(SCHEMA as unknown as { classes: unknown[] }), classes: [ - { name: "daytime", geometry: "classification_tag", color: null, attributes: [] }, - { name: "kerb", geometry: "polyline", color: null, attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: null, attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, ], } as unknown as Parameters[0]; render(mount({ schema: tagsOnly, suggest: { active: false, onToggle: vi.fn() } })); diff --git a/frontend/ui-core/src/annotator/topBar.test.tsx b/frontend/ui-core/src/annotator/topBar.test.tsx index a1510272..03c5efb3 100644 --- a/frontend/ui-core/src/annotator/topBar.test.tsx +++ b/frontend/ui-core/src/annotator/topBar.test.tsx @@ -39,8 +39,8 @@ const SCHEMA = { created_at: null, provenance: "curated", classes: [ - { name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }, - { name: "lane-area", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }, + { name: "lane-area", geometries: ["polygon"], color: null, attributes: [] }, ], }; @@ -273,18 +273,25 @@ describe("the class list, now in the panel (#420)", () => { expect(screen.getByTestId("class-row-lane-area").textContent).toContain("2"); }); - it("changes the derived tool when the class picked declares another geometry", async () => { - // The tool is *derived* from the active class and never stored - // (`core/interaction/tool.ts`), so this asserts the derivation still runs - // through the panel — it does not re-derive anything itself. + it("changes the tool, and the strip, when the class picked accepts another geometry", async () => { + // The tool is *resolved* from the active class and the held preference and + // never stored (`core/interaction/tool.ts`), so this asserts the resolution + // still runs through the panel — it does not re-derive anything itself. + // + // The strip narrows with it, which is the visible half of #584: with a + // polygon-only class held, a box is not something that could be drawn here, + // and offering the button would answer "what can I draw?" with a lie. Both + // fixture classes accept exactly one shape, so each selection leaves exactly + // one drawing tool. await open(); await userEvent.click(screen.getByTestId("class-row-vehicle")); expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("true"); + expect(screen.queryByTestId("tool-polygon")).toBeNull(); await userEvent.click(screen.getByTestId("class-row-lane-area")); expect(screen.getByTestId("tool-polygon").getAttribute("data-active")).toBe("true"); - expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("false"); + expect(screen.queryByTestId("tool-bbox")).toBeNull(); }); it("focuses the panel's filter on `c`, which is the whole point of the host action", async () => { diff --git a/frontend/ui-core/src/data/geometryCategory.ts b/frontend/ui-core/src/data/geometryCategory.ts index 05cfb532..769b6e20 100644 --- a/frontend/ui-core/src/data/geometryCategory.ts +++ b/frontend/ui-core/src/data/geometryCategory.ts @@ -127,3 +127,24 @@ export function groupGeometries( geometries: offered.filter((geometry) => GEOMETRY_CATEGORY[geometry] === category), })).filter((group) => group.geometries.length > 0); } + +/** + * A class's geometry set, as one phrase for a row, a badge or a refusal. + * + * One spelling, product-wide, for the reason `classColor` is one: a class list, a + * reassignment menu and a schema row all name the same set, and three joins would + * be three chances to render `bbox,polygon` beside `bbox, polygon` beside + * `bbox or polygon`. + * + * "or" rather than a comma at the end, because the set is a *choice* — an + * annotation carries one of them, never several — and a comma list reads as + * things a class has all of. + * + * The order is the caller's, which for anything off the wire is the kernel's own + * sorted order. Nothing re-sorts here: a surface that grouped by category would + * hand them over grouped, and this would silently undo it. + */ +export function formatGeometries(geometries: readonly GeometryType[]): string { + if (geometries.length <= 2) return geometries.join(" or "); + return `${geometries.slice(0, -1).join(", ")} or ${geometries[geometries.length - 1]}`; +} diff --git a/frontend/ui-core/src/palette.test.ts b/frontend/ui-core/src/palette.test.ts index 62b887b7..9e3d646f 100644 --- a/frontend/ui-core/src/palette.test.ts +++ b/frontend/ui-core/src/palette.test.ts @@ -19,14 +19,14 @@ import { CLASS_FILL_OPACITY, classColor, hexColor, type LabelClass } from "./pal const withColour: LabelClass = { name: "vehicle", - geometry: "bbox", + geometries: ["bbox"], color: "#38bdf8", attributes: [], }; const without: LabelClass = { name: "pedestrian", - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], }; @@ -128,7 +128,7 @@ describe("hexColor", () => { // declared colour is convertible. The moment that stops holding, the editor // goes grey again. for (const name of ["lane", "vehicle", "pedestrian", "weather", "", "ünïcodé", "a".repeat(64)]) { - const derived = classColor({ name, geometry: "bbox", color: null, attributes: [] }, name); + const derived = classColor({ name, geometries: ["bbox"], color: null, attributes: [] }, name); expect(derived).toMatch(/^hsl\(/); expect(hexColor(derived)).toMatch(/^#[0-9a-f]{6}$/); } diff --git a/frontend/ui-core/src/patterns/ClassFields.tsx b/frontend/ui-core/src/patterns/ClassFields.tsx index 32505b0f..0e60ce94 100644 --- a/frontend/ui-core/src/patterns/ClassFields.tsx +++ b/frontend/ui-core/src/patterns/ClassFields.tsx @@ -29,9 +29,7 @@ import { FieldHint, Input, Label } from "../primitives/Input"; import { Select, SelectContent, - SelectGroup, SelectItem, - SelectLabel, SelectTrigger, SelectValue, } from "../primitives/Select"; @@ -58,6 +56,22 @@ const GEOMETRIES = [ const KINDS = ["string", "number", "boolean", "select"] as const; type Kind = (typeof KINDS)[number]; +/** + * What the chosen set means, said once under the group. + * + * The hint used to read *"Singular — picking a class picks a tool"*, which is no + * longer true: a class accepts a set, and picking one narrows the tool strip + * rather than deciding it. Naming the count rather than restating the rule keeps + * the sentence useful in the case somebody is most likely to have got wrong — + * having ticked one box and not realised a second was allowed. + */ +export function describeGeometries(geometries: readonly GeometryType[]): string { + if (geometries.length <= 1) { + return "One shape for now. Tick another and this class accepts both."; + } + return `An annotation of this class may be any of the ${geometries.length}.`; +} + export interface ClassFieldsProps { readonly declared: LabelClassBody; /** What this instance's `data-testid`s are built from. See the module docstring. */ @@ -88,39 +102,83 @@ export function ClassFields({ onChange={(event) => onChange({ ...declared, name: event.target.value })} /> -
- - - Singular — picking a class picks a tool. -
+ {/* Grouped, not flat, for the reason the dropdown was: a flat list of + every name the product can address says nothing about which ones + belong to the work somebody is actually doing, and the list only + grows. Native checkboxes rather than a new primitive — the + attribute `required` flag below is the same answer to the same + question, and a multi-select dropdown would hide the answer behind + a click on a control whose whole job is to show it. */} + {groupGeometries(GEOMETRIES).map((group) => ( +
+ + {group.category} + +
+ {group.geometries.map((geometry) => { + const checked = declared.geometries.includes(geometry); + // The last one standing does not come off. A class accepting + // nothing is refused by the kernel and by the wire, so the + // honest control is one that says why rather than one that + // lets you build a version the API will reject — and a bare + // disabled box would be principle 9's forbidden shape. + const last = checked && declared.geometries.length === 1; + return ( + + ); + })} +
+
+ ))} + + + {describeGeometries(declared.geometries)} + +
@@ -185,7 +243,7 @@ export function swatchOf(declared: LabelClassBody, index: number): string { return classColor( { name: declared.name, - geometry: declared.geometry, + geometries: declared.geometries, color: declared.color ?? null, attributes: [], }, diff --git a/frontend/ui-core/src/screens/OverviewPanel.tsx b/frontend/ui-core/src/screens/OverviewPanel.tsx index 216c004b..4c3a4fe1 100644 --- a/frontend/ui-core/src/screens/OverviewPanel.tsx +++ b/frontend/ui-core/src/screens/OverviewPanel.tsx @@ -438,7 +438,7 @@ function swatchFor(declared: readonly LabelClassBody[] | undefined, labelClass: ? undefined : { name: found.name, - geometry: found.geometry, + geometries: found.geometries, color: found.color ?? null, attributes: [], }, diff --git a/frontend/ui-core/src/screens/ProjectScreen.tsx b/frontend/ui-core/src/screens/ProjectScreen.tsx index 088f92f3..60053219 100644 --- a/frontend/ui-core/src/screens/ProjectScreen.tsx +++ b/frontend/ui-core/src/screens/ProjectScreen.tsx @@ -85,6 +85,7 @@ import { } from "lucide-react"; import { useState, type ComponentType, type FormEvent, type JSX } from "react"; +import { formatGeometries } from "../data/geometryCategory"; import { Async } from "../data/Async"; import { asApiError } from "../data/errors"; import { refusalProse } from "../data/refusals"; @@ -1039,10 +1040,12 @@ function AnnotationRun({ ); } -/** `name (geometry)`, in the schema's own authored order — which is the palette's. */ +/** `name (geometries)`, in the schema's own authored order — which is the palette's. */ function summarise(version: SchemaVersion): string { if (version.classes.length === 0) return "no classes"; - return version.classes.map((declared) => `${declared.name} (${declared.geometry})`).join(", "); + return version.classes + .map((declared) => `${declared.name} (${formatGeometries(declared.geometries)})`) + .join(", "); } function RenameDialog({ diff --git a/frontend/ui-core/src/screens/SchemaEditor.tsx b/frontend/ui-core/src/screens/SchemaEditor.tsx index 0077275a..4123b7d4 100644 --- a/frontend/ui-core/src/screens/SchemaEditor.tsx +++ b/frontend/ui-core/src/screens/SchemaEditor.tsx @@ -93,6 +93,7 @@ import { Plus, Trash2 } from "lucide-react"; import { useMemo, useRef, useState, type JSX, type KeyboardEvent } from "react"; +import { formatGeometries } from "../data/geometryCategory"; import { asApiError } from "../data/errors"; import { Alert, Badge } from "../primitives/Badge"; import { Button } from "../primitives/Button"; @@ -383,7 +384,7 @@ export function SchemaEditor({ } function addClass(): void { - edit([...classes, { name: "", geometry: "bbox", color: null, attributes: [] }]); + edit([...classes, { name: "", geometries: ["bbox"], color: null, attributes: [] }]); // Selected, and the filter cleared — a new class has an empty name, so any // filter at all would hide the row that was just created. setSelected(classes.length); @@ -563,7 +564,7 @@ export function SchemaEditor({
{entry.name} - {entry.geometry} + {formatGeometries(entry.geometries)} {entry.attributes.length > 0 && ( {formatCount(entry.attributes.length)}{" "} diff --git a/frontend/ui-core/src/screens/schemaDraft.test.tsx b/frontend/ui-core/src/screens/schemaDraft.test.tsx index 7e8fee21..ba2657f0 100644 --- a/frontend/ui-core/src/screens/schemaDraft.test.tsx +++ b/frontend/ui-core/src/screens/schemaDraft.test.tsx @@ -37,8 +37,8 @@ const PROJECT = "11111111-1111-4111-8111-111111111111"; const OTHER = "33333333-3333-4333-8333-333333333333"; const CLASSES = [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, ]; /** What the server is currently answering for `GET .../schema`. Mutable on purpose. */ @@ -442,5 +442,5 @@ describe("saving twice with nothing edited in between", () => { }); }); -const PEDESTRIAN = { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }; -const TRAFFIC_LIGHT = { name: "traffic light", geometry: "bbox", color: null, attributes: [] }; +const PEDESTRIAN = { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }; +const TRAFFIC_LIGHT = { name: "traffic light", geometries: ["bbox"], color: null, attributes: [] }; diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index edeada5d..66081504 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -100,8 +100,8 @@ function mount(node: ReactNode): JSX.Element { } const CLASSES = [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, ]; describe("the project list", () => { @@ -380,15 +380,15 @@ describe("the schema editor", () => { render(mount()); await screen.findByTestId("schema-editor"); - await userEvent.click(screen.getByTestId("class-geometry-0")); - + // No press: the boxes are on the page, which is the point of the control + // being a checkbox group rather than a dropdown — what a class accepts is + // readable without opening anything. const basic = screen.getByTestId("geometry-category-Basic Computer Vision"); const robotics = screen.getByTestId("geometry-category-Robotics and AD"); - // Radix labels a group by its `SelectLabel`, so the members under a heading - // are that group's — read from the DOM rather than from the map, which is - // what makes this a check on the rendering and not on the table. + // Read from the DOM rather than from the map, which is what makes this a + // check on the rendering and not on the table. const membersOf = (label: HTMLElement): string[] => - [...(label.parentElement?.querySelectorAll('[role="option"]') ?? [])].map( + [...(label.parentElement?.querySelectorAll("label") ?? [])].map( (option) => option.textContent ?? "", ); @@ -404,16 +404,41 @@ describe("the schema editor", () => { * changed what selecting one does, it would have silently rewritten the schema * editor's only real interaction. */ - it("still writes the picked geometry onto the class", async () => { + it("adds a geometry to a class rather than replacing the one it had", async () => { projectWithSchema(); render(mount()); await screen.findByTestId("schema-editor"); - await userEvent.click(screen.getByTestId("class-geometry-0")); + const before = screen.getByTestId("class-geometry-0-bbox") as HTMLInputElement; + expect(before.checked).toBe(true); + // Across a group boundary deliberately: `polyline` is the only member of the - // second category, so picking it proves a grouped option is still an option. - await userEvent.click(screen.getByRole("option", { name: "polyline" })); - expect(screen.getByTestId("class-geometry-0").textContent).toContain("polyline"); + // second category, so ticking it proves a grouped box is still a box. + await userEvent.click(screen.getByTestId("class-geometry-0-polyline")); + + // Both, which is the whole feature — a control that replaced would leave the + // first box clear and this would still find the second one ticked. + expect((screen.getByTestId("class-geometry-0-bbox") as HTMLInputElement).checked).toBe(true); + expect((screen.getByTestId("class-geometry-0-polyline") as HTMLInputElement).checked).toBe( + true, + ); + }); + + it("refuses to untick the last geometry, and says why rather than greying out", async () => { + projectWithSchema(); + render(mount()); + await screen.findByTestId("schema-editor"); + + const only = screen.getByTestId("class-geometry-0-bbox") as HTMLInputElement; + expect(only.checked).toBe(true); + + await userEvent.click(only); + + expect((screen.getByTestId("class-geometry-0-bbox") as HTMLInputElement).checked).toBe(true); + // Principle 9: the control that will not move carries the reason. `title` + // rather than the `disabled` attribute, so a keyboard still reaches it. + expect(only.closest("label")?.getAttribute("title")).toMatch(/at least one geometry/i); + expect(only.getAttribute("aria-disabled")).toBe("true"); }); /** @@ -435,7 +460,7 @@ describe("the schema editor", () => { // *agreement* — a change to the palette moves both sides together, and only a // swatch that stopped reading `classColor` fails. const derived = hexColor( - classColor({ name: "lane", geometry: "polygon", color: null, attributes: [] }, "lane"), + classColor({ name: "lane", geometries: ["polygon"], color: null, attributes: [] }, "lane"), ); expect(derived).not.toBeNull(); // One panel at a time, so each class is asserted from its own. @@ -495,7 +520,7 @@ describe("the schema editor", () => { await userEvent.click(screen.getByTestId("clear-color-0")); const derived = hexColor( - classColor({ name: "vehicle", geometry: "bbox", color: null, attributes: [] }, "vehicle"), + classColor({ name: "vehicle", geometries: ["bbox"], color: null, attributes: [] }, "vehicle"), ); expect(screen.getByTestId("class-color-0")).toHaveProperty("value", derived); // The button still means something: the stored value went back to null, which @@ -803,7 +828,7 @@ describe("the schema editor's two panels", () => { /** Fifty classes: an ordinary Physical AI ontology, and what the stack broke at. */ const MANY = Array.from({ length: 50 }, (_, index) => ({ name: `class-${String(index).padStart(2, "0")}`, - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], })); @@ -863,8 +888,8 @@ describe("the schema editor's two panels", () => { it("filters case-insensitively on a substring", async () => { withClasses([ - { name: "Vehicle", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: null, attributes: [] }, + { name: "Vehicle", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: null, attributes: [] }, ]); render(mount()); await screen.findByTestId("class-filter"); From bfa694251f93d30b670e5c832b6a89e32dfaae05 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 20:07:15 -0700 Subject: [PATCH 06/17] feat(app): the demo, styleguide and browser suites follow the geometry set cf. #584 --- frontend/app/cycle/cycle.spec.ts | 20 +++++++++++++++-- frontend/app/e2e/annotate.spec.ts | 8 +++---- frontend/app/e2e/navigation.spec.ts | 2 +- frontend/app/e2e/viewport.spec.ts | 2 +- frontend/app/src/demo/AnnotatorDemo.tsx | 7 ++++-- frontend/app/src/demo/ToolStrip.tsx | 26 +++++++++++----------- frontend/app/src/demo/sampleSchema.ts | 12 +++++----- frontend/app/src/styleguide/Styleguide.tsx | 13 ++++++----- frontend/ui-core/src/index.ts | 1 + 9 files changed, 57 insertions(+), 34 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index 6453bb15..4081cb17 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -232,11 +232,27 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await page.getByTestId("add-class").click(); await page.getByTestId(`class-name-${index}`).fill(name); if (geometry !== "bbox") { - await page.getByTestId(`class-geometry-${index}`).click(); - await page.getByRole("option", { name: geometry, exact: true }).click(); + // Tick the wanted shape *before* clearing the default, which is also the + // only order the control permits: a class never passes through accepting + // nothing, so the last ticked box refuses to come off. + await page.getByTestId(`class-geometry-${index}-${geometry}`).click(); + await page.getByTestId(`class-geometry-${index}-bbox`).click(); } } + /* + * One class, two shapes, against a real `create_version` (#584). + * + * The whole point of a geometry set is that a class labelled as a box on some + * frames and as an outline on others is one class — and the only place that + * can be shown end to end is here, where the kernel actually judges the + * document. `vehicle` keeps its box and gains a polygon. + */ + await page.locator('[data-row="0"] button').first().click(); + await page.getByTestId("class-geometry-0-polygon").click(); + await expect(page.getByTestId("class-geometry-0-bbox")).toBeChecked(); + await expect(page.getByTestId("class-geometry-0-polygon")).toBeChecked(); + /* * The draft survives leaving the tab, in a real DOM. * diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index fd86cadd..4ef18638 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -23,12 +23,12 @@ const SCHEMA = { project_id: PROJECT, version: 3, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, // A second **bbox** class, so a reassignment has somewhere to land. // It adds no tool — the palette is per geometry — and one hotkey row, which // the shortcut-sheet scenario below counts. - { name: "pedestrian", geometry: "bbox", color: "#22c55e", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: "#22c55e", attributes: [] }, ], }; @@ -152,7 +152,7 @@ function schemaOfSize(size: SchemaSize | undefined): typeof SCHEMA { ...SCHEMA.classes, ...Array.from({ length: want - SCHEMA.classes.length }, (_unused, index) => ({ name: `filler-${index + 1}`, - geometry: "bbox", + geometries: ["bbox"], color: "#94a3b8", attributes: [], })), diff --git a/frontend/app/e2e/navigation.spec.ts b/frontend/app/e2e/navigation.spec.ts index 059ec1b3..cb79cdb7 100644 --- a/frontend/app/e2e/navigation.spec.ts +++ b/frontend/app/e2e/navigation.spec.ts @@ -44,7 +44,7 @@ const NO_PROGRESS = { const SCHEMA = { project_id: PROJECT, version: 1, - classes: [{ name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }], }; /** A 1x1 PNG, so the annotator has real pixels to lay out. */ diff --git a/frontend/app/e2e/viewport.spec.ts b/frontend/app/e2e/viewport.spec.ts index 3f9c10f5..c7548177 100644 --- a/frontend/app/e2e/viewport.spec.ts +++ b/frontend/app/e2e/viewport.spec.ts @@ -37,7 +37,7 @@ const NO_PROGRESS = { const SCHEMA = { project_id: PROJECT, version: 1, - classes: [{ name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }], }; const PIXEL = Buffer.from( diff --git a/frontend/app/src/demo/AnnotatorDemo.tsx b/frontend/app/src/demo/AnnotatorDemo.tsx index 808bc3ea..10376072 100644 --- a/frontend/app/src/demo/AnnotatorDemo.tsx +++ b/frontend/app/src/demo/AnnotatorDemo.tsx @@ -42,6 +42,7 @@ import { annotationsInDrawOrder, classColor, hotkeyForClass, + drawableGeometries, isTaggableClass, randomUuid, selectedAnnotations, @@ -200,11 +201,13 @@ export function AnnotatorDemo(): JSX.Element { testId={`class-${declared.name}`} hotkey={hotkeyForClass(schema, declared.name) ?? "—"} name={declared.name} - note={declared.geometry} + note={declared.geometries.join(" or ")} swatch={classColor(declared, declared.name)} active={activeClass === declared.name} onClick={() => - isTaggableClass(declared) + // Drawable wins: since #584 a class may accept a tag *and* a + // shape, and the two stopped being each other's negation. + isTaggableClass(declared) && drawableGeometries(declared).length === 0 ? toggleTag(declared.name) : setActiveClass(declared.name) } diff --git a/frontend/app/src/demo/ToolStrip.tsx b/frontend/app/src/demo/ToolStrip.tsx index 5c7e4204..b3b4ded1 100644 --- a/frontend/app/src/demo/ToolStrip.tsx +++ b/frontend/app/src/demo/ToolStrip.tsx @@ -19,7 +19,7 @@ * is labelled. The tool did not move, so nothing moves. * 2. The strip shows only the tools **this schema can reach** — `select`, plus one * button per distinct drawable geometry among the declared classes. It is built - * from `drawableGeometry`, the export `tool.ts` provides for exactly this: a + * from `drawableGeometries`, the export `tool.ts` provides for exactly this: a * `classification_tag` and a `polyline` both answer `null`, and neither gets a * canvas tool. The demo's schema declares both, so both omissions are visible * rather than theoretical. @@ -42,7 +42,7 @@ * not extended, when the real icon set arrives. */ -import { drawableGeometry, hotkeyForClass } from "@visionset/annotator"; +import { drawableGeometries, hotkeyForClass } from "@visionset/annotator"; import type { AnnotationSchema, Tool } from "@visionset/annotator"; import type { CSSProperties, JSX, MouseEvent, ReactNode } from "react"; @@ -51,13 +51,13 @@ import { COLOR, RADIUS, SHADOW, SPACE, TEXT } from "./theme"; /** A schema's tools, in the order the strip lists them. */ /** - * What each drawing tool is called. Total over what `drawableGeometry` answers, so + * What each drawing tool is called. Total over what `drawableGeometries` answers, so * a fourth geometry gaining a tool cannot reach the strip unnamed — which is what * a ternary would let `polyline` do, silently reading "Polygon". * * The showcase keeps its own strip on purpose (see `ToolPalette.tsx`), so this is * a second table rather than an import; what it must not be is a second *rule*, - * and it is not — `drawableGeometry` is still the only thing deciding which tools + * and it is not — `drawableGeometries` is still the only thing deciding which tools * exist. */ const TOOL_LABELS: Readonly> = { @@ -86,15 +86,15 @@ function toolChoices(schema: AnnotationSchema): readonly ToolChoice[] { { tool: "select", label: "Select", labelClass: null, hotkey: "V" }, ]; for (const declared of schema.classes) { - const geometry = drawableGeometry(declared); - if (geometry === null) continue; - if (choices.some((choice) => choice.tool === geometry)) continue; - choices.push({ - tool: geometry, - label: TOOL_LABELS[geometry], - labelClass: declared.name, - hotkey: hotkeyForClass(schema, declared.name) ?? "—", - }); + for (const geometry of drawableGeometries(declared)) { + if (choices.some((choice) => choice.tool === geometry)) continue; + choices.push({ + tool: geometry, + label: TOOL_LABELS[geometry], + labelClass: declared.name, + hotkey: hotkeyForClass(schema, declared.name) ?? "—", + }); + } } return choices; } diff --git a/frontend/app/src/demo/sampleSchema.ts b/frontend/app/src/demo/sampleSchema.ts index 7b3526bc..81258009 100644 --- a/frontend/app/src/demo/sampleSchema.ts +++ b/frontend/app/src/demo/sampleSchema.ts @@ -36,13 +36,13 @@ export const SAMPLE_SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, // No colour: `classColor` derives a stable hue from the name instead, which is // the branch `LabelClass.color`'s own docstring blesses. - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "centerline", geometry: "polyline", color: "#c084fc", attributes: [] }, - { name: "pose", geometry: "keypoints", color: "#facc15", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "centerline", geometries: ["polyline"], color: "#c084fc", attributes: [] }, + { name: "pose", geometries: ["keypoints"], color: "#facc15", attributes: [] }, ], } as const; diff --git a/frontend/app/src/styleguide/Styleguide.tsx b/frontend/app/src/styleguide/Styleguide.tsx index 993be9ea..a1d529b0 100644 --- a/frontend/app/src/styleguide/Styleguide.tsx +++ b/frontend/app/src/styleguide/Styleguide.tsx @@ -73,6 +73,7 @@ import { TooltipProvider, TooltipTrigger, classColor, + formatGeometries, toast, } from "@visionset/ui-core"; import { MousePointer2, Plus, Square, Trash2 } from "lucide-react"; @@ -80,11 +81,11 @@ import type { JSX, ReactNode } from "react"; /** The demo schema, borrowed so the swatches show the real palette rule. */ const CLASSES = [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, // No colour — `classColor` derives a stable hue from the name instead. - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, ] as const; const BATCHES = [ @@ -208,7 +209,9 @@ export function Styleguide(): JSX.Element { style={{ background: classColor(declared, declared.name) }} /> {declared.name} - {declared.geometry} + + {formatGeometries(declared.geometries)} + ))}
diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 20a6eba8..280a1e8e 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -233,6 +233,7 @@ export { export { GEOMETRY_CATEGORIES, GEOMETRY_CATEGORY, + formatGeometries, groupGeometries, type GeometryCategory, type GeometryGroup, From 1bb6b48c532944ee68b66dfd5f4b7c08bcfad1d9 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 20:14:08 -0700 Subject: [PATCH 07/17] docs: geometry sets, the rescue flow, and export unchanged cf. #584 --- DESIGN.md | 19 ++++++-- docs/annotations.md | 14 ++++-- docs/mcp-walkthrough.md | 6 ++- docs/releases.md | 14 +++++- docs/schemas.md | 77 +++++++++++++++++++++++++++---- docs/tutorial.md | 9 ++-- frontend/app/e2e/annotate.spec.ts | 12 +++-- frontend/app/e2e/panel.spec.ts | 2 +- 8 files changed, 122 insertions(+), 31 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index ddb76f20..d5d1c266 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -916,7 +916,7 @@ The page the reference design shows (#56), with measurements verified in v1's so class selection lives. It was the side panel's Labels tab, then a `Combobox` in the centre of the top bar, and it is a **list** now — because what is being chosen between is the ontology, and a picker keeps all of it one click away, so the answer to *what can - I draw here* was never on screen. Rows carry swatch · name · geometry · hotkey badge, in + I draw here* was never on screen. Rows carry swatch · name · geometries · hotkey badge, in the **schema's authored order and only that**: a persistent list that reordered itself by recency would move rows under the cursor, and the digits are schema positions, so a recency-ordered list would print `3` against the row sitting first. `c` focuses its @@ -952,7 +952,13 @@ The page the reference design shows (#56), with measurements verified in v1's so how many it will publish (`Add 3 classes`). Opened from the class list's create row it starts on the name that was typed; opened from the tool strip's `+` or the region's own `+` it starts empty, - because that press means "I want a class", not a particular one. When it lands, the + because that press means "I want a class", not a particular one. **A name the published + version already declares is an offer, not an error** (#584): the dialog says what that + class accepts today and what publishing would add, and the primary reads `Add polygon to + "sign"`. It carries the existing class's colour and attributes, so a form opened to make a + new class cannot quietly overwrite what the old one declared. A name typed twice in one + sitting stays a refusal, because both are being written now and merging them would be + guessing which was meant. When it lands, the **last** class written becomes the drawing class and a toast says so — a session publishes one version and arms one class, neither of which anybody watched happen. Cancelling with classes banked **asks**, and it asks on Escape and the overlay too: @@ -974,7 +980,14 @@ The page the reference design shows (#56), with measurements verified in v1's so surface, `border`, 12px radius, 8px padding; 36px icon buttons; **active tool = primary variant** (the near-black), inactive = ghost; a `h-px w-6` divider; help at the bottom. Tooltips open right with the shortcut ("Select (V)", "Box (B)", "Polygon (P)"). - Icons: MousePointer2 / Square / Spline; only tools the schema's geometries allow. + Icons: MousePointer2 / Square / Spline; only tools the schema's geometries allow — and, + **with a class selected, only that class's own** (#584). A class accepts a set of + geometries, so what can be drawn depends on which class is held: offering a polygon + button under a boxes-only class would answer *what can I draw here* with a lie. With no + class selected it is the schema's union, which is still the right answer to *what does + this project label*. Switching to a class that forbids the active tool never strands it — + `toolFor` resolves to the class's first allowed shape — and the route to a different + class's geometry is the class list, which is where choosing a class belongs. **Last of them, below the `+`, the hand** (#576, `Hand`, `H`) — the one button here that the schema does not gate, because it answers a question about the *device* rather than about the project: a pan had exactly one spelling, a middle- or secondary-button drag, diff --git a/docs/annotations.md b/docs/annotations.md index c62e0210..82e0c28d 100644 --- a/docs/annotations.md +++ b/docs/annotations.md @@ -39,7 +39,7 @@ before anything is stored, and the whole call rolls back on the first refusal. | Refusal | When | | --- | --- | | `LabelClassNotInSchema` | The class is not in the pinned version. Matched by **exact** name. | -| `DisallowedGeometry` | The geometry is not the one that class declares. | +| `DisallowedGeometry` | The geometry is not one that class accepts. | | `MissingRequiredAttribute` | A `required` attribute has no value. A `default` is *not* filled in. | | `UnknownAttribute` | The annotation carries an attribute the class does not declare. | | `InvalidAttributeValue` | Wrong type for the kind, or outside a `select`'s options. | @@ -50,10 +50,14 @@ them. Catching the base is safe here in a way catching `DestructiveSchemaChange` flag overrides any of these, so there is nothing to retry into a loop. The remedy is to fix the annotation, or to write a schema version that describes it. -The geometry rule is **per class**, not per version: a `LabelClass` declares one `geometry`, so -this is an equality test. `SchemaService.allowed_geometries` is the union across a version's -classes - the right answer to "what may this project draw?" and the wrong tool here, where it -would let a polygon through under a bbox class. +The geometry rule is **per class**, not per version: a `LabelClass` declares a set of +`geometries` and this is membership in *that* set. `SchemaService.allowed_geometries` is the +union across a version's classes - the right answer to "what may this project draw?" and the +wrong tool here, where it would let a polygon through under a boxes-only class. + +A class accepting more than one shape is ordinary, not a corner case: the same sign is worth +boxing at a distance and worth outlining close up, and it is one class either way. Which shape +a given label carries is the annotation's own business. See [schemas.md](schemas.md). ## The version is the batch's, not the project's diff --git a/docs/mcp-walkthrough.md b/docs/mcp-walkthrough.md index 22d9676f..a985dbe2 100644 --- a/docs/mcp-walkthrough.md +++ b/docs/mcp-walkthrough.md @@ -61,8 +61,10 @@ create_schema_version project="road-signs" classes=[ -> {"version": 1, ...} ``` -A `LabelClass` is bound to exactly one geometry, so "a box round a sign" and "a tag on a picture -with nothing in it" are two classes, not one class with two shapes. The whole list is sent every +A `LabelClass` declares a **set** of geometries, so a class labelled as a box on some frames and +as an outline on others is one class — pass every shape it accepts. "A box round a sign" and "a +tag on a picture with nothing in it" are still two classes here, because they mean two different +things rather than two shapes of one thing. The whole list is sent every time: a version is the complete contract, never a patch against the last one, which is what lets [schemas.md](schemas.md) call removal *narrowing* and gate it. diff --git a/docs/releases.md b/docs/releases.md index 997fc8ab..a22895ca 100644 --- a/docs/releases.md +++ b/docs/releases.md @@ -209,6 +209,16 @@ against one format's declaration and returns an `ExportCompatibility`: } ``` +**One row per `(label_class, geometry)`, not per class.** A class accepts a *set* of +geometries ([schemas.md](schemas.md)), and a format's answer can differ across it: a class +labelled both as boxes and as outlines is, to YOLO, one half written whole and one half +written reduced. It contributes two rows. A single row could carry only one of those verdicts +and would describe half its own output wrongly whichever it picked - the same defect +`compatible: bool` had before three statuses replaced it, one level down. + +A class the schema declares and nobody used still gets a row per geometry, at zero. Zero +excludes nothing, so it never makes a report incompatible however unsupported its shape is. + ### Dropped is not degraded, and one word for both was a lie `status` has three values, and the reason is #158. Until then a class was `supported: true` or @@ -361,8 +371,8 @@ many. A classification tag has no location at all and is dropped rather than giv **v1 had two COCO exporters and neither described a dataset.** One skipped every annotation that was not a box, the other every one that was not a polygon, so a project holding both - the -ordinary case, and the reason a schema declares a geometry per class - had to pick an export and -silently lose the other half. There is one exporter here: COCO has always carried both, and +ordinary case, and now expressible in a single class since a class declares a *set* of +geometries - had to pick an export and silently lose the other half. There is one exporter here: COCO has always carried both, and `bbox` is a required field on every annotation whether or not it also has a `segmentation`. **`area` is the polygon's own area, by the shoelace formula, not its bounding box's.** v1 wrote diff --git a/docs/schemas.md b/docs/schemas.md index 085b9b2a..cb264a88 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -1,7 +1,7 @@ # Annotation schemas -A schema is a project's ontology. It defines the classes being labeled, the geometry for each -class, and the additional information carried by a label. The previous system called this the +A schema is a project's ontology. It defines the classes being labeled, the geometries each +class may be drawn as, and the additional information carried by a label. The previous system called this the "task type," but a task type remained fixed for the life of a project. A schema is **versioned**, and every version is immutable. @@ -19,7 +19,9 @@ with WorkspaceService.open("./road-signs") as workspace: sign = LabelClass( name="sign", - geometries=(GeometryType.BBOX,), + # A set: the same sign is worth boxing at the end of the street and + # worth outlining close up, and it is one class either way. + geometries=(GeometryType.BBOX, GeometryType.POLYGON), attributes=[ Attribute(name="occluded", kind="boolean", default=False), Attribute(name="weather", kind="select", options=["dry", "wet"]), @@ -34,7 +36,7 @@ with WorkspaceService.open("./road-signs") as workspace: schemas.get_active(project.id) # the highest version schemas.get(project.id, 1) # any version, forever schemas.list_versions(project.id) # oldest first - schemas.allowed_geometries(project.id) # {BBOX, POLYGON} + schemas.allowed_geometries(project.id) # {BBOX, POLYGON} — the union, not the test ``` **Over HTTP:** `POST`/`GET /projects/{project_id}/schema/versions`, @@ -193,16 +195,34 @@ not damage. ## Geometries a class may use +**A class accepts a set, not one.** `LabelClass.geometries` is non-empty, deduplicated, and +kept in one sorted order; an annotation carries **one** of them, and `AnnotationService` +tests membership in *that class's* set. Splitting `car` into `car` and `car_polygon` to label +the same object two ways is what this replaces - two classes that mean one thing, which every +consumer downstream then has to re-unify. + +The order is sorted rather than authored on purpose. `release.canonical_bytes` dumps these +straight into the document it hashes, so a set whose order depended on how a caller typed it +would make two identical schemas produce two different release hashes. Class *order* is +authored and preserved; geometry order carries no meaning and nobody can read one into it. + `GeometryType` names eight geometries; four have a model in the `Geometry` union today - `bbox`, `polygon`, `polyline` and `classification_tag`. `IMPLEMENTED_GEOMETRIES` is read *off* the union, so shipping a variant widens it with no -second edit, and `create_version` refuses anything outside it: +second edit, and `create_version` refuses a class naming anything outside it: ```python LabelClass(name="road", geometries=(GeometryType.MASK,)) # constructs fine schemas.create_version(project.id, [that]) # UnsupportedGeometry ``` +A document written before this was plural spells the field `geometry` and singular. +`LabelClass` reads one and lifts it into a set of one, which is why #584 needed **no +migration**: schema classes live in a JSON column and release manifests carry these +verbatim, so every stored version and every published release still loads. The REST body +deliberately does *not* accept the old key - a client sending it is told so rather than +silently reinterpreted. + Declaring a class whose geometry has no implementation would create a class nobody could ever label. Refusing at the schema is better than discovering it at the first annotation. @@ -228,10 +248,11 @@ categorises it. Nothing on the wire carries a category, and an exporter's capabi declaration never names one: `supported_geometries` is per geometry, because a lane exporter supports `polyline` and has said nothing at all about `cuboid_3d`. -`allowed_geometries` is the flip side, derived the same way: the set of geometries a -version's classes are bound to. It is what an annotation's `geometry.type` is -membership-tested against - the union's discriminator values *are* `GeometryType` members, -so nothing translates in between. +`allowed_geometries` is the flip side, derived the same way: the **union** across a version's +classes. It answers *what may this project draw?* and it is deliberately **not** the test a +write goes through - an annotation is judged against its own class's `geometries`, which the +union is wider than as soon as two classes accept different shapes. The union's discriminator +values *are* `GeometryType` members, so nothing translates in between. ## Publishing catches the open batches up @@ -260,7 +281,8 @@ annotation that was valid under the previous version stay valid under this one?* | Class added | additive | | Class removed | **destructive** | | Class renamed | **destructive** (a removal) plus an addition | -| Class geometry changed | **destructive** | +| Geometry added to a class | additive | +| Geometry removed from a class | **destructive** | | Class color changed | not a change at all | | Optional attribute added | additive | | Required attribute added | **destructive** | @@ -441,3 +463,38 @@ both questions about a *draft* before it publishes; `compare` remains the questi *published* versions, which is what the version navigator asks. See [ui.md](ui.md#the-schema-editor-and-the-two-409s) and [api.md](api.md#asking-before-you-are-refused). + + +## The rescue flow, when a class already exists + +Creating a class whose name the published version already declares is **not** answered with +an error. It is answered with an offer: the annotator's add-a-class dialog says what that +class accepts today and what publishing would add to it, and its primary button reads +`Add polygon to "sign"`. Somebody typing a name that exists almost always wants to draw that +class as a shape it does not have yet, and the product can simply do that. + +The widening carries the **existing** class's colour and attributes, not the form's: the +dialog was opened to make a new class, so publishing its blank colour would quietly wipe what +the class already declared. Only the geometries move. It goes out through the ordinary +`create_version` path, so it is an ordinary schema change - additive, needing no flag, and +producing the next version like any other. + +Two collisions, and only one of them is an offer. A name typed twice in **one sitting** stays +a refusal: both entries are being written now, so merging them would be guessing which of the +two was meant. + +Class names are still unique within a version, ignoring case. What changed is what the +interface does about it. + +## Export is unchanged + +A format declares which geometries it can carry and which it carries reduced, and every +exporter branches on the geometry **an annotation** holds - never on its class. So a class +accepting two shapes needs no exporter change: YOLO writes its boxes whole and writes its +polygons as bounding boxes, COCO carries both, and the pre-export report names all of it +before anything is written. See [releases.md](releases.md). + +The one thing that did move is the report's shape. It is now one row per `(class, geometry)` +rather than per class, because a class holding boxes and polygons is *two* answers under a +boxes-only format and a single row could only carry one of them - describing half its own +output wrongly whichever it picked. diff --git a/docs/tutorial.md b/docs/tutorial.md index 4bd43728..9c66dcf3 100644 --- a/docs/tutorial.md +++ b/docs/tutorial.md @@ -36,8 +36,9 @@ visionset project create road-signs ``` A project owns one dataset - its *trunk*, the curated set that releases are cut from. Before -anything can be labelled it needs a **schema**: the list of classes, and what geometry each one -takes. +anything can be labelled it needs a **schema**: the list of classes, and which geometries each +one accepts. A class may accept more than one — the same sign is worth boxing at a distance and +worth outlining close up, and it is one class either way. ```json { @@ -56,7 +57,7 @@ visionset schema apply schema.json --project road-signs Schema versions are numbered and **immutable**: applying a new list creates version 2, and version 1 stays readable forever. That matters more than it sounds - every annotation records which version it was judged against, and a release freezes the version it was cut with. Narrowing a -schema (removing a class, tightening a geometry) needs `--allow-destructive`, and if annotations +schema (removing a class, taking a geometry away from one) needs `--allow-destructive`, and if annotations already depend on what you are removing it is refused outright with no override. See [schemas.md](schemas.md). @@ -121,7 +122,7 @@ it and no "show token" anywhere. Paste it into the form; it is verified against anything is stored, so a typo is refused immediately rather than becoming a broken session. From there: **Projects → road-signs → the batch → a job**. The annotation page is the left rail, -the image, a floating tool strip with one tool per geometry your schema allows, and the +the image, a floating tool strip carrying the shapes the selected class accepts, and the Objects/Labels panel on the right. | | | diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 4ef18638..945dc376 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -2391,8 +2391,8 @@ test("the palette reports the tool whatever moved the class", async ({ page }) = const sent: Request[] = []; await openJob(page, sent); - // The tool is derived and never stored, so the digit row and the top bar's class - // field must light the same button the palette's own press does. A palette + // The tool is resolved and never stored, so the digit row and the panel's class + // list must light the same button the palette's own press does. A palette // holding its own idea of the tool is the pair v1 spent two mechanisms keeping // in step. await page.getByTestId("annotator-root").focus(); @@ -2402,7 +2402,11 @@ test("the palette reports the tool whatever moved the class", async ({ page }) = await page.getByTestId("class-row-vehicle").click(); await expect(page.getByTestId("tool-bbox")).toHaveAttribute("data-active", "true"); - await expect(page.getByTestId("tool-polygon")).toHaveAttribute("data-active", "false"); + // **Gone, not inactive** (#584). With a boxes-only class held, a polygon is not + // something that could be drawn here, and a button offering one would answer + // "what can I draw?" with a lie. The route to a polygon is the class list, + // which is where choosing a different class belongs. + await expect(page.getByTestId("tool-polygon")).toHaveCount(0); }); test("pressing a tool leaves the keyboard alive", async ({ page }) => { @@ -2813,7 +2817,7 @@ test("right-clicking a shape opens its class picker, and the class lands through // is offered, and the polygon class is present and refused rather than filtered // out — the panel's rule, because it is the panel's component. await expect(page.getByTestId("canvas-reclass-lane")).toHaveAttribute("aria-disabled", "true"); - await expect(page.getByTestId("canvas-reclass-lane")).toContainText("needs a polygon"); + await expect(page.getByTestId("canvas-reclass-lane")).toContainText("needs polygon"); await page.getByTestId("canvas-reclass-pedestrian").click(); diff --git a/frontend/app/e2e/panel.spec.ts b/frontend/app/e2e/panel.spec.ts index 92b488b2..e15adcb9 100644 --- a/frontend/app/e2e/panel.spec.ts +++ b/frontend/app/e2e/panel.spec.ts @@ -142,7 +142,7 @@ test("reassigning a class refuses the wrong geometry and says why, in one histor // refuses for a bbox. They are listed anyway, disabled and carrying the reason — // a short list with no explanation reads as a schema missing its classes. await expect(page.getByTestId("reclass-0-lane")).toHaveAttribute("aria-disabled", "true"); - await expect(page.getByTestId("reclass-0-lane")).toContainText("needs a polygon"); + await expect(page.getByTestId("reclass-0-lane")).toContainText("needs polygon"); await expect(page.getByTestId("reclass-0-centerline")).toHaveAttribute("aria-disabled", "true"); await page.getByTestId("reclass-0-pedestrian").click(); From b9ab6b62dd118487c3d9deab5c6419b2c84b6d37 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 20:30:07 -0700 Subject: [PATCH 08/17] test(ui): the retarget guard, which no fixture with a two-shape class could see cf. #584 --- .../src/annotator/toolPalette.test.tsx | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/frontend/ui-core/src/annotator/toolPalette.test.tsx b/frontend/ui-core/src/annotator/toolPalette.test.tsx index eac56842..0cc74968 100644 --- a/frontend/ui-core/src/annotator/toolPalette.test.tsx +++ b/frontend/ui-core/src/annotator/toolPalette.test.tsx @@ -123,6 +123,60 @@ describe("the tools a schema can reach", () => { ]); }); + describe("a class that accepts more than one shape (#584)", () => { + /** One class, two shapes — the whole point of a geometry set. */ + const BOTH = { + ...SCHEMA, + classes: [ + { name: "sign", geometries: ["bbox", "polygon"], color: "#38bdf8", attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, + ], + } as typeof SCHEMA; + + it("offers both of the held class's shapes and nothing else", () => { + render(mount({ schema: BOTH, activeClass: "sign", tool: "bbox" })); + + expect(screen.getByTestId("tool-bbox")).toBeTruthy(); + expect(screen.getByTestId("tool-polygon")).toBeTruthy(); + // `kerb`'s shape is not something this class could draw. + expect(screen.queryByTestId("tool-polyline")).toBeNull(); + }); + + it("changes only the tool when the held class already accepts the shape", () => { + // **The retarget guard.** Pressing polygon here means "draw this class as a + // polygon", not "switch to whatever class declares polygon first". A strip + // that re-armed the geometry's first declaring class would silently move + // somebody's labels to a different class than the one they had selected — + // and with a two-shape class there is no visible tell that it happened. + const onActivateClass = vi.fn(); + const onActivateTool = vi.fn(); + render( + mount({ schema: BOTH, activeClass: "sign", tool: "bbox", onActivateClass, onActivateTool }), + ); + + fireEvent.click(screen.getByTestId("tool-polygon")); + + expect(onActivateTool).toHaveBeenCalledWith("polygon"); + expect(onActivateClass).not.toHaveBeenCalled(); + }); + + it("moves the class when the held one cannot draw the shape pressed", () => { + // The other direction of the same site, which a single-direction mutation + // leaves green: with no class held, nothing accepts the tool, so the press + // has to arm the class that declares it. + const onActivateClass = vi.fn(); + const onActivateTool = vi.fn(); + render( + mount({ schema: BOTH, activeClass: null, tool: "select", onActivateClass, onActivateTool }), + ); + + fireEvent.click(screen.getByTestId("tool-polyline")); + + expect(onActivateTool).toHaveBeenCalledWith("polyline"); + expect(onActivateClass).toHaveBeenCalledWith("kerb"); + }); + }); + it("offers no polyline button at all when the schema declares no lane class", () => { // The affordance is about *this* schema. A strip advertising a geometry // nobody declared would be a roadmap, not a tool strip. From dfdf7d07724fff5ea2aa62402ffef9729147e494 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Fri, 14 Aug 2026 20:32:03 -0700 Subject: [PATCH 09/17] docs: the last two places that called a class's geometry singular cf. #584 --- docs/examples.md | 8 +++++--- frontend/app/src/styleguide/Styleguide.tsx | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/examples.md b/docs/examples.md index e70b9bbc..a3ecaa61 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -127,9 +127,11 @@ for Pillow - it is not carrying a determinism argument about folds.) ## Why three classes for "two classes" -A `LabelClass` is bound to exactly one `GeometryType` - `geometry` is singular. Showing a -bounding box, a polygon and a whole-frame classification therefore takes three classes -(`stop-sign`, `lane-marking`, `weather`), not one class listing three shapes. Exactly one +A `LabelClass` accepts a *set* of geometries, so one class could carry all three shapes - and +these are three classes anyway, because they mean three different things rather than three +shapes of one thing. `stop-sign`, `lane-marking` and `weather` is what an ontology looks like; +a single class accepting a box and an outline is what one *object* looks like from two +distances. Exactly one attribute is *required* (`occlusion` on `stop-sign`), which is what makes `MissingRequiredAttribute` a live rule in the example rather than a paragraph. diff --git a/frontend/app/src/styleguide/Styleguide.tsx b/frontend/app/src/styleguide/Styleguide.tsx index a1d529b0..c667e0a1 100644 --- a/frontend/app/src/styleguide/Styleguide.tsx +++ b/frontend/app/src/styleguide/Styleguide.tsx @@ -236,7 +236,7 @@ export function Styleguide(): JSX.Element { classification_tag - Singular per class — picking a class picks a tool. + A hint, under a field that needs one.
{/* The two-line option. Here because it is a primitive variant From c07941563ee216e8e983f75d6d8394ddd53a1f82 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 00:49:41 -0700 Subject: [PATCH 10/17] feat(ui): one vocabulary for geometries, and it is not the wire's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interface was showing users database identifiers, in two vocabularies. `ToolPalette` had a private `TOOL_LABELS` saying `Box`; every other surface — class rows, the reassignment menu, the add-a-class dialog's checkboxes and prose and its primary button, the schema editor's badges, the project summary — printed the raw `GeometryType`. So one thing was `Box` on the left of the canvas and `bbox` on the right, and a tag class's row read `classification_tag`. `GEOMETRY_LABELS` lives beside `GEOMETRY_CATEGORY` in the module that already owns geometry presentation, total over the union by `satisfies` so a ninth member fails the build until somebody names it. The strip capitalises at its own control; every other caller reads the word as-is. **Lowercase, because the same word is used two ways** — as a chip in a dense row (`box · polygon`) and inside a sentence ("Publishing adds polygon to it"). Only the first letter is a sentence-position question, which the test states as *never starts with a capital* rather than *is lowercase*: `3D box` is an acronym and the stricter rule would have forced `3d box`, wrong in every position. `formatGeometries` joins with ` · ` rather than ` or `. A middot is what a set reads as at this density, and in a 248px row those four characters come out of the class name. It is also the largest width saving available in the class list — a tag class's row spent about 110px of 248 on `classification_tag` and now spends 22 on `tag`, against the 32px widening the whole panel would buy. Tests address a checkbox by `data-testid`, which keeps the wire value, so a test says *which* geometry without also asserting what it is called. cf. #584 --- .../ui-core/src/annotator/ToolPalette.tsx | 22 ++++--- .../src/annotator/addClassDialog.test.tsx | 4 +- .../ui-core/src/data/geometryCategory.test.ts | 61 +++++++++++++++++++ frontend/ui-core/src/data/geometryCategory.ts | 56 ++++++++++++++--- frontend/ui-core/src/index.ts | 2 + frontend/ui-core/src/patterns/ClassFields.tsx | 8 ++- frontend/ui-core/src/screens/screens.test.tsx | 18 ++++-- 7 files changed, 145 insertions(+), 26 deletions(-) diff --git a/frontend/ui-core/src/annotator/ToolPalette.tsx b/frontend/ui-core/src/annotator/ToolPalette.tsx index ea97c9cb..cc32572c 100644 --- a/frontend/ui-core/src/annotator/ToolPalette.tsx +++ b/frontend/ui-core/src/annotator/ToolPalette.tsx @@ -89,8 +89,11 @@ import { hotkeyForClass, schemaCanSuggest, type AnnotationSchema, + type GeometryType, type Tool, } from "@visionset/annotator"; + +import { geometryLabel } from "../data/geometryCategory"; import { CircleHelp, Hand, @@ -125,15 +128,16 @@ const PENDING_TOOLS: Readonly> = {}; /** * What each drawing tool is called on the strip. * - * Total over what `drawableGeometries` can answer, so a fourth geometry gaining a - * tool cannot reach the strip unnamed — which is what the ternary this replaced - * would have let it do, silently reading "Polygon". + * Read off the product's one geometry vocabulary rather than kept here. This used + * to be a private map saying `Box` while every other surface printed `bbox`, so + * the same tool had two names depending on which side of the canvas you read it + * from. `geometryLabel` is now the single source; this only capitalises, because + * a control label takes a capital and a word inside a sentence does not. */ -const TOOL_LABELS: Readonly> = { - bbox: "Box", - polygon: "Polygon", - polyline: "Polyline", -}; +function toolLabel(geometry: GeometryType): string { + const word = geometryLabel(geometry); + return word.charAt(0).toUpperCase() + word.slice(1); +} /** A schema's tools, in the order the strip lists them. */ interface ToolChoice { @@ -189,7 +193,7 @@ export function toolChoices( if (choices.some((choice) => choice.tool === geometry)) continue; choices.push({ tool: geometry, - label: TOOL_LABELS[geometry], + label: toolLabel(geometry), labelClass: declared.name, hotkey: hotkeyForClass(schema, declared.name) ?? "—", unavailable: null, diff --git a/frontend/ui-core/src/annotator/addClassDialog.test.tsx b/frontend/ui-core/src/annotator/addClassDialog.test.tsx index 915462cd..16c83dc2 100644 --- a/frontend/ui-core/src/annotator/addClassDialog.test.tsx +++ b/frontend/ui-core/src/annotator/addClassDialog.test.tsx @@ -75,7 +75,7 @@ describe("what the dialog refuses before it asks", () => { const offer = screen.getByTestId("widen-offer"); expect(offer.textContent).toContain("“sign” already exists"); - expect(offer.textContent).toContain("declares it as bbox"); + expect(offer.textContent).toContain("declares it as box"); const submit = screen.getByTestId("add-class-submit"); expect(submit).toHaveProperty("disabled", false); // The button says what it does, rather than "Add class". @@ -195,7 +195,7 @@ describe("what it submits", () => { (option) => option.textContent ?? "", ); - expect(membersOf(basic)).toEqual(["bbox", "polygon", "classification_tag"]); + expect(membersOf(basic)).toEqual(["box", "polygon", "tag"]); expect(membersOf(robotics)).toEqual(["polyline"]); }); diff --git a/frontend/ui-core/src/data/geometryCategory.test.ts b/frontend/ui-core/src/data/geometryCategory.test.ts index 196d7938..a1f57fad 100644 --- a/frontend/ui-core/src/data/geometryCategory.test.ts +++ b/frontend/ui-core/src/data/geometryCategory.test.ts @@ -25,6 +25,9 @@ import { firstMismatch } from "./check"; import { GEOMETRY_CATEGORIES, GEOMETRY_CATEGORY, + GEOMETRY_LABELS, + formatGeometries, + geometryLabel, groupGeometries, type GeometryCategory, } from "./geometryCategory"; @@ -99,3 +102,61 @@ describe("grouping what a surface offers", () => { expect(groupGeometries([])).toEqual([]); }); }); + + +describe("what a geometry is called on screen", () => { + it("is total over the wire's geometry union", () => { + // Same shape as the category map's own claim above, and for the same reason: + // the `satisfies` is the proof, this is the copy of it that does not move + // when somebody edits the declaration. + const total: Record = GEOMETRY_LABELS; + expect(Object.keys(total).length).toBeGreaterThan(0); + }); + + it("names nothing the wire does not call a geometry", () => { + for (const geometry of Object.keys(GEOMETRY_LABELS)) { + expect(firstMismatch(checkGeometryType, geometry)).toBeNull(); + } + }); + + it("does not print the wire value where the two differ", () => { + // **The assertion that matters.** A map whose every entry equalled its key + // would type-check, satisfy totality, and be exactly the defect this exists + // to remove — the interface showing users identifiers. These are the two the + // kernel spells for itself rather than for a person, so they are the two that + // prove the map is doing work. + expect(geometryLabel("bbox")).toBe("box"); + expect(geometryLabel("classification_tag")).toBe("tag"); + }); + + it("never starts with a capital, because the same word goes in a sentence", () => { + // A capital reads fine as a chip and wrong mid-sentence ("Publishing adds + // Polygon to it"). The tool strip capitalises at its own control instead. + // + // **Starts** lowercase rather than *is* lowercase, and the difference is a + // real one this caught: `3D box` is an acronym, and a rule demanding the + // whole string be lowercase would have forced `3d box`, which is wrong in + // every position. Only the first letter is a sentence-position question. + for (const label of Object.values(GEOMETRY_LABELS)) { + expect(label).toBe(label.charAt(0).toLowerCase() + label.slice(1)); + } + }); +}); + +describe("a set of geometries, as one phrase", () => { + it("joins with a middot, in the order it was given", () => { + expect(formatGeometries(["bbox", "polygon"])).toBe("box · polygon"); + }); + + it("uses the display words, so a tag class does not print its enum member", () => { + // ~110px of a 248px row, before this. The single largest width saving + // available in the class list, larger than widening the panel. + expect(formatGeometries(["classification_tag"])).toBe("tag"); + }); + + it("says nothing for an empty set, rather than a stray separator", () => { + // The kernel cannot produce one, but a refusal renders `?? []` while a class + // is being typed, and " · " alone would read as damage. + expect(formatGeometries([])).toBe(""); + }); +}); diff --git a/frontend/ui-core/src/data/geometryCategory.ts b/frontend/ui-core/src/data/geometryCategory.ts index 769b6e20..04f7eb14 100644 --- a/frontend/ui-core/src/data/geometryCategory.ts +++ b/frontend/ui-core/src/data/geometryCategory.ts @@ -128,23 +128,65 @@ export function groupGeometries( })).filter((group) => group.geometries.length > 0); } +/** + * What each geometry is **called on screen**, which is not what it is called on + * the wire. + * + * `GeometryType`'s members are the kernel's identifiers — `bbox` because that is + * the discriminator every payload carries, `classification_tag` because that is + * what the variant is. Neither is a word to show somebody. Until this map existed + * the product had **two vocabularies**: the tool strip's private `TOOL_LABELS` + * said `Box`, and every other surface — class rows, the reassignment menu, the + * add-a-class dialog's checkboxes and prose, the schema editor's badges, the + * project summary — printed the enum. So one thing was `Box` on the left of the + * canvas and `bbox` on the right, and a tag class's row spent about 110px of a + * 248px row saying `classification_tag`. + * + * **Lowercase**, because the same word is used two ways and only lowercase reads + * correctly in both: as a chip in a dense row (`box · polygon`) and inside a + * sentence (*"Publishing adds polygon to it"*). A control that wants a capital + * — the tool strip's `Box (1)` — capitalises at the point of use, so there is one + * source and one transform rather than two lists free to drift apart again. + * + * Total over the union by `satisfies`, exactly as `GEOMETRY_CATEGORY` above, so a + * ninth member fails the build until somebody names it. The four with no + * implementation are named too: a schema may legally declare `mask`, and the + * surface that has to refuse it should refuse it in words. + */ +export const GEOMETRY_LABELS = { + bbox: "box", + polygon: "polygon", + polyline: "polyline", + classification_tag: "tag", + mask: "mask", + keypoints: "keypoints", + cuboid_3d: "3D box", + polyline_3d: "3D polyline", +} as const satisfies Record; + +/** What to call this geometry on screen. Never the wire value. */ +export function geometryLabel(geometry: GeometryType): string { + return GEOMETRY_LABELS[geometry]; +} + /** * A class's geometry set, as one phrase for a row, a badge or a refusal. * * One spelling, product-wide, for the reason `classColor` is one: a class list, a * reassignment menu and a schema row all name the same set, and three joins would - * be three chances to render `bbox,polygon` beside `bbox, polygon` beside - * `bbox or polygon`. + * be three chances to render `box,polygon` beside `box, polygon` beside + * `box or polygon`. * - * "or" rather than a comma at the end, because the set is a *choice* — an - * annotation carries one of them, never several — and a comma list reads as - * things a class has all of. + * **A middot, not "or".** The set is a choice — an annotation carries one of them, + * never several — and a comma list would read as things a class has all of. "or" + * says that correctly and costs four characters in a row where the class *name* + * is what those characters come out of. `·` is what a set reads as at this + * density, and the row has no room to be polite. * * The order is the caller's, which for anything off the wire is the kernel's own * sorted order. Nothing re-sorts here: a surface that grouped by category would * hand them over grouped, and this would silently undo it. */ export function formatGeometries(geometries: readonly GeometryType[]): string { - if (geometries.length <= 2) return geometries.join(" or "); - return `${geometries.slice(0, -1).join(", ")} or ${geometries[geometries.length - 1]}`; + return geometries.map(geometryLabel).join(" · "); } diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 280a1e8e..b10d45be 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -233,7 +233,9 @@ export { export { GEOMETRY_CATEGORIES, GEOMETRY_CATEGORY, + GEOMETRY_LABELS, formatGeometries, + geometryLabel, groupGeometries, type GeometryCategory, type GeometryGroup, diff --git a/frontend/ui-core/src/patterns/ClassFields.tsx b/frontend/ui-core/src/patterns/ClassFields.tsx index 0e60ce94..793bcdfd 100644 --- a/frontend/ui-core/src/patterns/ClassFields.tsx +++ b/frontend/ui-core/src/patterns/ClassFields.tsx @@ -22,7 +22,7 @@ import { Plus, Trash2 } from "lucide-react"; import type { JSX } from "react"; -import { groupGeometries } from "../data/geometryCategory"; +import { geometryLabel, groupGeometries } from "../data/geometryCategory"; import { classColor, hexColor } from "../palette"; import { Button } from "../primitives/Button"; import { FieldHint, Input, Label } from "../primitives/Input"; @@ -167,7 +167,11 @@ export function ClassFields({ // unreachable by exactly the people who need it. aria-disabled={last || undefined} /> - {geometry} + {/* The word, never the wire value: the `data-testid` + above keeps the enum so a test addresses the box by + what it *is*, and the person reading the form sees + what it is called. */} + {geometryLabel(geometry)} ); })} diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index 66081504..be4b6fed 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -352,17 +352,21 @@ describe("the schema editor", () => { render(mount()); await screen.findByTestId("schema-editor"); - await userEvent.click(screen.getByTestId("class-geometry-0")); // `polyline` is offered even where no tool draws one: the API // accepts it, the exporters need it, and the tool strip is where a person // learns there is nothing to draw with. Offering it is not offering a refusal. + // + // Addressed by `data-testid`, which keeps the **wire** value, while the label + // beside it is the display word — so this asserts which geometries are on + // offer without also asserting what they are called, which is + // `geometryCategory.test.ts`'s job. for (const geometry of ["bbox", "polygon", "polyline", "classification_tag"]) { - expect(screen.getAllByText(geometry).length).toBeGreaterThan(0); + expect(screen.getByTestId(`class-geometry-0-${geometry}`)).toBeTruthy(); } // `GeometryType` has eight members; four are refused at write time with // `UnsupportedGeometry`, so offering them would be offering a refusal. for (const geometry of ["mask", "keypoints", "cuboid_3d", "polyline_3d"]) { - expect(screen.queryByRole("option", { name: geometry })).toBeNull(); + expect(screen.queryByTestId(`class-geometry-0-${geometry}`)).toBeNull(); } }); @@ -392,7 +396,9 @@ describe("the schema editor", () => { (option) => option.textContent ?? "", ); - expect(membersOf(basic)).toEqual(["bbox", "polygon", "classification_tag"]); + // The display words, not the wire values: `classification_tag` is an + // identifier and `tag` is what it is called. + expect(membersOf(basic)).toEqual(["box", "polygon", "tag"]); expect(membersOf(robotics)).toEqual(["polyline"]); // Order of the sections is the map's declaration order, and it is the order // somebody reads down the list in. @@ -1217,7 +1223,7 @@ describe("version history", () => { // `findBy` on the row, not on the card: the card renders immediately and holds // the skeletons, so a `getBy` here asserts against a loading state. await within(history).findByTestId("version-1"); - expect(within(history).getByTestId("version-1").textContent).toContain("vehicle (bbox)"); + expect(within(history).getByTestId("version-1").textContent).toContain("vehicle (box)"); expect(within(history).getByTestId("version-2").textContent).toContain("lane (polygon)"); // Active is *derived* — the highest version, never a stored flag. @@ -1356,7 +1362,7 @@ describe("version history", () => { expect(run.textContent).toContain("lane (polygon)"); // And not v2's, which declares one class — the assertion above is only a // claim about *which* version is summarised because the two differ. - expect(run.textContent).toContain("vehicle (bbox)"); + expect(run.textContent).toContain("vehicle (box)"); }); it("gives back every row when it is expanded", async () => { From fe1b2e0fbd2a02a1156a5bfc56ba7919b113b67f Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 00:54:32 -0700 Subject: [PATCH 11/17] feat(ui): the armed class row is the shape picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Arming a class stopped answering which shape the next drag produces, and until now the only place that answer lived was the tool strip at the **far left** of the canvas while the class was chosen on the right — one decision split across the width of the picture, in a loop repeated hundreds of times a job. The armed row's geometry words become a segmented control: the active shape lit, pressing another switches the tool **without moving the class**. That retarget rule already shipped in `ToolPalette` and is tested in both directions, so the panel is a second caller of an existing rule rather than a new one. **Only the armed row, and the accessible answer and the density answer agree.** `ClassListRow` is documented as "a real ` + {shapes.map((shape) => ( + + ))} + {hotkey != null && ( + + {hotkey} + + )} + + ); + } + return (