Fix excessive memory usage of CollisionPipeline#2384
Conversation
…g instead of linear scaling (w.r.t. number of worlds)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughComputes collision shape-pair limits per-world (including global shapes per-world) instead of a global N² bound, updating CollisionPipeline and Kamino unified geometry initialization and adding tests and a changelog entry to prevent quadratic memory growth across multiple worlds. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/tests/test_collision_pipeline.py (1)
803-822: Add one mixed-ShapeFlagscase here.The new helper in
newton/_src/sim/collide.pyalso filters byShapeFlags.COLLIDE_SHAPES, but every model built in this block marks all shapes collidable. A single mixed-flags test would lock down the branch that keeps NXN/SAP capacity aligned with broad-phase filtering for visual-only shapes.Also applies to: 824-912
🤖 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 803 - 822, Tests always set all shapes to ShapeFlags.COLLIDE_SHAPES, so add a mixed-flags scenario to exercise the new broad-phase filter: in the test that calls _make_model, create a model where some shapes keep ShapeFlags.COLLIDE_SHAPES and others are set to a non-colliding flag (e.g., ShapeFlags.VISUAL_ONLY or 0) by mutating model.shape_flags (or passing an array of flags) after _make_model returns; this will ensure the branch in the new collision filtering logic (involving ShapeFlags.COLLIDE_SHAPES) and the NXN/SAP capacity alignment is covered.
🤖 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/_src/solvers/kamino/_src/geometry/unified.py`:
- Around line 471-477: The current pair-counting uses np.unique(wid_np) and
treats each UID independently, which undercounts because geoms with wid == -1
(shared/global geoms) participate in every regular-world slice; update the logic
in the block that sets self._max_shape_pairs to first compute global_count =
np.count_nonzero(wid_np == -1), then iterate only over world IDs uid != -1 and
for each compute n = np.count_nonzero(wid_np == uid) + global_count and add (n *
(n - 1)) // 2 to per_world_pairs; if there are no regular worlds but
global_count > 0, also add (global_count * (global_count - 1)) // 2 once to
account for the dedicated global-only slice; assign the final sum to
self._max_shape_pairs.
---
Nitpick comments:
In `@newton/tests/test_collision_pipeline.py`:
- Around line 803-822: Tests always set all shapes to ShapeFlags.COLLIDE_SHAPES,
so add a mixed-flags scenario to exercise the new broad-phase filter: in the
test that calls _make_model, create a model where some shapes keep
ShapeFlags.COLLIDE_SHAPES and others are set to a non-colliding flag (e.g.,
ShapeFlags.VISUAL_ONLY or 0) by mutating model.shape_flags (or passing an array
of flags) after _make_model returns; this will ensure the branch in the new
collision filtering logic (involving ShapeFlags.COLLIDE_SHAPES) and the NXN/SAP
capacity alignment is covered.
🪄 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: 12a32417-1a26-4e13-b261-57d8c626dc7d
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/sim/collide.pynewton/_src/solvers/kamino/_src/geometry/unified.pynewton/tests/test_collision_pipeline.py
The CollisionPipeline fix was not cherry-picked but its changelog entry came along via a merge conflict resolution in newton-physics#2385.
Description
Fix O(W²·S²) memory explosion in
CollisionPipelineshape-pair buffer allocationfor NXN and SAP broad phase modes. The buffer capacity
shape_pairs_maxwas computedas
N*(N-1)/2over the global shape count across all worlds, but shapes indifferent worlds can never collide. The fix computes the sum of per-world
n*(n-1)/2instead, matching the thread layout already used by the broad phasekernels. At 1024 worlds with ~20 shapes each this reduces
shape_pairs_maxfrom~210 M to ~215 K (~975×), saving 10+ GB of GPU memory across the pipeline's
pair and narrow-phase buffers.
The same fix is applied to the Kamino
CollisionPipelineUnifiedKamino.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Seven new tests in
TestShapePairsMaxScalingverify:CollisionPipelinewith NXN and SAP on a 64-world modelhas
shape_pairs_maxequal to the linear per-world sumBug fix
Steps to reproduce:
CollisionPipeline(model, broad_phase="nxn")or"sap"Minimal reproduction:
Summary by CodeRabbit
Bug Fixes
Tests