Refactor the narrow phase code of Newton - get rid of old id mapping#781
Conversation
📝 WalkthroughWalkthroughAdds a GeoData struct and a GeoData-driven collision dispatch in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BP as Broadphase Kernel
participant GD as GeoData Factory
participant AL as allocate_contact_points
participant DIS as Type Matcher (isCase)
participant CH as Collision Helper
participant CT as Contact Arrays
BP->>GD: create_geo_data(shapeA, body_q, shape_transform, type, scale, thickness)
GD-->>BP: GeoData_A (X_ws, X_bw, radius_eff, min_scale, ...)
BP->>GD: create_geo_data(shapeB, ...)
GD-->>BP: GeoData_B
BP->>AL: allocate_contact_points(num_contacts, shapeA, shapeB, pair_idx_ab/ba, limits)
alt allocation ok
AL-->>BP: true
BP->>DIS: isCase(typeA, typeB, targetA, targetB)
DIS-->>BP: ordered match
BP->>CH: specific_collision(GeoData_A, GeoData_B, ...)
CH-->>CT: write contact entries (shape0/1, point0/1, normals, ids)
else allocation failed
AL-->>BP: false
BP->>CT: set contact-limit error ("Number of rigid contacts exceeded limit.")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
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 (2)
1023-1027: Overly permissive broadphase distance heuristic inflates pair count.
d = length(p_a - p_b) * 0.5 - 0.1underestimates separation, producing many false positives and risking contact buffer exhaustion/perf regressions.- d = wp.length(p_a - p_b) * 0.5 - 0.1 + d = wp.length(p_a - p_b)If you intended padding, gate it behind a configurable tolerance.
1992-2000: Possible use of undefinedpair_index/contact_limitinhandle_contact_pairs.
pair_indexandcontact_limitare defined only ifcontact_point_limitis provided, but later used when onlycontact_pairwise_counteris set. Define both unconditionally and use-1as unlimited sentinel withlimited_counter_increment.- if contact_point_limit: - pair_index = shape_a * num_shapes + shape_b - contact_limit = contact_point_limit[pair_index] + pair_index = shape_a * num_shapes + shape_b + contact_limit = -1 + if contact_point_limit: + contact_limit = contact_point_limit[pair_index] if contact_point_limit: if contact_pairwise_counter[pair_index] >= contact_limit: return ... - if contact_pairwise_counter: + if contact_pairwise_counter: pair_contact_id = limited_counter_increment( contact_pairwise_counter, pair_index, contact_tids, tid, contact_limit )Also applies to: 2099-2113
🧹 Nitpick comments (5)
newton/tests/test_collision.py (1)
799-799: Don’t silently disable a passing test without rationale.Commenting out
test_mesh_ground_collision_indexreduces coverage and may hide regressions from the narrow‑phase refactor. If the test is flaky or temporarily failing, mark it with@unittest.skipand a reason, or fix/update it to the new semantics.Apply this diff to preserve intent while documenting status:
-# add_function_test(TestCollision, "test_mesh_ground_collision_index", test_mesh_ground_collision_index, devices=devices) +@unittest.skip("Temporarily disabled pending narrow-phase refactor updates; see PR #781") +def _skipped_test_mesh_ground_collision_index(*args, **kwargs): + pass +add_function_test(TestCollision, "test_mesh_ground_collision_index", test_mesh_ground_collision_index, devices=devices)newton/_src/geometry/kernels.py (4)
1200-1201: Duplicate plane–plane early‑return.There are two early returns for PLANE–PLANE; keep one.
- elif actual_type_a == GeoType.PLANE and actual_type_b == GeoType.PLANE: - return # no plane-plane contactsAlso applies to: 1011-1014
1293-1302: Preferwp.min/wp.maxinside device funcs; avoid Python builtins.Replace
min(...)andmax(...)in@wp.funcs withwp.min/wp.maxto ensure JIT compatibility.Example:
- min_scale = min(geo_a.min_scale, geo_b.min_scale) + min_scale = wp.min(geo_a.min_scale, geo_b.min_scale)Also applies to: 1418-1426
1144-1156: Contact-limit prints: gate under verbose for kernels.Unconditional
if wp.static(wp.config.verbose):.- print("Number of rigid contacts exceeded limit. Increase Model.rigid_contact_max.") + if wp.static(wp.config.verbose): + print("Number of rigid contacts exceeded limit. Increase Model.rigid_contact_max.")Apply similarly to other overflow messages.
Also applies to: 1176-1198, 1053-1069, 1099-1116
1418-1418: Unnecessaryint(0)casts.These are already integers; simplify to
0for clarity.- face_index = int(0) + face_index = 0Also applies to: 1561-1561, 1796-1796, 1909-1909
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/geometry/kernels.py(9 hunks)newton/tests/test_collision.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/kernels.py
🧬 Code graph analysis (1)
newton/_src/geometry/kernels.py (1)
newton/_src/geometry/types.py (1)
GeoType(25-64)
🪛 Ruff (0.12.2)
newton/_src/geometry/kernels.py
1418-1418: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1561-1561: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1796-1796: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1909-1909: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (1)
newton/_src/geometry/kernels.py (1)
2001-2043: Good: GeoData abstraction and symmetric wrappers simplify narrow‑phase dispatch.The split into small helpers and flipped wrappers (e.g., plane–sphere) improves readability and reuse.
Please confirm the new helpers fully cover all pairs previously handled inline in
handle_contact_pairsand that there’s parity in generated contacts across representative scenes (sphere–plane, box–mesh, capsule–plane finite/infinite).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/geometry/kernels.py (1)
1065-1072: Fix plane–X broadphase culling: wrong frame and scale cause false negatives.Distance is computed from the plane origin in B’s frame using B’s scale. You need B’s COM in A’s (plane) frame and A’s width/length; also compare to r_b, not r_a.
Apply:
p_a = wp.transform_get_translation(actual_X_ws_a) if actual_type_a == GeoType.PLANE and actual_type_b == GeoType.PLANE: return if actual_type_a == GeoType.PLANE: - query_b = wp.transform_point(wp.transform_inverse(actual_X_ws_b), p_a) - scale = shape_scale[actual_shape_b] - closest = closest_point_plane(scale[0], scale[1], query_b) - d = wp.length(query_b - closest) - r_a = shape_radius[actual_shape_a] - if d > r_a + rigid_contact_margin: + # distance from B's COM to plane A, evaluated in A's local frame + p_b = wp.transform_get_translation(actual_X_ws_b) + query_a = wp.transform_point(wp.transform_inverse(actual_X_ws_a), p_b) + plane_scale = shape_scale[actual_shape_a] + closest = closest_point_plane(plane_scale[0], plane_scale[1], query_a) + d = wp.length(query_a - closest) + r_b = shape_radius[actual_shape_b] + if d > r_b + rigid_contact_margin: return
🧹 Nitpick comments (2)
newton/_src/geometry/kernels.py (2)
1415-1460: Rename unused argument to silence linter (ARG001).edge_sdf_iter is unused in cylinder_plane_collision.
-def cylinder_plane_collision( +def cylinder_plane_collision( geo_a: GeoData, geo_b: GeoData, point_id: int, - edge_sdf_iter: int, + _edge_sdf_iter: int, ):No callers need changes.
523-563: Or remove or wire up closest_edge_coordinate_cylinder.Helper is added but currently unused; either use it in a cylinder narrow‑phase or drop to avoid dead code.
Do you intend to add CYLINDER/BOX soon? If so, I can stub it using this helper.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/geometry/kernels.py(10 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/kernels.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
PR: newton-physics/newton#584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/_src/geometry/kernels.py
🧬 Code graph analysis (1)
newton/_src/geometry/kernels.py (2)
newton/_src/geometry/types.py (1)
GeoType(25-64)newton/_src/geometry/collision_primitive.py (1)
collide_plane_cylinder(454-549)
🪛 Ruff (0.13.1)
newton/_src/geometry/kernels.py
1420-1420: Unused function argument: edge_sdf_iter
(ARG001)
1519-1519: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1662-1662: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1897-1897: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2010-2010: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (5)
newton/_src/geometry/kernels.py (5)
1280-1359: GeoData consolidation looks solid.Clear ownership of transforms/properties and precomputed min_scale/radius_eff reduces repeated reads.
2138-2144: Flipped wrappers are correct.Normal flipping for BOX–PLANE preserves A→B convention.
Please confirm tests cover both argument orders for plane–box and sphere–plane pairs.
1134-1142: Use ordered shapes when allocating BOX–BOX contacts.Allocation writes shape_a/shape_b instead of actual_shape_a/actual_shape_b, which desyncs pair indexing and per‑pair limits.
- for i in range(12): # 12 edges - contact_shape0[index + i] = shape_a - contact_shape1[index + i] = shape_b + for i in range(12): # 12 edges + contact_shape0[index + i] = actual_shape_a + contact_shape1[index + i] = actual_shape_b contact_point_id[index + i] = i # allocate contact points from box B against A - for i in range(12): - contact_shape0[index + 12 + i] = shape_b - contact_shape1[index + 12 + i] = shape_a + for i in range(12): + contact_shape0[index + 12 + i] = actual_shape_b + contact_shape1[index + 12 + i] = actual_shape_a contact_point_id[index + 12 + i] = i
877-879: Allocated contacts for unsupported pairs (CYLINDER/BOX, MESH/CONE) without handlers.handle_contact_pairs has no branches for these, so allocated work is dropped.
Either implement handlers or gate allocations:
elif actual_type_a == GeoType.CYLINDER and actual_type_b == GeoType.BOX: - num_contacts = 8 + # TODO: enable when narrow-phase handler exists + num_contacts = 0 ... elif (actual_type_a == GeoType.MESH) and actual_type_b == GeoType.CONE: - mesh_a = wp.mesh_get(shape_source_ptr[actual_shape_a]) - num_contacts_a = mesh_a.points.shape[0] - num_contacts = num_contacts_a + # TODO: enable when narrow-phase handler exists + num_contacts = 0Also applies to: 925-929
1523-1525: Warp JIT safety: replace Python min with wp.min.Python min inside @wp.func can break JIT/backends.
- min_scale = min(geo_a.min_scale, geo_b.min_scale) + min_scale = wp.min(geo_a.min_scale, geo_b.min_scale)
camevor
left a comment
There was a problem hiding this comment.
Looks good to me, I did not go through the collision funcs in detail though.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
newton/_src/geometry/kernels.py (3)
1405-1408: Silence unused param warning.
edge_sdf_iterisn’t used incylinder_plane_collision. Prefix with_or remove.-def cylinder_plane_collision( +def cylinder_plane_collision( geo_a: GeoData, geo_b: GeoData, point_id: int, - edge_sdf_iter: int, + _edge_sdf_iter: int, ):
1506-1506: Make device code JIT‑safe: avoid Pythonint()andmin().Use Warp intrinsics to prevent host‑side evaluation.
- face_index = int(0) + face_index = 0 @@ - face_index = int(0) + face_index = 0 @@ - face_index = int(0) + face_index = 0 @@ - face_index = int(0) + face_index = 0 @@ - min_scale = min(geo_a.min_scale, geo_b.min_scale) + min_scale = wp.min(geo_a.min_scale, geo_b.min_scale) @@ - idx = wp.min(int(point_id), 3) + idx = wp.min(point_id, 3)Also applies to: 1649-1649, 1884-1884, 1997-1997, 1510-1512, 1436-1439
1922-1928: Degenerate normals when centers coincide.
wp.normalize(diff)is undefined at zero. Add a safe fallback.- normal = wp.normalize(diff) + normal = wp.where(wp.length(diff) > 1e-12, wp.normalize(diff), wp.vec3(0.0, 0.0, 1.0))Apply similarly to sphere–box and sphere–capsule blocks.
Also applies to: 1942-1951, 1965-1975
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/geometry/kernels.py(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/kernels.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
PR: newton-physics/newton#584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/_src/geometry/kernels.py
🧬 Code graph analysis (1)
newton/_src/geometry/kernels.py (2)
newton/_src/geometry/types.py (1)
GeoType(25-64)newton/_src/geometry/collision_primitive.py (1)
collide_plane_cylinder(454-549)
🪛 Ruff (0.13.1)
newton/_src/geometry/kernels.py
1407-1407: Unused function argument: edge_sdf_iter
(ARG001)
1506-1506: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1649-1649: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1884-1884: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
1997-1997: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (5)
newton/_src/geometry/kernels.py (5)
523-563: Nice addition: cylinder edge search helper.This will enable CYLINDER–BOX and similar pairs to mirror the capsule path. Good foundation for the new handler.
1248-1264: Good reuse via allocate_contact_points.Centralizing allocation reduces repetition and error surface. Once
wp.printfis used, this is solid.Please confirm
contact_point_limitsemantics (AB = N, BA = 0) are relied upon downstream; otherwise we may want both sides symmetric when needed.
2080-2085: pair_index/contact_limit used uninitialized when contact_point_limit is None.This can cause undefined behavior. Initialize unconditionally and guard the counter usage.
- if contact_point_limit: - pair_index = shape_a * num_shapes + shape_b - contact_limit = contact_point_limit[pair_index] - if contact_pairwise_counter[pair_index] >= contact_limit: - # reached limit of contact points per contact pair - return + pair_index = shape_a * num_shapes + shape_b + contact_limit = -1 # unlimited unless provided + if contact_point_limit: + contact_limit = contact_point_limit[pair_index] + if contact_pairwise_counter and contact_limit >= 0 and contact_pairwise_counter[pair_index] >= contact_limit: + return @@ - if contact_pairwise_counter: - pair_contact_id = limited_counter_increment( - contact_pairwise_counter, pair_index, contact_tids, tid, contact_limit - ) + if contact_pairwise_counter: + pair_contact_id = limited_counter_increment( + contact_pairwise_counter, pair_index, contact_tids, tid, contact_limit + )Also applies to: 2191-2194
842-849: Ground‑plane sentinel assumption.Normalization assumes
shape_b == -1for planes. If upstream ever emitsshape_a == -1, this path misses it.Run to confirm all plane sentinels are in
shape_bfor this kernel’s inputs:
981-994: Device code must use wp.printf, not print.
print()inside@wp.funcis not portable. Replace withwp.printf.- if index + num_contacts - 1 >= rigid_contact_max: - print("Number of rigid contacts exceeded limit. Increase Model.rigid_contact_max.") + if index + num_contacts - 1 >= rigid_contact_max: + wp.printf("Number of rigid contacts exceeded limit. Increase Model.rigid_contact_max.\n") return False
| query_b = wp.transform_point(wp.transform_inverse(actual_X_ws_b), p_a) | ||
| scale = shape_scale[actual_shape_b] | ||
| # unique ordering of shape pairs (swap when types are equal or A > B) | ||
| if not (type_a) < (type_b): |
There was a problem hiding this comment.
Why not simply if (type_a) > (type_b): ...?
There was a problem hiding this comment.
Will simplify in a follow up MR (it's already merged).
Description
Refactor collision detection system for improved organization and performance
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
New Features
Improvements
Tests