Add intersect_ray function that uses BVH accelerated raycasting, Remove SensorRaycast - #2971
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBuilds per-heightfield Warp meshes in ModelBuilder.finalize(), unifies ray intersection via a mesh_id-based dispatcher and BVH kernel, exposes newton.intersect_ray(), removes SensorRaycast from exports/docs, and updates callers and tests to the new interfaces. ChangesHeightfield Raycasting and Ray API Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant intersect_ray
participant _intersect_ray_kernel
participant BVH
participant ray_intersect_geom
Client->>intersect_ray: call(model, ray_origins, ray_dirs, out_*)
intersect_ray->>_intersect_ray_kernel: launch BVH kernel with model BVH & shape arrays
_intersect_ray_kernel->>BVH: query BVH nodes for candidate triangles/shape_ids
BVH-->>_intersect_ray_kernel: candidate mesh_id + tri info
_intersect_ray_kernel->>ray_intersect_geom: call(mesh_id, geom transform, ray)
ray_intersect_geom-->>_intersect_ray_kernel: (t_hit, normal)
_intersect_ray_kernel-->>intersect_ray: write out_dist, out_shape_id, out_normal
intersect_ray-->>Client: outputs populated in provided buffers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
![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: 2
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
100-121: ⚡ Quick winVectorize the heightfield mesh build path.
This helper now runs during
finalize()for every uniqueHeightfield, so the nested Python loops will add noticeable setup latency on large terrains. NumPy broadcasting can build the same vertex/index buffers in bulk and keep the BVH fast path from paying Python overhead up front.♻️ Proposed refactor
- verts = np.empty((nrow * ncol, 3), dtype=np.float32) - for r in range(nrow): - for c in range(ncol): - idx = r * ncol + c - verts[idx, 0] = -hf.hx + c * dx - verts[idx, 1] = -hf.hy + r * dy - verts[idx, 2] = hf.min_z + float(hf.data[r, c]) * z_range - - num_cells = (nrow - 1) * (ncol - 1) - indices = np.empty(num_cells * 6, dtype=np.int32) - i = 0 - for r in range(nrow - 1): - for c in range(ncol - 1): - v00 = r * ncol + c - v10 = r * ncol + (c + 1) - v01 = (r + 1) * ncol + c - v11 = (r + 1) * ncol + (c + 1) - # Triangle 0: (p00, p10, p11) — CCW from above - indices[i : i + 3] = [v00, v10, v11] - # Triangle 1: (p00, p11, p01) — CCW from above - indices[i + 3 : i + 6] = [v00, v11, v01] - i += 6 + xs = np.linspace(-hf.hx, hf.hx, ncol, dtype=np.float32) + ys = np.linspace(-hf.hy, hf.hy, nrow, dtype=np.float32) + grid_x, grid_y = np.meshgrid(xs, ys, indexing="xy") + grid_z = hf.min_z + np.asarray(hf.data, dtype=np.float32) * z_range + verts = np.stack((grid_x, grid_y, grid_z), axis=-1).reshape(-1, 3) + + cell_rows = np.arange(nrow - 1, dtype=np.int32)[:, None] + cell_cols = np.arange(ncol - 1, dtype=np.int32)[None, :] + v00 = (cell_rows * ncol + cell_cols).ravel() + v10 = v00 + 1 + v01 = v00 + ncol + v11 = v01 + 1 + indices = np.stack( + ( + v00, + v10, + v11, + v00, + v11, + v01, + ), + axis=-1, + ).reshape(-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 `@newton/_src/sim/builder.py` around lines 100 - 121, The nested loops building verts and indices (in finalize()/heightfield mesh path) should be replaced with NumPy vectorized operations: use np.arange and np.meshgrid (or broadcasting) over r and c to produce flattened row/col arrays, compute verts[:,0], verts[:,1], and verts[:,2] in bulk from hx/hy/dx/dy and hf.data.ravel() instead of per-element assignment, and build the triangle index arrays by computing the four corner indices v00/v10/v01/v11 as vectorized arrays for all cells and then stacking the two triangle index patterns ([v00,v10,v11] and [v00,v11,v01]) into the flat indices buffer; keep dtype and shapes the same (verts, indices) and ensure ordering matches the existing CCW winding.
🤖 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 `@CHANGELOG.md`:
- Line 22: Update the Changed entry for GeoType.HFIELD to include explicit
migration guidance: tell callers that the raycast kernel no longer accepts
shape_heightfield_index, heightfield_data, or heightfield_elevations and that
direct callers of geometry.raycast_kernel should stop passing those arrays and
instead use the mesh-backed raycast path built during ModelBuilder.finalize()
(via wp.Mesh BVH) by routing through model.shape_source_ptr; also note that
Model still retains the old heightfield arrays for collision kernels that still
rely on them.
In `@newton/_src/geometry/raycast.py`:
- Around line 903-942: The intersect_ray wrapper currently dereferences
model.bvh_shapes, model.bvh_shapes_group_roots and
model.bvh_shape_world_transforms unconditionally; add a guard at the top of
intersect_ray that checks these fields are not None and raises a clear exception
(e.g., ValueError) instructing the caller to call build_bvh_shape() or
refit_bvh_shape() first if they are missing, and update the function docstring
to remove the nonexistent `state` parameter and document the prerequisite that
the shape BVH/world transforms must be built via build_bvh_shape() or
refit_bvh_shape() before calling intersect_ray.
---
Nitpick comments:
In `@newton/_src/sim/builder.py`:
- Around line 100-121: The nested loops building verts and indices (in
finalize()/heightfield mesh path) should be replaced with NumPy vectorized
operations: use np.arange and np.meshgrid (or broadcasting) over r and c to
produce flattened row/col arrays, compute verts[:,0], verts[:,1], and verts[:,2]
in bulk from hx/hy/dx/dy and hf.data.ravel() instead of per-element assignment,
and build the triangle index arrays by computing the four corner indices
v00/v10/v01/v11 as vectorized arrays for all cells and then stacking the two
triangle index patterns ([v00,v10,v11] and [v00,v11,v01]) into the flat indices
buffer; keep dtype and shapes the same (verts, indices) and ensure ordering
matches the existing CCW winding.
🪄 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: 247852f1-2833-4cf7-a7ae-8e83a047d7ff
📒 Files selected for processing (16)
CHANGELOG.mddocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstdocs/concepts/sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/raycast.pynewton/_src/sensors/sensor_raycast.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/viewer/picking.pynewton/geometry.pynewton/sensors.pynewton/tests/test_raycast.pynewton/tests/test_sensor_raycast.py
💤 Files with no reviewable changes (5)
- docs/api/newton_sensors.rst
- docs/concepts/sensors.rst
- newton/tests/test_sensor_raycast.py
- newton/_src/sensors/sensor_raycast.py
- newton/sensors.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
StafaH
marked this pull request as ready for review
June 1, 2026 07:05
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
newton/_src/geometry/raycast.py (1)
903-942:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing prerequisite guard and stale docstring parameter.
The past review comment about this issue is still valid:
Stale docstring: Line 915 references a
stateparameter that doesn't exist in the function signature.Missing None guard: The function dereferences
model.bvh_shapes.id,model.bvh_shapes_group_roots,model.bvh_shape_enabled, andmodel.bvh_shape_world_transformswithout checking if they areNone. Ifbuild_bvh_shape()was not called, these will crash with an unhelpfulAttributeErrororTypeError.Suggested fix
def intersect_ray( model, ray_origins: wp.array2d[wp.vec3], ray_directions: wp.array2d[wp.vec3], out_dist: wp.array2d[float], out_shape_id: wp.array2d[wp.int32], out_normal: wp.array2d[wp.vec3], ) -> None: """Intersect rays with model shapes for all worlds. + Requires a built shape BVH. Call :func:`build_bvh_shape` before this + function and :func:`refit_bvh_shape` after transforms change. + Args: model: Model containing the shapes to query. - state: State containing current body transforms. ray_origins: Ray origins in world space [m]. ray_directions: Ray directions in world space. Values must be normalized and nonzero. out_dist: Output array of hit distances. out_shape_id: Output array of hit shape indices. out_normal: Output array of hit normals. """ + if ( + model.bvh_shapes is None + or model.bvh_shapes_group_roots is None + or model.bvh_shape_enabled is None + or model.bvh_shape_world_transforms is None + ): + raise RuntimeError( + "intersect_ray() requires a built shape BVH. " + "Call build_bvh_shape(model, state) first." + ) wp.launch(🤖 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` around lines 903 - 942, The intersect_ray docstring and precondition checks need tightening: remove the stale reference to a non-existent state parameter from intersect_ray's docstring, and add a guard before dereferencing model.bvh_shapes.id / model.bvh_shapes_group_roots / model.bvh_shape_enabled / model.bvh_shape_world_transforms to detect when the BVH has not been built; if any of those are None, raise a clear ValueError (or similar) instructing the caller to run build_bvh_shape() on the model (mention intersect_ray and the bvh fields in the message) instead of letting AttributeError/TypeError occur.
🤖 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.
Duplicate comments:
In `@newton/_src/geometry/raycast.py`:
- Around line 903-942: The intersect_ray docstring and precondition checks need
tightening: remove the stale reference to a non-existent state parameter from
intersect_ray's docstring, and add a guard before dereferencing
model.bvh_shapes.id / model.bvh_shapes_group_roots / model.bvh_shape_enabled /
model.bvh_shape_world_transforms to detect when the BVH has not been built; if
any of those are None, raise a clear ValueError (or similar) instructing the
caller to run build_bvh_shape() on the model (mention intersect_ray and the bvh
fields in the message) instead of letting AttributeError/TypeError occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7c334449-58fa-46bc-ac3e-817fce422871
📒 Files selected for processing (1)
newton/_src/geometry/raycast.py
Member
|
This looks all good to me! :D Only question I'd have, do we still really need that raycast kernel? Because it really doesn't do anything that the render kernel doesn't do as well, does it? It is only used for the viewer-picking as far as I can tell, right? While I personally would probably be in favor of using the render-kernel for this (assuming the performance would be the same), I don't have a super uber strong opinion on this and can see reasons for keeping it separated as well. @mmacklin @eric-heiden do you guys have an opinion? |
daniela-hase
previously approved these changes
Jun 1, 2026
Member
There was a problem hiding this comment.
Approving, but lets wait for @mmacklin / @eric-heiden to comment on my above before we merge please :)
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
reviewed
Jun 1, 2026
eric-heiden
previously approved these changes
Jun 2, 2026
eric-heiden
enabled auto-merge
June 2, 2026 18:32
eric-heiden
approved these changes
Jun 2, 2026
jkuzmeski
pushed a commit
to jkuzmeski/newton
that referenced
this pull request
Jun 2, 2026
This was referenced Jun 5, 2026
3 tasks
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
5 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.
Add
intersect_ray, heightfield mesh raycast, removeSensorRaycastDescription
newton.intersect_ray()— new public raycast APIAdds
newton.intersect_ray(), a functional helper for casting batched rays against all shapes in a model across multiple worlds. It uses the shape BVH for broad-phase culling and dispatches to per-shape intersection routines (primitives, meshes, and heightfields).This is the recommended building block for custom raycast sensors, lidar simulations, and picking — replacing ad-hoc kernel launches against internal kernel symbols.
Heightfield raycast via
wp.MeshPreviously
GeoType.HFIELDshapes used a per-thread 2D DDA grid traversal in the raycast kernel, requiring three extra arrays (shape_heightfield_index,heightfield_data,heightfield_elevations) threaded through every raycast kernel signature and a factory pattern (_make_raycast_funcs) to generate two compiled variants.Now
ModelBuilder.finalize()builds awp.Meshfrom each heightfield (same CCW triangulation as the collision path) and stores its ID inmodel.shape_source_ptr, exactly likeMESHandCONVEX_MESHshapes.ray_intersect_geomhandlesHFIELDin the same branch as meshes.Benefits:
Heightfieldobject reuses onewp.Meshacross instances; per-instance scale is applied at query timemodel.heightfield_mesheskeeps thewp.Meshobjects alive;heightfield_data/heightfield_elevationsare unchanged (still used by collision kernels)Remove
SensorRaycastSensorRaycastwas deprecated in 1.2 in favour ofSensorTiledCamera. This PR removes it along with thesensor_raycast_kernel,sensor_raycast_particles_kernel, andsensor_raycast_kernel_no_hfieldthat existed solely to support it.ray_intersect_particle_sphereis retained — still used bywarp_raytrace.Migration: use
SensorTiledCamerawithSensorTiledCamera.utils.compute_pinhole_camera_rays()andcreate_depth_image_output().Closes #2465.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
New feature / API change
Summary by CodeRabbit