diff --git a/packages/viewer/src/components/viewer/index.tsx b/packages/viewer/src/components/viewer/index.tsx index 0153b087e..e28dfca10 100644 --- a/packages/viewer/src/components/viewer/index.tsx +++ b/packages/viewer/src/components/viewer/index.tsx @@ -25,6 +25,7 @@ import type { ColorPreset, RenderShading } from '../../lib/materials' import { getSceneTheme } from '../../lib/scene-themes' import useViewer, { type RenderContext } from '../../store/use-viewer' import { FloorElevationSystem } from '../../systems/floor-elevation/floor-elevation-system' +import { GeometryDisposalFlushSystem } from '../../systems/geometry/geometry-disposal-flush' import { GeometrySystem } from '../../systems/geometry/geometry-system' import { ErrorBoundary } from '../error-boundary' import { SceneRenderer } from '../renderers/scene-renderer' @@ -541,6 +542,12 @@ const Viewer = forwardRef(function Viewer( )} + {/* Single authoritative flush point for the deferred geometry-disposal + queue. Runs at a strongly negative frame priority so it drains the + queue before ANY rebuild system runs this frame, guaranteeing only + geometries dropped on a previous frame (already released by the + renderer) are disposed. See lib/deferred-dispose.ts. */} + {/* Generic slab-elevation lift for any kind that declares `capabilities.floorPlaced`. Runs at frame priority 1 so it lands its mesh.position.y override before the priority-2 diff --git a/packages/viewer/src/lib/deferred-dispose.ts b/packages/viewer/src/lib/deferred-dispose.ts new file mode 100644 index 000000000..005c888b6 --- /dev/null +++ b/packages/viewer/src/lib/deferred-dispose.ts @@ -0,0 +1,62 @@ +import type { BufferGeometry } from 'three' + +/** + * WebGPU dispose-race mitigation. + * + * Disposing a `BufferGeometry` that is *still referenced by a live, rendered + * mesh* synchronously inside a `useFrame` rebuild loop is unsafe on the + * WebGPU backend. Three's WebGPU renderer keeps a `RenderObject` that holds + * the geometry's attributes; `BufferGeometry.dispose()` synchronously fires + * the `dispose` event, and the renderer's handler later reads `.id` / `.count` + * off attributes that have already been freed by the time the render pass + * unwinds — throwing `Cannot read properties of undefined (reading 'id')` + * (Sentry MONOREPO-EDITOR-DK / EG / EH). + * + * The renderer releases its `RenderObject` for a geometry once that geometry + * is no longer drawn (i.e. after the frame in which we swapped in the new + * geometry). So instead of disposing the *outgoing* live geometry mid-frame, + * we hand it to this queue and flush it at the START of the next frame, after + * the render pass that dropped it has completed. Behavior is identical — + * every geometry is still freed, just one frame later, after the renderer has + * let go of it. + * + * This is deliberately module-global (not per-system): every geometry system + * shares one flush point, and the queue is drained wholesale each frame. Only + * use `queueGeometryDispose` for geometries that were attached to a *rendered* + * mesh. Transient CSG intermediates that never entered the scene graph should + * still be disposed synchronously — the renderer never held a RenderObject for + * them, so there is no race. + */ +const pendingGeometryDisposals = new Set() + +/** + * Queue a live geometry for disposal after the current render. Safe to call + * with the same geometry more than once (Set-deduped). The mesh should already + * have been detached / reassigned to a fresh geometry before calling this. + */ +export function queueGeometryDispose(geometry: BufferGeometry | null | undefined): void { + if (!geometry) return + if (typeof (geometry as { dispose?: () => void }).dispose !== 'function') return + pendingGeometryDisposals.add(geometry) +} + +/** + * Dispose everything queued since the last flush. Called once per frame by + * `GeometryDisposalFlushSystem` at a negative `useFrame` priority, before any + * rebuild system runs, so the renderer has had a full frame to release the + * outgoing geometries' RenderObjects. Do not call this from a rebuild + * system's `useFrame` — doing so risks disposing geometries queued by another + * system in the same frame, before the render pass. + */ +export function flushGeometryDisposals(): void { + if (pendingGeometryDisposals.size === 0) return + for (const geometry of pendingGeometryDisposals) { + try { + geometry.dispose() + } catch { + // A geometry may already be torn down (scene unload, HMR); freeing it + // twice is harmless and must never break the frame loop. + } + } + pendingGeometryDisposals.clear() +} diff --git a/packages/viewer/src/systems/geometry/geometry-disposal-flush.tsx b/packages/viewer/src/systems/geometry/geometry-disposal-flush.tsx new file mode 100644 index 000000000..04bea3504 --- /dev/null +++ b/packages/viewer/src/systems/geometry/geometry-disposal-flush.tsx @@ -0,0 +1,30 @@ +import { useFrame } from '@react-three/fiber' +import { flushGeometryDisposals } from '../../lib/deferred-dispose' + +/** + * Single, authoritative flush point for the deferred geometry-disposal queue + * (Sentry MONOREPO-EDITOR-DK/EG/EH). + * + * Correctness depends on the flush running *before any rebuild system queues a + * new disposal in the same frame*. If a rebuild system (GeometrySystem, + * RoofSystem, …) both flushed and queued from its own `useFrame`, a system + * running later in the frame could flush geometries that an earlier system had + * just queued *this* frame — disposing them before the render pass and + * reintroducing the very race deferral is meant to prevent. + * + * R3F runs `useFrame` callbacks in ascending `priority` order (ties broken by + * mount order). Running this flush at a strongly negative priority guarantees + * it executes ahead of every rebuild system every frame, regardless of mount + * order, so the queue only ever contains geometries dropped on a *previous* + * frame — which the renderer has already released. This must be the ONLY + * caller of `flushGeometryDisposals`. + */ +const FLUSH_PRIORITY = -1000 + +export const GeometryDisposalFlushSystem = () => { + useFrame(() => { + flushGeometryDisposals() + }, FLUSH_PRIORITY) + + return null +} diff --git a/packages/viewer/src/systems/geometry/geometry-system.tsx b/packages/viewer/src/systems/geometry/geometry-system.tsx index 8eb060792..a08b57e9e 100644 --- a/packages/viewer/src/systems/geometry/geometry-system.tsx +++ b/packages/viewer/src/systems/geometry/geometry-system.tsx @@ -12,7 +12,8 @@ import { } from '@pascal-app/core' import { useFrame } from '@react-three/fiber' import { useEffect, useRef } from 'react' -import { FrontSide, type Group, type Material, type Mesh } from 'three' +import { type BufferGeometry, FrontSide, type Group, type Material, type Mesh } from 'three' +import { queueGeometryDispose } from '../../lib/deferred-dispose' import { type ColorPreset, createSurfaceRoleMaterial, @@ -105,6 +106,11 @@ export const GeometrySystem = () => { }, [sceneMaterials]) useFrame(() => { + // Geometries queued for disposal on a previous frame are flushed by the + // dedicated `GeometryDisposalFlushSystem`, which runs at a negative frame + // priority ahead of every rebuild system. Flushing here would risk + // disposing geometries queued by another system *this* frame, before the + // render pass (Sentry MONOREPO-EDITOR-DK/EG/EH). if (dirtyNodes.size === 0) return const nodes = useScene.getState().nodes @@ -308,8 +314,10 @@ function disposeChildren(group: Group) { ?.__fromGeometry if (!fromGeometry) continue group.remove(child) - const mesh = child as Partial & { geometry?: { dispose?: () => void } } - if (mesh.geometry?.dispose) mesh.geometry.dispose() + const mesh = child as Partial & { geometry?: BufferGeometry } + // Defer the outgoing geometry's dispose to the next frame — the WebGPU + // renderer still holds a RenderObject for it during this frame's render. + if (mesh.geometry) queueGeometryDispose(mesh.geometry) if ('material' in mesh) { const m = (mesh as { material: unknown }).material if (Array.isArray(m)) { diff --git a/packages/viewer/src/systems/roof/roof-system.tsx b/packages/viewer/src/systems/roof/roof-system.tsx index eeb47b467..044fcd379 100644 --- a/packages/viewer/src/systems/roof/roof-system.tsx +++ b/packages/viewer/src/systems/roof/roof-system.tsx @@ -25,6 +25,7 @@ import { mergeGeometries, mergeVertices } from 'three/examples/jsm/utils/BufferG import { ADDITION, Brush, Evaluator, SUBTRACTION } from 'three-bvh-csg' import { computeBoundsTree } from 'three-mesh-bvh' import { ensureRenderableGeometryAttributes } from '../../lib/csg-utils' +import { queueGeometryDispose } from '../../lib/deferred-dispose' function csgGeometry(brush: Brush): THREE.BufferGeometry { return brush.geometry as unknown as THREE.BufferGeometry @@ -168,6 +169,13 @@ export const RoofSystem = () => { useLiveNodeOverrides((s) => s.overrides) useFrame(() => { + // Geometries queued for disposal on a previous frame are flushed by the + // dedicated `GeometryDisposalFlushSystem` (negative frame priority), which + // runs before any rebuild system queues new disposals. Flushing here would + // run *after* GeometrySystem's rebuild in the same frame and could dispose + // geometries it just queued, before the render pass — reintroducing the + // dispose race (Sentry MONOREPO-EDITOR-DK/EG/EH). + // Clear stale pending updates when the scene is unloaded if (rootNodeIds.length === 0) { pendingRoofUpdates.clear() @@ -240,7 +248,7 @@ export const RoofSystem = () => { // while roofMaterials only has 4 entries. Three.js raycasts into invisible groups, // so MeshBVH hits groups[4].materialIndex → undefined.side → crash. if (mesh.geometry.type === 'BoxGeometry') { - mesh.geometry.dispose() + queueGeometryDispose(mesh.geometry) mesh.geometry = createDegenerateRoofPlaceholder() } mesh.position.set( @@ -305,7 +313,9 @@ function updateRoofSegmentGeometry( ) { const newGeo = generateRoofSegmentGeometry(node, nodes) - mesh.geometry.dispose() + // Defer the outgoing live geometry's dispose — the WebGPU renderer still + // references it this frame (Sentry MONOREPO-EDITOR-DK/EG/EH). + queueGeometryDispose(mesh.geometry) mesh.geometry = newGeo computeGeometryBoundsTree(newGeo) @@ -496,7 +506,8 @@ function updateMergedRoofGeometry( .filter((n): n is RoofSegmentNode => n !== undefined && !hasSegmentMaterialOverride(n)) if (children.length === 0) { - mergedMesh.geometry.dispose() + // Defer the outgoing live geometry's dispose (Sentry MONOREPO-EDITOR-DK/EG/EH). + queueGeometryDispose(mergedMesh.geometry) // Not BoxGeometry: its 6 groups against the merged mesh's 4-material array // crash GLTFExporter (materials[4] → undefined) when the roof bakes. mergedMesh.geometry = createDegenerateRoofPlaceholder() @@ -612,7 +623,8 @@ function updateMergedRoofGeometry( finalGeo.computeVertexNormals() ensureRenderableGeometryAttributes(finalGeo) - mergedMesh.geometry.dispose() + // Defer the outgoing live geometry's dispose (Sentry MONOREPO-EDITOR-DK/EG/EH). + queueGeometryDispose(mergedMesh.geometry) mergedMesh.geometry = finalGeo finalWallTrimmed.geometry.dispose()