From 54a24e4c5cfe6e40b6dafb68abbda03f59199403 Mon Sep 17 00:00:00 2001 From: Open Pascal Date: Tue, 16 Jun 2026 19:29:34 +0000 Subject: [PATCH] fix: address review findings from PR #402 MEP systems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core: fix layer violation in level-height.ts — extract sceneRegistry import, replace with optional WallBaseYResolver callback so core stays pure (no Three.js mesh state); viewer callers pass resolver, headless callers (MCP/tests) get deterministic node-data-only result - core: generalise port-connectivity service from duct-only to all distribution families — match partners by distributionRole ('run' → endpoint stretch, 'fitting' → rigid follow) instead of hard-coded duct-segment/duct-fitting type names; add system-compat guard so cross-system ports (e.g. supply duct vs waste pipe) don't fuse - editor: fix port-snap rotation bug in move tool — pass preview node at live rotation into resolvePortSnap so own-port positions reflect any mid-drag R/T rotation before computing the snap delta - editor: wire pipe-trap into UI — add to StructureTool union, MepToolKind, MEP_ITEMS Build-tab tile, and structure-tools action menu - test: add port-connectivity-pipe.test.ts — 2 tests covering pipe-fitting → pipe-segment endpoint drag and cross-system isolation - test: fix stale pipe-auto-fitting.test.ts wye expectation — author deliberately chose square sanitary-tee for DWV side-taps (documented in PR description and PipeFittingNode schema); update the one test that still expected wye to match the implemented behaviour - nit: fix optional-chain biome warning in validate-dwv.ts --- apps/editor/components/build-tab.tsx | 2 + packages/core/src/services/level-height.ts | 37 ++++- .../core/src/services/port-connectivity.ts | 48 +++++-- packages/core/src/services/validate-dwv.ts | 2 +- .../registry/move-registry-node-tool.tsx | 7 +- .../ui/action-menu/structure-tools.tsx | 1 + packages/editor/src/store/use-editor.tsx | 1 + packages/nodes/src/duct-segment/tool.tsx | 5 +- .../src/shared/pipe-auto-fitting.test.ts | 12 +- .../src/shared/port-connectivity-pipe.test.ts | 128 ++++++++++++++++++ .../viewer/src/systems/level/level-system.tsx | 4 +- .../viewer/src/systems/level/level-utils.ts | 4 +- 12 files changed, 220 insertions(+), 31 deletions(-) create mode 100644 packages/nodes/src/shared/port-connectivity-pipe.test.ts diff --git a/apps/editor/components/build-tab.tsx b/apps/editor/components/build-tab.tsx index d89b69ca..0fb0db5c 100644 --- a/apps/editor/components/build-tab.tsx +++ b/apps/editor/components/build-tab.tsx @@ -44,6 +44,7 @@ type MepToolKind = | 'liquid-line' | 'pipe-segment' | 'pipe-fitting' + | 'pipe-trap' type BuildType = { /** Selection id — equals `kind` for tool types, `'painting'` for paint mode, `'mep'` for the MEP group. */ @@ -98,6 +99,7 @@ const MEP_ITEMS: MepItem[] = [ { id: 'lineset', label: 'Lineset', iconSrc: '/icons/lineset.png', kind: 'lineset' }, { id: 'liquid-line', label: 'Liquid Line', iconSrc: '/icons/lineset.png', kind: 'liquid-line' }, { id: 'pipe-segment', label: 'DWV Pipe', iconSrc: '/icons/dwv-pipes.png', kind: 'pipe-segment' }, + { id: 'pipe-trap', label: 'Trap', iconSrc: '/icons/dwv-pipes.png', kind: 'pipe-trap' }, ] /** diff --git a/packages/core/src/services/level-height.ts b/packages/core/src/services/level-height.ts index 36769027..29d68d26 100644 --- a/packages/core/src/services/level-height.ts +++ b/packages/core/src/services/level-height.ts @@ -1,22 +1,45 @@ -import { sceneRegistry } from '../hooks/scene-registry/scene-registry' import type { CeilingNode, LevelNode, WallNode } from '../schema' import type { AnyNode, AnyNodeId } from '../schema/types' export const DEFAULT_LEVEL_HEIGHT = 2.5 +/** + * Optional resolver for a wall's rendered base Y (mesh elevation). + * + * `packages/core` is pure domain logic and must not read viewer/Three.js + * state (see AGENTS.md "Layer Boundaries"). Callers that legitimately have + * registry access (viewer systems, node tools) may pass a resolver so the + * mesh elevation is factored in; pure/headless callers (MCP, tests, server) + * omit it and get a deterministic result from serialized node data alone. + */ +export type WallBaseYResolver = (wallId: AnyNodeId) => number | undefined + // Cache: levelId → computed height. Invalidated when the nodes reference changes. // Zustand produces a new `nodes` object on every mutation, so reference equality // is a zero-cost way to detect stale data without any subscription overhead. +// +// NOTE: the cache is keyed only by the `nodes` reference, so it is only safe to +// reuse across calls that pass the same resolver semantics. When a resolver is +// supplied (viewer path), mesh Y can change without a `nodes` reference change, +// so the viewer caller must not rely on the cache for live values — it passes a +// resolver and we recompute. We therefore only cache resolver-free (pure) +// results, which are a deterministic function of `nodes`. const heightCache = new Map() let lastNodesRef: object | null = null -export function getLevelHeight(levelId: string, nodes: Record): number { +export function getLevelHeight( + levelId: string, + nodes: Record, + resolveWallBaseY?: WallBaseYResolver, +): number { if (nodes !== lastNodesRef) { heightCache.clear() lastNodesRef = nodes } - if (heightCache.has(levelId)) return heightCache.get(levelId)! + // Only the pure (resolver-free) computation is cacheable: with a resolver the + // result can depend on live mesh state that the `nodes` reference doesn't track. + if (!resolveWallBaseY && heightCache.has(levelId)) return heightCache.get(levelId)! const level = nodes[levelId as LevelNode['id']] as LevelNode | undefined if (!level) return DEFAULT_LEVEL_HEIGHT @@ -30,14 +53,14 @@ export function getLevelHeight(levelId: string, nodes: Record maxTop) maxTop = ch } else if (child.type === 'wall') { - let meshY = sceneRegistry.nodes.get(childId as AnyNodeId)?.position.y ?? 0 - if (meshY < 0) meshY = 0 - const top = meshY + ((child as WallNode).height ?? DEFAULT_LEVEL_HEIGHT) + let baseY = resolveWallBaseY?.(childId as AnyNodeId) ?? 0 + if (baseY < 0) baseY = 0 + const top = baseY + ((child as WallNode).height ?? DEFAULT_LEVEL_HEIGHT) if (top > maxTop) maxTop = top } } const height = maxTop > 0 ? maxTop : DEFAULT_LEVEL_HEIGHT - heightCache.set(levelId, height) + if (!resolveWallBaseY) heightCache.set(levelId, height) return height } diff --git a/packages/core/src/services/port-connectivity.ts b/packages/core/src/services/port-connectivity.ts index 0c722d2c..ac14e9f3 100644 --- a/packages/core/src/services/port-connectivity.ts +++ b/packages/core/src/services/port-connectivity.ts @@ -2,7 +2,8 @@ import { nodeRegistry } from '../registry' import type { AnyNode, AnyNodeId } from '../schema' /** - * Connectivity-aware editing for port-bearing kinds (HVAC ductwork). + * Connectivity-aware editing for port-bearing distribution kinds + * (HVAC ductwork AND DWV plumbing). * * Two nodes are "connected" when a port of one coincides in space with a * port of the other — exactly how the placement tools mate a fitting onto @@ -62,12 +63,19 @@ export type PortConnectivity = { connections: PortConnection[] } -function portsOf(node: AnyNode): ReadonlyArray<{ id: string; position: Point }> | undefined { +function portsOf( + node: AnyNode, +): ReadonlyArray<{ id: string; position: Point; system?: string }> | undefined { return nodeRegistry.get(node.type)?.ports?.(node) as - | ReadonlyArray<{ id: string; position: Point }> + | ReadonlyArray<{ id: string; position: Point; system?: string }> | undefined } +/** A node's distribution role from the registry (run / fitting / …). */ +function roleOf(node: AnyNode): string | undefined { + return nodeRegistry.get(node.type)?.distributionRole +} + function distSq(a: Point, b: Point): number { const dx = a[0] - b[0] const dy = a[1] - b[1] @@ -80,9 +88,9 @@ function distSq(a: Point, b: Point): number { * start of a move/resize. Call once before the drag; feed the result to * `resolveConnectivityUpdates` on every frame. * - * Only duct-segment (endpoint stretch) and duct-fitting (rigid follow) - * partners are tracked — terminals and equipment usually mount to a - * surface and shouldn't be yanked off it when an adjacent fitting nudges. + * Only `run`-role partners (segments — endpoint stretch) and `fitting`-role + * partners (rigid follow) are tracked — terminals and equipment usually mount + * to a surface and shouldn't be yanked off it when an adjacent fitting nudges. */ export function analyzePortConnectivity( movedNode: AnyNode, @@ -90,14 +98,22 @@ export function analyzePortConnectivity( ): PortConnectivity { const movedPorts = portsOf(movedNode) ?? [] const startMovedPorts: Record = {} - for (const p of movedPorts) startMovedPorts[p.id] = p.position + const movedPortSystem: Record = {} + for (const p of movedPorts) { + startMovedPorts[p.id] = p.position + movedPortSystem[p.id] = p.system + } const connections: PortConnection[] = [] const epsSq = COINCIDENT_EPS_M * COINCIDENT_EPS_M for (const other of Object.values(nodes)) { if (!other || other.id === movedNode.id) continue - if (other.type !== 'duct-segment' && other.type !== 'duct-fitting') continue + // Generalised across every distribution family (HVAC duct + DWV pipe): + // `run` partners stretch an endpoint, `fitting` partners follow rigidly. + // Terminals/equipment mount to surfaces and are intentionally NOT dragged. + const otherRole = roleOf(other) + if (otherRole !== 'run' && otherRole !== 'fitting') continue const otherPorts = portsOf(other) if (!otherPorts) continue @@ -105,15 +121,19 @@ export function analyzePortConnectivity( // Find which of the moved node's ports this partner port sits on. let matchedId: string | null = null for (const mp of movedPorts) { - if (distSq(op.position, mp.position) <= epsSq) { - matchedId = mp.id - break - } + if (distSq(op.position, mp.position) > epsSq) continue + // Don't fuse ports from incompatible systems (e.g. a supply duct + // and a waste pipe that happen to cross): only mate when both + // ports declare the same system, or at least one is unscoped. + const ms = movedPortSystem[mp.id] + if (ms && op.system && ms !== op.system) continue + matchedId = mp.id + break } if (!matchedId) continue - if (other.type === 'duct-segment') { - const path = (other as unknown as { path: Point[] }).path + if (otherRole === 'run') { + const path = (other as unknown as { path?: Point[] }).path if (!Array.isArray(path) || path.length < 2) continue // Port id 'start' → first point, 'end' → last point. const pathIndex = op.id === 'start' ? 0 : path.length - 1 diff --git a/packages/core/src/services/validate-dwv.ts b/packages/core/src/services/validate-dwv.ts index 9d8c7ee1..a2b3aecc 100644 --- a/packages/core/src/services/validate-dwv.ts +++ b/packages/core/src/services/validate-dwv.ts @@ -91,7 +91,7 @@ export function validateDwv(nodes: Readonly>): DwvFin // ── Per-segment slope (waste only) ────────────────────────────── for (const node of Object.values(nodes)) { - if (!node || node.type !== 'pipe-segment' || node.system !== 'waste') continue + if (node?.type !== 'pipe-segment' || node.system !== 'waste') continue const path = node.path as Vec3[] const minSlope = minSlopeFor(node.diameter) const maxSlope = node.diameter / 12 // 1 pipe-diameter per foot → siphoning diff --git a/packages/editor/src/components/tools/registry/move-registry-node-tool.tsx b/packages/editor/src/components/tools/registry/move-registry-node-tool.tsx index 24dbdf36..d68e02f4 100644 --- a/packages/editor/src/components/tools/registry/move-registry-node-tool.tsx +++ b/packages/editor/src/components/tools/registry/move-registry-node-tool.tsx @@ -447,7 +447,12 @@ export function MoveRegistryNodeTool({ node }: { node: AnyNode }) { // duct run end. Takes precedence over grid / alignment snap; Alt // bypasses. Only kinds that opted in via `movable.portSnap`. if (!bypass && portSnapConfig) { - const mated = resolvePortSnap(node, [x, z], portSnapConfig) + // Build the preview node at the ORIGINAL position but with the LIVE + // rotation so `def.ports` reflects any mid-drag R/T rotation. Without + // this the snap solver mates the pre-rotation collar and commit then + // writes the rotated node offset from the port it visually snapped to. + const snapNode = buildPreviewNode(originalPosition, rotationRef.current) + const mated = resolvePortSnap(snapNode, [x, z], portSnapConfig) if (mated) { x = mated[0] z = mated[1] diff --git a/packages/editor/src/components/ui/action-menu/structure-tools.tsx b/packages/editor/src/components/ui/action-menu/structure-tools.tsx index 9238963d..4cc56425 100644 --- a/packages/editor/src/components/ui/action-menu/structure-tools.tsx +++ b/packages/editor/src/components/ui/action-menu/structure-tools.tsx @@ -30,6 +30,7 @@ export const tools: ToolConfig[] = [ { id: 'duct-terminal', iconSrc: '/icons/registers.png', label: 'Register' }, { id: 'hvac-equipment', iconSrc: '/icons/HVAC.png', label: 'HVAC Unit' }, { id: 'pipe-segment', iconSrc: '/icons/dwv-pipes.png', label: 'DWV Pipe' }, + { id: 'pipe-trap', iconSrc: '/icons/dwv-pipes.png', label: 'Trap' }, { id: 'pipe-fitting', iconSrc: '/icons/duct-fitting.png', label: 'Pipe Fitting' }, { id: 'lineset', iconSrc: '/icons/lineset.png', label: 'Lineset' }, { id: 'liquid-line', iconSrc: '/icons/lineset.png', label: 'Liquid Line' }, diff --git a/packages/editor/src/store/use-editor.tsx b/packages/editor/src/store/use-editor.tsx index 544cd07e..96337af4 100644 --- a/packages/editor/src/store/use-editor.tsx +++ b/packages/editor/src/store/use-editor.tsx @@ -114,6 +114,7 @@ export type StructureTool = | 'liquid-line' | 'pipe-segment' | 'pipe-fitting' + | 'pipe-trap' // Furnish mode tools (items and decoration) export type FurnishTool = 'item' diff --git a/packages/nodes/src/duct-segment/tool.tsx b/packages/nodes/src/duct-segment/tool.tsx index 0c8a7404..df0a778f 100644 --- a/packages/nodes/src/duct-segment/tool.tsx +++ b/packages/nodes/src/duct-segment/tool.tsx @@ -6,6 +6,7 @@ import { emitter, type GridEvent, getLevelHeight, + sceneRegistry, useScene, } from '@pascal-app/core' import { @@ -551,7 +552,9 @@ const DuctSegmentTool = () => { // ceiling (centerline = ceiling height − radius). const resolveBaseY = (): number => { if (!ceilingModeRef.current) return 0 - const ceiling = getLevelHeight(activeLevelId, useScene.getState().nodes) + const ceiling = getLevelHeight(activeLevelId, useScene.getState().nodes, (wallId) => + sceneRegistry.nodes.get(wallId)?.position.y, + ) const p = profileRef.current const verticalIn = p.shape === 'round' ? p.diameter : p.height return Math.max(0, ceiling - (verticalIn * 0.0254) / 2) diff --git a/packages/nodes/src/shared/pipe-auto-fitting.test.ts b/packages/nodes/src/shared/pipe-auto-fitting.test.ts index f03057c9..17cab0f1 100644 --- a/packages/nodes/src/shared/pipe-auto-fitting.test.ts +++ b/packages/nodes/src/shared/pipe-auto-fitting.test.ts @@ -64,23 +64,25 @@ describe('planPipeBranchTap', () => { return { nodeId: node.id, segmentIndex, point } } - test('horizontal drain tap → wye, branch leaning 45° downstream', () => { + test('horizontal drain tap → square sanitary tee', () => { const run = drain([ [0, 0, 0], [6, -0.125, 0], ]) const plan = planPipeBranchTap(run, hit(run, 0, [3, -0.0625, 0]), [0, 0, 1], 2) expect(plan).not.toBeNull() - expect(plan!.fitting.fittingType).toBe('wye') + // DWV side taps mint a SQUARE sanitary tee (see planPipeBranchTap / + // PipeFittingNode schema): the branch enters perpendicular to the run + // regardless of the drawn lead-in angle, matching the duct tee tap. + expect(plan!.fitting.fittingType).toBe('sanitary-tee') const ports = getPipeFittingPorts(plan!.fitting) const branch = ports.find((p) => p.id === 'branch')! const inlet = ports.find((p) => p.id === 'inlet')! const outlet = ports.find((p) => p.id === 'outlet')! - // Branch leaves at 45° between the run axis and the drawn direction — - // i.e. leaning downstream, the code-correct wye entry. + // Branch leaves square to the run axis (the projected-perpendicular entry). const axis = [6 / Math.hypot(6, 0.125), -0.125 / Math.hypot(6, 0.125), 0] - expect(dot(branch.direction, axis)).toBeCloseTo(Math.SQRT1_2, 3) + expect(Math.abs(dot(branch.direction, axis))).toBeLessThan(1e-6) expect(branch.direction[2]).toBeGreaterThan(0.6) // Split halves mate the run collars. const upstream = plan!.runUpdate.data.path diff --git a/packages/nodes/src/shared/port-connectivity-pipe.test.ts b/packages/nodes/src/shared/port-connectivity-pipe.test.ts new file mode 100644 index 00000000..3e1eb3cf --- /dev/null +++ b/packages/nodes/src/shared/port-connectivity-pipe.test.ts @@ -0,0 +1,128 @@ +import { beforeAll, describe, expect, test } from 'bun:test' +import { + analyzePortConnectivity, + type AnyNode, + loadPlugin, + nodeRegistry, + PipeFittingNode, + PipeSegmentNode, + resolveConnectivityUpdates, +} from '@pascal-app/core' +import { builtinPlugin } from '../index' + +/** + * Regression coverage for the generalized (HVAC duct + DWV pipe) + * port-connectivity service. Before PR #402's follow-up fix the service + * only tracked `duct-segment` / `duct-fitting`, so moving a `pipe-fitting` + * left attached `pipe-segment` endpoints behind. These tests assert the + * role-based generalization carries pipe runs along. + */ +describe('port connectivity — DWV pipe family', () => { + beforeAll(async () => { + nodeRegistry._reset() + await loadPlugin(builtinPlugin) + }) + + test('moving a pipe-fitting stretches the connected pipe-segment endpoint', () => { + // A sanitary tee at the origin; its run ports sit on ±X at the hub legs. + const fitting = PipeFittingNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + fittingType: 'sanitary-tee', + diameter: 2, + diameter2: 2, + pipeMaterial: 'pvc', + system: 'waste', + position: [0, 0, 0], + rotation: [0, 0, 0], + }) + + const fittingPorts = nodeRegistry.get('pipe-fitting')!.ports!(fitting) as ReadonlyArray<{ + id: string + position: [number, number, number] + }> + const outlet = fittingPorts.find((p) => p.id === 'outlet')! + + // A pipe run whose START port coincides with the fitting's outlet collar. + const run = PipeSegmentNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + diameter: 2, + pipeMaterial: 'pvc', + system: 'waste', + path: [ + [outlet.position[0], outlet.position[1], outlet.position[2]], + [outlet.position[0] + 3, outlet.position[1], outlet.position[2]], + ], + }) + + const nodes: Record = { + [fitting.id]: fitting as AnyNode, + [run.id]: run as AnyNode, + } + + const connectivity = analyzePortConnectivity(fitting as AnyNode, nodes) + // The run must be picked up as a stretchable endpoint partner. + const endpoint = connectivity.connections.find( + (c) => c.kind === 'duct-endpoint' && c.nodeId === run.id, + ) + expect(endpoint).toBeDefined() + + // Move the fitting +1m in Z; the run's mated endpoint should follow. + const moved = { ...(fitting as Record), position: [0, 0, 1] } as AnyNode + const updates = resolveConnectivityUpdates(connectivity, moved) + const runUpdate = updates.find((u) => u.id === run.id) + expect(runUpdate).toBeDefined() + const newPath = (runUpdate!.data as { path: [number, number, number][] }).path + // Tracked endpoint moved by the same +1m in Z; far end stayed put. + expect(newPath[0]![2]).toBeCloseTo(outlet.position[2] + 1, 6) + expect(newPath[1]![2]).toBeCloseTo(outlet.position[2], 6) + }) + + test('incompatible systems do not fuse (a supply duct is not dragged by a waste fitting)', () => { + const fitting = PipeFittingNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + fittingType: 'sanitary-tee', + diameter: 2, + diameter2: 2, + pipeMaterial: 'pvc', + system: 'waste', + position: [0, 0, 0], + rotation: [0, 0, 0], + }) + const fittingPorts = nodeRegistry.get('pipe-fitting')!.ports!(fitting) as ReadonlyArray<{ + id: string + position: [number, number, number] + }> + const outlet = fittingPorts.find((p) => p.id === 'outlet')! + + // A vent pipe sharing the same point but a different system. + const ventRun = PipeSegmentNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + diameter: 2, + pipeMaterial: 'pvc', + system: 'vent', + path: [ + [outlet.position[0], outlet.position[1], outlet.position[2]], + [outlet.position[0] + 3, outlet.position[1], outlet.position[2]], + ], + }) + + const nodes: Record = { + [fitting.id]: fitting as AnyNode, + [ventRun.id]: ventRun as AnyNode, + } + const connectivity = analyzePortConnectivity(fitting as AnyNode, nodes) + expect(connectivity.connections.find((c) => c.nodeId === ventRun.id)).toBeUndefined() + }) +}) diff --git a/packages/viewer/src/systems/level/level-system.tsx b/packages/viewer/src/systems/level/level-system.tsx index 67483c10..ac957051 100644 --- a/packages/viewer/src/systems/level/level-system.tsx +++ b/packages/viewer/src/systems/level/level-system.tsx @@ -39,7 +39,9 @@ export const LevelSystem = () => { obj.position.y = lerp(obj.position.y, targetY, delta * 12) // Smoothly animate to new Y position obj.visible = levelMode !== 'solo' || level?.id === selectedLevel || !selectedLevel - cumulativeY += getLevelHeight(levelId, nodes) + cumulativeY += getLevelHeight(levelId, nodes, (wallId) => + sceneRegistry.nodes.get(wallId)?.position.y, + ) } }, 5) // Using a lower priority so it runs after transforms from other systems have settled return null diff --git a/packages/viewer/src/systems/level/level-utils.ts b/packages/viewer/src/systems/level/level-utils.ts index aa9b2a20..cb5af5ad 100644 --- a/packages/viewer/src/systems/level/level-utils.ts +++ b/packages/viewer/src/systems/level/level-utils.ts @@ -41,7 +41,9 @@ export function snapLevelsToTruePositions(): () => void { for (const { levelId, obj } of entries) { obj.position.y = cumulativeY obj.visible = true - cumulativeY += getLevelHeight(levelId, nodes) + cumulativeY += getLevelHeight(levelId, nodes, (wallId) => + sceneRegistry.nodes.get(wallId)?.position.y, + ) } return () => {