Fix default values of world id and group id to get proper collision filtering#990
Conversation
📝 WalkthroughWalkthroughDefault ShapeConfig.collision_group changed from -1 to 1; broad-phase pair filtering in ModelBuilder was refactored to use kernel-like helpers ( Changes
Sequence Diagram(s)sequenceDiagram
participant B as ModelBuilder
participant M as Model (shapes)
Note over B: find_shape_contact_pairs (shapes sorted by world)
B->>M: iterate shape i
B->>M: iterate shape j (j>i)
alt same world or one global (-1)
B->>B: _test_world_and_group_pair(world_i, world_j, group_i, group_j)
alt world+group pass
B->>B: _test_group_pair(group_i, group_j)
alt group allows
B->>M: emit pair (i,j) (respect explicit filter-pairs)
else
Note right of B: skip pair (group filtered)
end
else
Note right of B: skip pair (world filtered)
end
else different non-global worlds
Note right of B: early-break (no further collisions possible)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
|
Without looking at the code - the API does make sense to me. Will have a look at implementation once we agree the API is good. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/_src/sim/builder.py (3)
4437-4456: Group filter mirrors kernel; add tiny testsLogic matches stated rules (0 disables, >0 equals or neg, <0 all except same). Consider making this a @staticmethod to ease unit testing and add a small table-driven test to lock behavior.
4503-4513: Consistent filtering and canonical pairs: handle empty safelyThe loop now respects ShapeFlags and kernel parity; good. Minor robustness: constructing wp.array from an empty Python list can be brittle for vec types. Guard the empty case.
Apply this diff:
- 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: + pairs_np = np.asarray(contact_pairs, dtype=np.int32) + model.shape_contact_pairs = wp.array(pairs_np, dtype=wp.vec2i, device=model.device) + else: + model.shape_contact_pairs = wp.empty(0, dtype=wp.vec2i, device=model.device) + model.shape_contact_pair_count = int(len(contact_pairs))Also applies to: 4512-4519, 4527-4529, 4531-4532
4183-4185: Store collision_group as device array for kernelsModel.shape_collision_group is a Python list; consider wp.array(int32) for consistency and potential GPU use in dynamic broadphase paths.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
newton/_src/sim/builder.py(6 hunks)newton/_src/utils/import_usd.py(1 hunks)newton/examples/basic/example_basic_urdf.py(1 hunks)newton/examples/example_mujoco.py(1 hunks)newton/examples/robot/example_robot_allegro_hand.py(1 hunks)newton/examples/robot/example_robot_anymal_d.py(1 hunks)newton/examples/robot/example_robot_g1.py(1 hunks)newton/examples/robot/example_robot_h1.py(1 hunks)newton/examples/robot/example_robot_humanoid.py(1 hunks)newton/examples/robot/example_robot_ur10.py(1 hunks)newton/examples/selection/example_selection_articulations.py(1 hunks)newton/examples/selection/example_selection_materials.py(1 hunks)newton/examples/sensors/example_sensor_contact.py(1 hunks)newton/tests/test_model.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)newton/tests/test_rigid_contact.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
PR: newton-physics/newton#899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_rigid_contact.py
🧬 Code graph analysis (14)
newton/examples/robot/example_robot_h1.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/tests/test_rigid_contact.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/examples/example_mujoco.py (1)
newton/_src/sim/builder.py (1)
add_ground_plane(2481-2501)
newton/examples/sensors/example_sensor_contact.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/tests/test_mujoco_solver.py (1)
newton/_src/sim/builder.py (1)
add_shape_plane(2428-2479)
newton/examples/robot/example_robot_g1.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/examples/selection/example_selection_materials.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/examples/robot/example_robot_allegro_hand.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/examples/basic/example_basic_urdf.py (1)
newton/_src/sim/builder.py (1)
add_ground_plane(2481-2501)
newton/examples/selection/example_selection_articulations.py (1)
newton/_src/sim/builder.py (1)
add_ground_plane(2481-2501)
newton/examples/robot/example_robot_humanoid.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
newton/examples/robot/example_robot_ur10.py (1)
newton/_src/sim/builder.py (1)
add_ground_plane(2481-2501)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
newton/examples/robot/example_robot_anymal_d.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)add_builder(879-1156)
⏰ 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (19)
newton/examples/robot/example_robot_ur10.py (1)
98-105: LGTM! Clear global ground plane initialization pattern.The two-step initialization (global ground plane in world -1, then replication starting at world 0) ensures a single ground plane collides with all worlds, consistent with the updated collision filtering semantics.
newton/examples/robot/example_robot_allegro_hand.py (1)
118-124: LGTM! Consistent with updated world initialization pattern.The global ground plane setup matches the pattern used across other examples in this PR.
newton/examples/selection/example_selection_articulations.py (1)
113-119: LGTM! Global ground plane properly initialized.newton/examples/selection/example_selection_materials.py (2)
91-97: LGTM! Global ground plane properly initialized.
102-102: Note: Increased contact capacity for global ground plane.The
ncon_per_world=100parameter addition likely accounts for increased contact count now that a global ground plane collides with all worlds. This is a reasonable adjustment.newton/examples/robot/example_robot_h1.py (1)
75-81: LGTM! Global ground plane properly initialized.newton/_src/utils/import_usd.py (1)
1092-1098: LGTM! Collision group defaults updated correctly.The changes align with the PR's collision filtering fixes:
- Default
collision_group = 1(was -1) enables collisions by default- Starting collision group IDs at 1 (via
len(...) + 1) ensures no group receives 0, which would disable collisions per the filtering rulesThe comment on line 1092 helpfully references
test_world_and_group_pairfor the full filtering logic.newton/examples/basic/example_basic_urdf.py (1)
77-83: LGTM! Global ground plane properly initialized.The initialization sequence is clear and well-commented.
newton/examples/robot/example_robot_g1.py (1)
72-78: LGTM! Global ground plane properly initialized.The pattern is consistent with all other examples in this PR.
newton/_src/sim/builder.py (3)
165-167: Default collision_group -> 1: semantics check and migration noteChanging default from -1 to 1 makes shapes collide with same positive groups and any negative groups; 0 still disables all. This aligns with the documented rules and avoids the “-1 vs -1 doesn’t collide” footgun. Please confirm importers (USD/URDF/MJCF) also set 1 when unspecified, and add a brief migration note in docs.
455-459: Default current_world -> 0Good change; avoids accidental “global” by default. Please verify all entry points that relied on -1 (e.g., example loaders, tests) are updated, and include a short deprecation/migration blurb.
4457-4479: Parity confirmed—implementation matches kernel exactlyThe kernel
test_world_and_group_pairfunction is defined innewton/_src/geometry/broad_phase_common.py, and the Python implementation at lines 4457–4479 mirrors it precisely. Both check world indices identically (if both are non-global and unequal, reject; otherwise check groups) and delegate to their respective group filter functions. No drift detected.newton/examples/sensors/example_sensor_contact.py (1)
118-125: Global ground then per‑world replication: LGTMPattern (current_world=-1; add_ground_plane; then replicate from 0) matches the new filtering semantics. ContactSensor targets "ground_plane" key correctly.
Based on learnings
newton/examples/robot/example_robot_anymal_d.py (1)
77-85: Global ground applied once across worlds: LGTMClean split between global plane and per‑world robots. Keeps world count correct and avoids duplicate planes.
newton/examples/example_mujoco.py (1)
411-417: Builder sequencing updated: LGTMGlobal plane before replication is consistent with new defaults and examples elsewhere.
newton/examples/robot/example_robot_humanoid.py (1)
66-73: Adopted global-ground pattern: LGTMMatches the new world/group semantics; no further changes needed.
newton/tests/test_rigid_contact.py (1)
543-549: LGTM! Correct implementation of global ground plane pattern.The explicit world context handling is correct:
- Setting
current_world = -1creates a global ground plane that collides with all worlds- Resetting to
current_world = 0before replication ensures per-world entities are properly scopedThis aligns with the PR's collision filtering fixes and matches the pattern used across other multi-world tests.
newton/tests/test_model.py (1)
273-274: LGTM! Explicit world context improves test clarity.Making the global world context (
current_world = -1) explicit before adding particles aligns with the updated world grouping semantics introduced in this PR. The updated comment accurately describes the intent.newton/tests/test_mujoco_solver.py (1)
228-237: LGTM! Consistent global ground plane implementation.The changes correctly implement the global ground plane pattern:
- Global context (
current_world = -1) for ground plane that collides with all worlds- Per-world context (
current_world = 0) before template replicationThis is consistent with the identical pattern in test_rigid_contact.py and aligns with the PR's collision filtering fixes.
…/nvtw/newton into dev/tw/collision_filtering_fixes
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/examples/robot/example_robot_allegro_hand.py (2)
118-125: Optional: make the plane’s collision group explicit for clarity.To avoid future surprises if defaults change, consider passing a
cfgwithcollision_group=1toadd_ground_plane(...). This keeps the example robust and self‑documenting.
209-210: Name and centralize the relaxed tolerance.Hard‑coding
0.8in two places hides intent and can mask regressions. Define a singlepos_toland use it for both bounds; document why 0.8 is needed.def test(self): num_bodies_per_world = self.model.body_count // self.num_worlds + pos_tol = 0.8 # relaxed tolerance to account for observed numerical drift across worlds for i in range(self.num_worlds): world_pos = wp.vec3(*self.initial_world_positions[i]) - world_lower = world_pos - wp.vec3(0.8, 0.8, 0.8) # Relaxed tolerance to account for numerical drift - world_upper = world_pos + wp.vec3(0.8, 0.8, 0.8) # Relaxed tolerance to account for numerical drift + world_lower = world_pos - wp.vec3(pos_tol, pos_tol, pos_tol) + world_upper = world_pos + wp.vec3(pos_tol, pos_tol, pos_tol)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/examples/robot/example_robot_allegro_hand.py(2 hunks)newton/tests/test_mujoco_solver.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
🧬 Code graph analysis (1)
newton/examples/robot/example_robot_allegro_hand.py (1)
newton/_src/sim/builder.py (2)
add_ground_plane(2481-2501)replicate(580-609)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (1)
newton/examples/robot/example_robot_allegro_hand.py (1)
118-125: Global plane and replication implementation verified as correct.The code pattern is sound:
- Ground plane collision_group defaults to 1 and will collide with all Allegro shapes (also collision_group=1).
replicate()correctly assigns each copy to a new world starting at 0 viaadd_builder()'s defaultworld=Nonebehavior, which incrementsnum_worldson each iteration.- World -1 entities collide with all worlds per the built-in collision filtering rules.
- Pattern aligns with official documentation examples.
No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
4503-4539: Empty-pairs construction can mis-shape; ensure (0,2) vec2i array.If no pairs exist,
np.array(contact_pairs)is shape (0,), which may not map cleanly towp.vec2i. Create an explicit (0,2) int32 array before wrapping.- model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device) + pairs_np = np.asarray(contact_pairs, dtype=np.int32) + if pairs_np.size == 0: + pairs_np = pairs_np.reshape(0, 2) + model.shape_contact_pairs = wp.array(pairs_np, dtype=wp.vec2i, device=model.device) model.shape_contact_pair_count = len(contact_pairs)Optional: pre-bucket by world (including a bucket for -1 globals) to reduce the O(n^2) inner loop to sum_w n_w^2.
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (2)
165-166: Default group = 1 is good; clarify full group semantics in docstring.Add one sentence so users know how positives/negatives interact.
- """The collision group ID for the shape. Defaults to 1 (default group). Set to 0 to disable collisions for this shape.""" + """The collision group ID for the shape. Defaults to 1 (default group). 0 disables collisions. + Positive (>0) groups collide only with the same positive ID and any negative group; negative (<0) + groups collide with everything except the same negative ID."""
4437-4456: Implementation matches kernel; add direct unit tests for _test_group_pair.The builder's
_test_group_pairmethod correctly mirrors the kernel'stest_group_pairlogic (lines 4437–4456). Comprehensive tests exist fortest_world_and_group_paircovering collision group edge cases, but direct unit tests for_test_group_pairare absent. Consider adding a small dedicated test method covering group combinations: (0, any), (positive, positive), (positive, negative), (negative, negative), and (negative, zero).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (2)
newton/_src/sim/builder.py (2)
4457-4479: World+group filter parity with kernel: good.Checks world first, then groups; matches design. No changes required.
455-459: Verification complete; no issues found.All usage sites have been properly updated:
- Multi-world examples (11 with geometry): All explicitly set
current_world = -1beforeadd_ground_plane()for global shared geometry- Single-world examples (13): Safe with new default of 0; ground plane correctly placed in world 0
- Tests: Properly assert
current_world = -1when creating global entities (particles, bodies, shapes)- Documentation: Comments and docstring example accurately describe the new defaults and usage pattern
The behavior change from -1 to 0 is correctly propagated across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
458-458: Critical:current_worlddefault does not match PR objectives.The PR objectives explicitly state: "Changed ModelBuilder.current_world default from -1 to 0." However, the code still shows
self.current_world = -1.This is a critical discrepancy. With the current default of -1, entities added directly to the builder will be global (colliding with all worlds) rather than world-specific. According to the PR objectives, the default should be 0 to make entities world-specific by default.
Apply this diff to match the PR objectives:
- self.current_world = -1 + self.current_world = 0Based on the PR description, this change ensures that entities are world-specific by default (world 0), with explicit assignment to world -1 needed for global entities.
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
4437-4456: Minor: Unreachable return statement could use clarification.The logic in
_test_group_pairis correct and matches the collision filtering rules. However, the finalreturn Falseat line 4455 is technically unreachable since all integer cases are handled by the preceding conditions (group_a can only be <0, >0, or ==0). While this is good defensive programming, consider adding a comment to clarify this is a safety fallback.Apply this diff to add a clarifying comment:
if group_a < 0: return group_a != group_b + # Safety fallback (should not be reached for integer inputs) return False
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
⏰ 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (3)
newton/_src/sim/builder.py (3)
165-166: LGTM! Collision group default change is correct.The change from -1 to 1 for the default
collision_groupaligns with the collision filtering rules where positive groups collide with themselves. The updated docstring clearly documents that 0 disables collisions.
4457-4479: LGTM! World and group filtering logic is correct.The
_test_world_and_group_pairmethod correctly implements the two-stage filtering:
- World index check: non-global entities (world >= 0) only collide within the same world, while global entities (world == -1) collide with all worlds
- Collision group check: delegates to
_test_group_pairfor group-specific filteringThis matches the documented collision filtering rules in the PR objectives.
4503-4522: Early break optimization is clever and correct.The sorting by world index combined with the early break optimization is a nice performance improvement. Let me trace through an example to confirm correctness:
Given sorted shapes: [s1(world=-1), s2(world=0), s3(world=0), s4(world=1)]
- i1=0 (s1, world=-1): Compares with s2, s3, s4 (no break since world=-1 is global)
- i1=1 (s2, world=0): Compares with s3 (same world), then s4 (world=1) → breaks here
- i1=2 (s3, world=0): Compares with s4 (world=1) → breaks here
The break at s2 vs s4 is valid because all remaining shapes after s4 have world >= 1, and s2 (world=0) won't collide with any of them. This correctly skips unnecessary comparisons while still checking all valid pairs.
|
@nvtw can you please add documentation that explains the collision filtering mechanisms? |
|
@adenzler-nvidia Where should that documentation end up? I did not find a well matching file in newton/docs, the closest would be guide/tutorials.rst. And the in-code documentation is present, see method test_group_pair and others. |
|
Probably makes sense to start a new section next to conventions/articulations about collisions? I assume we'll have more to say about collisions soon.. should the test_group_pair be part of the public API to be included in the auto generated docs? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
4906-4945: Collision filter pairs must be canonicalized to prevent unwanted contacts.model.shape_collision_filter_pairs can contain (b,a) while the builder constructs pairs as (a,b). The membership check on (a,b) will miss (b,a), re-enabling filtered contacts. Canonicalize the filter set once upfront.
- filters: set[tuple[int, int]] = model.shape_collision_filter_pairs + # Canonicalize filter pairs to (min, max) so membership tests are consistent + _filters_raw: set[tuple[int, int]] = model.shape_collision_filter_pairs + filters: set[tuple[int, int]] = {(a, b) if a <= b else (b, a) for (a, b) in _filters_raw} @@ - if (shape_a, shape_b) not in filters: + if (shape_a, shape_b) not in filters: contact_pairs.append((shape_a, shape_b)) @@ - model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device) + # Robust empty handling + if contact_pairs: + model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device) + else: + model.shape_contact_pairs = wp.zeros(0, dtype=wp.vec2i, device=model.device) model.shape_contact_pair_count = len(contact_pairs)The empty-case guard avoids dtype/shape surprises when no pairs exist.
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
1347-1353: Avoid hardcoding the default collision group; source it from the builder.Using a literal 1 risks drift if the default changes again. Pull it from builder.default_shape_cfg for consistency.
- collision_group = 1 # See test_world_and_group_pair for full filtering logic + # Default to the builder's shape default; 0 disables collisions, so defaults are non‑zero. + collision_group = builder.default_shape_cfg.collision_group if len(shape_spec.collisionGroups) > 0: cgroup_name = str(shape_spec.collisionGroups[0]) if cgroup_name not in collision_group_ids: - # Start from 1 to avoid collision_group = 0 (which means "no collisions") - collision_group_ids[cgroup_name] = len(collision_group_ids) + 1 + # Start from 1 to avoid collision_group = 0 (which means "no collisions") + collision_group_ids[cgroup_name] = len(collision_group_ids) + 1 collision_group = collision_group_ids[cgroup_name]
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/sim/builder.py(5 hunks)newton/_src/utils/import_usd.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (3)
newton/_src/sim/builder.py (3)
167-169: Default collision group = 1 — aligned and clear.Docstring states 0 disables collisions; good baseline.
4843-4861: Group filtering logic matches the stated rules.0 disables; positive collides with same positive or any negative; negatives collide with everything except same negative. Looks correct.
Please add a tiny unit test covering (1,1), (1,2), (1,-1), (-1,-1), (-1,-2), (0,1).
4863-4885: World+group test mirrors kernel semantics.Global (-1) interacts with all; different non-global worlds don’t collide. LGTM.
I would also vote for another conventions section on collision handling. There we can explain this world / collision group logic and also the kinds of pair-wise collision shape types we support for each collision pipeline we have: #853 (can be a follow-up PR of course). |
There is some documentation now and the test functions are now in the public API |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
4854-4860: Potential empty-array bug when no pairs; ensure vec2i shape.If no pairs are found, np.array(contact_pairs) yields a 1-D empty array; constructing a wp.array(dtype=wp.vec2i) from it can mis-shape. Create an explicit (0,2) int32 array.
Apply this diff:
- 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: + pairs_np = np.asarray(contact_pairs, dtype=np.int32).reshape(-1, 2) + else: + pairs_np = np.empty((0, 2), dtype=np.int32) + model.shape_contact_pairs = wp.array(pairs_np, dtype=wp.vec2i, device=model.device) + model.shape_contact_pair_count = int(pairs_np.shape[0])Notes:
- Keeps dtype/shape stable for Warp even when empty.
- Cast count to int to avoid numpy scalar surprises.
Also applies to: 4868-4876, 4877-4891, 4898-4901, 4902-4903
🧹 Nitpick comments (3)
newton/_src/sim/builder.py (1)
4868-4876: Early-break by world is correct; minor micro-optimizations optional.Looks good. Optional: pull local refs (world2, collision_group2) once per loop and hoist filters set length check if it’s large. Low priority.
Also applies to: 4882-4891
docs/concepts/collisions.rst (2)
111-147: Group semantics are clear; add one “environment” note.Call out that add_ground_plane uses the default ShapeConfig (group=1). If users place gameplay layers in other positive groups (2,3,...) they won’t collide with the ground unless the ground uses a negative group (e.g., -1). Add a one-line tip and tiny code snippet.
329-334: Performance tips: minor addition.Mention that minimizing the count of shapes in world -1 (“global”) reduces O(G*N) pairing since globals pair with all worlds.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/concepts/collisions.rst(1 hunks)docs/index.rst(1 hunks)newton/_src/geometry/__init__.py(2 hunks)newton/_src/sim/builder.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/__init__.py
🧬 Code graph analysis (2)
newton/_src/geometry/__init__.py (1)
newton/_src/geometry/broad_phase_common.py (2)
test_group_pair(70-87)test_world_and_group_pair(91-117)
newton/_src/sim/builder.py (2)
newton/_src/geometry/broad_phase_common.py (1)
test_world_and_group_pair(91-117)newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
⏰ 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (6)
docs/index.rst (1)
30-31: Concept added: good placement.Link to Collisions/Contacts under Concepts looks correct; path matches new file.
newton/_src/sim/builder.py (2)
56-56: Importing shared filtering helper improves consistency.Centralizing on test_world_and_group_pair removes divergence between modes. Good.
168-170: Behavior change: default collision group now 1.Matches PR objective; docs mention 0 disables collisions. Ensure migration notes clearly call out this default change for users relying on -1.
Please confirm Migration Guide includes this default flip and a brief “how to keep old behavior” snippet.
docs/concepts/collisions.rst (2)
176-203: Filtering snippet mirrors kernel logic.Good, matches test_world_and_group_pair semantics.
20-33: All referenced APIs exist and are correctly documented.Verification confirms:
- CollisionPipelineUnified class exists
- BroadPhaseMode enum with NXN, SAP, and EXPLICIT members exists
- Model.collide() method accepts collision_pipeline parameter
The documentation accurately reflects the API implementation.
newton/_src/geometry/__init__.py (1)
16-16: Re-exports are correctly included in__all__and properly exposed as public API.Verification confirms both
test_group_pairandtest_world_and_group_pairare:
- Properly imported at line 16
- Included in the
__all__list at lines 69-70- Available to the autodoc system for API documentation generation
The code is correct as-is.
…ated counterpart leads to a big performance regression
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
4843-4861: Collision group filtering logic is correct.The implementation correctly matches the kernel-level logic:
- Group 0 disables collisions
- Positive groups collide only with themselves and negative groups
- Negative groups collide with everything except their negative counterpart
Minor observation: the final
return Falseat line 4861 is technically unreachable since all cases are handled by the preceding conditions. This is likely defensive programming to satisfy linters, which is fine.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)
⏰ 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). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (3)
newton/_src/sim/builder.py (3)
167-168: LGTM: Default collision group change fixes core issue.The new default of
1is correct—the previous default of-1was problematic because negative groups don't collide with themselves. This ensures shapes collide by default while preserving the ability to disable collisions via0.
4863-4884: World and group filtering logic is correct.The two-stage filtering approach is sound:
- World indices: global (-1) entities collide with all worlds; world-specific entities only collide within the same world
- Collision groups: delegates to
_test_group_pairfor group-level filteringThis ensures consistency with the kernel-level implementation across all collision detection modes.
4909-4932: Excellent optimization with correct filtering logic.The world-based sorting (line 4911) enables the early-break optimization at lines 4924-4928. Since shapes are sorted by world index (-1, 0, 1, ...), once we encounter a shape pair in different non-global worlds, all subsequent shapes will also be incompatible with the first shape's world—making the early break both correct and beneficial for performance.
The use of
_test_world_and_group_pair(line 4931) ensures consistency with the kernel-level broad-phase filtering across EXPLICIT, NXN, and SAP collision modes.
Description
This PR fixes inconsistencies in collision filtering logic and updates default collision group values.
Problem
Inconsistent filtering: The
find_shape_contact_pairsmethod inbuilder.pywas not using the same collision filtering rules astest_world_and_group_pairinbroad_phase_common.py, causing discrepancies.Invalid defaults: Default
group_indexvalues were set to-1in the USD importer andShapeFlagsstruct. According to the filtering rules, negative indices don't collide with themselves, so nothing was colliding. Setting default to0for groups also doesn't work since group0means "no collisions".Solution
Updated
find_shape_contact_pairsto use the exact same filtering logic astest_world_and_group_pairfor consistency across all collision modes.Changed default collision group from
-1to1in:ModelBuilder.ShapeConfig.collision_groupparse_usd)Updated affected unit tests to reflect new default values.
Collision Filtering Rules
The
test_world_and_group_pairfunction implements these rules (agreed upon in Zurich meeting):Collision groups:
0: No collisions (disabled)1, 2, 3, ...): Only collide with themselves and negative groups-1, -2, -3, ...): Collide with everything except their negative counterpartNewton 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
Bug Fixes
Refactor
Documentation
Chores