Small fixes for the CollisionPipeline by nvtw · Pull Request #2632 · newton-physics/newton

nvtw · 2026-04-29T10:14:43Z

Description

Fix non-determinism when sticky mode is used in contact matching and allow to user control more collision buffer sizes (relevant for very big scenes)

Checklist

New or existing tests cover these changes
The documentation is up to date with these changes
CHANGELOG.md has been updated (if user-facing change)

Summary by CodeRabbit

New Features
- Added shape_pairs_max to CollisionPipeline to cap broad‑phase candidate sizing, with validation for invalid values and runtime overflow warnings.
Bug Fixes
- Deterministic sticky contact matching: tie‑break behavior and narrow‑phase buffer sizing corrected to ensure stable, memory‑bounded collision results.
Tests
- Added a long deterministic regression test verifying identical sticky contact matching across parallel pipelines.

…allow to user control more collision buffer sizes (relevant for very big scenes)

coderabbitai · 2026-04-29T10:14:57Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Contact matching tie-breaks now use the low 32 bits of per-contact sort keys instead of thread IDs; CollisionPipeline gained a shape_pairs_max parameter to control broad-phase candidate-pair buffer sizing and align deterministic narrow-phase sort-scratch sizing with rigid-contact capacity.

Changes

Contact matching (deterministic tie-break and resolve)

Layer / File(s)	Summary
Packing / Data shape `newton/_src/geometry/contact_match.py`	Packed claim now encodes `(float_flip(dist_sq) << 32)
Ownership resolution (kernel) `newton/_src/geometry/contact_match.py`	`_match_contacts_kernel` uses `wp.atomic_min` with packed `target_key` (low32 of contact sort key) so equal-distance contenders order by smallest `sort_key_low32`.
Claim resolution (kernel input / logic) `newton/_src/geometry/contact_match.py`	`_resolve_claims_kernel` signature extended with `sort_keys: wp.array[wp.int64]`; winner check compares low32(prev_claim[cand]) against low32(sort_keys[tid]) and non-matching stakers are demoted to `MATCH_BROKEN`.
Wiring / API `newton/_src/geometry/contact_match.py`	`ContactMatcher.match()` forwards `sort_keys` into `_resolve_claims_kernel`.
Tests / Docs `newton/tests/test_collision_pipeline.py`, `CHANGELOG.md`	Added deterministic regression test `test_deterministic_pipeline_sticky_500_steps`; changelog documents tie-break determinism fix.

Collision pipeline & sizing

Layer / File(s)	Summary
Resolver / Validation `newton/_src/sim/collide.py`	Added `_resolve_shape_pairs_max(model, override)` to select candidate-pair capacity: `None` delegates to `_compute_per_world_shape_pairs_max(model)`, `override <= 0` raises `ValueError`, positive values returned as-is.
API surface `newton/_src/sim/collide.py`	`CollisionPipeline.__init__` gains `shape_pairs_max: int
Broad-phase sizing `newton/_src/sim/collide.py`	`"nxn"` and `"sap"` construction paths now use `_resolve_shape_pairs_max(model, shape_pairs_max)` instead of always using the computed per-world bound.
Narrow-phase sizing / wiring `newton/_src/sim/collide.py`	`NarrowPhase(...)` is constructed with `contact_max=rigid_contact_max` and `max_candidate_pairs=self.shape_pairs_max` so deterministic narrow-phase sort-scratch sizing aligns to rigid-contact capacity.
Docs `CHANGELOG.md`	Documented new `shape_pairs_max` override and deterministic narrow-phase sizing correction.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

newton-physics/newton#2384: Related changes to CollisionPipeline shape-pair capacity computation and NXN/SAP allocations.
newton-physics/newton#2409: Related per-contact sort-key infrastructure and deterministic contact sorting used for tie-breaks.
newton-physics/newton#2393: Related adjustments to broad-phase candidate-pair sizing logic in collide.py.

Suggested reviewers

