feat(viz): allow URL overrides of WebGPU world bounds - #85
Merged
Merged
Conversation
Lets ops/dev override the brick-map world without redeploying — the key motivation is testing the 4 km terrain vs 1 km default sensor world gap surfaced by `LosQueryStats.raysOutsideWorld` (PR #79). The defaults stay exactly where #69 set them; nothing changes for normal users. URL params (all optional): ?worldGrid=N gridSize, must be > 0 and divisible by 8 ?voxelScale=V metres per voxel, positive finite ?worldOriginX/Y/Z=K finite numbers; if omitted, the cube auto-recentres on world X/Z and starts at Y=0 so the new cube still straddles the terrain. Example: `?worldGrid=256&voxelScale=16` → 4096 m cube at 16 m/voxel covering the full 4000 m terrain (8× GPU voxel-buffer memory). Boot log emits the resolved params so the audit can correlate `raysOutsideWorld` against actual world bounds. Defaults preserved: gridSize: 128, voxelScale: 8, origin: [-512, 0, -512] Invalid values warn-and-fall-back per param; boot continues. Sensor chunk +1 KB (now 21.5 KB); main bundle unchanged at 756.4 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 55 minutes and 28 seconds.Comment |
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 adds functionality to override WebGPU world parameters via URL search parameters, enabling dynamic configuration of the grid size, voxel scale, and origin. The review feedback recommends using the "BRICK" constant instead of a hardcoded value for consistency and adopting "Number()" for stricter validation of numeric inputs compared to "parseInt" and "parseFloat".
Address Gemini review on PR #85: - Import `BRICK` from `./brickmap` and use it as the divisibility check in `_readPositiveInt` instead of the hardcoded `8`. Keeps the world parser in sync if BRICK ever changes. - Replace `parseInt`/`parseFloat` with `Number()`. The lenient parsers silently accept trailing garbage like "128abc" and truncate decimals like "128.9" — for a config override that should fall back to the default rather than partially parse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
Address Gemini review on PR #86: - Move the 4096 capacity constant out of `effects.ts:LIDAR_MAX_RAYS` and into `webgpu/registry.ts:LIDAR_MANAGER_CAPACITY`. Both `sensors.ts` (constructing the LosQueryManager) and `effects.ts` (validating user-overridden scan params) now read the same number, eliminating the maintenance risk of two literal copies drifting apart. registry.ts is already the chunk-split-safe seam effects.ts uses for getSensorContext, so this doesn't change the bundle layout. - Replace `parseInt`/`parseFloat` with `Number()` in the three URL parser helpers — same change as PR #85; lenient parsers silently truncate decimals and accept trailing garbage, which for a config override should fall back rather than partially parse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
Defaults preserved exactly (16 × 256 = 4096 rays, ±22.5° FOV, 200 m range, 1 s interval); operators can tune any of them from the URL for testing. Pairs with the world-bounds overrides in PR #85 — both share the same warn-and-fall-back validation pattern, both read at boot only. URL params (all optional): ?lidarElev=N elevationCount, positive integer ?lidarAzim=N azimuthCount, positive integer ?lidarFov=D elevationFov in degrees, 1–180 ?lidarRange=M max range in metres, positive finite ?lidarInterval=S seconds between scans, positive finite Per-scan ray count must not exceed 4096 (the LiDAR manager's per-slot capacity from `webgpu/sensors.ts`); both elev/azim fall back to defaults together if `elev × azim` exceeds it, since partial fallbacks would produce unexpected aspect ratios. A full Settings-panel UI is deliberately deferred: tuning these knobs is mostly a dev/audit workflow, not a per-session user preference, and URL-driven overrides keep the Settings/UI surface from growing without a clear use case. Bundle: main 756.4 → 757.7 KB (+1.3 KB; 92.5 % of cap). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
Address Gemini review on PR #86: - Move the 4096 capacity constant out of `effects.ts:LIDAR_MAX_RAYS` and into `webgpu/registry.ts:LIDAR_MANAGER_CAPACITY`. Both `sensors.ts` (constructing the LosQueryManager) and `effects.ts` (validating user-overridden scan params) now read the same number, eliminating the maintenance risk of two literal copies drifting apart. registry.ts is already the chunk-split-safe seam effects.ts uses for getSensorContext, so this doesn't change the bundle layout. - Replace `parseInt`/`parseFloat` with `Number()` in the three URL parser helpers — same change as PR #85; lenient parsers silently truncate decimals and accept trailing garbage, which for a config override should fall back rather than partially parse. 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): URL-overridable LiDAR scan params Defaults preserved exactly (16 × 256 = 4096 rays, ±22.5° FOV, 200 m range, 1 s interval); operators can tune any of them from the URL for testing. Pairs with the world-bounds overrides in PR #85 — both share the same warn-and-fall-back validation pattern, both read at boot only. URL params (all optional): ?lidarElev=N elevationCount, positive integer ?lidarAzim=N azimuthCount, positive integer ?lidarFov=D elevationFov in degrees, 1–180 ?lidarRange=M max range in metres, positive finite ?lidarInterval=S seconds between scans, positive finite Per-scan ray count must not exceed 4096 (the LiDAR manager's per-slot capacity from `webgpu/sensors.ts`); both elev/azim fall back to defaults together if `elev × azim` exceeds it, since partial fallbacks would produce unexpected aspect ratios. A full Settings-panel UI is deliberately deferred: tuning these knobs is mostly a dev/audit workflow, not a per-session user preference, and URL-driven overrides keep the Settings/UI surface from growing without a clear use case. Bundle: main 756.4 → 757.7 KB (+1.3 KB; 92.5 % of cap). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(viz): centralize LiDAR capacity + tighten URL parser Address Gemini review on PR #86: - Move the 4096 capacity constant out of `effects.ts:LIDAR_MAX_RAYS` and into `webgpu/registry.ts:LIDAR_MANAGER_CAPACITY`. Both `sensors.ts` (constructing the LosQueryManager) and `effects.ts` (validating user-overridden scan params) now read the same number, eliminating the maintenance risk of two literal copies drifting apart. registry.ts is already the chunk-split-safe seam effects.ts uses for getSensorContext, so this doesn't change the bundle layout. - Replace `parseInt`/`parseFloat` with `Number()` in the three URL parser helpers — same change as PR #85; lenient parsers silently truncate decimals and accept trailing garbage, which for a config override should fall back rather than partially parse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
WomB0ComB0
added a commit
that referenced
this pull request
Apr 29, 2026
Address the security review on PRs #85 / #86. URL overrides for world bounds and LiDAR scan params previously accepted any positive finite value; a typo like `?worldGrid=8192` (~2 TB voxel buffer) or `?lidarRange=1e30` (DDA walks every voxel + float precision erosion) would lock up the GPU. Self-DOS only — no server amplification, no cross-origin attack surface — but a sane upper bound prevents "my browser tab died" footguns. Caps: worldGrid ≤ 1024 (1024³ × 4 B = 4 GB voxel buffer ceiling) voxelScale ≤ 256 (1024 × 256 = 256 km cube ceiling) lidarElev/azim ≤ 4096 (the LiDAR manager capacity) lidarRange 0.1–10000 (m; visualization terrain is only 4 km) lidarInterval 0.001–60 (s) Also: truncate `raw` log value to 64 chars (+ `…` ellipsis) so a poisoned URL with megabytes of garbage can't bloat the console or log aggregator. Helpers in both files share the same `_truncRaw` implementation. The lenient `_readPositiveFinite` helper in effects.ts is removed — the two call sites (lidarRange, lidarInterval) now use `_readNumberInRange` with explicit bounds, matching the pattern already used for `lidarFov`. Bundle: main 757.7 → 757.9 KB (+0.2 KB). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
WomB0ComB0
added a commit
that referenced
this pull request
Sep 10, 2026
* docs: correct the SDK pin, and guard what the compiler cannot CLAUDE.md said the submodule is "pinned to a release tag". It is not. It is pinned to a3f8b89 on release/0.6.x, three commits past v0.6.0, and those three commits (#85-#87) are what added drone attitude, the explicit yaw command, and landing recovery. No tag contains them. The line invited exactly the change that would drop them. Two findings while checking it, one of which corrects an assumption I had been repeating. FIRST, THE SDK's main HAS MOVED PAST US. ResQ.Simulation.Engine, ResQ.Mavlink, ResQ.Mavlink.Dialect and ResQ.Mavlink.Mesh do not exist on main - MAVLink is gone from it entirely, and main carries a different set (Core, Clients, Protocols, Blockchain, Storage, Simulation). All four are ProjectReferences in ResQ.Viz.Web.csproj. So viz is not on a stale pin; it is on a branch main has structurally abandoned, 43 commits back on a divergent line. Reconciling that is an architecture decision for a human, and this commit does not attempt it - it writes it down where the next person will look. SECOND, THE SILENT-REVERT RISK IS NOT REAL, and I had been asserting it was. Both ways of moving the pin fail loudly: v0.6.0 -> compile error; viz calls Hover(yaw) and GoTo(..., yaw:), which that tag has no overloads for main -> the four project references do not resolve at all Measured, not reasoned: checking out v0.6.0 in the submodule produced five CS1501/CS1739 errors in viz's own production code before any test ran. What neither catches is a behaviour that changed without changing a signature. Landing recovery is a re-arm inside ApplyCommand, and attitude integration is the roll/pitch term inside IntegrateAttitude; reverting either compiles cleanly. Verified by doing it: the re-arm revert and the heading-only attitude revert each build green and each fail exactly one of the new tests. So SdkFlightContractTests covers the part that is genuinely silent. Four tests: explicit yaw steers heading, forward flight produces real pitch, a landed drone re-arms and climbs on a non-Land command, and Land still latches so the re-arm did not make HasLanded meaningless. They duplicate tests the SDK already has, on purpose - this is viz asserting the submodule it is pinned to still behaves the way its client and physics assume. 1376 tests green. dotnet format clean. The submodule pin is unchanged and its working tree was restored after every mutation. * test: make the re-arm test cover what its name claims, and drop a stale claim Two review findings, both right. FIRST, the test file's own doc still said moving the pin to v0.6.0 "still builds, still passes every other test, and silently reverts takeoff and rotation". That was the assumption this PR exists to correct, and CLAUDE.md now says the opposite two files away: v0.6.0 is a compile error because viz calls Hover(yaw) and GoTo(..., yaw:). The stale wording survived where the argument for the tests lives, which is the worst place for it. Rewritten to say what was measured, and to say plainly what the tests actually guard - behaviour that changed without changing a signature, which no compiler can see. SECOND, the re-arm test was named "on any non-Land command" and exercised GoTo alone. The re-arm is keyed on command.Type, so a regression that cleared HasLanded for GoTo and not for Hover or RTL would have passed. Now a Theory over GoTo, Hover and RTL. Verified rather than assumed: rewriting the guard to `command.Type == FlightCommandType.GoToWaypoint` fails the Hover and RTL cases and passes GoTo - precisely the regression described, which the old test would have shipped green. The first attempt at that mutation used a FlightCommandType member that does not exist and failed to compile; the result above is from the one that actually applied. Hover asserts the flag rather than a climb, because RTL rewrites to GoTo(LaunchPosition) and has somewhere to go while Hover holds station. Asserting a climb for Hover would assert the wrong contract. 1378 tests green. dotnet format clean. Submodule restored after each mutation.
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
Boot-time URL overrides for the brick-map sensor world. Defaults preserved; nothing changes for normal users.
The motivation is the 4 km terrain vs 1 km default sensor world gap surfaced by
LosQueryStats.raysOutsideWorldin #79 — operators can now test?worldGrid=256&voxelScale=16(4096 m cube, 8× GPU memory) without redeploying, watch the counter drop, and decide whether the resize is justified before committing it as the default.URL params (all optional)
worldGrid=NvoxelScale=VworldOriginX/Y/Z=KOrigin auto-centres so a
?worldGrid=256&voxelScale=16URL gets a 4096 m cube straddling the terrain origin without the operator computing offsets. Invalid values warn-and-fall-back per param; boot continues.Boot log now emits the resolved world params so the audit script can correlate
raysOutsideWorldagainst actual bounds.Bundle
Test plan
npm run buildpasses;npm test14/14 greentsc --noEmitclean/?worldGrid=256&voxelScale=16and check the console for theWebGPU world boundslog linegetSensorContext()?.world.paramsreflects the override🤖 Generated with Claude Code