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

295-297: Guard against None for node_colors in draw call.

Passing node_color=None to NetworkX/Matplotlib can error depending on versions. Draw without the arg when colors aren’t provided.

Apply this diff:

-    nx.draw_networkx_nodes(G, pos, node_color=node_colors, **default_draw_args)
+    if node_colors is None:
+        nx.draw_networkx_nodes(G, pos, **default_draw_args)
+    else:
+        nx.draw_networkx_nodes(G, pos, node_color=node_colors, **default_draw_args)

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

1985-1991: Double newline in shape labels.

You append "\n(...)" and then "\n".join(...), yielding a blank line. Drop the leading newline in the type.

Apply this diff:

-                if show_shape_types:
-                    shape_label.append(f"\n({shape_type_str(self.shape_type[i])})")
+                if show_shape_types:
+                    shape_label.append(f"({shape_type_str(self.shape_type[i])})")

---

2413-2418: Canonicalize same-body filter pairs for robustness.

While current indices likely make (same_body_shape, shape) ordered, enforce canonical ordering to avoid future edge cases.

Apply this diff:

-        if cfg.has_shape_collision:
+        if cfg.has_shape_collision:
             # no contacts between shapes of the same body
             for same_body_shape in self.body_shapes[body]:
-                self.shape_collision_filter_pairs.append((same_body_shape, shape))
+                a, b = same_body_shape, shape
+                if a > b:
+                    a, b = b, a
+                self.shape_collision_filter_pairs.append((a, b))

---

2435-2440: Canonicalize parent–child filter pairs too.

Match the ordering used in find_shape_contact_pairs to ensure lookups succeed.

Apply this diff:

-                    for parent_shape in self.body_shapes[parent_body]:
-                        self.shape_collision_filter_pairs.append((parent_shape, shape))
+                    for parent_shape in self.body_shapes[parent_body]:
+                        a, b = parent_shape, shape
+                        if a > b:
+                            a, b = b, a
+                        self.shape_collision_filter_pairs.append((a, b))

---

4463-4465: Handle empty contact_pairs without constructing a NumPy array.

Warp arrays from np.array([]) can be brittle for vector dtypes; allocate an empty vec2i directly.

Apply this diff:

-        model.shape_contact_pairs = wp.array(np.array(contact_pairs), dtype=wp.vec2i, device=model.device)
+        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.empty(shape=(0,), dtype=wp.vec2i, device=model.device)

newton/_src/solvers/mujoco/solver_mujoco.py (5)

1670-1680: Avoid false positives by checking both (a,b) and (b,a) in filter set

If model.shape_collision_filter_pairs isn’t guaranteed to store both orders, current membership test can misclassify edges.

Apply this diff:

@@
-        num_shapes = len(selected_shapes)
-        shape_a, shape_b = np.triu_indices(num_shapes, k=1)
-        cgroup = [model.shape_collision_group[i] for i in selected_shapes]
+        num_shapes = len(selected_shapes)
+        shape_a, shape_b = np.triu_indices(num_shapes, k=1)
+        cgroup = [model.shape_collision_group[i] for i in selected_shapes]
+        pair_filters = model.shape_collision_filter_pairs
@@
-        graph_edges = [
-            (i, j)
-            for i, j in zip(shape_a, shape_b, strict=True)
-            if (
-                (selected_shapes[i], selected_shapes[j]) not in model.shape_collision_filter_pairs
-                and (cgroup[i] == cgroup[j] or cgroup[i] == -1 or cgroup[j] == -1)
-            )
-        ]
+        graph_edges = [
+            (i, j)
+            for i, j in zip(shape_a, shape_b, strict=True)
+            if (
+                (selected_shapes[i], selected_shapes[j]) not in pair_filters
+                and (selected_shapes[j], selected_shapes[i]) not in pair_filters
+                and (cgroup[i] == cgroup[j] or cgroup[i] == -1 or cgroup[j] == -1)
+            )
+        ]

---

1664-1669: Remove unused mapping

to_color_shape_index is created but never used.

---

2063-2063: Sanitize geom names derived from shape_key

MuJoCo names must be valid identifiers; if shape_key can contain spaces or special chars, sanitize (e.g., replace non-alnum with '', ensure first char is letter/).

---

2107-2111: Color bit budget: guard for >=32 colors

If graph coloring yields >=32, contype won’t be set and MuJoCo defaults may cause unintended collisions. Consider modulo bucketing or fallback to affinity-based excludes when color >= 32.

---

2360-2365: Add excludes safely; consider dedup and self-checks

