Render tiled-camera heightfields via the mesh path - #3088
Conversation
newton-physics#2971 added HFIELD ray-intersection to the tiled-camera render kernels but never wired heightfields into the render BVH: is_supported_shape_type and the local- and world-bounds kernels all omitted HFIELD, so heightfields never rendered. The unreachable branch also enlarged the color render kernel enough to lower its GPU occupancy, regressing tiled-camera rendering. Heightfields are already triangulated wp.Mesh shapes with no per-vertex normals, so render them through the existing mesh path: add HFIELD to the render-BVH inclusion and bounds, and have RenderContext present heightfields to the render kernels as MESH. This removes the HFIELD branch from the kernels entirely, so the render kernel is unchanged from before newton-physics#2971 while gaining heightfield support. model.shape_type is left untouched for collision and BVH bounds.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes ChangesHeightfield rendering via mesh-based path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
|
benchmarks one instance of the regression |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
newton/tests/test_sensor_tiled_camera_heightfield.py (2)
57-62: ⚡ Quick winTighten the hit-coverage assertion.
This setup should only lose a small edge fraction, so
hit > res * res // 2would still pass if a large chunk of the heightfield vanished. A near-full-coverage threshold will make this regression test much harder to fool.Proposed assertion tightening
- self.assertGreater( - hit, res * res // 2, msg=f"heightfield should fill most of the view; only {hit}/{res * res} pixels hit" - ) + min_expected_hits = int(res * res * 0.9) + self.assertGreaterEqual( + hit, + min_expected_hits, + msg=f"heightfield should fill nearly the whole view; only {hit}/{res * res} pixels hit", + )🤖 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 `@newton/tests/test_sensor_tiled_camera_heightfield.py` around lines 57 - 62, Replace the weak coverage check (self.assertGreater(hit, res * res // 2)) with a stricter requirement so almost all pixels must hit the heightfield; e.g., require hit >= int(res * res * 0.95) (or another high fraction like 0.9) and update the failure message to reflect the new threshold; locate the assertion referencing variables hit and res in test_sensor_tiled_camera_heightfield.py and change the numeric threshold and message accordingly.
53-55: ⚡ Quick winRemove the explicit sync before
depth.numpy().
.numpy()already blocks for the device-to-host copy, so Line 53 just adds an extra barrier and violates the Warp-array guideline.As per coding guidelines, "Never call
wp.synchronize()orwp.synchronize_device()right before.numpy()on a Warp array, as.numpy()performs a synchronous device-to-host copy that completes all outstanding work".🤖 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 `@newton/tests/test_sensor_tiled_camera_heightfield.py` around lines 53 - 55, Remove the redundant explicit synchronization before converting the Warp array to NumPy: in the test function (test_sensor_tiled_camera_heightfield) delete the wp.synchronize() call that appears immediately before the depth.numpy() call (the lines where wp.synchronize() is called then d = depth.numpy()[0, 0]); .numpy() already performs a synchronous device-to-host copy, so simply call depth.numpy() directly without the preceding wp.synchronize().
🤖 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.
Nitpick comments:
In `@newton/tests/test_sensor_tiled_camera_heightfield.py`:
- Around line 57-62: Replace the weak coverage check (self.assertGreater(hit,
res * res // 2)) with a stricter requirement so almost all pixels must hit the
heightfield; e.g., require hit >= int(res * res * 0.95) (or another high
fraction like 0.9) and update the failure message to reflect the new threshold;
locate the assertion referencing variables hit and res in
test_sensor_tiled_camera_heightfield.py and change the numeric threshold and
message accordingly.
- Around line 53-55: Remove the redundant explicit synchronization before
converting the Warp array to NumPy: in the test function
(test_sensor_tiled_camera_heightfield) delete the wp.synchronize() call that
appears immediately before the depth.numpy() call (the lines where
wp.synchronize() is called then d = depth.numpy()[0, 0]); .numpy() already
performs a synchronous device-to-host copy, so simply call depth.numpy()
directly without the preceding wp.synchronize().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1c7f06d5-aa54-4977-be1f-a9395ff10f83
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/geometry/bvh.pynewton/_src/sensors/warp_raytrace/raytrace.pynewton/_src/sensors/warp_raytrace/render_context.pynewton/tests/test_sensor_tiled_camera_heightfield.py
Tighten the coverage assertion from "> half the frame" to ">= 80%", and drop the wp.synchronize() before .numpy() (which already performs a synchronizing device-to-host copy). The terrain covers the whole frame, but a stable ~10-15% of rays miss along triangle edges (non-watertight mesh_query_ray) regardless of resolution or camera offset, so a 90-95% threshold would be flaky; the depth-value assertions remain the primary correctness check.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
SensorTiledCameradid not render heightfield (HFIELD) shapes, and #2971 introduced a tiled-camera render-performance regression. Both stem from the same gap.#2971 added an
HFIELDray-intersection branch to the three tiled-camera render kernels, but heightfields were never wired into the render BVH:is_supported_shape_type,compute_shape_local_bounds, andcompute_shape_bvh_bounds(all ingeometry/bvh.py) omittedHFIELD, so heightfields never entered the visible-shape BVH and never reached the renderer. The added branch was therefore unreachable, yet it still enlarged the color render megakernel's register footprint, dropping it below full GPU occupancy and regressing tiled-camera rendering.Fix
Heightfields are already triangulated
wp.Meshshapes — their mesh is stored inshape_source_ptrand they carry no per-vertex normals — so they can be rendered through the existing mesh path rather than a dedicated branch:HFIELDto the render-BVH inclusion (is_supported_shape_type) and to both the local- and world-bounds kernels, so heightfields enter the visible-shape BVH with correct AABBs.RenderContextpresents heightfields to the render kernels asMESH(via a per-shape render-type array passed to the kernel).model.shape_typeis left asHFIELDfor collision and BVH bounds.HFIELDbranch is removed from the render kernels entirely. The render kernel is now identical to its pre-Add intersect_ray function that uses BVH accelerated raycasting, Remove SensorRaycast #2971 form, so it regains full occupancy while gaining heightfield support, with no per-scene kernel specialization.Tests
Adds
newton/tests/test_sensor_tiled_camera_heightfield.py, which renders a flat heightfield from above and verifies the depth is geometrically correct. The existing tiled-camera, heightfield-collision, and raycast suites continue to pass.Summary by CodeRabbit
Release Notes