eric-heiden
adenzler-nvidia
preist-nvidia

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Small fixes for the CollisionPipeline' is overly vague and generic. While it relates to changes in the collision pipeline, it does not convey the specific nature of the fixes: determinism improvements in contact matching and user-controllable buffer sizing.	Consider a more specific title such as 'Fix deterministic contact matching and add shape_pairs_max parameter to CollisionPipeline' to clearly communicate the main changes.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

newton/tests/test_collision_pipeline.py (1)
1403-1408: Align test name with actual run length.

At Line 1407, num_frames = 100 conflicts with test_deterministic_pipeline_sticky_500_steps; consider renaming or restoring 500 frames for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@newton/tests/test_collision_pipeline.py` around lines 1403 - 1408, The test
function named test_deterministic_pipeline_sticky_500_steps is setting
num_frames = 100 which makes the name misleading; either rename the test to
reflect 100 frames or restore the loop to 500 frames by changing num_frames to
500 in the same test function (look for the variable num_frames and the function
name test_deterministic_pipeline_sticky_500_steps) so the test name and actual
run length match.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@newton/tests/test_collision_pipeline.py`:
- Line 1363: Replace the private Sphinx target
":mod:`newton._src.geometry.contact_match`" in the docstring with a public API
reference or plain inline code; specifically change it to a public module
cross-reference like ":mod:`newton.geometry.contact_match`" or use inline code
"newton.geometry.contact_match" so the docstring no longer references the
private newton._src path.

---

Nitpick comments:
In `@newton/tests/test_collision_pipeline.py`:
- Around line 1403-1408: The test function named
test_deterministic_pipeline_sticky_500_steps is setting num_frames = 100 which
makes the name misleading; either rename the test to reflect 100 frames or
restore the loop to 500 frames by changing num_frames to 500 in the same test
function (look for the variable num_frames and the function name
test_deterministic_pipeline_sticky_500_steps) so the test name and actual run
length match.

🪄 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: d8b96af9-d840-4328-9faf-f4ebb37349f1

📥 Commits

Reviewing files that changed from the base of the PR and between 9c97b7c and 901a02b.

📒 Files selected for processing (1)

newton/tests/test_collision_pipeline.py

codecov · 2026-04-29T10:39:53Z

Codecov Report

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

Files with missing lines	Patch %	Lines
newton/_src/sim/collide.py	62.50%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

adenzler-nvidia

Thanks for picking this up. The sticky-determinism fix is well-targeted and the regression test (two parallel pipelines, sticky on, identical state) is the right shape. The shape_pairs_max knob addresses a real multi-GB allocation issue on large SAP scenes.

A few questions inline — none blocking.

coderabbitai

♻️ Duplicate comments (1)

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

177-191: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Duplicate low-32 keys can still leave multiple winners on one previous contact.

The updated docstring already notes that the reduced-contact path can produce two contacts in the same shape pair with the same fingerprint-derived sort_sub_key. In that case, pass 2 now keeps both claimants alive because it only checks winner_key_low == my_key_low, even though only one packed claim actually won atomic_min. That breaks the injective new→prev mapping sticky replay assumes and is a worse failure mode than the upstream sort merely having an unstable order for equal keys.

Also applies to: 364-366

🧹 Nitpick comments (1)

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

501-553: ⚡ Quick win

Fail fast when shape_pairs_max is unsupported on the selected construction path.

Right now a non-None shape_pairs_max is accepted even though the expert-component branch and the "explicit" branch ignore it. Since this knob exists specifically to control VRAM, silently accepting a no-op value is easy to miss and can leave callers with the full allocation they were trying to avoid. Raising ValueError here would make misconfiguration obvious.

♻️ Suggested guard

         using_expert_components = broad_phase_instance is not None or narrow_phase is not None
+        resolved_mode = mode_from_broad_phase if mode_from_broad_phase is not None else "explicit"
+        if shape_pairs_max is not None and (using_expert_components or resolved_mode == "explicit"):
+            raise ValueError(
+                "shape_pairs_max is only supported for broad_phase='nxn'/'sap' when "
+                "CollisionPipeline constructs its own broad/narrow phase components"
+            )
         if using_expert_components:
