Fix broadphase collision detection#926
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReworks collision counting and allocation: adds per-pair contact limits, canonicalizes shape-pair ordering, updates counter increment to support index limits, changes contact counting/allocation kernels and their public signatures, propagates per-pair limits through CollisionPipeline/Model, and adds/updates tests and small doc/typing edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Python code / Pipeline
participant Broad as Broadphase kernel
participant Count as count_contact_points(kernel)
participant Alloc as allocate_contact_points(kernel)
participant Counter as counter_increment (with index_limit)
participant Handler as handle_contact_pairs
Note over Caller,Broad: Start broadphase with per-pair limit
Caller->>Broad: compute candidate pairs (A,B)
Broad->>Count: for each canonicalized pair -> count contacts per-direction with max_per_pair
Count-->>Caller: returns (num_contacts_a, num_contacts_b)
Caller->>Alloc: allocate contact slots using per-direction counts
Alloc->>Counter: increment per-slot counters (uses index_limit)
alt index_limit reached
Counter-->>Handler: returns -1 / early-exit
Handler-->>Caller: stop further allocations for that pair
else normal allocation
Alloc-->>Handler: assigned contact indices
Handler->>Handler: process closest-point / plane/mesh / mesh/misc with swapped transforms as needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Reason: multiple interdependent changes across kernels, pipeline, model API, and tests; semantic shifts (canonicalization, per-pair limits, early-exit) require careful validation across CPU/GPU kernel boundaries and tests. Possibly related PRs
Suggested reviewers
📝 WalkthroughReworks collision counting and allocation: adds per-pair contact limits, canonicalizes shape-pair ordering, updates counter increment to support index limits, changes contact counting/allocation kernels and their public signatures, propagates per-pair limits through CollisionPipeline/Model, and adds/updates tests and small doc/typing edits. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/pr_aws_gpu_asv.yml(1 hunks).github/workflows/pr_aws_gpu_tests.yml(1 hunks).github/workflows/push_aws_gpu_tests.yml(1 hunks)newton/_src/geometry/kernels.py(1 hunks)newton/_src/geometry/types.py(1 hunks)newton/examples/__init__.py(3 hunks)newton/tests/test_collision_pipeline.py(1 hunks)newton/tests/test_collision_primitives.py(1 hunks)newton/tests/test_inertia.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
newton/_src/geometry/kernels.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-64)
newton/tests/test_inertia.py (1)
newton/_src/utils/mesh.py (1)
create_sphere_mesh(22-83)
newton/tests/test_collision_pipeline.py (8)
newton/_src/geometry/types.py (2)
GeoType(25-64)Mesh(113-297)newton/examples/__init__.py (1)
test_body_state(38-112)newton/tests/unittest_utils.py (2)
add_function_test(312-331)get_cuda_test_devices(135-137)newton/_src/sim/builder.py (6)
add_joint_free(1621-1672)add_shape_box(2567-2604)add_shape_sphere(2534-2565)add_shape_capsule(2606-2646)add_shape_cylinder(2648-2688)add_shape_mesh(2733-2766)newton/_src/utils/mesh.py (1)
create_sphere_mesh(22-83)newton/_src/sim/state.py (1)
clear_forces(64-75)newton/_src/viewer/viewer_null.py (1)
ViewerNull(23-160)newton/_src/solvers/xpbd/solver_xpbd.py (1)
SolverXPBD(40-660)
🪛 Ruff (0.13.3)
newton/examples/__init__.py
112-112: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
newton/tests/test_collision_primitives.py (1)
1-8: LGTM! SPDX header update.The copyright header has been updated to SPDX format. No functional changes.
newton/examples/__init__.py (1)
44-112: LGTM! Enhanced error reporting for debugging.The addition of
show_body_qandshow_body_qdparameters improves debugging by optionally including body pose and twist in error messages. The implementation correctly builds optional extras and appends them to failure details.Note: The static analysis hint (TRY003) is a false positive here since the error message is contextual and dynamically built based on test failures.
newton/tests/test_inertia.py (1)
29-29: LGTM! Cleaner mesh utility abstraction.Switching from
OpenGLRenderer._create_sphere_meshto the dedicatedcreate_sphere_meshutility removes the dependency on OpenGL-specific code and provides a cleaner abstraction for mesh creation.newton/_src/geometry/types.py (1)
34-34: LGTM! Docstring clarification.The updated docstring is more concise and accurate, as planes in the system can be finite (bounded by width and length parameters).
newton/tests/test_collision_pipeline.py (1)
184-194: LGTM! Comprehensive collision test coverage.The test matrix covers a good range of shape-pair collisions, including sphere, box, capsule, and mesh combinations. The use of
TestLevelflags to control which velocity components are tested for each pair is a pragmatic approach to handle the varying collision dynamics.newton/_src/geometry/kernels.py (1)
1028-1049: Approve collision kernel updates.
Swap logic enforces canonical shape‐pair ordering across all subsequent collision branches, and the*operator is semantically equivalent towp.transform_multiply.
…ision Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/geometry/kernels.py (1)
669-674: Do not advance the counter beyond the declared limit
counter_incrementis documented to “increment the counter but only if it is smaller than index_limit,” yet the current implementation still bumps the counter before deciding to return-1. As soon as we hit the limit the counter keeps growing (and can race far past the cap), which leaves downstream checks working on a value that no longer reflects how many slots were actually granted. Please roll back the increment in the overflow case so the counter saturates at the limit.- count = wp.atomic_add(counter, counter_index, 1) - if count < index_limit or index_limit < 0: - tids[tid] = count - return count - tids[tid] = -1 - return -1 + count = wp.atomic_add(counter, counter_index, 1) + if index_limit >= 0 and count >= index_limit: + # undo the increment so the counter never exceeds the cap + wp.atomic_add(counter, counter_index, -1) + tids[tid] = -1 + return -1 + tids[tid] = count + return countnewton/_src/sim/collide.py (1)
127-134: Per‑pair limits are never enforced
rigid_contact_max_per_pairis exposed, butself.rigid_pair_point_limit/self.rigid_pair_point_countstayNone, so both kernels short-circuit their per-pair logic. The result is that the new parameter is ignored entirely. Please allocate these buffers when a limit is requested so the kernels can actually read/write the per-pair caps.with wp.ScopedDevice(device): self.rigid_pair_shape0 = wp.empty(self.rigid_contact_max, dtype=wp.int32) self.rigid_pair_shape1 = wp.empty(self.rigid_contact_max, dtype=wp.int32) - self.rigid_pair_point_limit = None # wp.empty(self.shape_count ** 2, dtype=wp.int32) - self.rigid_pair_point_count = None # wp.empty(self.shape_count ** 2, dtype=wp.int32) + if self.rigid_contact_max_per_pair > 0: + pair_capacity = shape_count * shape_count + self.rigid_pair_point_limit = wp.zeros(pair_capacity, dtype=wp.int32) + self.rigid_pair_point_count = wp.zeros(pair_capacity, dtype=wp.int32) + else: + self.rigid_pair_point_limit = None + self.rigid_pair_point_count = None self.rigid_pair_point_id = wp.empty(self.rigid_contact_max, dtype=wp.int32)
♻️ Duplicate comments (2)
newton/tests/test_collision_pipeline.py (2)
102-104: Fix import path for create_sphere_meshUse the correct utils path or export it.
- vertices, indices = newton.utils.create_sphere_mesh(radius=0.5) + from newton._src.utils.mesh import create_sphere_mesh + vertices, indices = create_sphere_mesh(radius=0.5)Or export via
newton/_src/utils/__init__.pyand keepnewton.utils.create_sphere_mesh. Based on past review comments.
142-144: Indexing bug: shape_key is per shape, not per bodyUse the body→shape mapping to get the shape index before indexing
shape_key.Example:
shape_idx = next(iter(self.model.body_shapes.get(body, [])), None) name = f"body {body}" if shape_idx is None else f"body {body} ({self.model.shape_key[shape_idx]})"Store
shape_idxduring setup for O(1) access if preferred.
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (2)
4447-4447: Canonicalize filter pairs defensivelyIf external code populated
model.shape_collision_filter_pairswith non‑canonical order, membership checks could miss. Normalize once on read.- filters: set[tuple[int, int]] = model.shape_collision_filter_pairs + # Normalize to canonical (min, max) to be robust to external inputs + filters: set[tuple[int, int]] = { + (a, b) if a < b else (b, a) for (a, b) in model.shape_collision_filter_pairs + }
4481-4482: Ensure empty contact array has vec2 shapeWhen no pairs exist,
np.array([])is shape (0,), which can be awkward forwp.vec2i. Emit (0,2) int array.- model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device) - model.shape_contact_pair_count = len(contact_pairs) + if contact_pairs: + pair_arr = np.asarray(contact_pairs, dtype=np.int32) + else: + pair_arr = np.empty((0, 2), dtype=np.int32) + model.shape_contact_pairs = wp.array(pair_arr, dtype=wp.vec2i, device=model.device) + model.shape_contact_pair_count = pair_arr.shape[0]
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newton/_src/geometry/kernels.py(13 hunks)newton/_src/sim/builder.py(1 hunks)newton/_src/sim/collide.py(8 hunks)newton/_src/sim/model.py(2 hunks)newton/tests/test_collision_pipeline.py(1 hunks)newton/tests/test_collision_primitives.py(1 hunks)newton/tests/test_rigid_contact.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_collision_primitives.py
🧰 Additional context used
🧬 Code graph analysis (4)
newton/_src/sim/collide.py (1)
newton/_src/sim/model.py (1)
Model(29-645)
newton/tests/test_rigid_contact.py (1)
newton/_src/sim/builder.py (1)
add_articulation(644-656)
newton/tests/test_collision_pipeline.py (6)
newton/_src/geometry/types.py (2)
GeoType(25-64)Mesh(113-297)newton/examples/__init__.py (1)
test_body_state(38-112)newton/tests/unittest_utils.py (2)
add_function_test(312-331)get_cuda_test_devices(135-137)newton/_src/sim/builder.py (5)
add_shape_box(2567-2604)add_shape_sphere(2534-2565)add_shape_capsule(2606-2646)add_shape_cylinder(2648-2688)add_shape_mesh(2733-2766)newton/_src/utils/mesh.py (1)
create_sphere_mesh(22-83)newton/_src/viewer/viewer_null.py (1)
ViewerNull(23-160)
newton/_src/geometry/kernels.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-64)
🔇 Additional comments (2)
newton/_src/sim/model.py (2)
140-141: Typed set for filter pairs looks goodSwitching to
set[tuple[int, int]]aligns with membership checks and avoids duplicates.
575-582: API change: keyword construction and removed args — verify callers and docsThe new kwargs and removal of legacy params (e.g., iterate_mesh_vertices) require call‑site and docs updates.
Run to find stale usages:
nvtw
left a comment
There was a problem hiding this comment.
Looks good and got much more readable. Thanks
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Description
Mesh-mesh collisions were skipped and a bug in the logic for bounding sphere collision pruning inside
broadphase_collision_pairsgot introduced somewhere in #781 or before.Besides those fixes, the PR adds some unit tests for the CollisionPipeline to run on all supported shape pair types.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit