docs(review-skill): require slots + world-scale UV convention for new nodes

Adds a Section B rule to the review-architecture skill: a new node kind or
geometry change that exposes paintable parts must carry overrides in a
schema `slots` record resolved via def.capabilities.paint (and thread it
through clone/duplicate/preset paths), and texturable def.geometry must
emit UVs in metres (1 unit = 1 m) so catalog finishes tile. Surfaced in the
skill description too.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Wassim SAMAD
2026-06-19 13:04:42 -04:00
co-authored by Claude Opus 4.8
parent 115a142d83
commit 08df21a96d
+4 -1
View File
@@ -1,6 +1,6 @@
---
name: review-architecture
description: Review a PR against the Pascal architectural rules — package boundaries (core/viewer/editor/nodes), the registry-driven composition model (def.geometry / def.renderer / def.system), legacy-dispatch regressions, hook hygiene (useEditor/useScene/useViewer), and selector performance. Use when the user asks to review a PR, audit a branch, or check that changes respect the codebase's architecture.
description: Review a PR against the Pascal architectural rules — package boundaries (core/viewer/editor/nodes), the registry-driven composition model (def.geometry / def.renderer / def.system), legacy-dispatch regressions, the slots + world-scale-UV convention for new nodes/geometry, hook hygiene (useEditor/useScene/useViewer), and selector performance. Use when the user asks to review a PR, audit a branch, or check that changes respect the codebase's architecture.
allowed-tools: Bash(git *) Bash(gh *) Read Grep Glob
---
@@ -101,6 +101,9 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de
- **Host kinds need `children` on the schema.** If `def.relations.hosts` is set, the schema must declare `children: z.array(z.string()).default([])` (and `migrateNodes` must patch existing scenes). Otherwise `useScene.createNode(child, parentId)` writes a `parent.children` entry into nothing and the host never sees the new child.
- **Movable opt-in.** `MoveTool` dispatches to `MoveRegistryNodeTool` only when `def.capabilities.movable` is set. Kinds with bespoke move semantics (wall endpoint drag with linked-wall cascade, slab vertex edit, etc.) deliberately omit `movable` and supply `def.affordanceTools.move` instead. Force-routing a bespoke-move kind through generic dispatch (`nodeRegistry.has(kind)` instead of `def.capabilities.movable`) is a regression — call it out. The bug history is documented in `plans/editor-node-registry.md` ("Capability-driven move dispatch").
- **Paint dispatch lives on `def.capabilities.paint`.** A paintable kind declares `resolveRole` / `buildPatch` / `applyPreview` (+ optional `getEffectiveMaterial`) on `PaintCapability`; the editor's selection-manager routes hover / click / preview through the generic dispatcher. A PR that adds an `if (node.type === '<kind>')` arm to paint-mode handling, paint-preview application, or material picker resolution is a regression — the behaviour belongs on the kind's `paint` capability. See `packages/core/src/registry/types.ts` (`PaintCapability`).
- **Slots + world-scale UVs for paintable surfaces.** A new kind (or geometry change) that exposes paintable parts must follow the unified slot convention, not reinvent it:
- **Paintable parts are slots, carried on the node.** Overrides live in a `slots` record (`slotId → MaterialRef`, `scene:`/`library:`) on the schema, resolved via `def.capabilities.paint` — not ad-hoc per-surface `material` / `materialPreset` fields, and not a parallel store. A new paintable kind whose schema lacks `slots` (or whose duplicate / preset / clone path drops it) is a blocker: it silently loses painted materials. (Slots are plain data — generic clone/parse preserves them; bespoke draft-rebuild placement paths must thread them through explicitly. Reference bug: item duplicate rebuilt the draft from `asset` and dropped `slots`.)
- **Texturable geometry emits UVs in metres (1 UV unit = 1 m).** Any `def.geometry` producing a surface a finish can tile onto must generate UVs at the same world scale walls / slabs / roofs use, because catalog finishes set `repeat` as tiles-per-metre. Unitless, bounding-box-normalised, or hardcoded UVs that don't scale with the surface are a blocker — finishes won't tile consistently. Flat-colour-only surfaces need no UVs. GLB item authoring follows the same contract via `slot_`-prefixed materials (case-insensitive, `slot_` → slot id). See `wiki/architecture/materials-and-themes.md` § "Texture world scale" and `wiki/architecture/item-authoring.md`.
- **Floor elevation lives on `def.capabilities.floorPlaced`.** Kinds that rest on a level and lift over overlapping slabs declare a `footprint` (and optional `applies` predicate) on `FloorPlacedConfig`; the generic `<FloorElevationSystem>` writes `slabElevation + node.position[1]` onto the registered mesh on each dirty mark. A new per-kind `useEffect` / per-kind system that recomputes Y from slab overlap is a regression — the per-kind block was lifted out of `ItemSystem` in Phase 6.1. See `packages/core/src/registry/types.ts` (`FloorPlacedConfig`).
- **Render-mode behaviour goes through `def.surfaceRole`.** Solid / Rendered / Clay viewer modes look up the kind's `surfaceRole` on `NodeDefinition` to pick the right material strategy (clay overrides, edge passes, theme overlays). New `if (node.type === '<kind>')` arms inside the render-mode pipeline or theme application are a regression — the behaviour belongs on `def.surfaceRole`. See `packages/core/src/registry/types.ts:529`.
- **Capability names must describe verbs, not host kinds.** `movable` / `paint` / `floorPlaced` / `cuttable` / `selectable` describe *what the node does*. A new capability named after a host kind (`slabAccessory`, `wallAccessory`, `siteAccessory`, anything `Xaccessory` / `Xhosted` shaped) couples the registry's type surface to one specific host and reads as precedent for the next reviewer. Blocker. Push back: generalise into a paired *host-side* capability ("I merge subtractive accessories from my children") + *accessory-side* capability ("I provide a cut geometry, cascade my dirty mark to my host's parent"). The single existing case — `capabilities.roofAccessory` (`packages/core/src/registry/types.ts:791`, consumed by `packages/viewer/src/systems/roof/roof-system.tsx`) — is documented tech debt; do not extend the pattern.