@@
-            self.broad_phase_mode = mode_from_broad_phase if mode_from_broad_phase is not None else "explicit"
+            self.broad_phase_mode = resolved_mode

Also applies to: 650-709

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@newton/_src/sim/collide.py` around lines 501 - 553, If shape_pairs_max is
provided but will be ignored on the selected construction path, raise a
ValueError instead of silently accepting it: in the CollisionPipeline
initializer (parameters shape_pairs_max, broad_phase, narrow_phase) detect when
shape_pairs_max is not None and the chosen path ignores it (e.g. broad_phase ==
"explicit" or when an expert prebuilt narrow_phase is supplied) and raise
ValueError with a clear message; apply the same guard in the other construction
branch handling the same parameters (the alternative initializer area around the
650-709 diff) so callers fail fast when they pass a no-op shape_pairs_max.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@newton/_src/sim/collide.py`:
- Around line 501-553: If shape_pairs_max is provided but will be ignored on the
selected construction path, raise a ValueError instead of silently accepting it:
in the CollisionPipeline initializer (parameters shape_pairs_max, broad_phase,
narrow_phase) detect when shape_pairs_max is not None and the chosen path
ignores it (e.g. broad_phase == "explicit" or when an expert prebuilt
narrow_phase is supplied) and raise ValueError with a clear message; apply the
same guard in the other construction branch handling the same parameters (the
alternative initializer area around the 650-709 diff) so callers fail fast when
they pass a no-op shape_pairs_max.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 3f9f9d51-b330-4bc7-b5ec-f16309ff2cb4

📥 Commits

Reviewing files that changed from the base of the PR and between 05f2c2b and 8a09c71.

📒 Files selected for processing (3)

CHANGELOG.md
newton/_src/geometry/contact_match.py
newton/_src/sim/collide.py

✅ Files skipped from review due to trivial changes (1)

CHANGELOG.md

Bumps the Newton pin from v1.2.0rc2 (current develop) directly to the v1.2.0 stable release across all five pin sites (isaaclab_newton, isaaclab_physx, isaaclab_visualizers x3, tools/wheel_builder/res/python_packages.toml), keeping the canonical `newton[sim] @ git+...` form everywhere. The full upstream release notes are at https://github.com/newton-physics/newton/releases/tag/v1.2.0 (published 2026-05-12). IsaacLab-relevant changes vs the rc2 pin: - MPR/GJK convex-hull centering fix (newton-physics/newton#2651) - Kamino FK solver performance (newton-physics/newton#2703) - HDR color output for tiled camera sensors (newton-physics/newton#2721) - Collada texture URDF import fix (newton-physics/newton#2743) - Kamino multi-GPU gravity-data device fix (newton-physics/newton#2823) - CollisionPipeline small fixes (newton-physics/newton#2632) - DelassusOperator attribute refactor (newton-physics/newton#2734) -- not used in IsaacLab source, no adapt needed. - SolverMuJoCo fixes: planar meshes, contact-anchor computation, distance conversion. mjwarp moves 3.8.0.1 -> 3.8.0.3 transitively via newton[sim]; no IsaacLab-side mujoco / mujoco-warp pin change since isaac-sim#5566 dropped explicit pins.