• If body_filters can contain duplicates across envs/runs, dedup to avoid bloating XML.
• Optionally skip when b1 or b2 not in body_mapping (defensive).

Minor, but helps robustness.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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.

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.

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

262-272: plot_graph: accept optional labels/colors more defensively

Small guardrails will prevent confusing errors when node_labels/node_colors lengths don’t match vertices, and avoid passing None to node_color.

Apply:

 def plot_graph(vertices, edges, edge_labels=None, node_labels=None, node_colors=None):
@@
-    if edge_labels is None:
-        edge_labels = []
+    if edge_labels is None:
+        edge_labels = []
+    if node_labels is not None and len(node_labels) != len(vertices):
+        raise ValueError("node_labels length must match vertices length")
+    if node_colors is not None and len(node_colors) != len(vertices):
+        raise ValueError("node_colors length must match vertices length")

---

296-301: Avoid passing node_color=None; ensure clean label fallback

• Only pass node_color when provided.
• Current type-line adds a leading newline when only shape type is shown; drop the prefix newline to avoid leading blank labels.

Apply:

-    nx.draw_networkx_nodes(G, pos, node_color=node_colors, **default_draw_args)
+    draw_args = dict(default_draw_args)
+    if node_colors is not None:
+        draw_args["node_color"] = node_colors
+    nx.draw_networkx_nodes(G, pos, **draw_args)
@@
-        labels=dict(enumerate(node_labels if node_labels is not None else vertices)),
+        labels=dict(enumerate(node_labels if node_labels is not None else vertices)),

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

1985-1991: Shape labels may start with a leading newline when only type is shown

When show_shape_keys=False and show_shape_types=True, shape_label becomes ["\n(type)"], producing a leading blank line. Drop the prefix newline and rely on join.

Apply:

-                shape_label = []
+                shape_label = []
                 if show_shape_keys:
                     shape_label.append(self.shape_key[i])
                 if show_shape_types:
-                    shape_label.append(f"\n({shape_type_str(self.shape_type[i])})")
-                vertices.append("\n".join(shape_label))
+                    shape_label.append(f"({shape_type_str(self.shape_type[i])})")
+                vertices.append("\n".join(shape_label))

---

1920-1937: plot_articulation: new show_shape_keys option — nice usability

The new toggle is sensible and defaults are reasonable. Please reflect the new argument in the public API docs/migration guide.

Add a brief note in docs/migration.rst describing:

• ShapeConfig.has_shape_collision (new)
• plot_articulation(show_shape_keys=True) (new default)

---

2413-2417: Optional: centralize pair canonicalization to avoid future drift

Consider a tiny helper (local function or private method) append_filter_pair(i, j) that sorts and appends once. Reduces repetition and prevents regressions.

Also applies to: 2435-2440

newton/_src/solvers/mujoco/solver_mujoco.py (2)

1071-1132: Clamp mu to non-negative and preempt invalid friction

If a material supplies a negative μ (bad input), MuJoCo will receive negative friction, which is non-physical. Clamp μ ≥ 0 in-kernel.

Apply this diff:

-    mu = shape_mu[shape_idx]
-    geom_friction[worldid, geom_idx] = wp.vec3f(mu, torsional_friction * mu, rolling_friction * mu)
+    mu = wp.max(shape_mu[shape_idx], 0.0)
+    geom_friction[worldid, geom_idx] = wp.vec3f(mu, torsional_friction * mu, rolling_friction * mu)

---

2034-2044: Body-exclude + color: order of operations is good; guard colliding_shapes empty

Sequence (compute excludes → colors) makes sense. If colliding_shapes is empty, both operations are no-ops; consider an early return to skip work.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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.

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.

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

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

2413-2418: Canonicalize and gate same‑body filter pairs; current order can miss filtering

Pairs are appended as (same_body_shape, shape) without normalization and without checking the existing shape’s COLLIDE_SHAPES flag. find_shape_contact_pairs checks membership using canonical (min, max), so reversed tuples won’t match and same‑body pairs may slip through.

Apply:

-        if cfg.has_shape_collision:
-            # no contacts between shapes of the same body
-            for same_body_shape in self.body_shapes[body]:
-                self.shape_collision_filter_pairs.append((same_body_shape, shape))
+        if cfg.has_shape_collision:
+            # no contacts between shapes of the same body
+            for same_body_shape in self.body_shapes[body]:
+                # skip non-colliding shapes
+                if not (self.shape_flags[same_body_shape] & ShapeFlags.COLLIDE_SHAPES):
+                    continue
+                # store in canonical order for consistent lookup
+                a, b = same_body_shape, shape
+                if a > b:
+                    a, b = b, a
+                self.shape_collision_filter_pairs.append((a, b))

