Canonical collision filter pairs by camevor · Pull Request #1475 · newton-physics/newton

camevor · 2026-01-28T15:38:23Z

Description

Model.shape_collision_filter_pairs is expected to have shape pairs (s1, s2) in canonical order, i.e. s1 < s2. When created from ModelBuilder.shape_collision_filter_pairs, this is is not being enforced.

This change enforces the pair ordering when creating Model.shape_collision_filter_pairs. It also adds a helper ModelBuilder.add_shape_collision_filter_pair to introduce this invariant on ModelBuilder.shape_collision_filter_pairs.

Resolves #1395.

Newton Migration Guide

Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.

The migration guide in docs/migration.rst is up-to date

Before your PR is "Ready for review"

Necessary tests have been added and new examples are tested (see newton/tests/test_examples.py)
Documentation is up-to-date
Code passes formatting and linting checks with pre-commit run -a

Summary by CodeRabbit

New Features
- Added a public API to register shape collision filter pairs instead of mutating internal lists.
Documentation
- Clarified that stored collision filter pairs use canonical ordering (s1 < s2).
Bug Fixes / Consistency
- Normalized collision-pair ordering across code paths to ensure consistent matching and storage.
Tests
- Added a test ensuring collision filter pairs are normalized to canonical order on finalization.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-28T15:38:43Z

📝 Walkthrough

Walkthrough

Introduces ModelBuilder.add_shape_collision_filter_pair(shape_a, shape_b) and updates callers to use it. All insertion sites now canonicalize pairs (min, max); finalize also normalizes stored pairs. Importers and SolverMuJoCo were updated to rely on canonical ordering. A test was added to verify canonical ordering on finalize.

Changes

Cohort / File(s)	Summary
Builder Public API & Core Logic `newton/_src/sim/builder.py`	Added `add_shape_collision_filter_pair(shape_a, shape_b)` that enforces canonical ordering; refactored `add_axis_dim`, `add_joint`, `add_shape` to use it; `finalize` now normalizes pairs; `find_shape_contact_pairs` compares pairs in canonical order.
Model Documentation `newton/_src/sim/model.py`	Docstring for `Model.shape_collision_filter_pairs` updated to specify pairs are stored as canonical tuples `(s1, s2)` with `s1 < s2`.
Import Utilities `newton/_src/utils/import_mjcf.py`, `newton/_src/utils/import_urdf.py`, `newton/_src/utils/import_usd.py`	Replaced direct `builder.shape_collision_filter_pairs.append(...)` calls with `builder.add_shape_collision_filter_pair(...)` at multiple filter-insertion locations.
Solver `newton/_src/solvers/mujoco/solver_mujoco.py`	`color_collision_shapes` now canonicalizes shape index pairs (min, max) before checking membership in `model.shape_collision_filter_pairs`.
Tests `newton/tests/test_model.py`	Added `test_collision_filter_pairs_canonical_order()` to assert pairs are normalized to canonical order after finalize.
Docs `docs/concepts/collisions.rst`	Example updated to use `builder.add_shape_collision_filter_pair(shape_a, shape_b)` instead of appending to internal list.

Sequence Diagram(s)

sequenceDiagram
    participant Importer as Importer (URDF/MJCF/USD)
    participant Builder as ModelBuilder
    participant Model as Model (finalize)
    participant Solver as SolverMuJoCo

    Importer->>Builder: add_shape_collision_filter_pair(a,b)
    Builder-->>Builder: normalize pair (min,max) and store
    Builder->>Model: finalize()
    Model-->>Model: normalize all stored pairs into set {(s1,s2)}
    Solver->>Model: query shape_collision_filter_pairs
    Solver-->>Solver: compare using canonical pairs (min,max)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Fix default values of world id and group id to get proper collision filtering #990: Modifies ModelBuilder.find_shape_contact_pairs and touches similar filtering logic and pair handling.
Exclude body contacts #799: Alters creation/normalization of shape-collision-pairs in builder-related codepaths.
Fix perf regression due to find_shape_contact_pairs #699: Rewrites parts of find_shape_contact_pairs; overlaps with canonical-ordering changes.

Suggested reviewers

