diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index 56cb4821..fbbf282b 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -120,6 +120,7 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de - New state added to `useViewer` must be presentation-only (selection, camera, level mode, display toggles). Editor-only state (active tool, phase, edit mode, paint preview, floorplan state) goes in `useEditor`. - **Node code does not import `useScene` directly.** A kind's geometry / system / tool should read and write through `SceneApi` (passed in by the framework) or `GeometryContext`. Direct `useScene.getState()` calls inside `packages/nodes/src//` are a smell — they bypass the registry's IoC point and make the code harder to test. - **Live drag motion is imperative, not store-driven.** Tools must not call `useLiveTransforms.set(...)` per `grid:move` tick to animate registered parametric kinds — the selector path doesn't reliably re-render and the mesh visibly disappears mid-drag. Use `sceneRegistry.nodes.get(node.id)?.position.set(x, y, z)` instead, and commit once at the end via `useScene.temporal.getState().resume() → updateNode → pause()`. The reference implementation is `MoveRegistryNodeTool`. This is the *only* sanctioned use of imperative mesh transforms by a tool; flag any other location that does the same. +- **Data-driven drags preview via `useLiveNodeOverrides`, never per-tick `useScene`.** A kind whose geometry is recomputed from data fields (wall `start`/`end`, opening host-cut, endpoint reshape) previews by publishing field patches to `useLiveNodeOverrides` (merged by `getEffectiveWall` / `getEffectiveNode`), writing the scene store **once on commit**. A tool that calls `useScene.updateNodes`/`updateNode` on `grid:move` (or any per-pointer-move tick) is a **blocker** — it swaps the `nodes` map ref and re-renders every `useScene(s => s.nodes)` subscriber app-wide each frame (`markDirty` per tick is fine). Grep tell: `updateNode(s)?(` in an `onGridMove`/`onMove`/`applyPreview` path under `packages/nodes/src//`. See `wiki/architecture/tools.md` § "Data-driven live drag". ### D. Selector performance @@ -127,6 +128,7 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de - Selectors that return new object or array references each call (e.g. `s => ({ a: s.a, b: s.b })`, `s => s.items.filter(...)`) without a custom equality function (shallow or custom) are re-render hazards. - Prefer subscribing by ID deep in the tree (one node per renderer) over subscribing to the full collection high up. - Inside a `` (legacy or `parametrics.customPanel`-mounted), avoid `useScene(s => s.nodes[selectedId])` as a callback dep — it changes every tick and pushes `useCallback` into infinite-loop territory. The recipe is in `plans/editor-node-registry.md` under "Panel slider-drag fix recipe". +- **Per-node list renderers subscribe per-node, not to the whole live Map.** A list that draws one child per node (`FloorplanRegistryLayer` → `FloorplanRegistryEntry`) must have each child subscribe to its **own** slice (`useLiveTransforms(s => s.transforms.get(id))` / `overrides.get(id)`) and be `memo`'d with referentially stable props; the parent subscribes only to the stable id list. Subscribing the parent or a child to the whole `transforms`/`overrides` Map, dropping a `memo`, or passing unstable props re-renders all N children every drag tick — a flood that type-checks and passes tests. Sibling invalidation goes through a per-node epoch, not a whole-layer re-render. See `wiki/architecture/tools.md` § "Floorplan registry: per-node subscriptions". ### E. Separation of concerns diff --git a/wiki/architecture/tools.md b/wiki/architecture/tools.md index 90328c7c..9426b38c 100644 --- a/wiki/architecture/tools.md +++ b/wiki/architecture/tools.md @@ -83,8 +83,11 @@ export function MyTool() { `modifiers.shiftKey` to bypass snapping, and never apply a grid step that isn't gated on `isGridSnapActive()` (`const step = isGridSnapActive() ? gridSnapStep : 0`). A snappable kind declares `NodeDefinition.snapProfile` (`'item' | 'structural'`) so its context, mode-set, and chip fall out - with no per-kind switch. See [interaction-scope](interaction-scope.md) § "Snapping mode & modifiers" - and `lib/snapping-mode.ts`. + with no per-kind switch. The contextual HUD renders the snapping chip for **any** tool that resolves + to a snap context — `helper-manager` gates the generic `RegisteredToolHelper` on `snapContext` (or + `continuationContext`), not on the presence of hand-written `def.toolHints`, so a snappable draft tool + with no bespoke hints (e.g. `zone`) still advertises the Shift = cycle control it already honors. See + [interaction-scope](interaction-scope.md) § "Snapping mode & modifiers" and `lib/snapping-mode.ts`. - **Constraints and guides can be decoupled.** When a stronger constraint owns the proposal — a wall segment's 45° lock while in `angles` mode — the tool may still publish passive dashed alignment/proximity guides as long as it does not apply the guide snap delta. Use this for chained @@ -151,6 +154,20 @@ The store name suggests a uniform contract; the writes in practice are not. Docu Anything that subscribes to `useLiveTransforms` to inform 2D rendering needs to handle these frames explicitly. The `FloorplanRegistryLayer` override currently branches by kind: `item` / `shelf` / `column` are treated as world-plan (it copies `live.position` onto the effective node and forces `parentId: null` so the resolver skips the parent-chain transform), while `slab` / `ceiling` / `zone` are treated as a polygon **delta** (it translates the polygon vertices by `live.position`). Each kind added to the live-drag path grows this consumer-side switch; the preferred long-term fix is to standardise the frame at the writer so the consumer stops branching by `node.type`. +## Data-driven live drag: `useLiveNodeOverrides`, never per-tick `useScene` + +`useLiveTransforms` (above) carries a rigid position/rotation offset — right when the renderer can preview the move by transforming the node's group. It's **wrong** when the geometry is *recomputed from data fields* (a wall re-miters from its `start`/`end`, an opening re-cuts its host wall, an endpoint drag reshapes the segment and cascades to linked walls): the shape itself changes, so there's no rigid offset to apply. Those preview via **`useLiveNodeOverrides`** (`@pascal-app/core`) — the tool publishes the changed fields per tick (`set(id, patch)` / `setMany(...)`) and the geometry systems merge them (`getEffectiveWall` in 3D, the floor-plan sibling-override merge in 2D, `getEffectiveNode` in panels). The scene store stays untouched during the drag; on commit the tool clears overrides and writes it **once** (`resumeSceneHistory → updateNodes([...]) → pauseSceneHistory`), so the gesture is a single undo step. Esc/unmount just clears overrides — cancel is free. + +**Writing `useScene.updateNodes`/`updateNode` per `grid:move` tick is a blocker:** it replaces the `nodes` map ref, so every `useScene(s => s.nodes)` subscriber app-wide (panels, HUD, tooltips, floor plan, catalog) re-renders each frame → FPS collapse. (`markDirty` per tick is fine — it never calls `set()`.) Reference: `packages/nodes/src/wall/{move-tool,move-endpoint-tool}.tsx`. + +## Floorplan registry: per-node subscriptions, stable props + +`FloorplanRegistryLayer` draws one `FloorplanRegistryEntry` per node. The perf invariant — a live drag must re-render only the changed node(s), not all ~150 entries — rests on three things, and breaking any of them is a re-render-flood regression that still type-checks and passes tests (see `floorplan-registry-layer.tsx`): + +- Each entry subscribes to **its own slice** — `useLiveTransforms(s => s.transforms.get(id))` / `useLiveNodeOverrides(s => s.overrides.get(id))`, never the whole Map. This works because the live stores write a fresh value only for the changed node (the Map is cloned but unchanged value refs are reused), so an unchanged node's selector stays identity-stable and Zustand skips it. The parent subscribes only to the stable id list. +- `FloorplanRegistryEntry` and `InteractiveGeometry` are `memo`'d, so the parent must pass **referentially stable props** (hoisted styles, `useCallback` handlers, memoized descriptors) — a fresh inline object/handler per entry defeats the memo. +- Sibling-dependent geometry (wall miters, opening cuts) invalidates via a **per-node sibling epoch** bumped from a store `subscribe` (`computeAffectedSiblingIds`), not a whole-layer re-render. + ## Wall-attached node rotations must be wall-local `door` / `window` / wall-attached `item` are children of the wall mesh in 3D. The wall's `mesh.rotation.y = -atan2(dy, dx)`. The child node's `rotation.y` therefore lives in the wall's local frame and composes with the wall's rotation at render time.