newton/tests/test_mujoco_solver.py (1)

234-241: Make the ground plane global so both environments collide with it.

With environment-group filtering, shapes inherit the builder’s current_env_group. The plane is added before the per-env sub-builders and will default to the current (non-global) group, so only env 0 collides with it. Mark it as global (-1) to share across all envs.

Apply this diff near the plane creation:

-        self.builder.add_shape_plane()
+        # Share the plane across all environments
+        prev_env = self.builder.current_env_group
+        self.builder.current_env_group = -1
+        self.builder.add_shape_plane()
+        self.builder.current_env_group = prev_env

newton/examples/example_robot_manipulating_cloth.py (1)

208-215: API change adoption looks correct.

Switching to builder.add_builder(articulation_builder, xform) matches the new signature. If this example later grows to multi-environment usage, consider passing environment explicitly or setting current_env_group accordingly to avoid unintended cross-env contacts.

newton/_src/geometry/broad_phase_common.py (1)

89-120: Clarify docstring to avoid conflating env -1 with collision-group negatives.

Because collision_group negatives have special “collide with all except same negative” semantics, it’s worth explicitly stating that env_group -1 is unrelated to collision_group -1 and only means “global” for environments.

Apply this doc tweak:

-    Environment groups define which simulation environment an entity belongs to:
+    Environment groups define which simulation environment an entity belongs to:
     - Group -1: Global entities that collide with all environments
     - Groups 0, 1, 2, ...: Environment-specific entities
@@
-    3. Within the same environment, collision groups determine interactions
+    3. Within the same environment (or if one side is global), collision groups determine interactions
+
+    Note:
+        env_group values are independent from collision_group values. In particular,
+        env_group == -1 indicates a global environment membership and must not be
+        confused with collision_group < 0 semantics.

newton/_src/geometry/kernels.py (1)

646-654: Prefer explicit 32-bit types for group arrays (GPU portability).

Using dtype=int can map to 64-bit on some hosts. Declare particle_group and shape_group as wp.int32 to match other integer buffers and avoid implicit casts on device backends.

Apply this change to the kernel signature:

-    particle_group: wp.array(dtype=int),  # Environment groups for particles
+    particle_group: wp.array(dtype=wp.int32),  # Environment groups for particles
@@
-    shape_group: wp.array(dtype=int),  # Environment groups for shapes
+    shape_group: wp.array(dtype=wp.int32),  # Environment groups for shapes

newton/_src/sim/style3d/builder_style3d.py (2)

108-117: Docstring/API alignment for environment parameter

Good addition. Minor mismatch: the parent’s doc explains auto-increment vs. explicit environment behavior; here it still says “incremented by 1”. Align the wording with ModelBuilder.add_builder to avoid confusion.

-            update_num_env_count (bool): if True, the number of environments is incremented by 1.
-            environment (int | None): environment group index to assign to ALL entities from this builder.
+            update_num_env_count (bool): if True, the number of environments is updated appropriately.
+            environment (int | None): Environment group index to assign to ALL entities from this builder.
+                If None, uses the current environment count as the group index. Use -1 for global entities.

---

119-124: Type narrowness on builder (consider widening to base type)

builder: Style3DModelBuilder limits composition with plain ModelBuilder sub-builders. Consider typing as ModelBuilder (or a union) since you delegate to super().add_builder(...) anyway.

-        builder: Style3DModelBuilder,
+        builder: ModelBuilder,

newton/_src/sim/builder.py (2)

742-755: Offsetting collision pairs/group map before shape arrays are extended is safe—but fragile if re-ordered later

Using shape_count_offset = self.shape_count before extending shape arrays is correct because shape_count is based on shape_type, which hasn’t been extended yet. Add a short comment to future-proof against accidental reordering.

-        shape_count_offset = self.shape_count
+        # NOTE: shape_count is based on shape_type length, which hasn't been extended yet,
+        # so this offset points to the first index of the soon-to-be-appended shapes.
+        shape_count_offset = self.shape_count

---

4070-4099: Broadphase-precontact filtering by environment is correct and matches semantics

• Skips cross-env pairs unless one side is global (-1).
• Applies both for intra-group combinations and group -1 vs. others.
  Consider reusing the same predicate as the kernels for parity (e.g., test_environment_and_group_pair) to keep one source of truth, but current logic is equivalent.

-                # Skip shapes from different environments (unless one is global)
-                if env1 != -1 and env2 != -1 and env1 != env2:
-                    continue
+                # Skip shapes from different environments (unless one is global)
+                # Equivalent to test_environment_and_group_pair(env1, env2, _, _)
+                if env1 != -1 and env2 != -1 and env1 != env2:
+                    continue

newton/tests/test_broad_phase.py (2)

129-131: Passing shape_groups as global (-1) is a good baseline

This keeps parity with the numpy reference (which ignores envs). Consider adding a tiny focused test with two distinct env IDs to assert cross-env filtering at the broadphase level (there’s a separate test file, but a smoke check here helps).

---

386-388: SAP path parity with NXN

Same comment as above; baseline use is fine. A small follow-up asserting env-separated filtering would increase coverage symmetry.

newton/_src/geometry/broad_phase_nxn.py (4)

98-106: Filter on env+collision groups before AABB

Functionally correct. If you want a minor perf tweak, you could first do the cheap AABB overlap then group filtering, but difference is likely negligible vs. memory access here.

---

151-156: Launch signature: specify dtype for geom_shape_groups in the API

To match other params, annotate dtype in the signature comment and enforce at runtime (assert) for clearer errors.

-        geom_shape_groups: wp.array,  # Environment group ID per box (dtype=int, ndim=1)
+        geom_shape_groups: wp.array(dtype=int, ndim=1),  # Environment group ID per box (-1=global)

---

170-179: Docstring mentions collision-group-only test

Update the docstring to reference the env+group predicate and the new parameter.

-    2. Their collision groups allow interaction (determined by test_group_pair())
+    2. Their environment and collision groups allow interaction
+       (determined by test_environment_and_group_pair())

---

187-201: Optional: accept None for geom_shape_groups (default to global)

SAP’s doc mentions None (see separate comment); for API parity, you could optionally accept None here and substitute a -1-filled array. Not required if you prefer stricter typing.