eric-heiden
adenzler-nvidia

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR partially addresses issue `#1395` by fixing a root cause (canonical ordering of collision filter pairs), but does not fully implement all required objectives to resolve the issue.	The PR fixes canonical ordering and reduces spurious collisions, but still requires validation that SensorContact returns identical forces across viewer modes and that contact population is fully consistent without use_mujoco_contacts=True workaround.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Canonical collision filter pairs' directly and clearly describes the main objective of the pull request: enforcing canonical ordering of collision filter pairs.
Out of Scope Changes check	✅ Passed	All code changes are directly scoped to enforcing and validating canonical collision filter pair ordering, which aligns with the stated objective of fixing the root cause for issue `#1395`.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-01-28T15:49:14Z

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
newton/_src/utils/import_usd.py	33.33%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

newton/_src/utils/import_mjcf.py (1)
1571-1573: Missed update: Direct list mutation bypasses canonical ordering.

This loop still appends directly to shape_collision_filter_pairs instead of using the new add_shape_collision_filter_pair helper. This is inconsistent with the PR's goal and may result in non-canonical pairs being stored.
🔧 Proposed fix
             # Add all shape pairs from these bodies to collision filter
             for shape1_idx in body1_shapes:
                 for shape2_idx in body2_shapes:
-                    builder.shape_collision_filter_pairs.append((shape1_idx, shape2_idx))
+                    builder.add_shape_collision_filter_pair(shape1_idx, shape2_idx)
Based on learnings: "narrow-phase kernel _primitive_narrowphase does not need to handle symmetric shape-pair orderings... because kamino.core.builder.ModelBuilder and kamino.geometry.primitive.broadphase guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order."

🧹 Nitpick comments (1)

newton/tests/test_model.py (1)

973-999: Prefer the public helper in tests (or clarify legacy intent).
Using builder.shape_collision_filter_pairs.append(...) risks implying that direct mutation is a supported API. Consider switching to add_shape_collision_filter_pair(...) with reversed inputs, or add a short note that the direct append is intentional to cover legacy/non-canonical inputs.

eric-heiden

Thanks!

camevor added 2 commits January 28, 2026 16:28

Enforce canonical order of model.shape_collision_filter_pairs

bec36a4

Rename & use collision filter pair helper

78e0a5c

camevor requested a review from eric-heiden January 28, 2026 15:38

camevor mentioned this pull request Jan 28, 2026

SensorContact returns incorrect force with --viewer null/usd and use_mujoco_contacts=False #1395

Closed

camevor self-assigned this Jan 28, 2026

Merge branch 'main' into canonical-collision-filter-pairs

34e6421

coderabbitai Bot reviewed Jan 28, 2026

View reviewed changes

camevor added 2 commits January 28, 2026 17:22

Add missed occurrence of add_shape_collision_filter_pair

e9b80af

Ensure check of collision pair is in canonical order

eeafbeb

eric-heiden approved these changes Jan 28, 2026

View reviewed changes

eric-heiden added this pull request to the merge queue Jan 28, 2026

Merged via the queue into newton-physics:main with commit 9a10642 Jan 28, 2026
22 checks passed

This was referenced Feb 6, 2026

Make NxN and SAP broad phase respect filtered pairs #1554

Merged

Fix child shape filtering #1559

Merged

coderabbitai Bot mentioned this pull request Feb 17, 2026

Make ModelBuilder.replicate fast-only and inline benchmark details #1659

Closed

mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026

Canonical collision filter pairs (newton-physics#1475)

f8a81af

camevor deleted the canonical-collision-filter-pairs branch April 21, 2026 13:28

This was referenced Apr 26, 2026

canonicalize collision pairs by GeoType instead of index #2591

Closed

Better handling of collision_filter_parent in ModelBuilder #2664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonical collision filter pairs#1475

Canonical collision filter pairs#1475
eric-heiden merged 5 commits into
newton-physics:mainfrom
camevor:canonical-collision-filter-pairs

camevor commented Jan 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jan 28, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

codecov Bot commented Jan 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

eric-heiden left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

camevor commented Jan 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Newton Migration Guide

Before your PR is "Ready for review"

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

codecov Bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

eric-heiden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

camevor commented Jan 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jan 28, 2026 •

edited

Loading

codecov Bot commented Jan 28, 2026 •

edited

Loading