Optional: also skip when body == -1 if global–global contacts are already excluded by the collision system.

Run to find any other non-canonical appends:

#!/bin/bash
rg -nP -C2 'shape_collision_filter_pairs\.append\('

newton/_src/solvers/mujoco/solver_mujoco.py (2)

1618-1651: Fix potential KeyError and canonicalize pair membership checks

The method has two issues:

1. Direct indexing of model.body_shapes[b1] will raise KeyError if a body has no shapes
2. Shape collision filter pairs may not be stored in canonical order, causing missed exclusions

Apply this diff to fix both issues:

     @staticmethod
     def find_body_collision_filter_pairs(
         model: Model,
         selected_bodies: nparray,
         colliding_shapes: nparray,
     ):
         """For shape collision filter pairs, find body collision filter pairs that are contained within."""
 
         body_exclude_pairs = []
         shape_set = set(colliding_shapes)
 
-        body_shapes = {}
-        for body in selected_bodies:
-            shapes = model.body_shapes[body]
-            shapes = [s for s in shapes if s in shape_set]
-            body_shapes[body] = shapes
-
         bodies_a, bodies_b = np.triu_indices(len(selected_bodies), k=1)
         for body_a, body_b in zip(bodies_a, bodies_b, strict=True):
             b1, b2 = selected_bodies[body_a], selected_bodies[body_b]
-            shapes_1 = body_shapes[b1]
-            shapes_2 = body_shapes[b2]
+            shapes_1 = model.body_shapes.get(b1, [])
+            shapes_2 = model.body_shapes.get(b2, [])
+            shapes_1 = [s for s in shapes_1 if s in shape_set]
+            shapes_2 = [s for s in shapes_2 if s in shape_set]
+            if not shapes_1 or not shapes_2:
+                continue
             excluded = True
             for shape_1 in shapes_1:
                 for shape_2 in shapes_2:
-                    if shape_1 > shape_2:
-                        s1, s2 = shape_2, shape_1
-                    else:
-                        s1, s2 = shape_1, shape_2
-                    if (s1, s2) not in model.shape_collision_filter_pairs:
+                    # Check both orderings since pairs may not be canonicalized
+                    if ((shape_1, shape_2) not in model.shape_collision_filter_pairs and
+                        (shape_2, shape_1) not in model.shape_collision_filter_pairs):
                         excluded = False
                         break
+                if not excluded:
+                    break
             if excluded:
                 body_exclude_pairs.append((b1, b2))
         return body_exclude_pairs

---

1681-1688: Normalize shape pair before membership check in solver_mujoco.py

Verified shape_collision_filter_pairs are appended from multiple importers and builder code and ultimately turned into a set; some append sites do not guarantee a consistent ordering. Make the membership test order-independent.

Location: newton/_src/solvers/mujoco/solver_mujoco.py (around lines 1681–1688)

Suggested change (use min/max to normalize the pair):

         graph_edges = [
             (i, j)
             for i, j in zip(shape_a, shape_b, strict=True)
             if (
-                (selected_shapes[i], selected_shapes[j]) not in model.shape_collision_filter_pairs
+                (min(selected_shapes[i], selected_shapes[j]), max(selected_shapes[i], selected_shapes[j]))
+                not in model.shape_collision_filter_pairs
                 and (cgroup[i] == cgroup[j] or cgroup[i] == -1 or cgroup[j] == -1)
             )
         ]

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

1920-1920: New plot_articulation flag works; avoid empty labels edge-case

When both show_shape_keys and show_shape_types are False, shape nodes get an empty label. Harmless, but consider defaulting to the shape index to keep labels non-empty.

Would you like a small patch to show the index when both toggles are False?

Also applies to: 1934-1935, 1985-1990

newton/_src/solvers/mujoco/solver_mujoco.py (2)

2121-2127: Friction setup uses undefined shape_mu variable

After fixing line 1955, ensure the geom friction setup uses the correct variable name consistently.

Verify the variable name is consistent after the fix at line 1955:

                # set friction from Newton shape materials using model's friction parameters
                mu = shape_mu[shape]
                geom_params["friction"] = [
                    mu,
                    model.rigid_contact_torsional_friction * mu,
                    model.rigid_contact_rolling_friction * mu,
                ]

---

2029-2029: Simplify joint order validation

The current validation using any() with a generator is correct but could be more readable.

Consider this more explicit approach:

-        if any(joint_order[i] != i for i in range(len(joints_simple))):
+        if not np.array_equal(joint_order, np.arange(len(joints_simple))):

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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.

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.