Fix MJCF collision mask filtering - #3104
Closed
m-abr wants to merge 1 commit into
Closed
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MuJoCo-style per-shape uint32 collision type/affinity masks throughout the physics engine. Masks are authored on shapes, compiled from collision groups during model finalization, and applied consistently in broad-phase filtering (NxN and SAP), builder contact-pair generation, MJCF import/export, and MuJoCo solver export; includes regression tests and documentation updates. ChangesCollision type/affinity filtering implementation
Sequence Diagram(s)sequenceDiagram
participant Builder as ModelBuilder
participant Compiler as _compile_shape_collision_masks()
participant Model as Model (runtime)
participant CollPipeline as CollisionPipeline
participant BroadPhase as BroadPhaseAllPairs / SAP
participant KernelTest as test_world_and_type_affinity_pair
Builder->>Compiler: finalize: convert collision_group or explicit masks
Compiler->>Model: set shape_collision_type & shape_collision_affinity arrays
Model->>CollPipeline: model contains mask arrays
CollPipeline->>BroadPhase: launch with shape_collision_type/affinity
BroadPhase->>KernelTest: per-pair test(world_a, world_b, type_a, affinity_a, type_b, affinity_b)
KernelTest->>BroadPhase: bool accept/reject pair
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
m-abr
had a problem deploying
to
external-pr-approval
June 8, 2026 16:03 — with 
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 8, 2026 16:03 — with
GitHub Actions
Error
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
newton/_src/sim/builder.py (1)
467-474:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't wipe authored masks when updating non-site flags.
This branch now resets
collision_typeandcollision_affinityto defaults every timeflagsis assigned without theSITEbit. That breaks round-tripping for configs with authored masks, and the remeshing path later in this file hits exactly that pattern, so decomposed convex parts fall back to default collision filtering.Suggested fix
else: - # SITE flag is being cleared - restore non-site defaults - defaults = self.__class__() + # Only restore site-specific defaults when transitioning from site -> non-site. + was_site = self.is_site self.is_site = False - self.density = defaults.density - self.collision_group = defaults.collision_group - self.collision_type = defaults.collision_type - self.collision_affinity = defaults.collision_affinity + if was_site: + defaults = self.__class__() + self.density = defaults.density + self.collision_group = defaults.collision_group + self.collision_type = defaults.collision_type + self.collision_affinity = defaults.collision_affinity self.has_shape_collision = bool(value & ShapeFlags.COLLIDE_SHAPES) self.has_particle_collision = bool(value & ShapeFlags.COLLIDE_PARTICLES)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sim/builder.py` around lines 467 - 474, When clearing the SITE flag in the flags-assignment branch, do not unconditionally reset authored collision masks; instead only restore non-site defaults that are not mask-related. Remove or stop setting self.collision_type and self.collision_affinity in that else-branch where defaults = self.__class__() and self.is_site is set to False, keeping restoration of density and collision_group as needed so that authored masks on collision_type/collision_affinity are preserved and remeshing/decomposition will round-trip correctly.newton/_src/geometry/broad_phase_nxn.py (1)
316-360: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winIncomplete docstring for new parameters.
The
launch()method signature addedshape_collision_typeandshape_collision_affinityoptional parameters (lines 331-332), but the docstring'sArgs:section doesn't document them. Additionally, theshape_collision_groupdescription (line 345) should note it's now optional and explain the either-or relationship with the new parameters (either provideshape_collision_group, or provide bothshape_collision_typeandshape_collision_affinity).📝 Add missing parameter documentation
Update the Args section to document all three collision-filtering parameters and their relationship:
Args: shape_lower: Array of lower bounds for each shape's AABB shape_upper: Array of upper bounds for each shape's AABB shape_gap: Optional array of per-shape effective gaps. If None or empty array, assumes AABBs are pre-expanded (gaps = 0). If provided, gaps are added during overlap checks. - shape_collision_group: Array of collision group IDs for each shape. + shape_collision_group: Optional array of collision group IDs for each shape. + If None, ``shape_collision_type`` and ``shape_collision_affinity`` must be provided. + When provided, used to derive collision masks if ``shape_collision_type`` + and ``shape_collision_affinity`` are not provided. shape_world: Array of world indices for each shape. Index -1 indicates global entities that collide with all worlds. Indices 0, 1, 2, ... indicate world-specific entities. shape_count: Number of active bounding boxes to check candidate_pair: Output array to store overlapping shape pairs candidate_pair_count: Output array to store number of overlapping pairs found device: Device to launch on. If None, uses the device of the input arrays. + filter_pairs: Optional array of shape-pair exclusions (sorted). + num_filter_pairs: Number of pairs in filter_pairs. skip_count_zero: If True, skip the internal ``candidate_pair_count.zero_()``. The caller guarantees ``candidate_pair_count[0] == 0`` on entry (e.g. when the counter was zeroed by a preceding fused kernel). Defaults to False so the launch remains self-contained. + shape_collision_type: Optional per-shape collision type bitmask (uint32). + If None, derived from ``shape_collision_group``. + shape_collision_affinity: Optional per-shape collision affinity bitmask (uint32). + If None, derived from ``shape_collision_group``.As per coding guidelines, follow Google-style docstrings with all parameters documented in the Args section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/broad_phase_nxn.py` around lines 316 - 360, The docstring for launch() is missing entries for the new parameters shape_collision_type and shape_collision_affinity and must be updated; add Google-style Args descriptions for both parameters and update the existing shape_collision_group description to mark it optional and explain the either-or relationship (caller must provide shape_collision_group OR provide both shape_collision_type and shape_collision_affinity), plus state how they affect filtering (e.g., type and affinity are bitmask-based filters applied per-shape), and mention defaults/behavior when any of these are None. Locate these symbols in the launch() signature (shape_collision_group, shape_collision_type, shape_collision_affinity) and edit the Args section accordingly.Source: Coding guidelines
newton/_src/geometry/broad_phase_sap.py (1)
521-565: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winIncomplete docstring for new parameters.
The
launch()method addedshape_collision_typeandshape_collision_affinityoptional parameters (lines 536-537), but they're not documented in theArgs:section. Additionally,shape_collision_group(line 550) should note it's now optional and explain the either-or contract with the new type/affinity parameters.📝 Add missing parameter documentation
Update the Args section:
Args: shape_lower: Array of lower bounds for each shape's AABB shape_upper: Array of upper bounds for each shape's AABB shape_gap: Optional array of per-shape effective gaps. If None or empty array, assumes AABBs are pre-expanded (gaps = 0). If provided, gaps are added during overlap checks. - shape_collision_group: Array of collision group IDs for each shape. + shape_collision_group: Optional array of collision group IDs for each shape. + If None, ``shape_collision_type`` and ``shape_collision_affinity`` must be provided. + When provided, used to derive collision masks if the type/affinity parameters are not provided. shape_world: Array of world indices for each shape. Index -1 indicates global entities that collide with all worlds. Indices 0, 1, 2, ... indicate world-specific entities. shape_count: Number of active bounding boxes to check (not used in world-based approach) candidate_pair: Output array to store overlapping shape pairs candidate_pair_count: Output array to store number of overlapping pairs found device: Device to launch on. If None, uses the device of the input arrays. + filter_pairs: Optional array of shape-pair exclusions (sorted). + num_filter_pairs: Number of pairs in filter_pairs. skip_count_zero: If True, skip the internal ``candidate_pair_count.zero_()``. The caller guarantees ``candidate_pair_count[0] == 0`` on entry (e.g. when the counter was zeroed by a preceding fused kernel). Defaults to False so the launch remains self-contained. + shape_collision_type: Optional per-shape collision type bitmask (uint32). + If None, derived from ``shape_collision_group``. + shape_collision_affinity: Optional per-shape collision affinity bitmask (uint32). + If None, derived from ``shape_collision_group``.As per coding guidelines, follow Google-style docstrings with all parameters documented in Args.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/broad_phase_sap.py` around lines 521 - 565, The docstring for launch() is missing entries for the new optional parameters shape_collision_type and shape_collision_affinity and should also update the shape_collision_group doc to reflect that it's now optional and mutually alternative to the type/affinity pair; edit the Args block inside the launch method docstring in broad_phase_sap.py to add Google-style descriptions for shape_collision_type (per-shape collision type mask), shape_collision_affinity (per-shape affinity mask), and clarify shape_collision_group is optional and that either shape_collision_group is used or the pair of shape_collision_type/shape_collision_affinity are used (explain the either-or contract and default/None behavior).Source: Coding guidelines
🧹 Nitpick comments (1)
newton/_src/geometry/edge_redundancy.py (1)
532-533: 💤 Low valueInconsistent dtype specification for warp arrays.
Line 532 uses
dtype=wp.uint32while line 533 usesdtype=np.uint32when constructing warp arrays. For consistency, both should usedtype=wp.uint32.🔧 Align dtype specification
shape_world_wp = wp.array(shape_world_np, dtype=wp.int32) shape_collision_type_wp = wp.array(shape_collision_type_np, dtype=wp.uint32) - shape_collision_affinity_wp = wp.array(shape_collision_affinity_np, dtype=np.uint32) + shape_collision_affinity_wp = wp.array(shape_collision_affinity_np, dtype=wp.uint32) shape_flags_wp = wp.array(shape_flags_np, dtype=wp.int32)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/edge_redundancy.py` around lines 532 - 533, The two warp arrays created for shape collision metadata use inconsistent dtype specifiers: change the dtype on shape_collision_affinity_wp to use wp.uint32 to match shape_collision_type_wp (i.e., ensure both wp.array calls use dtype=wp.uint32); update the wp.array invocation that constructs shape_collision_affinity_wp so it references wp.uint32 rather than np.uint32, leaving variable names shape_collision_type_wp and shape_collision_affinity_wp unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/geometry/broad_phase_common.py`:
- Around line 183-193: Update the docstring for the function
test_type_affinity_pair to a full Google-style docstring: expand the one-line
summary to describe MuJoCo-style collision filtering (two 32-bit bitmasks per
object: "collision_type" is the object's type bits and "collision_affinity" is
which types it can collide with), add an Args: section listing collision_type_a:
wp.uint32, collision_affinity_a: wp.uint32, collision_type_b: wp.uint32,
collision_affinity_b: wp.uint32 with brief descriptions, and add a Returns:
section describing that the function returns a bool which is True when
(collision_type_a & collision_affinity_b) != 0 or (collision_type_b &
collision_affinity_a) != 0 (i.e., either object A accepts B's type or B accepts
A's type), keeping types consistent with annotations and using plain language to
explain the bitwise logic; place this updated docstring immediately above the
def test_type_affinity_pair declaration.
- Around line 332-380: Update the docstring of
compile_group_masks_for_broad_phase to a Google-style docstring: add an Args:
section describing collision_group (wp.array | np.ndarray | list[int]) and
device (Any | None) and their semantics, a Returns: section describing the tuple
of wp.array collision_type and wp.array collision_affinity (both uint32 masks on
the specified device), and a Raises: section documenting the ValueError when
more than 32 non-zero groups are provided; keep the short summary line and
ensure types and behavior (zero group skipped, positive/negative group handling)
are noted concisely.
- Around line 196-209: Add a Google-style docstring to
test_world_and_type_affinity_pair that includes an Args section listing each
parameter with type and brief description (world_a: int — world index for entity
A, use -1 to ignore/world wildcard; world_b: int — world index for entity B, use
-1 to ignore; collision_type_a: wp.uint32 — collision type mask for entity A;
collision_affinity_a: wp.uint32 — collision affinity mask for entity A;
collision_type_b: wp.uint32 — collision type mask for entity B;
collision_affinity_b: wp.uint32 — collision affinity mask for entity B) and a
Returns section describing the boolean result (bool — True if the two entities
should collide based on world indices and type/affinity masks). Keep the
existing one-line summary and add the Args: and Returns: blocks in Google
docstring format above the function body; no behavior changes to
test_type_affinity_pair or logic are needed.
In `@newton/_src/sim/builder.py`:
- Around line 1464-1470: The code currently counts all values in
self.shape_collision_group when computing unique_groups, which erroneously
includes shapes that already have explicit collision_type or collision_affinity
and therefore shouldn't consume mask bits; update the computation to exclude
collision_group entries for shapes with authored collision_type or
collision_affinity. Concretely, replace the list comprehension that builds
unique_groups with one that filters by index (e.g.
enumerate(self.shape_collision_group)) and only includes group values when the
corresponding shape does NOT have an explicit collision_type or
collision_affinity (check the parallel arrays/attributes used to determine
authored types), then perform the sorted(set(...)) and the same >32 check on
that filtered set. Ensure you reference and use the existing attributes
(self.shape_collision_group and whatever arrays/flags indicate authored
collision_type/collision_affinity) so mixed models don't count explicit-mask
shapes toward the 32-bit limit.
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 4353-4360: The current graph_edges and recoloring logic converts
mask relations into a color-based compatibility, which is incorrect; instead
export the original masks into geom_params by assigning shape_collision_type and
shape_collision_affinity directly to geom_params["contype"] and
geom_params["conaffinity"] (use the existing variables shape_collision_type /
shape_collision_affinity), and change uses_mujoco_contacts to be computed from
the exact pairwise predicate ((type_a & affinity_b) != 0 or (type_b &
affinity_a) != 0) plus an OR branch that keeps explicit pair entries from
model.shape_collision_filter_pairs; leave the explicit-<pair> override handling
intact so shapes referenced by explicit pairs are included even if the automatic
mask predicate is false; update the three other locations you noted (around
lines ~5171-5175 and ~5204-5217) to the same pattern.
---
Outside diff comments:
In `@newton/_src/geometry/broad_phase_nxn.py`:
- Around line 316-360: The docstring for launch() is missing entries for the new
parameters shape_collision_type and shape_collision_affinity and must be
updated; add Google-style Args descriptions for both parameters and update the
existing shape_collision_group description to mark it optional and explain the
either-or relationship (caller must provide shape_collision_group OR provide
both shape_collision_type and shape_collision_affinity), plus state how they
affect filtering (e.g., type and affinity are bitmask-based filters applied
per-shape), and mention defaults/behavior when any of these are None. Locate
these symbols in the launch() signature (shape_collision_group,
shape_collision_type, shape_collision_affinity) and edit the Args section
accordingly.
In `@newton/_src/geometry/broad_phase_sap.py`:
- Around line 521-565: The docstring for launch() is missing entries for the new
optional parameters shape_collision_type and shape_collision_affinity and should
also update the shape_collision_group doc to reflect that it's now optional and
mutually alternative to the type/affinity pair; edit the Args block inside the
launch method docstring in broad_phase_sap.py to add Google-style descriptions
for shape_collision_type (per-shape collision type mask),
shape_collision_affinity (per-shape affinity mask), and clarify
shape_collision_group is optional and that either shape_collision_group is used
or the pair of shape_collision_type/shape_collision_affinity are used (explain
the either-or contract and default/None behavior).
In `@newton/_src/sim/builder.py`:
- Around line 467-474: When clearing the SITE flag in the flags-assignment
branch, do not unconditionally reset authored collision masks; instead only
restore non-site defaults that are not mask-related. Remove or stop setting
self.collision_type and self.collision_affinity in that else-branch where
defaults = self.__class__() and self.is_site is set to False, keeping
restoration of density and collision_group as needed so that authored masks on
collision_type/collision_affinity are preserved and remeshing/decomposition will
round-trip correctly.
---
Nitpick comments:
In `@newton/_src/geometry/edge_redundancy.py`:
- Around line 532-533: The two warp arrays created for shape collision metadata
use inconsistent dtype specifiers: change the dtype on
shape_collision_affinity_wp to use wp.uint32 to match shape_collision_type_wp
(i.e., ensure both wp.array calls use dtype=wp.uint32); update the wp.array
invocation that constructs shape_collision_affinity_wp so it references
wp.uint32 rather than np.uint32, leaving variable names shape_collision_type_wp
and shape_collision_affinity_wp unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 91c5e2f8-18d9-4a4e-b5b3-1ca717f9702b
📒 Files selected for processing (13)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/broad_phase_common.pynewton/_src/geometry/broad_phase_nxn.pynewton/_src/geometry/broad_phase_sap.pynewton/_src/geometry/edge_redundancy.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_broad_phase.pynewton/tests/test_import_mjcf.py
m-abr
force-pushed
the
m-abr/fix-mjcf-contype-conaffinity
branch
from
June 8, 2026 16:26
9ae35b1 to
5d3f72a
Compare
m-abr
had a problem deploying
to
external-pr-approval
June 8, 2026 16:27 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 8, 2026 16:27 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
4348-4360:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExport the exact type/affinity masks instead of recoloring them.
This still changes the collision relation before it reaches MuJoCo: Lines 4348-4360 build a compatibility graph from the masks, but Lines 5204-5217 collapse that graph into “same color never collides, different colors always collide.” That is not equivalent to MuJoCo’s exact rule
(type_a & affinity_b) != 0 or (type_b & affinity_a) != 0, so the exported model can admit spurious contacts or miss valid ones. The same approximation leaks into Lines 5171-5175, where any non-zero mask marks a mesh as using MuJoCo automatic contacts even if it has no compatible partner, which can incorrectly reject planar meshes.Please write
shape_collision_type/shape_collision_affinitydirectly intogeom_params["contype"]/geom_params["conaffinity"]instead of deriving colors, and computeuses_mujoco_contactsfrom the exact automatic-contact predicate plus the existing explicit-<pair>override. If you need to preserve the full 32-bit mask through MuJoCo’s signed storage, cast withnp.int32(...).item()rather than recoloring. Based on learnings, shapes referenced by explicit MuJoCo<pair>entries must remain included even when the automatic-contact branch switches to the exact mask predicate.Also applies to: 5171-5175, 5204-5217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 4348 - 4360, The current code recolors collision masks and derives a coarse color-based collision rule which changes MuJoCo semantics; instead, write the exact masks from shape_collision_type and shape_collision_affinity into geom_params["contype"] and geom_params["conaffinity"] (use np.int32(mask).item() to preserve full 32-bit signed storage), compute uses_mujoco_contacts using the exact predicate ((type_a & affinity_b) != 0 or (type_b & affinity_a) != 0) combined with the explicit override from model.shape_collision_filter_pairs, and ensure any shape referenced by explicit <pair> entries (look up via shape_a/shape_b and selected_shapes) remains included even if the automatic-contact predicate would exclude it. This change touches the logic that builds ctype/caffinity and the branches that set uses_mujoco_contacts and the geom_params entries.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/sim/builder.py`:
- Around line 265-269: Update the docstring for the collision_group attribute to
clarify that setting collision_group=0 only disables collisions that would be
derived from the group and does not override explicitly authored masks; mention
that during finalization the function _compile_shape_collision_masks() resolves
collision_group into collision_type and collision_affinity but will ignore
collision_group when explicit masks are present, so mixed configurations retain
explicit-mask behavior.
---
Duplicate comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 4348-4360: The current code recolors collision masks and derives a
coarse color-based collision rule which changes MuJoCo semantics; instead, write
the exact masks from shape_collision_type and shape_collision_affinity into
geom_params["contype"] and geom_params["conaffinity"] (use np.int32(mask).item()
to preserve full 32-bit signed storage), compute uses_mujoco_contacts using the
exact predicate ((type_a & affinity_b) != 0 or (type_b & affinity_a) != 0)
combined with the explicit override from model.shape_collision_filter_pairs, and
ensure any shape referenced by explicit <pair> entries (look up via
shape_a/shape_b and selected_shapes) remains included even if the
automatic-contact predicate would exclude it. This change touches the logic that
builds ctype/caffinity and the branches that set uses_mujoco_contacts and the
geom_params entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 05366e01-c417-42ae-866e-4d6cdf90b8ab
📒 Files selected for processing (13)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/broad_phase_common.pynewton/_src/geometry/broad_phase_nxn.pynewton/_src/geometry/broad_phase_sap.pynewton/_src/geometry/edge_redundancy.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_broad_phase.pynewton/tests/test_import_mjcf.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (10)
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/model.py
- docs/concepts/collisions.rst
- newton/tests/test_broad_phase.py
- newton/_src/geometry/edge_redundancy.py
- newton/_src/geometry/broad_phase_common.py
- newton/tests/test_import_mjcf.py
- newton/_src/geometry/broad_phase_sap.py
- newton/_src/geometry/broad_phase_nxn.py
- newton/_src/sim/collide.py
Member
|
Heads up for after the 1.3 changelog split: this PR currently adds or updates a |
m-abr
force-pushed
the
m-abr/fix-mjcf-contype-conaffinity
branch
from
June 14, 2026 18:40
5d3f72a to
1c4d0b3
Compare
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:40 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:40 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
force-pushed
the
m-abr/fix-mjcf-contype-conaffinity
branch
from
June 14, 2026 18:44
1c4d0b3 to
98dcb88
Compare
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:44 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:44 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
4925-4937: ⚡ Quick winSkip the quadratic auto-contact scan when MuJoCo contacts are off.
mujoco_auto_contact_shapesis only consumed by the planar-mesh rejection at Lines 5131-5134. Whenuse_mujoco_contacts=Falseordisable_contacts=True, thisO(n^2)loop does no useful work and can noticeably slow solver construction for large shape counts.♻️ Suggested change
mujoco_auto_contact_shapes: set[int] = set() - for i, shape_a in enumerate(colliding_shapes): - shape_a = int(shape_a) - for shape_b in colliding_shapes[i + 1 :]: - shape_b = int(shape_b) - if (min(shape_a, shape_b), max(shape_a, shape_b)) in shape_collision_filter_pairs: - continue - if not _shape_worlds_compatible(shape_a, shape_b): - continue - if _shape_masks_compatible(shape_a, shape_b): - mujoco_auto_contact_shapes.add(shape_a) - mujoco_auto_contact_shapes.add(shape_b) + if self._use_mujoco_contacts and not disable_contacts: + for i, shape_a in enumerate(colliding_shapes): + shape_a = int(shape_a) + for shape_b in colliding_shapes[i + 1 :]: + shape_b = int(shape_b) + if (min(shape_a, shape_b), max(shape_a, shape_b)) in shape_collision_filter_pairs: + continue + if not _shape_worlds_compatible(shape_a, shape_b): + continue + if _shape_masks_compatible(shape_a, shape_b): + mujoco_auto_contact_shapes.add(shape_a) + mujoco_auto_contact_shapes.add(shape_b)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/solvers/mujoco/solver_mujoco.py` around lines 4925 - 4937, The quadratic loop that populates mujoco_auto_contact_shapes starting at the enumeration of colliding_shapes performs unnecessary work when MuJoCo contacts are disabled. Add a guard condition before this loop that checks whether contacts are actually enabled (checking use_mujoco_contacts and disable_contacts flags), and skip the entire loop when contacts are off, since mujoco_auto_contact_shapes is only consumed by the planar-mesh rejection logic that won't be reached when contacts are disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/geometry/broad_phase_sap.py`:
- Around line 591-598: Change the condition in the block at shape_collision_type
and shape_collision_affinity validation from using OR to AND, so that
shape_collision_group is only used to compile masks when both
shape_collision_type and shape_collision_affinity are omitted together.
Additionally, add validation to reject cases where exactly one of the two masks
is provided (partial masks), raising a ValueError if either one is None while
the other is not. This ensures the mask arrays must be provided together and
unambiguously, with shape_collision_group only as a fallback when both are
omitted.
---
Nitpick comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 4925-4937: The quadratic loop that populates
mujoco_auto_contact_shapes starting at the enumeration of colliding_shapes
performs unnecessary work when MuJoCo contacts are disabled. Add a guard
condition before this loop that checks whether contacts are actually enabled
(checking use_mujoco_contacts and disable_contacts flags), and skip the entire
loop when contacts are off, since mujoco_auto_contact_shapes is only consumed by
the planar-mesh rejection logic that won't be reached when contacts are
disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3115d25e-9588-4648-b798-e17e6c911cca
📒 Files selected for processing (14)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/broad_phase_common.pynewton/_src/geometry/broad_phase_nxn.pynewton/_src/geometry/broad_phase_sap.pynewton/_src/geometry/edge_redundancy.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_broad_phase.pynewton/tests/test_import_mjcf.pynewton/tests/test_model.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (9)
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/collide.py
- newton/_src/sim/model.py
- newton/_src/geometry/edge_redundancy.py
- newton/_src/geometry/broad_phase_nxn.py
- newton/tests/test_broad_phase.py
- docs/concepts/collisions.rst
- newton/_src/geometry/broad_phase_common.py
- newton/_src/sim/builder.py
m-abr
force-pushed
the
m-abr/fix-mjcf-contype-conaffinity
branch
from
June 14, 2026 18:51
98dcb88 to
5d18cf2
Compare
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:51 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 18:51 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/geometry/broad_phase_nxn.py (1)
229-242: 💤 Low valueClass docstring references outdated filtering mechanism.
The docstring mentions "collision groups" and
test_group_pair(), but the implementation now uses type/affinity masks andtest_world_and_type_affinity_pair. Consider updating to reflect the current mask-based filtering semantics.📝 Suggested docstring update
"""A broad phase collision detection class that performs N x N collision checks between all geometry pairs. This class performs collision detection between all possible pairs of geometries by checking for axis-aligned bounding box (AABB) overlaps. It uses a lower triangular matrix approach to avoid checking each pair twice. - The collision checks take into account per-geometry cutoff distances and collision groups. Two geometries + The collision checks take into account per-geometry cutoff distances and collision masks. 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()) + 2. Their collision type/affinity masks allow interaction The class outputs an array of candidate collision pairs that need more detailed narrow phase collision checking. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/broad_phase_nxn.py` around lines 229 - 242, The docstring for the BroadPhaseNxN class contains outdated references to its filtering mechanism. Update the docstring to replace mentions of "collision groups" and the `test_group_pair()` method with the current implementation details that use type/affinity masks and the `test_world_and_type_affinity_pair` method. Ensure the documentation accurately reflects how geometry pairs are filtered based on mask-based criteria rather than collision group interactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/geometry/broad_phase_nxn.py`:
- Around line 229-242: The docstring for the BroadPhaseNxN class contains
outdated references to its filtering mechanism. Update the docstring to replace
mentions of "collision groups" and the `test_group_pair()` method with the
current implementation details that use type/affinity masks and the
`test_world_and_type_affinity_pair` method. Ensure the documentation accurately
reflects how geometry pairs are filtered based on mask-based criteria rather
than collision group interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 341a6cdc-6de8-4f98-9689-49453e4e9a60
📒 Files selected for processing (14)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/broad_phase_common.pynewton/_src/geometry/broad_phase_nxn.pynewton/_src/geometry/broad_phase_sap.pynewton/_src/geometry/edge_redundancy.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_broad_phase.pynewton/tests/test_import_mjcf.pynewton/tests/test_model.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (11)
- newton/_src/geometry/edge_redundancy.py
- docs/concepts/collisions.rst
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/collide.py
- newton/tests/test_model.py
- newton/_src/geometry/broad_phase_common.py
- newton/tests/test_broad_phase.py
- newton/_src/geometry/broad_phase_sap.py
- newton/tests/test_import_mjcf.py
- newton/_src/sim/builder.py
- newton/_src/solvers/mujoco/solver_mujoco.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/collisions.rst`:
- Around line 1081-1086: The documentation for `collision_group` currently
states that "0 disables collisions" without clarifying that this is only true
when the `collision_type` and `collision_affinity` masks are unset. Since these
masks can override group-derived filtering, the statement is misleading. Modify
the description of the `collision_group` parameter to scope the "0 disables
collisions" statement to only apply when both masks are unset (for example, by
changing it to "0 disables collisions when masks are unset" or similar
clarifying language that prevents users from misinterpreting this as a hard
guarantee when explicit masks are configured).
- Around line 431-445: The collision-type-affinity example in the testcode block
only builds the model but does not verify that the asymmetric filtering behavior
works correctly. After the `builder.finalize()` call, add a `collide()` call to
retrieve collision pairs and include assertions to verify that floor↔ball
collision pairs are present while ball↔ball collision pairs are absent. This
ensures the mask logic validation is part of the example and will catch any
regressions in the filtering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fb025a06-8666-47e8-88dc-f775546731ac
📒 Files selected for processing (14)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/broad_phase_common.pynewton/_src/geometry/broad_phase_nxn.pynewton/_src/geometry/broad_phase_sap.pynewton/_src/geometry/edge_redundancy.pynewton/_src/sim/builder.pynewton/_src/sim/collide.pynewton/_src/sim/model.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_broad_phase.pynewton/tests/test_import_mjcf.pynewton/tests/test_model.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (12)
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/collide.py
- newton/_src/geometry/broad_phase_common.py
- newton/tests/test_model.py
- newton/_src/geometry/broad_phase_nxn.py
- newton/tests/test_broad_phase.py
- newton/_src/geometry/edge_redundancy.py
- newton/tests/test_import_mjcf.py
- newton/_src/sim/model.py
- newton/_src/geometry/broad_phase_sap.py
- newton/_src/sim/builder.py
- newton/_src/solvers/mujoco/solver_mujoco.py
Comment on lines
+431
to
+445
| .. testcode:: collision-type-affinity | ||
|
|
||
| builder = newton.ModelBuilder() | ||
| floor_cfg = builder.ShapeConfig(collision_type=0, collision_affinity=1) | ||
| ball_cfg = builder.ShapeConfig(collision_type=1, collision_affinity=0) | ||
|
|
||
| floor = builder.add_shape_box(body=-1, hx=5.0, hy=5.0, hz=0.01, cfg=floor_cfg) | ||
| body_a = builder.add_body() | ||
| body_b = builder.add_body() | ||
| sphere_a = builder.add_shape_sphere(body_a, radius=0.1, cfg=ball_cfg) | ||
| sphere_b = builder.add_shape_sphere(body_b, radius=0.1, cfg=ball_cfg) | ||
|
|
||
| # The spheres collide with the floor, but not with each other. | ||
| model = builder.finalize() | ||
|
|
Contributor
There was a problem hiding this comment.
Make the example assert the asymmetric filtering behavior.
Right now the doctest only builds the scene; it never checks that floor↔ball pairs are present and ball↔ball pairs are absent. Add a tiny collide + assertion block so this example fails if the mask logic regresses.
✅ Possible check
sphere_a = builder.add_shape_sphere(body_a, radius=0.1, cfg=ball_cfg)
sphere_b = builder.add_shape_sphere(body_b, radius=0.1, cfg=ball_cfg)
- # The spheres collide with the floor, but not with each other.
model = builder.finalize()
+ state = model.state()
+ contacts = model.contacts()
+ model.collide(state, contacts)
+
+ pairs = {
+ tuple(sorted((int(a), int(b))))
+ for a, b in zip(
+ contacts.rigid_contact_shape0.numpy()[:contacts.rigid_contact_count.numpy()[0]],
+ contacts.rigid_contact_shape1.numpy()[:contacts.rigid_contact_count.numpy()[0]],
+ )
+ }
+ assert (floor, sphere_a) in pairs
+ assert (floor, sphere_b) in pairs
+ assert (sphere_a, sphere_b) not in pairs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/concepts/collisions.rst` around lines 431 - 445, The
collision-type-affinity example in the testcode block only builds the model but
does not verify that the asymmetric filtering behavior works correctly. After
the `builder.finalize()` call, add a `collide()` call to retrieve collision
pairs and include assertions to verify that floor↔ball collision pairs are
present while ball↔ball collision pairs are absent. This ensures the mask logic
validation is part of the example and will catch any regressions in the
filtering behavior.
m-abr
force-pushed
the
m-abr/fix-mjcf-contype-conaffinity
branch
from
June 14, 2026 19:29
5d18cf2 to
2c7115e
Compare
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 19:29 — with
GitHub Actions
Error
GitHub Actions
Error
m-abr
had a problem deploying
to
external-pr-approval
June 14, 2026 19:29 — with
GitHub Actions
Failure
GitHub Actions
Failure
Author
|
Thank @jcarius-nv. I think all the suggestions are now addressed |
Member
|
hi @m-abr - sorry for the late response. How critical is this feature for you? we are still undecided on our side if we want to adopt a full mujoco-style collision filtering in the newton core API and what the implications are. |
Author
|
Hi @adenzler-nvidia, it's not super critical for me, and I am also not fully happy with my PR as-is, because I would prefer Newton to have one coherent collision filtering method rather than a mix of methods. Also, I think Newton's collision groups plus excluded pairs are expressive enough for MJCF collisions. Unless I'm missing something, contype/conaffinity can be converted by assigning unique negative groups and excluding every pair that the Mujoco mask rule would reject. This would not be as efficient as the approach in this PR, but it would be cleaner, and MJCF collisions would at least be respected if you prefer to keep the current mechanism. Regarding efficiency, I think masks might sometimes be faster since they can avoid some excluded-pair lookups, but they are also limited by the fixed bit width, which is usually not a problem anyway. My impression is that the two approaches are probably in a similar range in terms of performance, and I am also not sure what the best long-term direction is. |
Contributor
|
Hi @adenzler-nvidia and @m-abr, I wanted to share some context from recent MJCF integration work where this limitation has a practical impact. |
This was referenced Jul 28, 2026
Member
|
Hi @han-xudong and @m-abr, I'm prototyping a solution to come up with a conversion from contype/conaffinity to Newton's collision groups and pairwise filters here: #3714. |
Contributor
|
Hi @eric-heiden, thank you for putting this together. I tested #3714 with our MJCF use case. The imported collision relationships match the original contype/conaffinity configuration, and SolverMuJoCo(..., use_mujoco_contacts=False) runs correctly with the expected contact exclusions. This approach covers our use case well. |
Member
|
Closing in favor of #3714. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3014.
This PR implements MuJoCo-style
contype/conaffinitycollision filtering in Newton contact generation.The change adds
collision_typeandcollision_affinitymasks to shape configuration/model data, maps MJCFcontypeandconaffinitydirectly to those masks, and resolves existingcollision_groupfiltering into the same runtime mask representation during model finalization.The broad phase and explicit contact-pair generation now use the same mask rule:
This fixes the reported case where two spheres with
contype=1,conaffinity=0should each collide with a floor usingcontype=0,conaffinity=1, but should not collide with each other.A full pair matrix would be very expressive, but it would scale poorly in memory and is not ideal for broad-phase kernels. This PR keeps the existing collision mechanisms rather than replacing them:
collision_groupremains the simple group-level API. It is useful for common cases and existing models, and is resolved into masks at finalization, so it does not add per-step cost.collision_type/collision_affinityprovide the more expressive category-level filtering needed for MJCF-style masks.shape_collision_filter_pairsremains the sparse exact-pair exclusion layer for same-body, parent-child, USDfilteredPairs, disabled collision shapes, articulation self-collision filtering, and other one-off pair exclusions.The runtime path therefore stays uniform and fast: broad phase and solver code use resolved type/affinity masks, while pairwise filters handle sparse exceptions.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
Minimal reproduction:
New feature / API change
This PR adds mask-based collision filtering through
ModelBuilder.ShapeConfig:collision_groupremains available for group-level filtering. During finalization, Newton resolves either group-derived or explicit type/affinity masks intoModel.shape_collision_typeandModel.shape_collision_affinity, which are used by the runtime collision pipeline.Summary by CodeRabbit
collision_type/collision_affinitymasks for model building and runtime collision filtering, used by both NXN and SAP broad-phase/contact pair generation (with automatic derivation fromcollision_groupwhen masks are not authored).contype/conaffinitymask collision filtering and improved contact-generation stability across multiple solver and example scenarios.