## Summary Bumps the Newton pin from `v1.2.0rc2` (current develop) directly to the [`v1.2.0` stable release](https://github.com/newton-physics/newton/releases/tag/v1.2.0) across all five pin sites, keeping the canonical `newton[sim] @ git+...` form everywhere. Per Kelly Guo's suggestion: skip the rc bump and go straight to stable. Upstream published `v1.2.0` on 2026-05-12. **Alternative**: [#5614](#5614) (rc3 bump) — pick whichever target based on CI signal. This one is the most forward target. > Branch is still named `jichuanh/newton-1.2.0rc4-bump` from when this PR was originally proposing rc4 — the branch name doesn't match the current target but the diff is correct. Force-pushing the rename would close/reopen the PR, which adds noise without changing the artifact. ## What's new in Newton v1.2.0 vs v1.2.0rc2 Full release notes: [newton-physics/newton release v1.2.0](https://github.com/newton-physics/newton/releases/tag/v1.2.0). Notable IsaacLab-relevant fixes: - [newton-physics/newton#2651](newton-physics/newton#2651) — MPR/GJK no longer assumes convex hulls are centered around the origin. - [newton-physics/newton#2703](newton-physics/newton#2703) — Kamino FK solver performance. - [newton-physics/newton#2721](newton-physics/newton#2721) — HDR color output for tiled camera sensors. - [newton-physics/newton#2743](newton-physics/newton#2743) — Collada textures in URDF import. - [newton-physics/newton#2823](newton-physics/newton#2823) — Gravity-data device allocation in Kamino (multi-GPU). - [newton-physics/newton#2632](newton-physics/newton#2632) — CollisionPipeline small fixes. - [newton-physics/newton#2734](newton-physics/newton#2734) — `DelassusOperator` attribute refactor. Not used in IsaacLab source today (verified by grep), no adapt needed. - SolverMuJoCo fixes: planar meshes, contact-anchor computation, distance conversion. ## Required dep bumps None on the IsaacLab side. The `mjwarp 3.8.0.1 → 3.8.0.3` bump flows in transitively through `newton[sim]`, since [#5566](#5566) dropped IsaacLab's explicit `mujoco` / `mujoco-warp` pins. `warp-lang` stays at `1.13.0` (set by [#5523](#5523)). ## Pins updated | File | Change | |---|---| | `source/isaaclab_newton/setup.py` | `v1.2.0rc2` → `v1.2.0` | | `source/isaaclab_physx/setup.py` | `v1.2.0rc2` → `v1.2.0` | | `source/isaaclab_visualizers/setup.py` | 3× `v1.2.0rc2` → `v1.2.0` | | `tools/wheel_builder/res/python_packages.toml` | `v1.2.0rc2` → `v1.2.0` | ## Test plan - [x] Pre-commit clean. - [ ] CI smoke verifies clean install picks up `newton 1.2.0` and downstream `mjwarp 3.8.0.3`.

## Summary Bumps the Newton pin from `v1.2.0rc2` (current develop) directly to the [`v1.2.0` stable release](https://github.com/newton-physics/newton/releases/tag/v1.2.0) across all five pin sites, keeping the canonical `newton[sim] @ git+...` form everywhere. Per Kelly Guo's suggestion: skip the rc bump and go straight to stable. Upstream published `v1.2.0` on 2026-05-12. **Alternative**: [isaac-sim#5614](isaac-sim#5614) (rc3 bump) — pick whichever target based on CI signal. This one is the most forward target. > Branch is still named `jichuanh/newton-1.2.0rc4-bump` from when this PR was originally proposing rc4 — the branch name doesn't match the current target but the diff is correct. Force-pushing the rename would close/reopen the PR, which adds noise without changing the artifact. ## What's new in Newton v1.2.0 vs v1.2.0rc2 Full release notes: [newton-physics/newton release v1.2.0](https://github.com/newton-physics/newton/releases/tag/v1.2.0). Notable IsaacLab-relevant fixes: - [newton-physics/newton#2651](newton-physics/newton#2651) — MPR/GJK no longer assumes convex hulls are centered around the origin. - [newton-physics/newton#2703](newton-physics/newton#2703) — Kamino FK solver performance. - [newton-physics/newton#2721](newton-physics/newton#2721) — HDR color output for tiled camera sensors. - [newton-physics/newton#2743](newton-physics/newton#2743) — Collada textures in URDF import. - [newton-physics/newton#2823](newton-physics/newton#2823) — Gravity-data device allocation in Kamino (multi-GPU). - [newton-physics/newton#2632](newton-physics/newton#2632) — CollisionPipeline small fixes. - [newton-physics/newton#2734](newton-physics/newton#2734) — `DelassusOperator` attribute refactor. Not used in IsaacLab source today (verified by grep), no adapt needed. - SolverMuJoCo fixes: planar meshes, contact-anchor computation, distance conversion. ## Required dep bumps None on the IsaacLab side. The `mjwarp 3.8.0.1 → 3.8.0.3` bump flows in transitively through `newton[sim]`, since [isaac-sim#5566](isaac-sim#5566) dropped IsaacLab's explicit `mujoco` / `mujoco-warp` pins. `warp-lang` stays at `1.13.0` (set by [isaac-sim#5523](isaac-sim#5523)). ## Pins updated | File | Change | |---|---| | `source/isaaclab_newton/setup.py` | `v1.2.0rc2` → `v1.2.0` | | `source/isaaclab_physx/setup.py` | `v1.2.0rc2` → `v1.2.0` | | `source/isaaclab_visualizers/setup.py` | 3× `v1.2.0rc2` → `v1.2.0` | | `tools/wheel_builder/res/python_packages.toml` | `v1.2.0rc2` → `v1.2.0` | ## Test plan - [x] Pre-commit clean. - [ ] CI smoke verifies clean install picks up `newton 1.2.0` and downstream `mjwarp 3.8.0.3`.

