Give each vehicle a camera, and a way to see the world without ArduPilot - #15
Merged
Conversation
SITL vehicles flew through a world nobody could look at. The video feed in
SkyHub was a canned MP4, so a survey flown in the simulator came back with
footage of somewhere else entirely.
This draws the world the physics already knows about. A software rasteriser
runs on its own thread with its own copy of the world, cuts the screen into
horizontal bands with one thread owning each — no locks in the inner loop —
and serves each vehicle a JPEG or MJPEG stream off the control plane at
/vehicles/{id}/camera.mjpg. 0.67 ms/frame at 256x144 on four threads.
Rendering started out on the tick thread and starved the physics: one row of
pixels is about a whole 1.25 ms budget at 400 Hz, and the autopilot refused to
arm. Hence the separate thread and the separate world.
The ray caster it replaces is kept as the test oracle. Both renderers now take
"ground or building" from which body the geometry came from rather than from
which way the surface faces — a flat roof's normal points straight up exactly
like a field's, so the old heuristic grew grass on every rooftop in the city.
Triangles are also culled against their far edge rather than their centroid,
which stops buildings shedding individual triangles as they cross the draw
distance and standing there with holes in them.
tools/getting_started.sh builds, cooks a 1.2 km demo city and renders it from
four poses in about a second, with no autopilot, no network and no simulator
running — you can see the thing work before committing to the half hour the
ArduPilot build takes. With ARDUPILOT_ROOT set, --fly puts a vehicle in that
city and serves its camera live.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds world geometry queries, ray and raster rendering, asynchronous JPEG frame production, camera streaming endpoints, build targets, acceptance tests, a render probe, and a quick-start workflow with optional ArduPilot integration. ChangesRendering and camera delivery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SimulationTick
participant RenderService
participant FrameStore
participant ControlServer
SimulationTick->>RenderService: publish vehicle poses
RenderService->>RenderService: render and encode JPEG frames
RenderService->>FrameStore: publish vehicle JPEG
ControlServer->>FrameStore: retrieve camera frame
FrameStore-->>ControlServer: return JPEG bytes
ControlServer-->>ControlServer: stream multipart MJPEG response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (12)
src/core/world.h (1)
108-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the
normal_nedcontract wording.The comment states the normal "points back towards the ray".
raycast_surfaceinsrc/core/world.cppreturnsGetWorldSpaceSurfaceNormal, which is the outward surface normal at the hit point. For a hit on a back face the normal points away from the ray origin. The current renderer tolerates this becauseshade_for_normalinsrc/render/shading.htakes the absolute value of the dot product. Document the actual guarantee so a later consumer does not depend on a sign that is not provided.📝 Proposed comment change
struct RayHit { bool hit{false}; double distance{-1.0}; - Vec3 normal_ned{0.0, 0.0, -1.0}; // unit, points back towards the ray + // Unit outward surface normal of the body that was hit. Not oriented + // against the ray: a back-face hit returns a normal pointing away. + Vec3 normal_ned{0.0, 0.0, -1.0}; bool is_ground{false}; // the ground slab, as opposed to a building };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/world.h` around lines 108 - 119, Update the normal_ned comment in RayHit to describe it as the outward unit surface normal returned by raycast_surface, without claiming it points back toward the ray or origin. Leave the raycast_surface implementation and shading behavior unchanged.src/api/control_server.cpp (2)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two anonymous namespaces.
Lines 18-22 open an anonymous namespace that closes immediately, and Line 25 opens another one. Move
kWorkerThreadsinto the existing block and remove the extra blank line at Line 24.♻️ Proposed change
namespace { +// Two long-lived camera streams per vehicle, plus headroom for the control +// plane, which must stay answerable while they run. +constexpr size_t kWorkerThreads = 64; + +// {"launch_process":true} — absent key means false. +bool parse_launch_process(const std::string &body) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/control_server.cpp` around lines 18 - 24, Merge the anonymous namespace containing kWorkerThreads into the existing anonymous namespace later in src/api/control_server.cpp, removing the immediately closing namespace block and extra blank line while preserving the constant and its value.
320-348: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShare the MJPEG provider body
Extract the common multipart framing and 40 ms polling logic. Pass a per-frame vehicle-ID resolver to the shared provider. cpp-httplib v0.18.3 detects failed
sink.writecalls after the callback, so the provider may continue returningtrueafter a write attempt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/control_server.cpp` around lines 320 - 348, Extract the MJPEG multipart framing, JPEG writes, 40 ms delay, and write-result handling into a shared provider helper. Update the camera route to pass a per-frame vehicle-ID resolver that calls resolve_instance(instance), while retaining the existing camera-disabled response and content type. Ensure the provider returns false when any sink.write call fails, rather than continuing after a failed client connection.src/core/world.cpp (1)
377-396: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared ray setup.
raycast_surfacerepeats the origin, direction, and ignore-body setup fromraycastat Lines 351-370 exactly. A small file-local helper that buildsJPH::RRayCastand the ignoreBodyIDwould keep the two entry points in step. This is optional; the duplication is short and both copies are correct today.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/world.cpp` around lines 377 - 396, Optionally extract the duplicated ray setup from raycast_surface and raycast into a file-local helper that constructs the JPH::RRayCast and resolved ignore JPH::BodyID. Update both entry points to use the helper while preserving their current filtering and hit behavior.tests/test_render.cpp (1)
79-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the temporary directory on the early return.
If
cook_obj_tilefails, the function returns at Line 82 and leavesskysim_render_tilesbehind. The same fixed path is reused by the next run, so stale files can change whatload_tilesfinds at Line 87.♻️ Proposed change
if (!skysim::terrain::cook_obj_tile(obj, dir / "building.jshape", nullptr, &err)) { std::printf("FAIL cook: %s\n", err.c_str()); + std::filesystem::remove_all(dir); return 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_render.cpp` around lines 79 - 83, Update the failure path in the test function surrounding cook_obj_tile to remove the temporary skysim_render_tiles directory before returning 1. Reuse the existing temporary-directory cleanup mechanism so failed runs cannot leave stale files for subsequent load_tiles calls.src/main.cpp (2)
989-1009: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a
RenderServiceconstruction failure.
RenderServicebuilds acore::World, loads tiles, and starts a thread. Any exception from that path propagates out ofmainand terminates the process without a message. The tile-streamer setup at Lines 962-976 uses atry/catchand returns 1. Match that behaviour so a bad--tilespath fails the same way for both subsystems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 989 - 1009, Wrap the RenderService setup in the camera block, including construction and initial status reporting, in a try/catch matching the tile-streamer setup’s behavior. Catch construction failures, print the exception message to stderr with appropriate context, and return 1 instead of allowing the exception from RenderService to escape main; leave successful initialization unchanged.
577-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo comments state that camera frames are produced on the tick thread.
RenderServicerenders on its own thread against its own static world copy, and publishes intoFrameStore. The tick thread only forwards poses throughpublish_poses.
src/main.cpp#L577-L579: rewrite the comment to state that rendering happens on theRenderServicethread and that this thread only publishes poses.src/api/control_server.h#L106-L109: replace "Published by the tick thread like everything else here" with a note that the render thread publishes frames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 577 - 579, The comments incorrectly attribute camera-frame rendering and publication to the tick thread. In src/main.cpp lines 577-579, update the comment near RenderService to state that rendering occurs on the RenderService thread while the current thread only publishes poses; in src/api/control_server.h lines 106-109, replace the tick-thread publication note with one stating that the render thread publishes frames.src/render/jpeg.h (1)
17-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument and enforce the buffer-size precondition.
encode_jpegpassesimage.rgb.data()tostbi_write_jpg_to_func, which reads exactlywidth * height * 3bytes. The implementation insrc/render/jpeg.cppLines 20-31 checks onlyempty(),width, andheight. If a caller supplies anImagewhosergbbuffer is smaller thanwidth * height * 3, stb reads out of bounds. Add the size check next to the existing guards, and state the precondition here.🛡️ Proposed guard in src/render/jpeg.cpp
if (image.empty() || image.width <= 0 || image.height <= 0 || image.rgb.size() < static_cast<size_t>(image.width) * image.height * 3) { return out; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/jpeg.h` around lines 17 - 18, Update the encode_jpeg declaration comment and implementation guards to require image.rgb to contain at least width * height * 3 bytes; return the existing empty output when this precondition is not met, alongside the current empty, width, and height checks.src/render/raster.cpp (1)
170-189: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
draw_backgroundcallscamera.rayfor every pixel.
Camera::rayperforms a quaternion rotation and a normalization per call. The background pass runs over every pixel of every frame, so it reintroduces the per-pixel cost that the rasterizer was introduced to remove. Onlydir[2]is used here, anddir[2]varies with bothxandyonly through the fixed camera basis, so the value can be computed once per row from the row's ray and the horizontal step, or the whole background can be precomputed once per pose.This is a performance suggestion, not a correctness problem.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/raster.cpp` around lines 170 - 189, Optimize Rasterizer::draw_background to avoid calling camera.ray for every pixel. Compute dir[2] per row using the row ray and the camera’s horizontal ray step, or cache the background for an unchanged camera pose, while preserving the existing below-horizon classification and sky/ground color blending.src/render/render_service.h (1)
61-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Frame::sim_time_sis never set.
src/render/render_service.cppLines 69-71 always publish0.0as the frame time, andframe()returns only the JPEG bytes. Thesim_time_sfield ofFrameStore::Frametherefore carries no information. Either publish the simulation time that the pose belongs to and expose it, for example through an HTTP header on the camera route, or remove the field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/render_service.h` around lines 61 - 62, Update the frame publication flow in render_service.cpp to set Frame::sim_time_s from the simulation time associated with the vehicle pose, and extend render_service.h’s frame() API or the camera route to expose that timestamp alongside the JPEG data. Ensure consumers receive the pose’s actual simulation time rather than the current constant 0.0.src/render/shading.h (1)
22-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
kSunDirNedis not exactly unit length.The vector length is about 0.9987.
shade_for_normaltreats it as a unit vector in the Lambert dot product, so the lighting term is scaled by that factor. The visual effect is negligible, but the constant states the intent that it is a direction. Consider normalizing the literal values, or add a comment that the small deviation is accepted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/shading.h` around lines 22 - 46, The kSunDirNed direction constant is slightly non-unit while shade_for_normal uses it directly for Lambert lighting. Normalize the literal vector values, or explicitly document that the deviation is intentional and accepted; preserve the existing shading behavior and symbol usage in shade_for_normal.src/render/camera.h (1)
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate render dimensions before allocation and use
std::numbers::pi.
- The CLI rejects non-positive dimensions only when the camera is enabled. Direct render calls remain unguarded, and
Renderer::renderallocates the image before constructingCamera. Validatewidth > 0andheight > 0before any render allocation. Do not rely only onassert.- Replace all remaining
M_PIuses insrc/render/camera.h,src/render/raster.cpp,src/vehicle/quad.cpp, andtools/render_probe/main.cpp. CMake requires C++20, so usestd::numbers::pi.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/camera.h` around lines 33 - 41, Validate positive render width and height at the start of the render entry path, before image allocation or Camera construction, without relying solely on assert; ensure direct render calls are guarded. In Camera and the affected raster, quad, and render_probe code, replace every M_PI use with std::numbers::pi and include the appropriate C++20 numbers header.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 89-101: Update the quick-start README text surrounding
tools/getting_started.sh to state that Python 3 and network access are required
because CMake fetches JoltPhysics, cpp-httplib, and stb; retain that ArduPilot
and a running simulator are not required.
In `@src/api/control_server.cpp`:
- Around line 351-358: The two camera route handlers must avoid exceptions for
oversized vehicle IDs. In src/api/control_server.cpp lines 351-358, replace the
std::stoul conversion in the camera.jpg handler with std::strtoul using base 10;
apply the same conversion to the id computation in lines 372-378. No other
changes are required.
In `@src/main.cpp`:
- Around line 63-77: Add validation in parse_args for the numeric options near
the existing validation block: require frame_grace to be within 0.0–1.0,
camera_threads to be non-negative, and camera_quality to be 1–100. Reuse the
existing startup error handling and ensure invalid values fail before render or
timing code uses them.
In `@src/render/frame_store.h`:
- Around line 10-19: Update the documentation comment above the frame store to
describe publication from the render thread to the HTTP thread, removing the
claim that rendering or production occurs on the tick thread. Preserve the
explanation that the HTTP side only reads tick-published data only if it remains
accurate, and ensure the comment reflects RenderService’s independent World copy
and render-thread ownership.
In `@src/render/jpeg.cpp`:
- Around line 3-7: Validate the camera quality value before calling
stbi_write_jpg_to_func in the JPEG rendering path, accepting only the documented
inclusive range of 1–100 and rejecting invalid values, including 0, before
encoding. Preserve the existing valid-quality encoding behavior and ensure the
rejection follows the renderer’s established error-handling path.
In `@src/render/raster.cpp`:
- Around line 191-249: Update Rasterizer::fill_band to convert the
camera-forward depth z into radial distance before passing it to haze_at, using
the per-pixel ray-length factor consistent with renderer.cpp’s hit.distance
calculation. Use the converted radial distance for haze and the max_range_m
cutoff while preserving the existing depth-buffer comparison in camera-space
depth.
In `@src/render/render_service.cpp`:
- Around line 10-31: The RenderService constructor currently calls
World::load_tiles without a memory bound, causing all configured tiles and
extracted geometry to remain resident. Update RenderService and its Config
handling to load only a bounded tile set, using an appropriate radius around the
fleet spawn area or an explicit tile limit, while preserving ground geometry and
scene_.build initialization.
- Around line 63-73: Update RenderService::Pose and App::publish_frames to carry
and populate sim_time_s, then pass that timestamp to frames_.publish instead of
0.0. In the pose-list update flow, remove FrameStore entries via frames_.erase
for vehicle IDs absent from the current poses, while preserving frames for
currently active vehicles.
In `@src/render/render_service.h`:
- Around line 59-65: Update App::despawn to remove the vehicle’s entry from
FrameStore when it is despawned, so camera requests cannot serve stale JPEGs.
Ensure in-flight rendering checks that the vehicle is still active before
republishing its frame, preventing removed vehicles from reappearing in the
store; preserve frame() behavior for active vehicles.
In `@src/render/renderer.cpp`:
- Around line 61-79: Set hit.is_ground to true when synthesizing the fallback
hit in the ground-intersection branch before the later shading logic selects the
base color. Keep the existing distance, normal, and hit handling unchanged so
fallback ground hits use kGround rather than kBuilding.
In `@src/render/shading.h`:
- Around line 60-63: Update haze_at to check max_range_m before dividing; when
the range is non-positive, return kMaxHaze directly, otherwise preserve the
existing clamping and cap calculation.
In `@tests/test_render.cpp`:
- Around line 166-171: Update the JPEG marker assertions in the encode_jpeg test
to validate sufficient buffer length before indexing the first or last bytes.
Ensure short or empty results cause recorded CHECK failures without evaluating
out-of-bounds accesses or allowing jpeg.size() - 2 to underflow.
In `@tools/getting_started.sh`:
- Around line 115-116: Update the api_up and vehicle_connected readiness probes
to pass curl connection and total-operation timeouts of one second, ensuring
either request returns promptly when the server is unresponsive so wait_for can
retry and report startup failure.
In `@tools/render_probe/main.cpp`:
- Around line 152-154: Update the output handling in main around the ofstream f
write path: validate that the stream opened successfully before writing, and
validate the stream state after writing jpeg. On either failure, report an error
and return a nonzero status; only print the “wrote” success message and return
zero after both checks pass.
---
Nitpick comments:
In `@src/api/control_server.cpp`:
- Around line 18-24: Merge the anonymous namespace containing kWorkerThreads
into the existing anonymous namespace later in src/api/control_server.cpp,
removing the immediately closing namespace block and extra blank line while
preserving the constant and its value.
- Around line 320-348: Extract the MJPEG multipart framing, JPEG writes, 40 ms
delay, and write-result handling into a shared provider helper. Update the
camera route to pass a per-frame vehicle-ID resolver that calls
resolve_instance(instance), while retaining the existing camera-disabled
response and content type. Ensure the provider returns false when any sink.write
call fails, rather than continuing after a failed client connection.
In `@src/core/world.cpp`:
- Around line 377-396: Optionally extract the duplicated ray setup from
raycast_surface and raycast into a file-local helper that constructs the
JPH::RRayCast and resolved ignore JPH::BodyID. Update both entry points to use
the helper while preserving their current filtering and hit behavior.
In `@src/core/world.h`:
- Around line 108-119: Update the normal_ned comment in RayHit to describe it as
the outward unit surface normal returned by raycast_surface, without claiming it
points back toward the ray or origin. Leave the raycast_surface implementation
and shading behavior unchanged.
In `@src/main.cpp`:
- Around line 989-1009: Wrap the RenderService setup in the camera block,
including construction and initial status reporting, in a try/catch matching the
tile-streamer setup’s behavior. Catch construction failures, print the exception
message to stderr with appropriate context, and return 1 instead of allowing the
exception from RenderService to escape main; leave successful initialization
unchanged.
- Around line 577-579: The comments incorrectly attribute camera-frame rendering
and publication to the tick thread. In src/main.cpp lines 577-579, update the
comment near RenderService to state that rendering occurs on the RenderService
thread while the current thread only publishes poses; in
src/api/control_server.h lines 106-109, replace the tick-thread publication note
with one stating that the render thread publishes frames.
In `@src/render/camera.h`:
- Around line 33-41: Validate positive render width and height at the start of
the render entry path, before image allocation or Camera construction, without
relying solely on assert; ensure direct render calls are guarded. In Camera and
the affected raster, quad, and render_probe code, replace every M_PI use with
std::numbers::pi and include the appropriate C++20 numbers header.
In `@src/render/jpeg.h`:
- Around line 17-18: Update the encode_jpeg declaration comment and
implementation guards to require image.rgb to contain at least width * height *
3 bytes; return the existing empty output when this precondition is not met,
alongside the current empty, width, and height checks.
In `@src/render/raster.cpp`:
- Around line 170-189: Optimize Rasterizer::draw_background to avoid calling
camera.ray for every pixel. Compute dir[2] per row using the row ray and the
camera’s horizontal ray step, or cache the background for an unchanged camera
pose, while preserving the existing below-horizon classification and sky/ground
color blending.
In `@src/render/render_service.h`:
- Around line 61-62: Update the frame publication flow in render_service.cpp to
set Frame::sim_time_s from the simulation time associated with the vehicle pose,
and extend render_service.h’s frame() API or the camera route to expose that
timestamp alongside the JPEG data. Ensure consumers receive the pose’s actual
simulation time rather than the current constant 0.0.
In `@src/render/shading.h`:
- Around line 22-46: The kSunDirNed direction constant is slightly non-unit
while shade_for_normal uses it directly for Lambert lighting. Normalize the
literal vector values, or explicitly document that the deviation is intentional
and accepted; preserve the existing shading behavior and symbol usage in
shade_for_normal.
In `@tests/test_render.cpp`:
- Around line 79-83: Update the failure path in the test function surrounding
cook_obj_tile to remove the temporary skysim_render_tiles directory before
returning 1. Reuse the existing temporary-directory cleanup mechanism so failed
runs cannot leave stale files for subsequent load_tiles calls.
🪄 Autofix
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: 14f4f304-08a4-44c9-89c9-36a0507068df
⛔ Files ignored due to path filters (1)
.github/assets/skysim-getting-started.jpgis excluded by!**/*.jpg
📒 Files selected for processing (22)
CMakeLists.txtREADME.mdsrc/api/control_server.cppsrc/api/control_server.hsrc/core/world.cppsrc/core/world.hsrc/main.cppsrc/render/camera.hsrc/render/frame_store.hsrc/render/image.hsrc/render/jpeg.cppsrc/render/jpeg.hsrc/render/raster.cppsrc/render/raster.hsrc/render/render_service.cppsrc/render/render_service.hsrc/render/renderer.cppsrc/render/renderer.hsrc/render/shading.htests/test_render.cpptools/getting_started.shtools/render_probe/main.cpp
The coverage gate caught what the tests did not: RenderService and FrameStore were at 0%, which is to say the thing that actually renders in flight was never run by a test. Both its failure modes are silent — a world that loads no geometry, and a thread that never produces a frame — and both look from outside like a working server serving nothing. So: the service loads its tiles, publishes a pose, and has to produce a real JPEG for that vehicle and nothing for any other, with the destructor joining its thread rather than hanging. FrameStore keeps one frame per vehicle, replaced whole. The four camera endpoints are exercised through the real HTTP stack for both the camera-off and camera-on cases, including instance-to-id resolution and the two ways a request can legitimately find no picture. Back over the gates: lines 80.7 -> 86.5%, functions 87.4 -> 96.0%, branches 66.4 -> 72.8%. Also drops a comment in frame_store.h that still described rendering as happening on the tick thread. It moved off it precisely because that starved the physics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ
…e leak CodeRabbit found more than nits. In order of how much they mattered: - The ray caster's synthesized ground hit left is_ground false, so every pixel taking that fallback was painted building grey on the ground plane. I introduced this in the same change that stopped guessing from the normal — the fallback constructs its own RayHit and I did not update it. - The two camera routes parsed vehicle ids with std::stoul, which throws on anything wider than unsigned long. /vehicles/99999999999999999999/camera.jpg was an uncaught exception; every other handler in the file already used strtoul, which saturates. - Frames of despawned vehicles were never dropped, so the camera route kept serving a picture of an aircraft that no longer existed and FrameStore grew for the life of the process. The pose list is already the whole fleet, so it is the natural owner of "which vehicles exist": anything absent is retained no longer. sim_time_s is carried through from the tick rather than stored as 0, so a consumer can tell a fresh frame from a stalled one. - The rasteriser hazed against camera-space depth while the ray caster used radial distance — about 25% apart at the edge of a 78 degree frame, on a pair of renderers whose shared header says they must draw the same picture. Both now work in squared distance, which also removes a division: 0.67 -> 0.58 ms/frame. - haze_at divided by max_range_m without checking it, and the NaN survived all the way to a cast that is undefined behaviour. - --camera-threads, --camera-quality and --frame-grace were unvalidated, and each fails silently rather than loudly: an undefined pool size, stb reading quality 0 as 90, and a grace above 1.0 stretching every tick past its period. - render_probe reported success when it could not open or write its output. - The getting-started probes could hang forever against a server that accepts the connection and says nothing. Skipped: bounding what the render service loads by radius or tile cap. It is a fair point — the physics world caps resident tiles and this one does not — but the Jolt shapes are already released after extraction, leaving only the triangle list, and doing it properly means streaming against the fleet's position. Worth its own change, not this one. Coverage stays over the gates: 86.4% lines, 96.0% functions, 72.7% branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/render/shading.h (1)
73-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the non-positive range guard.
haze_atsquaresmax_range_mbeforehaze_at_sqvalidates it. A negativemax_range_mthen becomes positive and returns partial haze instead ofkMaxHaze.Validate
max_range_mbefore squaring it.Proposed fix
inline double haze_at(double distance_m, double max_range_m) { + if (!(max_range_m > 0.0)) { + return kMaxHaze; + } return haze_at_sq(distance_m * distance_m, max_range_m * max_range_m); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/render/shading.h` around lines 73 - 74, Update haze_at so it checks max_range_m for the existing non-positive-range condition before squaring it; return kMaxHaze for non-positive values, and retain the current haze_at_sq delegation for valid positive ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/render/frame_store.h`:
- Around line 3-8: Add the direct <utility> include alongside the standard
headers in frame_store.h so std::move used by publish is declared and the header
remains self-contained.
In `@tests/test_api.cpp`:
- Around line 262-266: Rename the constant vector in the camera-frame test from
canned to kCanned, and update its capture and return references in the lambda
assigned to with_camera.camera_frame.
- Around line 253-255: Update the camera endpoint checks in the test around
client.Get to store each cpp-httplib result before accessing it, and validate
the result is non-null before asserting status == 404. Apply this to all three
requests while preserving the existing expected status.
- Around line 293-303: Update the MJPEG callback in the client.Get loop to
continue receiving data until the kCanned JPEG payload is present, rather than
stopping at the multipart header boundary. Add an assertion that got contains
the expected kCanned byte sequence before cancelling the response, while
preserving the existing multipart and Content-Type checks.
---
Duplicate comments:
In `@src/render/shading.h`:
- Around line 73-74: Update haze_at so it checks max_range_m for the existing
non-positive-range condition before squaring it; return kMaxHaze for
non-positive values, and retain the current haze_at_sq delegation for valid
positive ranges.
🪄 Autofix
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 Plus
Run ID: 0278f27d-2122-4a75-9e1b-1f7217e31723
⛔ Files ignored due to path filters (1)
.github/assets/skysim-getting-started.jpgis excluded by!**/*.jpg
📒 Files selected for processing (14)
README.mdsrc/api/control_server.cppsrc/core/world.hsrc/main.cppsrc/render/frame_store.hsrc/render/raster.cppsrc/render/render_service.cppsrc/render/render_service.hsrc/render/renderer.cppsrc/render/shading.htests/test_api.cpptests/test_render.cpptools/getting_started.shtools/render_probe/main.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- src/api/control_server.cpp
- src/render/renderer.cpp
- src/render/render_service.cpp
- src/render/raster.cpp
- src/main.cpp
The stream callback stopped at the blank line after the multipart header, so the test would have passed against a server that announces a JPEG and then sends none of it. It reads until the frame's actual bytes appear and asserts them. Also: three requests in the camera-off block were dereferenced inline, which crashes the run rather than failing it when a request never reaches the server; frame_store.h uses std::move without including <utility>; and the canned payload now carries the k prefix the rest of the file uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ
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.
SITL vehicles flew through a world nobody could look at. The video feed in SkyHub was a canned MP4, so a survey flown in the simulator came back with footage of somewhere else entirely.
What this does
A software rasteriser draws the world the physics already knows about, on its own thread with its own copy of the world. The screen is cut into horizontal bands with one thread owning each, so there are no locks or atomics in the inner loop. Each vehicle's feed is served off the control plane:
GET /vehicles/{id}/camera.jpg/.mjpgGET /instances/{n}/camera.jpg/.mjpg— addressed by ArduPilot instance, which survives skysim restarting0.67 ms/frame at 256x144 on four threads (1500 fps headroom against a 10 fps feed).
Getting started
tools/getting_started.shbuilds, cooks a 1.2 km demo city and renders it from four poses in about a second — no autopilot, no network, no simulator running. You can see the thing work before committing to the half hour the ArduPilot build takes.With
ARDUPILOT_ROOTset,--flyputs a vehicle in that city and serves its camera live. Verified end to end on this branch: handshake completes,connected: true, MJPEG streams.Two bugs found while looking at the output
Looking at rendered frames instead of at "did it produce bytes" turned up two defects that a pixel test would not have caught but an eye does:
tests/test_render.cppgained a regression test — verified it fails when the heuristic is put back.Notes
Testing
ctest --test-dir build— 17/17 passtools/getting_started.shand--flyboth run clean🤖 Generated with Claude Code
https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ
Summary by CodeRabbit
New Features
render_probetool for generating view images.Tests