Preserve MJCF collision filtering semantics - #3714
Conversation
Compile MuJoCo collision masks into Newton groups and sparse exclusions while retaining the authored masks for exact MuJoCo round trips. Compile native Newton filtering back into verified 32-bit MuJoCo masks and warn before the legacy fallback when the graph exceeds capacity.
|
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:
📝 WalkthroughWalkthroughMJCF collision masks are preserved during import, compiled into Newton groups and exclusions, and forwarded or converted for MuJoCo contacts. Newton collision graphs now support exact mask compilation with bounded fallbacks. Replicated contact-pair template reuse also gains stricter validation. ChangesCollision filtering integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MJCF
participant ModelBuilder
participant SolverMuJoCo
participant MuJoCo
MJCF->>ModelBuilder: contype/conaffinity values
ModelBuilder->>ModelBuilder: compile groups and exclusions
ModelBuilder->>SolverMuJoCo: preserved masks and Newton filters
SolverMuJoCo->>MuJoCo: geom contype/conaffinity
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 `@newton/_src/solvers/mujoco/collision_masks.py`:
- Around line 751-790: Guard the greedy biclique-cover path around the candidate
construction and selection logic in the collision-mask compiler when shape_count
exceeds a practical threshold, avoiding dense star-candidate generation and
repeated rescans for large scenes. Preserve the existing exact-cover behavior
for smaller graphs and ensure the caller uses the established legacy fallback
when the threshold is exceeded.
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Around line 5378-5408: Update the preserved-mask decision in the solver flow
around use_preserved_collision_masks so it also verifies every
shape_collision_filter_pairs entry involving colliding_shapes is already blocked
by the imported contype/conaffinity masks or covered by an installed body-level
exclusion. If any remaining shape-level pair is not represented by those masks,
set use_preserved_collision_masks false so execution falls through to
_compile_newton_collision_masks; retain the existing preserved path only when
all filtered pairs are safely enforced.
🪄 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 Plus
Run ID: 917ddb2f-29e4-45ab-a964-b5589d36d072
📒 Files selected for processing (11)
CHANGELOG.mddocs/solvers/mujoco.rstnewton/_src/sim/builder.pynewton/_src/solvers/mujoco/collision_masks.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_builder_replicate.pynewton/tests/test_collision_mask_compiler.pynewton/tests/test_import_mjcf.pynewton/tests/test_mujoco_solver.pyscripts/benchmark_collision_mask_compiler.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Honor Newton pair filters that are not already represented by preserved MJCF masks or body exclusions. Bound reverse-cover work for large and heavily filtered graphs so established graph coloring remains the practical fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/solvers/mujoco/collision_masks.py`:
- Around line 669-674: Update the validation for max_bits, max_shape_count, and
max_excluded_pair_count to reject NaN and any non-integral values before
applying their existing range checks. Preserve the current allowed bounds and
None handling, and ensure all three capacity parameters accept only integral
values.
🪄 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 Plus
Run ID: 3b462aaf-cf0c-4a8d-8faa-b15a977cbddd
📒 Files selected for processing (6)
CHANGELOG.mddocs/solvers/mujoco.rstnewton/_src/solvers/mujoco/collision_masks.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_collision_mask_compiler.pynewton/tests/test_import_mjcf.py
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- newton/tests/test_collision_mask_compiler.py
- docs/solvers/mujoco.rst
- newton/tests/test_import_mjcf.py
- newton/_src/solvers/mujoco/solver_mujoco.py
Use the established graph-color fallback without a new advisory when an exact MuJoCo mask cover is unavailable. Strict-warning test runs include supported models that legitimately take this path.
Declare preserved masks alongside the other MuJoCo geom attributes and use the unified registration path for direct MJCF parsing. Drop the exploratory collision-mask benchmark script from the PR.
vreutskyy
left a comment
There was a problem hiding this comment.
A couple of questions from my review:
Track independent MJCF imports as separate mask domains so SolverMuJoCo reverse-compiles their combined collision graph. Map sparse filter pairs directly and skip oversized graphs before allocating quadratic candidate arrays.
vreutskyy
left a comment
There was a problem hiding this comment.
One finding from the follow-up review:
vreutskyy
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for addressing the review findings.
|
Just fixed a performance regression in the ModelBuilder, should be good to go now. |
vreutskyy
left a comment
There was a problem hiding this comment.
Looks good to me. Re-reviewed the mask-domain remapping optimization and latest main merge.
Description
Closes #3014. Tracks the automatic-mask and round-trip work in #3713.
MuJoCo accepts an automatically generated geom pair when:
Newton's signed collision groups cannot express every such asymmetric relation directly. This change preserves the exact pair relation without adding public
collision_type/collision_affinityarrays or changing broad-phase kernels:model.mujoco.contype/model.mujoco.conaffinitycustom attributes, including inherited defaults.SolverMuJoCo(use_mujoco_contacts=True)forwards preserved source masks verbatim when every selected collision shape has them and all Newton pair filters are already enforced by those masks, same-body topology, or body-wide MuJoCo<exclude>elements. Otherwise it reverse-compiles the combined Newton graph so later partial pair-filter edits remain effective. Preserved masks remain authoritative over later collision-group edits.Each MuJoCo bit is an oriented biclique: geoms carrying the bit in
contypeform one side and geoms carrying it inconaffinityform the other. Minimum biclique cover is NP-hard, so the larger-graph compiler uses safe group-derived bicliques and per-shape stars, accepts only a fully verified result, and treats an uncovered edge as capacity failure rather than silently approximating it.MuJoCo references: collision selection,
contype/conaffinityXML fields.Menagerie analysis
Benchmark corpus:
mujoco_menagerie@71f066a.0/0and1/1; these occur in 11 representative asset directories.Representative complex patterns:
0/0×25,0/15×1,1/0×5,2/4×7,4/2×70/0×146,1/1×1,1/15×1,1/2×490/0×53,1/1×50,6/1×20/0×64,1/1×29,2/2×1Across all 233 compilable XMLs:
For a 1,024-world synthetic replication of SoftFoot's 51 active geom masks (median of 5 CPU runs):
This removes about 9.19 MiB of packed filter-pair storage, improves finalization by 3.1%, and improves combined replication/finalization by 2.2%. Preserved masks add two
int64custom-attribute values, or 16 bytes per imported shape.Related work
<pair>contacts remain separate from automatic mask filtering here.Checklist
CHANGELOG.mdhas been updatedTest plan
Bug fix
Steps to reproduce:
contype=0 conaffinity=1.contype=1 conaffinity=0.Summary by CodeRabbit
New Features
contype/conaffinitycollision filtering during import and MuJoCo contact generation.Bug Fixes
Documentation