feat(viz): per-drone LiDAR scans - #80
Merged
Merged
Conversation
Replace the single-drone demo with per-drone state. Each drone gets its own `LidarScan` + visualization `Points`, lazy-created on the first frame the drone is seen with a valid `pos` and disposed when it disappears from the frame. - `_lidarEntries: Map<droneId, LidarEntry>` replaces the four legacy `_lidar*` fields. Entry struct holds scan, points, in-flight promise, last scan time, and a disposed flag. - Per-drone throttle + skip-if-busy. Each drone's scan pipeline is independent; cross-drone concurrency rides the shared 3-slot `ctx.lidar` ring so peakSlotDepth surfaces real load. - The `disposed` flag avoids writing into a freed BufferGeometry when an in-flight scan resolves after the drone has been evicted. - Terrain-edit invalidation now resets every entry's draw range. - `dispose()` tears down all entries (the LiDAR Points are owned exclusively by this manager, unlike the survivor-pool spheres). `SensorContext` is now type-imported into `effects.ts` (TS strips it at runtime, so the dynamic-chunk split for the WebGPU stack stays intact). Build verified at 809.5 KB main bundle (under the 800 KiB cap; sensor chunk +0.5 KB). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 48 minutes and 26 seconds.Comment |
4 of 5 tasks
![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 refactors the LiDAR visualization system to support multiple drones by transitioning from a single-drone state to a per-drone management system using a LidarEntry map. Key updates include lazy initialization of drone-specific resources, independent scan throttling, and automatic eviction of stale drone data. The review feedback identifies a potential issue where using size-based optimizations to skip eviction loops could lead to memory leaks in 'churn' scenarios—specifically when the number of drones remains constant but their identifiers change, causing stale entries to persist in the map.
Address Gemini review on PR #80. Same churn-case fix as PR #77 — a `cache.size > seenIds.size` guard silently misses the case where one drone leaves and another joins on the same frame; sizes stay equal but a stale entry persists. Same logic applies to the no-context fallback (`size > 0` would desync if an entry's Three.js was torn down externally). Drop both guards. Drone / link counts are small, so the per-frame iteration is cheap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
* test(viz): add vitest smoke coverage for WebGPU host primitives The WebGPU sensor stack landed across PRs #66–#80 with zero automated test coverage — every regression was caught by manual build verification or bot review. Add a minimal vitest harness for the host-side primitives that have caused the most bugs in this arc: - `__tests__/rays.test.ts` — Ray/RayHit wire-format constants and writeRay/readHit roundtrip, including the OOB rejection paths. Catches stride / slot-index drift between host packing and the WGSL `Ray` struct in march.wgsl. - `__tests__/lidar.test.ts` — LidarScan constructor validation (capacity check, positive elevation/azimuth) using a structural mock of LosQueryManager. Catches silently-misconfigured scans before they reach a browser. 12 tests, ~350 ms. No browser / GPU — pure host-side primitives. Wired into `client-budget` between install and build so a failing primitive stops the pipeline before bundle measurement runs. Vitest 4.1.5 is peer-compatible with the existing Vite 8 toolchain; the lockfile gains 28 transitive packages but the production bundle is untouched (devDependency only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(viz): consolidate elev/azim cases and assert full direction vec Address Gemini review on PR #81: - lidar.test.ts: replace two near-duplicate `it()` blocks with a single `it.each([...])` covering both elevationCount and azimuthCount at zero AND negative values (4 cases vs the original 2). - rays.test.ts: assert all three direction components in the writeRay roundtrip — a slot-index drift would land y/z in the wrong cells, which an x-only assertion misses. 12 → 14 tests; suite still ~380 ms. 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
* feat(viz): LiDAR sensor mount offset `LidarScan.scan(origin, rot?, mountOffset?)` now accepts an optional drone-local mount offset (e.g. `[0, 0.5, 0]` for a sensor on a 0.5 m mast). The offset is rotated into world space by `rot` and added to `origin` to produce the actual scan origin. Mechanics: - Hoists the q→3x3 rotation matrix outside the existing per-ray loop so it can be reused for both the mount-offset transform and the direction rotation. No new compute cost on the scan side (matrix already needed to be built); offset application is a single matrix-vector multiply per scan. - Hit positions now derive from `this._origin` (the post-offset scan origin) rather than the raw `origin` parameter — without this, a mast-mounted sensor would report hits as if rays started at the drone center but ended at the mast tip, drifting by the offset length. - Backward-compatible: existing `effects.ts:_updateLidar` keeps passing two args; `mountOffset` defaults to undefined and the bare drone-origin path stays bit-identical to PR #80. Tests: - World-axis-aligned offset application (no rot) - Rotation of offset by quaternion (yaw 90° → +X local maps to −Z world) - Bare-origin path unchanged Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(viz): snapshot LidarScan origin before await Address Gemini review on PR #84: capture `ox/oy/oz` from `this._origin` BEFORE the `await this.los.query(rays)` rather than after. The tuple is shared mutable state; if another scan() landed on the same instance while the query was in flight, the post-await read would see the next scan's origin and the hit-position math would silently produce wrong world-space positions. The current consumer gates per-drone via the in-flight flag in effects.ts, so the race isn't reachable today — this just makes LidarScan robust to other call patterns and future mistakes. 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
LidarScan+ visualizationPoints, lazy-created on first valid frame and disposed when the drone disappears._lidarEntries: Map<droneId, LidarEntry>replaces the four legacy_lidar*fields. Per-drone throttle + skip-if-busy; cross-drone concurrency rides the shared 3-slotctx.lidarring sopeakSlotDepth(added in feat(viz): expose lifetime stats on LosQueryManager #78) surfaces real load.disposedflag avoids writing into a freed BufferGeometry when an in-flight scan resolves after the drone has been evicted.dispose()tears down all entries.Notes
SensorContextis now type-imported intoeffects.ts(TS strips at runtime — same patternregistry.tsuses — so the WebGPU dynamic-chunk split stays intact).getSensorContext()?.lidar.stats— ifpeakSlotDepthexceeds 1 routinely, we should bumpslotCountforctx.lidar.Test plan
npm run buildpasses; main bundle 809.5 KB (under 819.2 KB cap, ~10 KB headroom)tsc --noEmitclean🤖 Generated with Claude Code