diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index d740bd5c9..88e952d3d 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -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 === '')` 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 `` 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 === '')` 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. diff --git a/apps/editor/public/audios/sfx/paint_apply.mp3 b/apps/editor/public/audios/sfx/paint_apply.mp3 new file mode 100644 index 000000000..6fabae1f0 Binary files /dev/null and b/apps/editor/public/audios/sfx/paint_apply.mp3 differ diff --git a/packages/editor/src/components/editor/selection-manager.tsx b/packages/editor/src/components/editor/selection-manager.tsx index d1a42ee6d..b0416476e 100644 --- a/packages/editor/src/components/editor/selection-manager.tsx +++ b/packages/editor/src/components/editor/selection-manager.tsx @@ -1157,6 +1157,7 @@ export const SelectionManager = () => { } interaction.apply() + sfxEmitter.emit('sfx:paint-apply') if (activePreview?.key === interaction.key) { activePreview = null } else { diff --git a/packages/editor/src/components/tools/item/use-draft-node.ts b/packages/editor/src/components/tools/item/use-draft-node.ts index dfd752fb1..a53a47b90 100644 --- a/packages/editor/src/components/tools/item/use-draft-node.ts +++ b/packages/editor/src/components/tools/item/use-draft-node.ts @@ -27,12 +27,14 @@ export interface DraftNodeHandle { readonly current: ItemNode | null /** Whether the current draft was adopted (move mode) vs created (create mode) */ readonly isAdopted: boolean - /** Create a new draft item at the given position. Returns the created node or null. */ + /** Create a new draft item at the given position. Returns the created node or null. + * `slots` seeds painted slot overrides so duplicates keep their materials. */ create: ( gridPosition: Vector3, asset: AssetInput, rotation?: [number, number, number], scale?: [number, number, number], + slots?: ItemNode['slots'], ) => ItemNode | null /** Take ownership of an existing scene node as the draft (for move mode). */ adopt: (node: ItemNode) => void @@ -61,6 +63,7 @@ export function useDraftNode(): DraftNodeHandle { asset: AssetInput, rotation?: [number, number, number], scale?: [number, number, number], + slots?: ItemNode['slots'], ): ItemNode | null => { const currentLevelId = useViewer.getState().selection.levelId if (!currentLevelId) return null @@ -73,6 +76,7 @@ export function useDraftNode(): DraftNodeHandle { asset, parentId: currentLevelId, metadata: { isTransient: true }, + ...(slots ? { slots } : {}), }) useScene.getState().createNode(node, currentLevelId) @@ -180,6 +184,8 @@ export function useDraftNode(): DraftNodeHandle { rotation: updateProps.rotation ?? draft.rotation, scale: updateProps.scale ?? draft.scale, side: updateProps.side ?? draft.side, + // Carry painted slot overrides so a duplicated item keeps its materials. + ...(draft.slots ? { slots: draft.slots } : {}), // Roof host — see the move-mode commit above for why this must be // forwarded explicitly. roofSegmentId: updateProps.roofSegmentId, diff --git a/packages/editor/src/components/tools/item/use-placement-coordinator.tsx b/packages/editor/src/components/tools/item/use-placement-coordinator.tsx index dbe354146..4384da2a2 100644 --- a/packages/editor/src/components/tools/item/use-placement-coordinator.tsx +++ b/packages/editor/src/components/tools/item/use-placement-coordinator.tsx @@ -192,6 +192,9 @@ export interface PlacementCoordinatorConfig { initialState?: PlacementState /** Scale to use when lazily creating a draft (e.g. for wall/ceiling duplicates). Defaults to [1,1,1]. */ defaultScale?: [number, number, number] + /** Painted slot overrides to seed onto a lazily-created draft (wall/ceiling + * duplicates) so the duplicate keeps its materials. */ + slots?: ItemNode['slots'] /** Move-mode sessions keep the grabbed item offset from the first surface hit * (floor / wall / ceiling / item-surface / shelf) instead of snapping the * item's origin under the cursor. */ @@ -512,7 +515,13 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea 0, ] - draftNode.create(gridPosition.current, asset, initRotation, configRef.current.defaultScale) + draftNode.create( + gridPosition.current, + asset, + initRotation, + configRef.current.defaultScale, + configRef.current.slots, + ) const draft = draftNode.current if (draft) { @@ -857,7 +866,13 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea draftNode.commit(result.nodeUpdate) if (configRef.current.onCommitted()) { - draftNode.create(gridPosition.current, asset, currentRotation) + draftNode.create( + gridPosition.current, + asset, + currentRotation, + configRef.current.defaultScale, + configRef.current.slots, + ) const previewBounds = expandBoundsToGrid( getFallbackPreviewBounds(draftNode.current, asset, asset.attachTo), asset.attachTo, diff --git a/packages/editor/src/lib/sfx-bus.ts b/packages/editor/src/lib/sfx-bus.ts index ff5556120..262525d56 100644 --- a/packages/editor/src/lib/sfx-bus.ts +++ b/packages/editor/src/lib/sfx-bus.ts @@ -16,6 +16,7 @@ type SFXEvents = { 'sfx:snapshot-capture': undefined 'sfx:menu-hover': undefined 'sfx:menu-click': undefined + 'sfx:paint-apply': undefined } /** @@ -45,6 +46,7 @@ export function initSFXBus() { sfxEmitter.on('sfx:snapshot-capture', () => playSFX('snapshotCapture')) sfxEmitter.on('sfx:menu-hover', () => playSFX('menuHover')) sfxEmitter.on('sfx:menu-click', () => playSFX('menuClick')) + sfxEmitter.on('sfx:paint-apply', () => playSFX('paintApply')) } /** diff --git a/packages/editor/src/lib/sfx-player.ts b/packages/editor/src/lib/sfx-player.ts index 5106c7c7e..f4d12e65f 100644 --- a/packages/editor/src/lib/sfx-player.ts +++ b/packages/editor/src/lib/sfx-player.ts @@ -100,6 +100,15 @@ export const SFX: Record = { volumeRange: [0.5, 0.6], panJitter: 0.1, }, + // Fired when a material is applied to a surface in paint mode. Painting can + // fire in quick succession across faces, so keep variation + a small gap. + paintApply: { + src: '/audios/sfx/paint_apply.mp3', + rateRange: [0.95, 1.05], + volumeRange: [0.85, 1.0], + panJitter: 0.12, + minIntervalMs: 60, + }, } as const export type SFXName = keyof typeof SFX diff --git a/packages/nodes/src/item/move-tool.tsx b/packages/nodes/src/item/move-tool.tsx index f32c7ce43..852cd6c49 100644 --- a/packages/nodes/src/item/move-tool.tsx +++ b/packages/nodes/src/item/move-tool.tsx @@ -115,6 +115,9 @@ export function MoveItemTool({ node }: { node: ItemNode }) { const cursor = usePlacementCoordinator({ asset: node.asset, draftNode, + // Carry painted slot overrides onto the duplicate's draft (wall/ceiling + // items create their draft lazily inside the coordinator). + slots: node.slots, // Duplicates start fresh in floor mode; wall/ceiling draft is created lazily by ensureDraft. initialState: isNew ? { @@ -135,7 +138,7 @@ export function MoveItemTool({ node }: { node: ItemNode }) { // items are created lazily on surface entry. gridPosition.copy(new Vector3(...node.position)) if (!node.asset.attachTo) { - draftNode.create(gridPosition, node.asset, node.rotation, node.scale) + draftNode.create(gridPosition, node.asset, node.rotation, node.scale, node.slots) } } else { draftNode.adopt(node)