fix: multi-world particle BVH indexing#1641
fix: multi-world particle BVH indexing#1641Idate96 wants to merge 3 commits intonewton-physics:mainfrom
Conversation
SensorTiledCamera particle BVH queries already return global particle indices; don't apply a per-world offset.
Catches incorrect BVH particle index mapping across worlds (can cause CUDA illegal memory access).
📝 WalkthroughWalkthroughRefactored particle index handling in ray casting to use BVH query indices directly as global indices into particle arrays, removing local remapping. Added a regression test that builds a 4-world particle scene and verifies SensorTiledCamera depth images are identical across translated worlds. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/tests/test_sensor_tiled_camera_particles_multiworld.py`:
- Around line 1-2: Update the SPDX header in the new file by changing the
copyright year from 2025 to 2026: modify the two header lines starting with
"SPDX-FileCopyrightText" and "SPDX-License-Identifier" so the copyright year
reads 2026 (i.e., replace "Copyright (c) 2025" with "Copyright (c) 2026") to
match the current-year requirement.
🧹 Nitpick comments (1)
newton/tests/test_sensor_tiled_camera_particles_multiworld.py (1)
98-111: Inconsistent quaternion construction — preferwp.quat_identity()for clarity.Line 69 uses
wp.quat_identity()but line 98 constructs the identity quaternion manually aswp.quatf(0.0, 0.0, 0.0, 1.0). Using the same helper would be clearer and less error-prone.Proposed fix
- cam_quat = wp.quatf(0.0, 0.0, 0.0, 1.0) + cam_quat = wp.quat_identity()
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d1059f8cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- update SPDX year to 2026 - use wp.quat_identity() consistently - add docstrings to satisfy docstring coverage
|
Addressed CodeRabbit review nits in 71f669f: updated SPDX year to 2026, switched to wp.quat_identity() consistently, and added docstrings for the new regression test. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thank you for catching and fixing this! :D Looks all good to me! |
Problem
SensorTiledCamerabuilds the particle BVH over the global particle arrays (particles_position/particles_radius) and uses BVH groups to restrict queries to a given world.In
newton/_src/sensors/warp_raytrace/ray_cast.py, particle hits were indexed as ifwp.bvh_query_next()returned a world-local index (subtractingworld_index * bvh_particles_size).For multi-world particle scenes this produces invalid indices (and can trigger CUDA illegal memory access) and/or incorrect depth images for
world_index > 0.Fix
Treat the BVH hit index returned by
wp.bvh_query_next()as a global index into the particle arrays, and remove the per-world offset in:closest_hit_particles()first_hit_particles()Test
Add a minimal regression test that renders a tiny particle grid in multiple worlds and asserts the per-world depth images match (within a small fp32 tolerance):
newton/tests/test_sensor_tiled_camera_particles_multiworld.pyThe test uses the existing device parametrization helpers, so it runs on CPU and CUDA.
Summary by CodeRabbit
Bug Fixes
Tests