Reinstate SensorRaycast - #3097
Merged
adenzler-nvidia merged 5 commits intoJun 8, 2026
Merged
Conversation
SensorRaycast was deprecated in 1.2 and removed in newton-physics#2971, but the single-cycle deprecation policy had not been announced when the sensor was deprecated, so the removal was premature. Restore the sensor and its tests; it stays deprecated in favor of SensorTiledCamera. The removal commit also rewrote raycast.py (HFIELD now uses a wp.Mesh BVH, dropping the per-thread DDA grid), so this is not a plain revert. The kernels the sensor relied on are restored on top of the new standalone ray_intersect_geom: a single sensor_raycast_kernel (the gated no-HFIELD variant is gone), plus sensor_raycast_particles_kernel and ray_for_pixel. HFIELD shapes raycast through their wp.Mesh id in shape_source_ptr, so heightfield support is retained without the removed heightfield-array kernel plumbing. The intersect_ray function and BVH-based HFIELD raycasting added by newton-physics#2971 are left intact.
Contributor
|
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)
📝 WalkthroughWalkthroughReintroduces a deprecated SensorRaycast depth sensor with new Warp-based per-pixel geometry and particle raycasting, exports it publicly, adds comprehensive tests, and updates docs to mark it deprecated in favor of SensorTiledCamera. ChangesSensorRaycast Sensor Implementation and Testing
Sequence Diagram(s)sequenceDiagram
participant Client
participant SensorRaycast
participant GeometryKernels
participant HashGrid
participant DepthBuffer
Client->>SensorRaycast: initialize(...) / update(...)
SensorRaycast->>GeometryKernels: sensor_raycast_kernel (shapes)
GeometryKernels->>DepthBuffer: atomic_min write (shape hits)
SensorRaycast->>HashGrid: build / query (if include_particles)
SensorRaycast->>GeometryKernels: sensor_raycast_particles_kernel (particles)
GeometryKernels->>DepthBuffer: atomic_min write (particle hits)
SensorRaycast->>DepthBuffer: clamp max-distance to -1.0
SensorRaycast-->>Client: get_depth_image / get_depth_image_numpy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (4)
newton/_src/geometry/raycast.py (1)
1199-1199: 💤 Low valueRemove unnecessary
int()cast.The literal
0is already an integer; the explicit cast is redundant.- candidate = int(0) + candidate = 0🤖 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/_src/geometry/raycast.py` at line 1199, The assignment "candidate = int(0)" uses an unnecessary int() cast; change it to a plain integer literal by assigning "candidate = 0" where the variable candidate is initialized in the raycast logic (look for the candidate variable in raycast.py around the initialization point).Source: Linters/SAST tools
newton/_src/sensors/sensor_raycast.py (1)
120-120: ⚡ Quick winAdd SI units to
max_distancedocstring.Per coding guidelines, public API docstrings should specify SI units for physical quantities.
- max_distance: Maximum ray distance; rays beyond this return no hit + max_distance: Maximum ray distance [m]; rays beyond this return no 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/_src/sensors/sensor_raycast.py` at line 120, Update the docstring for the public API that documents the parameter max_distance in newton._src.sensors.sensor_raycast (the docstring near the definition of the raycast-related function/class that contains the parameter name max_distance) to include SI units (meters, "m") — e.g., change "Maximum ray distance" to "Maximum ray distance (meters, m)". Ensure the updated docstring keeps the same phrasing and formatting style as other parameters in the module.Source: Coding guidelines
newton/tests/test_sensor_raycast.py (2)
164-183: ⚡ Quick winRemove verbose comments that narrate obvious loop steps.
Lines 164-181 contain multiple inline comments that restate what each line does ("Render each cube map face", "Update camera pose", "Evaluate the sensor", "Get depth image", "Count hits", "Verify each face", "Save depth image"). As per coding guidelines, these narrate obvious steps and should be removed. Keep only the comment at line 139 which explains why
eval_fkis needed.♻️ Suggested refactor
- # Render each cube map face for view_name, position, direction, up in cubemap_views: - # Update camera pose for this view sensor.update_camera_pose(position=position, direction=direction, up=up) - - # Evaluate the sensor sensor.update(state) - - # Get depth image depth_image = sensor.get_depth_image_numpy() - - # Count hits for this view hits_in_view = np.sum(depth_image > 0) - - # Verify each face has at least one hit test.assertGreater(hits_in_view, 0, f"Face {view_name} should detect at least one object hit") - # Save depth image (if enabled) if EXPORT_IMAGES: save_depth_image_as_grayscale(depth_image, f"cubemap_{view_name}")🤖 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_raycast.py` around lines 164 - 183, Remove redundant inline comments in the cubemap face loop that simply narrate each step; keep only the explanatory comment about why eval_fk is needed (line ~139). Specifically, delete the comments around the loop that describe "Render each cube map face", "Update camera pose", "Evaluate the sensor", "Get depth image", "Count hits", "Verify each face", and "Save depth image", leaving the code blocks invoking sensor.update_camera_pose(position, direction, up), sensor.update(state), depth_image = sensor.get_depth_image_numpy(), hits_in_view = np.sum(depth_image > 0), test.assertGreater(hits_in_view, 0, ...), and the EXPORT_IMAGES conditional with save_depth_image_as_grayscale intact.Source: Coding guidelines
32-67: ⚡ Quick winRemove verbose inline comments that restate code.
Lines 50 and 61 narrate what the code already shows. As per coding guidelines, inline comments should be brief and explain why or non-obvious intent, not what the code does.
♻️ Suggested refactor
# Replace -1.0 (no hit) with 0 (black) img_data[img_data < 0] = 0 - # Normalize positive values to 0-255 range pos_mask = img_data > 0 if np.any(pos_mask): pos_vals = img_data[pos_mask] min_depth = pos_vals.min() max_depth = pos_vals.max() denom = max(max_depth - min_depth, 1e-6) # Invert: closer objects = brighter, farther = darker # Scale to 50-255 range (so background/no-hit stays at 0) img_data[pos_mask] = 255 - ((pos_vals - min_depth) / denom) * 205 - # Convert to uint8 and save img_data = np.clip(img_data, 0, 255).astype(np.uint8) image = Image.fromarray(img_data)🤖 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_raycast.py` around lines 32 - 67, In save_depth_image_as_grayscale, remove the verbose inline comments that simply restate the code (e.g., the comments explaining replacing -1.0 with 0 and the normalization/inversion scaling logic) and keep or replace them with a short, intent-focused comment if needed (for example a one-liner about treating -1.0 as no-hit or why we invert/scale to 50–255). Update comments near img_data[img_data < 0] = 0 and the block that computes min_depth/max_depth/denom so they explain intent or non-obvious choices rather than narrate the exact operations.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 `@newton/tests/test_sensor_raycast.py`:
- Line 1: Update the SPDX copyright header year from 2025 to 2026 by editing the
SPDX-FileCopyrightText line (the SPDX header at top of the file, e.g., the line
starting with "# SPDX-FileCopyrightText") so it reads 2026 instead of 2025.
---
Nitpick comments:
In `@newton/_src/geometry/raycast.py`:
- Line 1199: The assignment "candidate = int(0)" uses an unnecessary int() cast;
change it to a plain integer literal by assigning "candidate = 0" where the
variable candidate is initialized in the raycast logic (look for the candidate
variable in raycast.py around the initialization point).
In `@newton/_src/sensors/sensor_raycast.py`:
- Line 120: Update the docstring for the public API that documents the parameter
max_distance in newton._src.sensors.sensor_raycast (the docstring near the
definition of the raycast-related function/class that contains the parameter
name max_distance) to include SI units (meters, "m") — e.g., change "Maximum ray
distance" to "Maximum ray distance (meters, m)". Ensure the updated docstring
keeps the same phrasing and formatting style as other parameters in the module.
In `@newton/tests/test_sensor_raycast.py`:
- Around line 164-183: Remove redundant inline comments in the cubemap face loop
that simply narrate each step; keep only the explanatory comment about why
eval_fk is needed (line ~139). Specifically, delete the comments around the loop
that describe "Render each cube map face", "Update camera pose", "Evaluate the
sensor", "Get depth image", "Count hits", "Verify each face", and "Save depth
image", leaving the code blocks invoking sensor.update_camera_pose(position,
direction, up), sensor.update(state), depth_image =
sensor.get_depth_image_numpy(), hits_in_view = np.sum(depth_image > 0),
test.assertGreater(hits_in_view, 0, ...), and the EXPORT_IMAGES conditional with
save_depth_image_as_grayscale intact.
- Around line 32-67: In save_depth_image_as_grayscale, remove the verbose inline
comments that simply restate the code (e.g., the comments explaining replacing
-1.0 with 0 and the normalization/inversion scaling logic) and keep or replace
them with a short, intent-focused comment if needed (for example a one-liner
about treating -1.0 as no-hit or why we invert/scale to 50–255). Update comments
near img_data[img_data < 0] = 0 and the block that computes
min_depth/max_depth/denom so they explain intent or non-obvious choices rather
than narrate the exact operations.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5d00f26f-8773-4e6e-beee-143f491dcd24
📒 Files selected for processing (8)
CHANGELOG.mddocs/api/newton_sensors.rstdocs/concepts/sensors.rstnewton/_src/geometry/raycast.pynewton/_src/sensors/sensor_raycast.pynewton/sensors.pynewton/tests/test_sensor_raycast.pynewton/tests/test_sensor_tiled_camera_heightfield.py
💤 Files with no reviewable changes (1)
- CHANGELOG.md
The migration snippet in the SensorRaycast docstring called newton.geometry.build_bvh_shape(model, state), which was itself deprecated in 1.3 in favor of model.bvh_build_shapes(state). Point the guidance for the deprecated sensor at the non-deprecated API.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
jcarius-nv
previously approved these changes
Jun 8, 2026
jcarius-nv
left a comment
jcarius-nv
left a comment
Member
There was a problem hiding this comment.
Thanks for the detailed PR context. I reviewed this as a targeted restoration of a prematurely removed deprecated API, not as a plain revert.
The approach looks sound to me. The restored SensorRaycast implementation preserves the prior API shape while adapting the geometry path to the newer
standalone ray_intersect_geom flow. I also checked the HFIELD behavior called out in the description: the mesh-backed shape_source_ptr path works as expected
in a manual smoke test.
Validation I ran locally from PR head a20046ac:
uv run --extra dev -m newton.tests -k test_sensor_raycastuv run --extra dev -m newton.tests -k test_raycastuv run --extra dev -m newton.tests -k test_pickinguv run --extra dev python docs/generate_api.py
All passed, and docs/generate_api.py produced no API-doc drift.
No blocking findings from me. The only small follow-up I’d consider is adding a dedicated SensorRaycast HFIELD regression test, since the PR specifically
preserves that behavior, but I don’t think that should block this change.
camevor
previously approved these changes
Jun 8, 2026
camevor
left a comment
camevor
left a comment
Member
There was a problem hiding this comment.
Thanks, agree that this looks good!
The reinstated SensorRaycast raycasts HFIELD shapes through their wp.Mesh BVH via shape_source_ptr, but the new test suite covered only analytic shapes and particles. The prior heightfield coverage had migrated to SensorTiledCamera when the sensor was removed, leaving the sensor's HFIELD path untested end to end -- exactly the layer that broke in newton-physics#2560 (sensor ignoring HFIELD geometry). Mirror the test_sensor_tiled_camera_heightfield scenario against the SensorRaycast API: a flat terrain seen from directly above must register hits at the expected depth.
Member
Author
|
added a regression test. Thanks for the reviews! |
camevor
reviewed
Jun 8, 2026
camevor
previously approved these changes
Jun 8, 2026
camevor
left a comment
camevor
left a comment
Member
There was a problem hiding this comment.
The height field regression test looks good.
No blockers , but since the tiled camera renders height fields through the mesh query path, we should confirm whether the high miss rate translates to the preferred tiled camera sensor.
The initial test carried over a "~10-15% of rays miss" rationale and an 80% hit threshold from test_sensor_tiled_camera_heightfield. That figure reflects the tiled camera's default backface culling; SensorRaycast runs with culling off and hits every ray for this scene (verified on CPU and CUDA, robust to small camera offsets). Assert all pixels hit, with tight depth bounds, and drop the inaccurate comment.
adenzler-nvidia
enabled auto-merge
June 8, 2026 13:54
jcarius-nv
approved these changes
Jun 8, 2026
3 tasks
adenzler-nvidia
added a commit
that referenced
this pull request
Jun 8, 2026
3 tasks
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
3 participants
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.
Description
#2971 removed
SensorRaycast, which had been deprecated in 1.2. The single-cycle deprecation policy had not been announced when the sensor was deprecated, so the removal was premature. This PR reinstates the sensor (still deprecated in favor ofSensorTiledCamera) and its test suite.Because #2971 also rewrote
raycast.py—GeoType.HFIELDnow raycasts through awp.MeshBVH instead of the per-thread DDA grid, andray_intersect_geom/raycast_kernelbecame standalone — this is not a plain revert. The kernels the sensor relied on are restored on top of the newray_intersect_geom: a singlesensor_raycast_kernel(the gated no-HFIELD variant is gone), plussensor_raycast_particles_kernelandray_for_pixel. HFIELD shapes raycast via theirwp.Meshid inshape_source_ptr, so heightfield support is retained without the removed heightfield-array kernel plumbing. Theintersect_rayhelper and BVH-based HFIELD raycasting introduced by #2971 are left intact.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Summary by CodeRabbit
New Features
Documentation
Tests
Changelog