-        wp.launch(
+        # (Optional) enable if you want to accept None:
+        # if geom_shape_groups is None:
+        #     geom_shape_groups = wp.array(np.full(geom_count, -1, dtype=np.int32), dtype=wp.int32)
+        wp.launch(

newton/_src/geometry/broad_phase_sap.py (1)

458-481: Kernel invocation includes env groups—good. Add a dtype assertion for clearer failures.

-        wp.launch(
+        # Defensive check for dtype/shape
+        assert geom_shape_groups.dtype == wp.int32 and geom_shape_groups.ndim == 1, \
+            "geom_shape_groups must be int32, 1D (values -1 for global or >=0 per environment)"
+        wp.launch(

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.

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

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).

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

newton/tests/test_mujoco_solver.py (1)

237-241: API migration: add_builder call updated correctly.

Dropping separate_collision_group aligns with the new environment-group semantics. Call site looks good.

newton/_src/geometry/broad_phase_common.py (1)

89-120: Environment-aware collision predicate is correct and minimal.

The early-return on different non-global envs followed by test_group_pair preserves intended semantics (global -1 interacts with all, per-env isolation otherwise).

newton/_src/geometry/kernels.py (1)

674-681: Early env-group rejection is in the right place.

The check is before any expensive geometry work, after the standard ACTIVE/COLLIDE_PARTICLES gates. This minimizes cost for cross-env pairs.

newton/_src/sim/collide.py (2)

160-180: create_soft_contacts call site updated correctly (ordering matches kernel).

particle_group is passed after particle_flags; shape_group after shape_source_ptr; shape_flags after shape_count. Matches the kernel signature.

---

160-180: ✅ Verified create_soft_contacts kernel signature

All references to create_soft_contacts have been located, and the sole invocation in newton/_src/sim/collide.py (lines 160–180) passes the 15 inputs in the exact order of the updated kernel definition, with the outputs list matching the seven output parameters. No other call sites were found.

newton/_src/sim/builder.py (5)

603-607: Environment-group semantics: clear and correct

The added notes accurately capture the new rules (global -1 collides with all; other envs isolated; collision groups preserved). Nice.

---

640-646: Group index selection covers all cases; edge-case sanity

Logic for selecting group_idx with/without update_num_env_count is sound and matches the doc. No changes needed.

---

756-782: Uniform environment assignment across entity kinds

Overriding particle/body/shape/joint/articulation groups to current_env_group is consistent and easy to reason about. LGTM.

---

863-872: num_envs update correctly skips globals and respects explicit env indices

Update logic is correct for auto-increment vs. explicit indexing. No change.

---

589-595: No remaining references to separate_collision_group

I ran a global search for any usages of the removed separate_collision_group parameter and found none, so the change to the add_builder() signature does not break any call sites.

newton/_src/geometry/broad_phase_nxn.py (1)

18-19: Importing combined predicate is the right call

Switch to test_environment_and_group_pair centralizes the policy. Good.

newton/_src/geometry/broad_phase_sap.py (2)

18-23: Good centralization on common helpers

Import set is tidy and consistent with NxN.

---

269-277: SAP pair filtering respects env+group policy

Correctly mirrors NxN behavior. LGTM.

newton/tests/test_environment_group_collision.py (8)

1-22: LGTM - Well-structured test module setup.

The imports and module structure look good. The import of test_environment_and_group_pair from the internal module is appropriate for testing the environment-aware collision filtering functionality.

---

24-30: LGTM - Standard test class setup.

The test class follows unittest conventions with proper setup method for device initialization.

---

31-86: LGTM - Comprehensive shape collision filtering test.

This test effectively validates the core environment group functionality:

• Shapes in different environments (0 vs 1) correctly avoid collision
• Global environment (-1) properly collides with all environments
• The test assertions verify both negative (no cross-environment collision) and positive (global collisions) cases

The test design aligns well with the PR objectives for environment-aware collision detection.

---

87-122: LGTM - Thorough particle-shape collision filtering test.

This test validates the particle collision filtering works correctly with environment groups:

• Particle in environment 0 doesn't collide with shape in environment 1
• Particle correctly collides with global shape (environment -1)
• Proper verification of soft contact counts and contact shape indices

This addresses the particle collision aspect mentioned in the PR objectives.

---

123-178: LGTM - Excellent test for add_builder environment assignment.

This test thoroughly validates the add_builder functionality:

• Environment groups are correctly assigned when adding builders
• Both shape and body groups are properly set
• Collision groups are preserved during the builder merge process
• The test covers the breaking API change mentioned in the PR where add_builder now handles environment groups

The assertions comprehensively verify the expected behavior described in the PR objectives.

---

179-260: LGTM - Comprehensive mixed collision and environment groups test.

This is an excellent stress test that validates the interaction between collision groups and environment groups:

• Tests multiple environments with various collision group combinations
• Verifies that environment isolation takes precedence over collision group compatibility
• Only global environment (-1) can cross environment boundaries
• The expected pairs calculation and verification is thorough and correct

This test ensures the complex interaction logic works as intended per the PR design.

---

262-299: LGTM - Thorough broadphase kernel testing.

This test validates the low-level test_environment_and_group_pair function with comprehensive test cases:

• Covers same environment scenarios with different collision group combinations
• Tests cross-environment isolation
• Validates global environment (-1) behavior
• Uses Warp kernels to test the actual GPU/CPU function implementation

The test cases cover all the important edge cases and align with the broadphase kernel changes mentioned in the PR.

---

301-304: LGTM - Proper test runner setup.

The kernel cache clearing and main runner setup is appropriate for Warp-based tests.

newton/_src/sim/builder.py (2)

665-669: Doc nit: spell out world-attached (-1 body) same-body filtering to avoid confusion with “global collides with all.”

The bullets are accurate but can be misread. Global env-group (-1) shapes still won’t collide with other shapes attached to the same world body (-1) because same-body collisions are filtered. Calling this out here prevents confusion.

Proposed doc tweak:

 Environment groups automatically handle collision filtering between different environments:
 - Entities from different environments (except -1) do not collide with each other
 - Global entities (group -1) collide with all environments
 - Collision groups from the source builder are preserved as-is for fine-grained collision control within each environment
+Note: Shapes attached to the world body (body == -1) do not collide with other shapes on the world body due to same‑body
+collision filtering. This remains true even when those shapes are in the global environment (-1).

---

4186-4193: Unify env+group predicate with the kernel-side helper to prevent drift.

These blocks re-implement the environment compatibility rule inline. Since the PR introduced test_environment_and_group_pair(envA, envB, groupA, groupB) for kernels, reuse it here to keep CPU-side prefiltering in lockstep with GPU semantics.

Minimal change:

+from ..geometry.broad_phase_common import test_environment_and_group_pair
@@
-                # Check environment groups
-                env1 = self.shape_group[s1] if s1 < len(self.shape_group) else -1
-                env2 = self.shape_group[s2] if s2 < len(self.shape_group) else -1
-
-                # Skip shapes from different environments (unless one is global)
-                if env1 != -1 and env2 != -1 and env1 != env2:
-                    continue
+                env1 = self.shape_group[s1] if s1 < len(self.shape_group) else -1
+                env2 = self.shape_group[s2] if s2 < len(self.shape_group) else -1
+                grp1 = self.shape_collision_group[s1]
+                grp2 = self.shape_collision_group[s2]
+                if not test_environment_and_group_pair(env1, env2, grp1, grp2):
+                    continue
@@
-                    # Check environment groups
-                    env1 = self.shape_group[s1] if s1 < len(self.shape_group) else -1
-                    env2 = self.shape_group[s2] if s2 < len(self.shape_group) else -1
-
-                    # Skip shapes from different environments (unless one is global)
-                    if env1 != -1 and env2 != -1 and env1 != env2:
-                        continue
+                    env1 = self.shape_group[s1] if s1 < len(self.shape_group) else -1
+                    env2 = self.shape_group[s2] if s2 < len(self.shape_group) else -1
+                    grp1 = self.shape_collision_group[s1]
+                    grp2 = self.shape_collision_group[s2]
+                    if not test_environment_and_group_pair(env1, env2, grp1, grp2):
+                        continue

Note: keep the cross-product with collision-group -1; the helper only answers “allowed?”, not “which sets to iterate.”

Also applies to: 4209-4216

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.

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

newton/_src/sim/builder.py (1)

807-811: Canonicalize filter pairs on builder merge to avoid missed filters

find_shape_contact_pairs() canonicalizes pairs as (min, max) before lookup. Merging non-canonical pairs here means filters won’t match and undesired contact pairs can slip through, especially after replication/merging multiple builders.

Apply canonicalization during insertion:

-        shape_count_offset = self.shape_count
-        for i, j in builder.shape_collision_filter_pairs:
-            self.shape_collision_filter_pairs.add((i + shape_count_offset, j + shape_count_offset))
+        shape_count_offset = self.shape_count
+        for i, j in builder.shape_collision_filter_pairs:
+            a = i + shape_count_offset
+            b = j + shape_count_offset
+            if a > b:
+                a, b = b, a
+            self.shape_collision_filter_pairs.add((a, b))

Also consider normalizing other insertion sites (e.g., same-body and parent–child insertions) for consistency; see note in the follow-up comment below.

Run this quick scan to find other non-canonical insertions of shape_collision_filter_pairs.add:

#!/bin/bash
# Show all call sites with context and highlight obvious canonicalization.
rg -nC3 -P '\bshape_collision_filter_pairs\.add\s*\(' newton/_src/sim/builder.py
# Heuristic search for min/max normalization near these sites.
rg -nC1 -P '\b(min|max)\s*\(' newton/_src/sim/builder.py

newton/_src/sim/builder.py (2)

4190-4198: Environment filtering in contact-pair generation: logic is correct; consider small cleanup

• The predicate “skip when env1 != -1 and env2 != -1 and env1 != env2” implements “same env or either is global,” which matches the PR’s spec.
• Minor: the defensive indexing defaulting to -1 will silently treat absent env-tags as global. If that’s not expected, consider asserting lengths match shape_count during finalize.

Optional micro-refactor to reduce duplication in both loops:

• Extract a tiny predicate:
  same_env_or_global = (env1 == -1) or (env2 == -1) or (env1 == env2)

Then use if not same_env_or_global: continue.

Also, leveraging the retrieved learning that world-attached shapes (body == -1) don’t collide with each other automatically, you can save work by pruning world–world pairs here:

• Early-continue when both self.shape_body[s1] == -1 and self.shape_body[s2] == -1.

This can reduce contact-pair list size and downstream kernel work without changing behavior.

If you want, I can draft a follow-up diff that adds a local _same_env_or_global(env1, env2) predicate and a world–world pruning check in both loops.

---

4205-4225: Global collision group (-1) cross-pairing looks right; consider DRY-ing the predicate

The special-case loop for group != -1 vs -1 is correct and respects env rules. To reduce duplication and the risk of drift, consider factoring the shared flag/shape/env checks into a local helper used by both loops.

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.

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

newton/_src/sim/builder.py (1)

1056-1060: Canonicalize merged filter pairs to avoid missed filters

When importing builder.shape_collision_filter_pairs, pairs may be stored non-canonically upstream. Since find_shape_contact_pairs() looks up canonicalized pairs (min, max), a non-canonical insertion here can reintroduce contacts that were meant to be filtered. Canonicalize before insertion.

Apply:

-        for i, j in builder.shape_collision_filter_pairs:
-            self.shape_collision_filter_pairs.add((i + shape_count_offset, j + shape_count_offset))
+        for i, j in builder.shape_collision_filter_pairs:
+            a = i + shape_count_offset
+            b = j + shape_count_offset
+            if a > b:
+                a, b = b, a
+            self.shape_collision_filter_pairs.add((a, b))

#!/bin/bash
# Verify all insertions into shape_collision_filter_pairs are canonicalized.
rg -nC2 -P '\bshape_collision_filter_pairs\.add\s*\('

newton/_src/sim/builder.py (4)

914-918: Doc: clarify world-attached (-1 body) non-collision and cross-reference behavior

Nice summary. Consider adding a one-liner that shapes attached to the world (body == -1) never collide with each other, so users don’t need to add manual filters for those. This aligns expectations with runtime behavior. Based on retrieved learnings from PR #587.

---

1061-1066: Optional: guard against accidental duplicates in group map

If the source builder ever contained duplicates (or future code paths append the same shape twice), itertools.combinations(shapes, 2) will emit degenerate pairs and extra work. Not critical today, but you can defensively deduplicate during merge.

Example (keep list type, preserve order of first occurrences):

-            self.shape_collision_group_map[group].extend([s + shape_count_offset for s in shapes])
+            seen = set(self.shape_collision_group_map[group])
+            for s in shapes:
+                idx = s + shape_count_offset
+                if idx not in seen:
+                    self.shape_collision_group_map[group].append(idx)
+                    seen.add(idx)

---

4440-4447: Env filtering logic matches the stated semantics

The check env1 != -1 and env2 != -1 and env1 != env2: continue enforces “same env or either global (-1)” correctly before adding pairs.

Minor maintainability: factor this into a tiny helper to keep parity with the kernel-side predicate and reduce duplication:

+def _envs_collide(e1: int, e2: int) -> bool:
+    return e1 == -1 or e2 == -1 or e1 == e2
...
-                if env1 != -1 and env2 != -1 and env1 != env2:
+                if not _envs_collide(env1, env2):
                     continue

---

4463-4470: Consistent env filtering for group -1 cross-pairs

Same comment as above; logic is correct and consistent with the doc. Consider reusing the same helper here to avoid divergence over time.

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.

Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.042Z
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.

newton/_src/geometry/broad_phase_nxn.py (1)

133-140: Docstring is outdated; mention environment groups and new predicate

Update to reflect env-group semantics and the combined predicate.

-    The collision checks take into account per-geometry cutoff distances and collision groups. Two geometries
-    will only be considered as a candidate pair if:
-    1. Their AABBs overlap when expanded by their cutoff distances
-    2. Their collision groups allow interaction (determined by test_group_pair())
+    The collision checks take into account per-geometry cutoff distances, environment groups, and collision
+    groups. Two geometries will only be considered as a candidate pair if:
+    1. Their AABBs overlap when expanded by their cutoff distances
+    2. Their environment groups are compatible (same environment or at least one is global, -1)
+    3. Their collision groups allow interaction (see test_environment_and_group_pair()) 

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.

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).