fix: address review findings from PR #402 MEP systems

- 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
2026-06-16 19:30:51 +00:00
parent 5551500d98
commit 54a24e4c5c
12 changed files with 220 additions and 31 deletions
@@ -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<string, number>()
 let lastNodesRef: object | null = null

-export function getLevelHeight(levelId: string, nodes: Record<AnyNodeId, AnyNode>): number {
+export function getLevelHeight(
+  levelId: string,
+  nodes: Record<AnyNodeId, AnyNode>,
+  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<AnyNodeId, AnyNode
      const ch = (child as CeilingNode).height ?? DEFAULT_LEVEL_HEIGHT
      if (ch > 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
 }
@@ -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<string, Point> = {}
-  for (const p of movedPorts) startMovedPorts[p.id] = p.position
+  const movedPortSystem: Record<string, string | undefined> = {}
+  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
@@ -91,7 +91,7 @@ export function validateDwv(nodes: Readonly<Record<AnyNodeId, AnyNode>>): 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
@@ -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]
@@ -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' },
@@ -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'
@@ -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)
@@ -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
@@ -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<string, AnyNode> = {
+      [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<string, unknown>), 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<string, AnyNode> = {
+      [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()
+  })
+})
@@ -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
@@ -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 () => {