Fix non-determinism when sticky mode is used in contact matching and …

9c97b7c

…allow to user control more collision buffer sizes (relevant for very big scenes)

nvtw self-assigned this Apr 29, 2026

nvtw marked this pull request as draft April 29, 2026 10:14

Add determinism unit test with sticky contact matching enabled

901a02b

coderabbitai Bot reviewed Apr 29, 2026

View reviewed changes

Comment thread newton/tests/test_collision_pipeline.py Outdated

nvtw added 2 commits April 30, 2026 11:38

Merge branch 'main' into dev/tw3/collision_pipeline_fixes

ee46fff

Update the changelog

05f2c2b

nvtw marked this pull request as ready for review April 30, 2026 09:44

nvtw requested review from adenzler-nvidia and jumyungc April 30, 2026 09:44

adenzler-nvidia reviewed Apr 30, 2026

View reviewed changes

Comment thread newton/_src/geometry/contact_match.py

Comment thread newton/_src/sim/collide.py Outdated

Comment thread newton/_src/sim/collide.py Outdated

nvtw added 2 commits May 4, 2026 10:45

Merge branch 'main' into dev/tw3/collision_pipeline_fixes

a06101e

Implement the MR comments

8a09c71

coderabbitai Bot reviewed May 4, 2026

View reviewed changes

Merge branch 'main' into dev/tw3/collision_pipeline_fixes

c81a306

nvtw requested a review from adenzler-nvidia May 11, 2026 05:33

Use graph capture in unit tests

8945a14

adenzler-nvidia approved these changes May 11, 2026

View reviewed changes

adenzler-nvidia enabled auto-merge May 11, 2026 09:54

adenzler-nvidia added this pull request to the merge queue May 11, 2026

Merged via the queue into newton-physics:main with commit 3180bc0 May 11, 2026
25 checks passed

preist-nvidia mentioned this pull request May 12, 2026

Bump version to 1.2.0rc4 + Cherry-Picks from Main #2849

Merged

3 tasks

preist-nvidia pushed a commit that referenced this pull request May 12, 2026

Small fixes for the CollisionPipeline (#2632)

7398ada

This was referenced May 14, 2026

[Newton] Bump Newton pin to v1.2.0rc4 (depends on #5614) isaac-sim/IsaacLab#5615

Closed

[Newton] Bump Newton pin to v1.2.0 (stable) isaac-sim/IsaacLab#5616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small fixes for the CollisionPipeline#2632

Small fixes for the CollisionPipeline#2632
adenzler-nvidia merged 8 commits into
newton-physics:mainfrom
nvtw:dev/tw3/collision_pipeline_fixes

nvtw commented Apr 29, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 29, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Apr 29, 2026 •

edited

Loading

Uh oh!

adenzler-nvidia left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nvtw commented Apr 29, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

adenzler-nvidia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nvtw commented Apr 29, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 29, 2026 •

edited

Loading

codecov Bot commented Apr 29, 2026 •

edited

Loading