From a89a1efccfeb251a772354439c9ee3a042fdc684 Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Thu, 14 May 2026 14:27:08 -0400 Subject: [PATCH] Fix spawn flag inlining + shelf cursor frame + simpler renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three concrete bugs surfaced when first-running the spike in community: 1) NEXT_PUBLIC_USE_REGISTRY_FOR_SPAWN flag never detected: The previous readEnvFlag used dynamic bracket access (`env?.[name]`), which Next.js / Turbopack does NOT substitute at build time. Only literal `process.env.NEXT_PUBLIC_FOO` references get inlined into the client bundle. Switched to literal access plus a `typeof process` guard. Spawn now toggles via the flag as designed. 2) Shelf cursor appeared offset from the mouse: The cursor mesh lives inside the ToolManager's building-local group, but the tool was setting `cursorRef.current.position` to level-local coordinates (computed via `worldToLocal(level)`). Result: cursor shifted by (building-pos − level-pos) in worst case. Switched cursor display to use `event.localPosition` (already building-local) with grid snap — matches the legacy spawn-tool pattern. The commit path keeps the worldToLocal(level) conversion since the shelf node's `position` field is stored relative to its level parent. 3) Shelf rendered invisibly after click (suspected): The renderer used a useEffect-swap pattern where it mounted an empty and imperatively added Three.js children from a buildShelfGeometry() Group. Plausibly fragile under StrictMode double-invoke or fast HMR. Switched to inline R3F JSX — top board + brackets as plain primitives. The pure geometry function still exists in geometry.ts for tests and AI-authored consumers; renderer just doesn't go through it. Diagnostics added (dev-only; removed once spawn parity ships): - `[shelf] placed level-local parent ` on click - `[shelf] rendered at ` on mount Also: types: ["node"] in nodes/tsconfig.json so the typeof process guard typechecks cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/nodes/src/index.ts | 16 ++--- packages/nodes/src/shelf/renderer.tsx | 87 +++++++++++++++++---------- packages/nodes/src/shelf/tool.tsx | 29 ++++++--- packages/nodes/tsconfig.json | 3 +- 4 files changed, 83 insertions(+), 52 deletions(-) diff --git a/packages/nodes/src/index.ts b/packages/nodes/src/index.ts index fa392a65..e2474ee0 100644 --- a/packages/nodes/src/index.ts +++ b/packages/nodes/src/index.ts @@ -10,15 +10,15 @@ import { spawnDefinition } from './spawn' * Removed in the PR that signs off parity (legacy spawn files deleted in the * same commit). All other built-in node migrations follow the same pattern. */ -function readEnvFlag(name: string): boolean { - const env = (globalThis as { process?: { env?: Record } }).process - ?.env - const flag = env?.[name] - return flag === '1' || flag === 'true' -} - function isSpawnRegistryEnabled(): boolean { - return readEnvFlag('NEXT_PUBLIC_USE_REGISTRY_FOR_SPAWN') + // Next.js / Turbopack inlines `process.env.NEXT_PUBLIC_*` references at + // build time, but ONLY when the access is a literal property — dynamic + // bracket access (`env[name]`) is not substituted and resolves to + // undefined in the browser. Keep this as a literal so the value is baked + // into the client bundle. + if (typeof process === 'undefined') return false + const flag = process.env.NEXT_PUBLIC_USE_REGISTRY_FOR_SPAWN + return flag === '1' || flag === 'true' } function getBuiltinNodes(): AnyNodeDefinition[] { diff --git a/packages/nodes/src/shelf/renderer.tsx b/packages/nodes/src/shelf/renderer.tsx index 6d5933b4..773090d9 100644 --- a/packages/nodes/src/shelf/renderer.tsx +++ b/packages/nodes/src/shelf/renderer.tsx @@ -2,24 +2,23 @@ import { useLiveTransforms, useRegistry } from '@pascal-app/core' import { useEffect, useMemo, useRef } from 'react' -import type { Group } from 'three' -import { buildShelfGeometry } from './geometry' +import { Color, type Group } from 'three' import type { ShelfNode } from './schema' -// Note: `useNodeEvents` from @pascal-app/viewer has a hardcoded kind list and -// doesn't yet know about 'shelf'. Phase 4 generalizes it to consume the -// registry — until then, shelf selection works via R3F's default raycast -// (clicks bubble through the scene; the editor's selection manager handles -// them by hit-testing the registered Object3D). +// Note: useNodeEvents from @pascal-app/viewer has a hardcoded kind list and +// doesn't yet know about 'shelf'. Phase 4 generalizes it via the registry — +// until then, shelf selection works via R3F's default raycast (clicks bubble +// through; the editor's selection manager hit-tests the registered Object3D). /** - * Registry-driven shelf renderer. + * Registry-driven shelf renderer. Renders top board + brackets as inline R3F + * primitives so React owns the scene graph end-to-end — no imperative + * children swap. * - * The pure `buildShelfGeometry` function returns a Group of meshes. We mount - * an empty group, attach event handlers, register with `sceneRegistry`, and - * imperatively swap in the built geometry whenever the schema-relevant fields - * change. This pattern keeps the JSX trivial and centralizes parametric work - * in the pure function — better for AI authoring and easier to swap out. + * The pure `buildShelfGeometry` function in `./geometry.ts` produces the same + * shape outside of React (used by tests + reachable by AI-authored consumers + * that want a Three.js Group). Keeping both costs nothing because the shape + * primitives are tiny. */ const ShelfRenderer = ({ node }: { node: ShelfNode }) => { const ref = useRef(null!) @@ -27,27 +26,23 @@ const ShelfRenderer = ({ node }: { node: ShelfNode }) => { useRegistry(node.id, 'shelf', ref) - // Build a fresh Group each time the parametric fields change. - const built = useMemo( - () => buildShelfGeometry(node), - [node.width, node.depth, node.thickness, node.height, node.bracketStyle, node.color], - ) + const color = useMemo(() => new Color(node.color), [node.color]) + const topY = node.height + node.thickness / 2 + + // Bracket dimensions mirror buildShelfGeometry — keep these in sync if the + // geometry function evolves. Phase 4 may consolidate. + const inset = Math.min(0.12, node.width / 6) + const bracketHeight = Math.max(0.01, node.height) + const bracketWidth = + node.bracketStyle === 'industrial' + ? Math.max(0.04, node.depth * 0.2) + : Math.max(0.02, node.depth * 0.12) + const bracketDepth = node.bracketStyle === 'industrial' ? node.depth * 0.95 : node.depth * 0.7 - // Mount the built children under our group ref. Re-runs when `built` - // changes (parametric edit) or when the parent ref mounts. useEffect(() => { - const root = ref.current - if (!root) return - // Clear previous children. We don't dispose the buffer geometries here - // because they're owned by the previous `built` and were already - // discarded by React's reconciler when useMemo recomputed. - while (root.children.length > 0) { - root.remove(root.children[0]!) - } - for (const child of [...built.children]) { - root.add(child) - } - }, [built]) + // biome-ignore lint/suspicious/noConsole: dev-only verification log + console.info('[shelf] rendered', node.id, 'at', node.position) + }, [node.id, node.position]) return ( { ref={ref} rotation={liveTransform?.rotation ? [0, liveTransform.rotation, 0] : node.rotation} visible={node.visible} - /> + > + {/* Top board */} + + + + + + {/* Brackets (skipped for 'hidden' style) */} + {node.bracketStyle !== 'hidden' && ( + <> + + + + + + + + + + )} + ) } diff --git a/packages/nodes/src/shelf/tool.tsx b/packages/nodes/src/shelf/tool.tsx index b06bd047..48934624 100644 --- a/packages/nodes/src/shelf/tool.tsx +++ b/packages/nodes/src/shelf/tool.tsx @@ -3,8 +3,8 @@ import { emitter, type GridEvent, - sceneRegistry, ShelfNode, + sceneRegistry, snapPointToGrid, useScene, } from '@pascal-app/core' @@ -13,8 +13,17 @@ import { useEffect, useRef } from 'react' import { type Group, Vector3 } from 'three' const worldVector = new Vector3() -const GRID_STEP = 0.5 // match the editor's default placement grid +const GRID_STEP = 0.5 +/** + * Convert a click event into the shelf's commit position (level-local). The + * shelf node's `position` field is stored relative to its level parent, so + * we project the click point into the level's local frame before storing. + * + * Different from the cursor preview path: the cursor lives inside the + * ToolManager's building-local group and snaps to `event.localPosition` + * directly. This conversion only applies to the *committed* data. + */ function getLevelLocalPosition(levelId: string, event: GridEvent): [number, number, number] { const levelObject = sceneRegistry.nodes.get(levelId) if (!levelObject) { @@ -36,11 +45,12 @@ const ShelfTool = () => { if (!activeLevelId) return const onGridMove = (event: GridEvent) => { - // Imperative position update — no React state, so the component - // doesn't re-render. R3F-applied props would otherwise clobber the - // imperative `position.set` on the next render. - const next = getLevelLocalPosition(activeLevelId, event) - cursorRef.current?.position.set(next[0], next[1], next[2]) + // Cursor lives in the ToolManager's building-local group. Use + // `event.localPosition` (already building-local) so the visual cursor + // sits where the mouse hits the floor. Legacy spawn-tool does the + // same — don't apply worldToLocal here. + const [sx, sz] = snapPointToGrid([event.localPosition[0], event.localPosition[2]], GRID_STEP) + cursorRef.current?.position.set(sx, event.localPosition[1], sz) } const onGridClick = (event: GridEvent) => { @@ -52,6 +62,8 @@ const ShelfTool = () => { }) useScene.getState().createNode(shelf, activeLevelId) useViewer.getState().setSelection({ selectedIds: [shelf.id] }) + // biome-ignore lint/suspicious/noConsole: dev-only verification log + console.info('[shelf] placed', shelf.id, 'level-local', position, 'parent', activeLevelId) } emitter.on('grid:move', onGridMove) @@ -65,9 +77,6 @@ const ShelfTool = () => { if (!activeLevelId) return null - // Cursor preview — a translucent shelf-shaped slab. No `position` prop on - // the group; we move it imperatively via the ref so React re-renders don't - // reset it to the origin. return ( diff --git a/packages/nodes/tsconfig.json b/packages/nodes/tsconfig.json index 52df6093..43e40455 100644 --- a/packages/nodes/tsconfig.json +++ b/packages/nodes/tsconfig.json @@ -7,7 +7,8 @@ "composite": true, "incremental": true, "jsx": "react-jsx", - "lib": ["DOM", "ES2022"] + "lib": ["DOM", "ES2022"], + "types": ["node"] }, "include": ["src"], "exclude": ["node_modules", "dist", "**/*.test.ts", "**/*.test.tsx"],