Fix perf regression due to find_shape_contact_pairs#699
Conversation
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Builder as ModelBuilder
participant Finder as find_shape_contact_pairs
Caller->>Builder: build()
activate Builder
Builder->>Finder: find_shape_contact_pairs(shapes, collision_groups, envs)
activate Finder
Note over Finder: Sort shapes by environment -> sorted_indices
loop for each i1 in sorted_indices
Finder->>Finder: colliding_indices = candidates after i1
loop for each i2 in colliding_indices
alt env1 != -1 and env1 != env2
Note right of Finder: break inner loop (env-sorted)
else
alt both collision_group != -1 and groups differ
Finder-->>Finder: skip pair
else
Finder->>Finder: canonicalize indices (a<b)
Finder-->>Finder: add if not duplicate
end
end
end
end
Finder-->>Builder: contact_pairs
deactivate Finder
Builder-->>Caller: result
deactivate Builder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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)
4440-4472: Fix incorrect indexing after sort; skip self-pairs; handle empty output safely.You sort by env but then index shape arrays with the loop counter (position in the sorted list), not the original shape index. This produces wrong pairs, allows self-pairs (s1 == s2), and defeats the early-exit-by-env logic. Also, constructing wp.array from an empty Python list can fail. Patch below uses sorted indices, j starts at i+1, preserves early-exit, and builds a stable empty array.
- # Sort shapes by env group in case theya re not sorted - sorted_shape_key = [x for _, x in sorted(zip(self.shape_group, self.shape_key, strict=False))] - # Iterates over all shapes candidates - for s1 in range(len(sorted_shape_key)): - if not (self.shape_flags[s1] & ShapeFlags.COLLIDE_SHAPES): + # Sort shapes by env group (primary), then by key for determinism. + sorted_indices = sorted( + range(self.shape_count), + key=lambda i: (self.shape_group[i], self.shape_key[i]), + ) + # Iterate over candidate pairs in sorted order; skip self-pairs and duplicate permutations. + for idx1_pos, s1 in enumerate(sorted_indices): + if not (self.shape_flags[s1] & ShapeFlags.COLLIDE_SHAPES): continue - env1 = self.shape_group[s1] - collision_group1 = self.shape_collision_group[s1] - for s2 in range(s1, len(sorted_shape_key)): - if not (self.shape_flags[s2] & ShapeFlags.COLLIDE_SHAPES): + env1 = self.shape_group[s1] + collision_group1 = self.shape_collision_group[s1] + for idx2_pos in range(idx1_pos + 1, len(sorted_indices)): + s2 = sorted_indices[idx2_pos] + if not (self.shape_flags[s2] & ShapeFlags.COLLIDE_SHAPES): continue - # Skip shapes from different environments (unless one is global). As the shapes are sorted, - # this means the shapes in this environment group have all been processed. - env2 = self.shape_group[s2] - if env1 != -1 and env2 != -1 and env1 != env2: - break + # Skip shapes from different environments (unless one is global). With env-sorted indices, + # once env2 differs from env1 (and both are non-global), we can early-exit. + env2 = self.shape_group[s2] + if env1 != -1 and env2 != -1 and env1 != env2: + break - # Skip shapes from different collision group (unless one is global). - collision_group2 = self.shape_collision_group[s2] - if collision_group1 != -1 and collision_group2 != -1 and collision_group1 != collision_group2: + # Skip shapes from different collision groups (unless one is global). + collision_group2 = self.shape_collision_group[s2] + if collision_group1 != -1 and collision_group2 != -1 and collision_group1 != collision_group2: continue - # Ensure canonical order (smaller_element, larger_element) - shape_a, shape_b = min(s1, s2), max(s1, s2) + # Ensure canonical order (smaller_index, larger_index) + shape_a, shape_b = (s1, s2) if s1 < s2 else (s2, s1) if (shape_a, shape_b) not in filters: contact_pairs.append((shape_a, shape_b)) filters.add((shape_a, shape_b)) - model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device) - model.shape_contact_pair_count = len(contact_pairs) + arr = np.array(contact_pairs, dtype=np.int32).reshape(-1, 2) + model.shape_contact_pairs = wp.array(arr, dtype=wp.vec2i, device=model.device) + model.shape_contact_pair_count = arr.shape[0]
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (2)
4453-4463: Optional: skip world–world pairs for perf.Given global/world-attached shapes (body == -1) never collide in Newton, skip those pairs up-front to reduce pair count.
- # Skip shapes from different collision groups (unless one is global). + # Skip shapes attached to world on both sides (no contacts are generated for them). + if self.shape_body[s1] == -1 and self.shape_body[s2] == -1: + continue + # Skip shapes from different collision groups (unless one is global).
4440-4441: Nit: fix typo in comment.“theya re” → “they are”.
- # Sort shapes by env group in case theya re not sorted + # Sort shapes by env group in case they are not sorted
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
newton/_src/sim/builder.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-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). (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 (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
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 (1)
newton/_src/sim/builder.py (1)
4475-4476: Guard empty contact_pairs to avoid vec2i shape issuesWhen no pairs exist, np.array(contact_pairs) is shape (0,), which can misalign with dtype=wp.vec2i. Create an explicit (0, 2) 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: + arr = np.asarray(contact_pairs, dtype=np.int32) + else: + arr = np.empty((0, 2), dtype=np.int32) + model.shape_contact_pairs = wp.array(arr, dtype=wp.vec2i, device=model.device) + model.shape_contact_pair_count = arr.shape[0]
🧹 Nitpick comments (3)
newton/_src/sim/builder.py (3)
4468-4474: Skip world-vs-world pairs to cut O(K^2) overhead for globalsPairs where both shapes are attached to the world (body == -1) are ignored by the collision system; don’t emit them here to reduce pair count and speed finalize(), per prior guidance.
# Ensure canonical order (smaller_element, larger_element) shape_a, shape_b = min(s1, s2), max(s1, s2) + # Skip pairs of two world-attached/global shapes; the collider ignores these anyway. + if self.shape_body[shape_a] == -1 and self.shape_body[shape_b] == -1: + continue + if (shape_a, shape_b) not in filters: contact_pairs.append((shape_a, shape_b)) filters.add((shape_a, shape_b))
4440-4443: Minor: use shape_count directly and drop the indices tempSlightly clearer and marginally cheaper.
- indices = range(len(self.shape_group)) - sorted_indices = sorted(indices, key=lambda i: self.shape_group[i]) + sorted_indices = sorted(range(self.shape_count), key=lambda i: self.shape_group[i])
4420-4435: Add tests and doc note for new pairing semanticsPlease add unit tests that cover:
- env early-exit (global vs per-env, cross-env filtering),
- collision_group == -1 (“all collide”) behavior,
- same-body filters respected,
- empty-scene case (no shapes) and no-collidable-flags case.
Update the migration guide for warp.sim users to call out:
- environment-aware contact generation,
- canonical pair ordering and dedup semantics,
- removal of separate “all-collide” path.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
newton/_src/sim/builder.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-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). (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 (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
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)
4475-4476: Fix empty-pairs shape to avoid runtime error when no contact pairs existWhen
contact_pairsis empty,np.array(contact_pairs)has shape (0,), which can breakwp.array(..., dtype=wp.vec2i). Ensure a (0,2) 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) + pairs_np = np.asarray(contact_pairs, dtype=np.int32) + if pairs_np.size == 0: + 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])
♻️ Duplicate comments (1)
newton/_src/sim/builder.py (1)
4453-4456: Good fix: skip non-collidable shapes with continue (no premature loop exit)This resolves the earlier “break vs continue” issue and prevents dropping valid candidates later in the same env.
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (2)
4457-4462: Early-exit by environment is correct but fragile to ordering changesRelies on shapes being sorted by env and -1 < 0 < 1 ... Consider guarding with a brief comment or computing env block boundaries once to make the intent robust.
Apply this minimal note:
- # Skip shapes from different environments (unless one is global). As the shapes are sorted, - # this means the shapes in this environment group have all been processed. + # Skip shapes from different environments (unless one is global). + # Shapes are sorted by env (-1, 0, 1, ...), so encountering env2 > env1 means + # the remaining entries are all >= env2 and no further matches for env1 exist.
4440-4443: Minor perf: precompute env buckets to reduce O(N^2) scanningYou can build env→[indices] once, then:
- pair globals (-1) with all others,
- pair within each env bucket,
- apply collision-group filter inside each bucket.
This avoids scanning cross-env ranges repeatedly.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
newton/_src/sim/builder.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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-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/sim/builder.py
📚 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). (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 (1)
newton/_src/sim/builder.py (1)
4464-4466: Collision-group filter semantics look rightTreating -1 as “collide with all groups” preserves prior behavior.
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)
4458-4460: Fix empty-contact case: constructing wp.vec2i from an empty 1D numpy array will failWhen no pairs are found,
np.array(contact_pairs)is shape (0,) and cannot be cast towp.vec2i. Guard the empty case.- 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: + model.shape_contact_pairs = wp.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)
🧹 Nitpick comments (3)
newton/_src/sim/builder.py (1)
4433-4444: Reduce inner-loop work by bucketing indices per environmentCurrent O(K^2) scan on
sorted_indicesstill iterates across all later shapes for eachi1. Bucket by env first and only form:
- intra-env pairs per env>=0, and
- cross pairs: global(-1) × each env.
This keeps behavior identical and trims iterations for many-env scenes. I can provide a drop-in patch if you’d like.
newton/tests/test_environment_group_collision.py (2)
223-234: Prefer body = -1 for truly global shapes (optional)F and G are global (env -1). Attaching them to the world (
body=-1) clarifies intent and aligns with common usage; it won’t affect expected pairs here.- builder.add_shape_sphere( - body=body_f, xform=wp.transform(wp.vec3(0, 2, 0), wp.quat_identity()), radius=0.5, cfg=cfg_f - ) + builder.add_shape_sphere( + body=-1, xform=wp.transform(wp.vec3(0, 2, 0), wp.quat_identity()), radius=0.5, cfg=cfg_f + ) @@ - builder.add_shape_sphere( - body=body_g, xform=wp.transform(wp.vec3(0, 4, 0), wp.quat_identity()), radius=0.5, cfg=cfg_g - ) + builder.add_shape_sphere( + body=-1, xform=wp.transform(wp.vec3(0, 4, 0), wp.quat_identity()), radius=0.5, cfg=cfg_g + )
251-257: Fix comment indices (G is shape 6, not 5)The explanatory comment still references index 5; the code expects 6. Update to avoid confusion.
- # - (0, 5): A (env0, group1) and G (global, group1) - # - (2, 5): C (env0, group-1) and G (global, group1) - # - (3, 5): D (env1, group1) and G (global, group1) + # - (0, 6): A (env0, group1) and G (global, group1) + # - (2, 6): C (env0, group-1) and G (global, group1) + # - (3, 6): D (env1, group1) and G (global, group1)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
newton/_src/sim/builder.py(1 hunks)newton/_src/sim/model.py(0 hunks)newton/tests/test_environment_group_collision.py(3 hunks)
💤 Files with no reviewable changes (1)
- newton/_src/sim/model.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/tests/test_environment_group_collision.pynewton/_src/sim/builder.py
📚 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/sim/builder.py
🧬 Code graph analysis (2)
newton/tests/test_environment_group_collision.py (1)
newton/_src/sim/builder.py (3)
add_body(1179-1241)ShapeConfig(141-193)add_shape_sphere(2519-2550)
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). (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 (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (2)
newton/tests/test_environment_group_collision.py (2)
190-191: OK: new body_g for the added global shapeBody allocation looks fine and matches the new test scenario.
258-264: LGTM: expected pair set matches the new layoutThe expected pairs correctly reflect env and collision-group rules with G at index 6 and F non-colliding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/tests/test_environment_group_collision.py (3)
225-234: Good call making F non-colliding; capture IDs to reduce brittle index assumptionsMarking F with
has_shape_collision=Falseensures it’s excluded byfind_shape_contact_pairs(it filters onShapeFlags.COLLIDE_SHAPES). To future-proof the test against index shifts, capture the returned IDs for F/G and reuse them below.Apply:
-# Shape F: collision group 2, not a colliding shape -cfg_f = ModelBuilder.ShapeConfig(collision_group=2, has_shape_collision=False) -builder.add_shape_sphere( - body=body_f, xform=wp.transform(wp.vec3(0, 2, 0), wp.quat_identity()), radius=0.5, cfg=cfg_f -) -# Shape G: collision group 1 -cfg_g = ModelBuilder.ShapeConfig(collision_group=1) -builder.add_shape_sphere( - body=body_g, xform=wp.transform(wp.vec3(0, 4, 0), wp.quat_identity()), radius=0.5, cfg=cfg_g -) +# Shape F: collision group 2, not a colliding shape +cfg_f = ModelBuilder.ShapeConfig(collision_group=2, has_shape_collision=False) +f_id = builder.add_shape_sphere( + body=body_f, xform=wp.transform(wp.vec3(0, 2, 0), wp.quat_identity()), radius=0.5, cfg=cfg_f +) +# Shape G: collision group 1 +cfg_g = ModelBuilder.ShapeConfig(collision_group=1) +g_id = builder.add_shape_sphere( + body=body_g, xform=wp.transform(wp.vec3(0, 4, 0), wp.quat_identity()), radius=0.5, cfg=cfg_g +)If you’d like, I can also refactor earlier adds to capture
a_id,b_id,c_id,d_id,e_idand eliminate hard-coded 0–4.
251-257: Make the “no F pairs” expectation explicit and use captured IDsSince F is non-colliding, assert it never appears in computed pairs; also prefer
g_idover literal6.-# Expected cross-environment pairs (only with global): -# - (0, 6): A (env0, group1) and G (global, group1) -# - (2, 6): C (env0, group-1) and G (global, group1) -# - (3, 6): D (env1, group1) and G (global, group1) +# Expected cross-environment pairs (only with global G): +# - (0, g_id): A (env0, group1) and G (global, group1) +# - (2, g_id): C (env0, group-1) and G (global, group1) +# - (3, g_id): D (env1, group1) and G (global, group1) # No pairs between env0 and env1 -# F is not a colliding shape +# F is not a colliding shape +# Sanity check: F must not appear in any pair +self.assertTrue(all(f_id not in pair for pair in contact_set), "Non-colliding shape F should not appear in contact pairs")
261-264: Replace hard-coded index 6 with g_id in expectationsThis avoids coupling to shape creation order.
- (0, 6), # A-G (env0-global) - (2, 6), # C-G (env0-global) - (3, 6), # D-G (env1-global) + (0, g_id), # A-G (env0-global) + (2, g_id), # C-G (env0-global) + (3, g_id), # D-G (env1-global)Optional: similarly replace
(0, 2)and(1, 2)with captureda_id,b_id,c_idfor full robustness.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
newton/tests/test_environment_group_collision.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/tests/test_environment_group_collision.py
🧬 Code graph analysis (1)
newton/tests/test_environment_group_collision.py (1)
newton/_src/sim/builder.py (3)
ModelBuilder(67-4459)ShapeConfig(141-193)add_shape_sphere(2519-2550)
🔇 Additional comments (1)
newton/tests/test_environment_group_collision.py (1)
190-190: LGTM: separate body for G is correctAdding
body_gavoids same-body filter interactions and keeps pair generation unambiguous.
Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Description
Changed the algorithm in find_shape_contact_pairs to speed up builder
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
Refactor
Breaking Changes
Tests