feat(viz): terrain-edit invalidation for the brick-map sensor stack - #74
Merged
Merged
Conversation
Closes the highest-risk loose end from PR #70 — `setActivePreset` and `setHeightmapOverride` at runtime previously left the brick map silently stale, so mesh-link LoS and LiDAR scans would lie about occlusion until the next page load. Architecture (event-driven, no terrain↔WebGPU coupling): - terrain.ts exports `onTerrainChange(listener) -> unsubscribe`. Fires after `setActivePreset` and `setHeightmapOverride` mutate the active height field. Listener errors are caught + logged via the project logger so a failing subscriber doesn't break the others. - world.ts adds `rebuildWorld(device, heightFn, world)` — re-voxelizes via a shared `_voxelize` helper into the existing `voxelBuf` and re-runs `buildBrickMap` against the existing top_grid / brick_pool. Zero new GPU buffer allocation; the transient CPU Uint32Array gets GC'd after `writeBuffer` copies it. - sensors.ts subscribes to `onTerrainChange` after publishing the context. Listener calls `rebuildWorld` with the world it just built. - effects.ts subscribes too, clears `_meshLinkOccluded` (cached occlusion booleans go stale instantly on terrain change) and resets the LiDAR `Points` draw range to 0 (next scan repopulates). Logger consolidation (per session feedback "I have a logger remember that"): converted all WebGPU sensor stack `console.warn` / `console.info` calls to the project's `getLogger` pattern. terrain.ts already used it; sensors.ts and the LiDAR + LoS catch handlers in effects.ts are now consistent. Module names: `webgpu/sensors`, `effects`. Structured context via the `{ error, ... }` second-arg style matching terrain.ts. Bundle: main 808,316 B (under 819,200 cap), +424 B for the event API + logger imports + the `rebuildWorld` helper. Validation: typecheck, vite build, dotnet Release all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe changes implement a terrain change notification system that allows modules to subscribe to terrain updates. When terrain state changes via preset updates or heightmap overrides, subscribers (sensors and effects) are notified to invalidate cached data and rebuild dependent structures, such as voxel brick maps and LiDAR visualizations. Changes
Sequence DiagramsequenceDiagram
participant Terrain
participant EffectsManager
participant SensorContext
participant World
participant GPU as GPU Device
Note over Terrain: Preset/Heightmap Updated
Terrain->>Terrain: _fireTerrainChange()
par Parallel Subscriptions
Terrain->>EffectsManager: notify (terrain change)
EffectsManager->>EffectsManager: Clear mesh-link cache
EffectsManager->>EffectsManager: Hide LiDAR cloud
Terrain->>SensorContext: notify (terrain change)
SensorContext->>World: rebuildWorld(device, heightFn, world)
World->>World: Allocate voxel array
World->>World: _voxelize(heightFn, voxels)
World->>GPU: writeBuffer(voxelBuf, voxelData)
World->>World: buildBrickMap(brickMap, voxels)
Note over World: Brick map updated for LoS/LiDAR
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a terrain-change event system to synchronize state across the visualization engine, including cache invalidation in the EffectsManager and efficient WebGPU buffer updates via a new rebuildWorld function. It also transitions the codebase from console logging to a centralized logging utility. Feedback focuses on ensuring the sensor initialization process is idempotent to prevent duplicate subscriptions and managing the lifecycle of event listeners to avoid potential memory leaks.
sensors.ts (Gemini high + medium): - bootSensors() is now sequentially idempotent. Previously, calling bootSensors() a second time after the first resolved would re-init the device, allocate fresh GPU buffers (leaking the old ones), AND register a duplicate onTerrainChange listener — every preset change would trigger N rebuildWorld() calls instead of one. Now we check getSensorContext() first; if a context already exists, return it directly without re-running boot. - Added the corresponding `getSensorContext` import alongside `setSensorContext` from registry. effects.ts (Gemini medium): - Capture the unsubscribe handle returned by onTerrainChange() into a new private `_terrainUnsub` field. Add a `dispose()` method that calls it. EffectsManager is long-lived in the production viz, but holding the handle is best practice for hot-reload, tests, or scenario teardown — and avoids the linter complaint about an ignored return value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed all 3 review threads: HIGH — MEDIUM — MEDIUM — capture Build green: typecheck, vite build (808 KB / 819,200 cap), dotnet Release. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/ResQ.Viz.Web/client/webgpu/world.ts (1)
107-109: Consider reusing a CPU voxel scratch buffer to reduce GC churn.Line 107 allocates a new
Uint32Array(N^3)on every rebuild. During repeated terrain edits, this can produce avoidable allocation pressure.♻️ Suggested refactor
export type World = { params: WorldParams; brickMap: BrickMap; voxelBuf: GPUBuffer; gridBuf: GPUBuffer; + voxelScratch: Uint32Array; }; // createWorld(...) - const voxels = new Uint32Array(N * N * N); - _voxelize(heightFn, params, voxels); + const voxelScratch = new Uint32Array(N * N * N); + _voxelize(heightFn, params, voxelScratch); const voxelBuf = device.createBuffer({ - size: voxels.byteLength, + size: voxelScratch.byteLength, usage: GPUBufferUsage.STORAGE | GPUBufferUsage.COPY_DST, }); - device.queue.writeBuffer(voxelBuf, 0, voxels); + device.queue.writeBuffer(voxelBuf, 0, voxelScratch); - return { params, brickMap, voxelBuf, gridBuf }; + return { params, brickMap, voxelBuf, gridBuf, voxelScratch }; // rebuildWorld(...) - const voxels = new Uint32Array(N * N * N); - _voxelize(heightFn, world.params, voxels); - device.queue.writeBuffer(world.voxelBuf, 0, voxels); + _voxelize(heightFn, world.params, world.voxelScratch); + device.queue.writeBuffer(world.voxelBuf, 0, world.voxelScratch);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ResQ.Viz.Web/client/webgpu/world.ts` around lines 107 - 109, The code allocates a fresh Uint32Array voxels (size N*N*N) each rebuild which causes GC churn; change this to reuse a preallocated scratch buffer (e.g., attach voxelsScratch to the world object or a module-level variable) and ensure its length >= N*N*N before reuse, then call _voxelize(heightFn, world.params, voxelsScratch) and device.queue.writeBuffer(world.voxelBuf, 0, voxelsScratch); also clear or fill the existing buffer when necessary to avoid stale data and update any code referencing voxels to use the reused voxelsScratch variable.src/ResQ.Viz.Web/client/terrain.ts (1)
288-291: Consider coalescing terrain-change notifications for batched mutations.Line 290 emits immediately, and the current heightmap install flow (
setHeightmapOverride(...)followed by preset switch insrc/ResQ.Viz.Web/client/app.tsLine 280-291) can trigger duplicate downstream rebuilds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ResQ.Viz.Web/client/terrain.ts` around lines 288 - 291, The immediate call to _fireTerrainChange() inside setHeightmapOverride causes duplicate rebuilds during batched mutations; change setHeightmapOverride to schedule a coalesced terrain-change notification instead of emitting synchronously (e.g., debounce/microtask/rAF) so multiple sequential calls (setHeightmapOverride + preset switch) result in a single _fireTerrainChange invocation; update the logic around the _fireTerrainChange helper to support a scheduled/flushable trigger and ensure callers still get a timely notification when no other changes are pending.src/ResQ.Viz.Web/client/webgpu/sensors.ts (1)
116-122: Capture and manage the terrain unsubscribe handle for sensor lifecycle parity.
onTerrainChangeis registered but never unsubscribed. TheonTerrainChange()function returns an unsubscribe handle (seeterrain.tsline 53), but sensors.ts doesn't capture it. If sensor context is ever torn down/rebooted (tests/hot-reload/future feature), stale listeners accumulate.effects.tsalready follows the correct pattern at line 130 withthis._terrainUnsub = onTerrainChange(...)— adopt the same approach here for consistency and resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ResQ.Viz.Web/client/webgpu/sensors.ts` around lines 116 - 122, The onTerrainChange listener in sensors.ts is registered without capturing its unsubscribe handle, causing leaked listeners on teardown; capture the return value from onTerrainChange and store it the same way effects.ts does (assign to this._terrainUnsub) when calling onTerrainChange(() => rebuildWorld(...)); also ensure the stored this._terrainUnsub is called during the sensor context teardown/dispose path so the listener is removed. Use the existing symbols onTerrainChange, rebuildWorld and this._terrainUnsub to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ResQ.Viz.Web/client/effects.ts`:
- Around line 138-146: Call EffectsManager.dispose() to unsubscribe the terrain
listener before recreating terrain in _switchPreset(): locate the
_switchPreset() function and, right before the line that constructs the new
Terrain (the code that does "terrain = new Terrain(...)" or similar), invoke
effectsMgr.dispose() so the stored unsubscribe (_terrainUnsub) is executed; this
ensures EffectsManager's subscriber (set up in its constructor and removed by
dispose()) does not leak and fire stale callbacks against the newly created
Terrain instance.
---
Nitpick comments:
In `@src/ResQ.Viz.Web/client/terrain.ts`:
- Around line 288-291: The immediate call to _fireTerrainChange() inside
setHeightmapOverride causes duplicate rebuilds during batched mutations; change
setHeightmapOverride to schedule a coalesced terrain-change notification instead
of emitting synchronously (e.g., debounce/microtask/rAF) so multiple sequential
calls (setHeightmapOverride + preset switch) result in a single
_fireTerrainChange invocation; update the logic around the _fireTerrainChange
helper to support a scheduled/flushable trigger and ensure callers still get a
timely notification when no other changes are pending.
In `@src/ResQ.Viz.Web/client/webgpu/sensors.ts`:
- Around line 116-122: The onTerrainChange listener in sensors.ts is registered
without capturing its unsubscribe handle, causing leaked listeners on teardown;
capture the return value from onTerrainChange and store it the same way
effects.ts does (assign to this._terrainUnsub) when calling onTerrainChange(()
=> rebuildWorld(...)); also ensure the stored this._terrainUnsub is called
during the sensor context teardown/dispose path so the listener is removed. Use
the existing symbols onTerrainChange, rebuildWorld and this._terrainUnsub to
locate and implement the change.
In `@src/ResQ.Viz.Web/client/webgpu/world.ts`:
- Around line 107-109: The code allocates a fresh Uint32Array voxels (size
N*N*N) each rebuild which causes GC churn; change this to reuse a preallocated
scratch buffer (e.g., attach voxelsScratch to the world object or a module-level
variable) and ensure its length >= N*N*N before reuse, then call
_voxelize(heightFn, world.params, voxelsScratch) and
device.queue.writeBuffer(world.voxelBuf, 0, voxelsScratch); also clear or fill
the existing buffer when necessary to avoid stale data and update any code
referencing voxels to use the reused voxelsScratch variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c726548-565d-4486-8596-62d0df9a0a5a
📒 Files selected for processing (4)
src/ResQ.Viz.Web/client/effects.tssrc/ResQ.Viz.Web/client/terrain.tssrc/ResQ.Viz.Web/client/webgpu/sensors.tssrc/ResQ.Viz.Web/client/webgpu/world.ts
6 tasks
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
- "voxelizes once" was misleading — the brick-map rebuilds when the terrain preset switches or a heightmap override is installed (the `onTerrainChange` path landed in #74). Reword to "voxelizes at boot (and rebuilds on terrain edits)" so the docs match the code. - Drop "(~17 cases, ~380 ms)" from the Vitest row in Tech Stack — machine-specific runtime that drifts with each new test, not a capability claim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
* docs: refresh README for the WebGPU sensor stack + version bumps The README was written before the .NET 10 / Vite 8 / TS 6 upgrade and predates the entire WebGPU sensor arc (PRs #66–#87). Bring it back into sync with reality. - Tech Stack: .NET 9 → 10, Three.js r175 → 0.184, TS 5 + Vite 6 → TS 6 + Vite 8; new rows for the WebGPU sensor primitive and Vitest frontend tests. - Features: add the brick-map raymarcher (mesh-link LoS + per-drone LiDAR off one kernel) and the SignalR lazy chunk. - Project Layout: include `webgpu/` (device, sensors, registry, world, brickmap, los, lidar, rays, shaders/) and `__tests__/`, plus the new `sensorStatsOverlay.ts`. - Keyboard shortcuts: add `i` for the sensor stats overlay. - License footer: 2024 ResQ Technologies Ltd. → 2026 ResQ Systems, Inc. — matches the SPDX headers across the codebase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: address review feedback on PR #88 - "voxelizes once" was misleading — the brick-map rebuilds when the terrain preset switches or a heightmap override is installed (the `onTerrainChange` path landed in #74). Reword to "voxelizes at boot (and rebuilds on terrain edits)" so the docs match the code. - Drop "(~17 cases, ~380 ms)" from the Vitest row in Tech Stack — machine-specific runtime that drifts with each new test, not a capability claim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
* docs: refresh README for the WebGPU sensor stack + version bumps The README was written before the .NET 10 / Vite 8 / TS 6 upgrade and predates the entire WebGPU sensor arc (PRs #66–#87). Bring it back into sync with reality. - Tech Stack: .NET 9 → 10, Three.js r175 → 0.184, TS 5 + Vite 6 → TS 6 + Vite 8; new rows for the WebGPU sensor primitive and Vitest frontend tests. - Features: add the brick-map raymarcher (mesh-link LoS + per-drone LiDAR off one kernel) and the SignalR lazy chunk. - Project Layout: include `webgpu/` (device, sensors, registry, world, brickmap, los, lidar, rays, shaders/) and `__tests__/`, plus the new `sensorStatsOverlay.ts`. - Keyboard shortcuts: add `i` for the sensor stats overlay. - License footer: 2024 ResQ Technologies Ltd. → 2026 ResQ Systems, Inc. — matches the SPDX headers across the codebase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: address review feedback on PR #88 - "voxelizes once" was misleading — the brick-map rebuilds when the terrain preset switches or a heightmap override is installed (the `onTerrainChange` path landed in #74). Reword to "voxelizes at boot (and rebuilds on terrain edits)" so the docs match the code. - Drop "(~17 cases, ~380 ms)" from the Vitest row in Tech Stack — machine-specific runtime that drifts with each new test, not a capability claim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: document missing keyboard shortcuts in README The marketing intro mentioned `5`, `K`, and `Ctrl+Shift+R` but those never appeared in the Keyboard Shortcuts table. The `multi-agency-sar` scenario was also missing from the REST scenarios list, and the camera presets (`Shift+1..5`) plus the drone-strip cycling (`[`/`]`) were undocumented. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: correct wwwroot description (not committed) The Project Layout entry claimed wwwroot/ is "committed for zero-install deploys", but `git ls-files src/ResQ.Viz.Web/wwwroot/` returns 0 files and `.gitignore` excludes index.html, assets/, and .vite/. The zero-install path is actually the CI artifact `viz-wwwroot-{sha}` produced by ci.yml's client/build job, not anything in the repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: split long WebGPU sensor primitive sentence Addresses gemini-code-assist review feedback on PR #89 — the "voxelizes ... and serves ..." compound was a single 72-word sentence. Splitting at the rebuild-trigger clause keeps the trigger conditions visible while letting the LoS/LiDAR purpose start a fresh sentence. Backticks around `peakSlotDepth`, `raysOutsideWorld`, and `i` were already present in the source; only the bot's suggestion block had stripped them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the highest-risk loose end from PR #70 —
setActivePresetandsetHeightmapOverrideat runtime previously left the brick map silentlystale, so mesh-link LoS and LiDAR scans would lie about occlusion until
the next page load.
This was specifically called out as a check in the scheduled 2-week
audit (routine
trig_01HKh41wYUg5Phrszwkn3vUN) — fixing it now meansthat audit can downgrade it from "potential silent bug" to "verified
fixed."
Architecture (event-driven, no terrain↔WebGPU coupling)
terrain.tsexportsonTerrainChange(listener) → unsubscribe.Fires after
setActivePreset/setHeightmapOverridemutate theactive height field. Listener errors are caught + logged so a single
failing subscriber can't break the others.
world.tsaddsrebuildWorld(device, heightFn, world):re-voxelizes via a shared
_voxelizehelper into the existingvoxelBuf, then re-runsbuildBrickMapagainst the existingtop_grid/brick_pool. Zero new GPU buffer allocation — thetransient CPU
Uint32Array(N³)gets GC'd afterdevice.queue.writeBuffercopies it.
sensors.tssubscribes after publishing the context; listenercalls
rebuildWorld(device, terrainHeight, world).effects.tssubscribes too — clears_meshLinkOccluded(cachedocclusion booleans go stale instantly on terrain change) and resets
the LiDAR
Pointsdraw range to 0 (next scan repopulates).Logger consolidation
Per session feedback, converted all WebGPU sensor stack
console.warn/console.infocalls to the project'sgetLoggerpattern (already usedin
terrain.ts). Now consistent acrosswebgpu/sensors,effects,terrain. Structured{ error: msg }second-arg style throughout.Files modified
client/terrain.ts—onTerrainChangeAPI + listener registry +_fireTerrainChange()calls in the two mutators.client/webgpu/world.ts— extracts_voxelizehelper, exports newrebuildWorldfunction.client/webgpu/sensors.ts— subscribes after boot, callsrebuildWorld. All console.* converted to logger.client/effects.ts— subscribes in constructor, clears caches.Both pre-existing console.warn calls (LiDAR + mesh-link LoS error
handlers) converted to logger.
Bundle impact
+424 B for the event API + logger imports +
rebuildWorldhelper.Comfortably under cap.
Test plan
npm run typecheckpassesnpm run buildpasses (no new warnings)dotnet build -c Releasepasses (pre-push hook)- mesh-link line opacity recomputes against the new terrain
- LiDAR cyan point cloud disappears briefly then repopulates
(next scan within ~1 s)
- browser console shows no errors from the rebuild path
(no listener fires, no crash, sensor stack stays disabled).
What's deferred
_lidarPoints.geometry.dispose()when terrain changes —current code just resets draw range. Geometry is reused. Fine for
now; if memory churn surfaces, a follow-up can recreate.
Set.Adequate for two subscribers; if a third needs to run before/after a
specific other, we'll need explicit priority levels.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance