Contact margin improvements #1314
Conversation
…r than the thickness. Also switches max(contact_margin_shape_a, contact_margin_shape_b) to their sum such that it is compatible with the thickness.
📝 WalkthroughWalkthroughReplaced per-shape margin combination from max(...) to summing per-shape margins across broad-phase, narrow-phase, SDF/hydroelastic, and contact-write kernels; added ShapeConfig doc clarifications, a finalize-time warning for shapes with thickness > contact_margin, and tests for the warning and orphan-joint validation. Changes
Sequence Diagram(s)(omitted — changes are margin-value adjustments across many kernels and validation, not a new multi-component control-flow feature) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-08-12T17:51:37.474ZApplied to files:
📚 Learning: 2025-10-10T10:47:41.082ZApplied to files:
📚 Learning: 2025-09-26T05:58:21.013ZApplied to files:
📚 Learning: 2025-08-20T18:02:36.099ZApplied to files:
📚 Learning: 2025-12-03T09:33:27.083ZApplied to files:
📚 Learning: 2025-12-12T17:41:15.097ZApplied to files:
📚 Learning: 2026-01-12T10:11:39.312ZApplied to files:
📚 Learning: 2025-09-26T05:58:21.013ZApplied to files:
🧬 Code graph analysis (1)newton/tests/test_collision_pipeline.py (2)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @newton/_src/sim/builder.py:
- Around line 5818-5839: The warning loop currently flags any shape where
shape_thickness[i] > shape_contact_margin[i]; change it to only consider shapes
that participate in collision tests by gating the check on the per-shape
collision flag (i.e., the flag/field that indicates the shape actually collides
with shapes or particles), so iterate i in range(shape_count) and skip shapes
where the collision flag is false before evaluating thickness vs contact_margin;
keep the existing message and formatting (shape_key, example_shapes,
warnings.warn) but only include collidable shapes in shapes_with_bad_margin so
visual-only sites/meshes do not trigger the warning.
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (1)
167-174: Clarify thickness/contact_margin semantics vs enforcementThe new docstrings capture the “thickened surface + summed thickness” idea, but two minor inconsistencies stand out:
- Line 171–174 says
contact_marginmust be>= thickness, whilefinalize()only emits a warning when that isn’t true. Consider softening this to “should be” or explicitly mentioning that violating it results in a runtime warning rather than an error.- Line 168–170 describes
thicknesspurely as a collision offset. It’s also used in mass/inertia and SDF/hydro setup paths, so a short note that it influences both collision and mass properties would make the behavior less surprising to users tuning physical thickness vs. margin.These are documentation-level only, but tightening them would reduce confusion around how to choose
thicknessandcontact_margin.newton/_src/solvers/mujoco/kernels.py (1)
140-143: Margin/gap mixing now uses sum instead of max; verify MuJoCo interop behavior
contact_paramsnow computesmarginandgapas the sum of both geoms’ values. This matches Newton’s new “sum-of-margins” semantics but will increase the effective MuJoCoincludemarginvs the previous max-based rule. Please recheck MuJoCo-bridge tests / benchmarks where geom margins or gaps were tuned assuming max-combination, to ensure contact onsets and constraint regularization still behave as expected.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
newton/_src/geometry/kernels.pynewton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_contact.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.pynewton/_src/sim/collide_unified.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_model.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/geometry/sdf_contact.pynewton/_src/sim/collide_unified.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/geometry/kernels.pynewton/_src/geometry/narrow_phase.pynewton/_src/sim/builder.pynewton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/sdf_contact.pynewton/_src/sim/collide_unified.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/geometry/kernels.pynewton/_src/geometry/narrow_phase.pynewton/_src/sim/builder.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/geometry/sdf_contact.pynewton/_src/sim/collide_unified.py
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/geometry/sdf_contact.pynewton/_src/geometry/kernels.pynewton/_src/geometry/narrow_phase.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/sim/collide_unified.pynewton/tests/test_model.pynewton/_src/geometry/narrow_phase.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/collide_unified.pynewton/_src/geometry/kernels.pynewton/_src/geometry/narrow_phase.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/collide_unified.pynewton/_src/geometry/kernels.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (11)
newton/_src/geometry/sdf_hydroelastic.py (3)
1197-1200: LGTM! Margin calculation updated to sum for consistency.The change from using individual margins to summing both shape margins aligns with the PR objective to make margin calculation compatible with thickness. The comment clearly documents the rationale.
Note: This effectively increases the collision detection threshold compared to the previous max-based approach, which could impact performance and simulation behavior but is intentional per the PR design.
1389-1392: LGTM! Consistent margin calculation for unreduced contacts.The margin summation is correctly applied in the decode path for unreduced contacts, maintaining consistency with the count_faces_kernel changes and ensuring the contact solver receives the combined margin value.
1671-1674: LGTM! Consistent margin calculation for reduced contacts.The margin summation is correctly applied in the binning path for reduced contacts, ensuring all contact generation paths (count_faces, decode, and binning) use consistent margin calculation logic.
newton/tests/test_model.py (4)
18-18: LGTM! Required import for warning tests.The
warningsimport is correctly added to support the new test methods that verify warning behavior.
867-885: LGTM! Well-structured test for thickness warning.The test correctly verifies that a
UserWarningis raised whenthickness > contact_margin, and validates that the warning message contains the expected diagnostic information (shape key, condition, and consequence).
887-904: LGTM! Proper negative test case.The test correctly verifies that no warning is raised when
thickness <= contact_margin, using appropriate warning capture and filtering to avoid false positives.
906-932: LGTM! Comprehensive test for multiple shape violations.The test effectively validates that the warning correctly reports multiple shapes with
thickness > contact_marginwhile excluding compliant shapes, demonstrating good coverage of the reporting logic.newton/_src/sim/collide_unified.py (1)
120-124: Summed per-shape margin inwrite_contactis consistent with thickness semanticsSwitching
contact_margintoshape_contact_margin[shape_a] + shape_contact_margin[shape_b]aligns the writer’s distance filter (d > contact_margin) with the rest of the pipeline, which already sums per-shape thickness (thickness_a + thickness_b) and now uses summed margins in the narrow-phase kernels. This keeps the pair-wise acceptance condition symmetric and avoids discrepancies between where contacts are generated and where they are written out.newton/_src/geometry/sdf_contact.py (1)
761-765: SDF mesh–mesh margin changes are internally consistent with thickness handlingBoth the direct and reduced mesh–SDF kernels now form a pair-wise
marginasshape_contact_margin[a] + shape_contact_margin[b], then usecontact_threshold = margin + triangle_mesh_thicknessconsistently in:
- Midphase culling (bounding-sphere distance tests in scaled/unscaled space), and
- Final contact acceptance (
dist < contact_threshold) andcontact_data.margin.Given SDF-side thickness is already baked into the volume, this keeps the effective separation budget as “sum of per-shape margins plus explicit triangle-mesh thickness” without double-counting and matches the rest of the PR’s margin semantics.
Also applies to: 967-969
newton/_src/geometry/kernels.py (1)
1110-1111: Broadphase and rigid narrow-phase now share a consistent sum-of-margins budgetBoth
broadphase_collision_pairsandhandle_contact_pairsnow derive a pair-wise contact margin asshape_contact_margin[shape_a] + shape_contact_margin[shape_b]. That value:
- Inflates the broadphase bounding-sphere checks, and
- Is used in narrow-phase both for mesh/plane SDF queries (
max_dist) and for the final acceptance testd < rigid_contact_margin, wheredis distance minus(radius_eff_a + radius_eff_b + thickness_a + thickness_b).This keeps the separation allowance consistent across stages and symmetric in the two shapes, which is exactly what you want given thickness is already summed. The main effect is to admit slightly more candidate pairs than the previous max-based rule, but in a predictable way.
Also applies to: 2006-2008
newton/_src/geometry/narrow_phase.py (1)
345-349: Narrow-phase consistently upgrades to pair-wise “sum-of-margins” across all pathsIn all the updated kernels here:
- GJK/MPR (
narrow_phase_kernel_gjk_mpr),- Mesh–triangle midphase (
narrow_phase_find_mesh_triangle_overlaps_kernel+narrow_phase_process_mesh_triangle_contacts_kernel), and- Mesh–plane (both direct and reduced variants),
the effective margin is now always
shape_contact_margin[a] + shape_contact_margin[b], and that pair-wise value is combined with the summed thicknesses when comparing distances. Per-shape AABBs are still expanded by each shape’s own margin, so broadphase, midphase, and final distance gating all agree on the contact “reach.”Given the single-contact solvers intentionally push acceptance filtering to the caller, using a single, summed pair-wise margin here is the right abstraction layer and keeps behavior consistent with the rest of the PR.
Also applies to: 431-435, 513-517, 611-615, 743-747
|
Do we have any tests that generate a contact when the distance between two surfaces is slightly smaller than marginA+marginB and fail to generate a contact when the distance is slightly bigger than marginA + marginB? If not, then it would be a great idea to add a ticket to add these tests. |
gyeomannvidia
left a comment
gyeomannvidia
left a comment
There was a problem hiding this comment.
Marking as approved.
Just a minor point about shapes that don't participate in collision and a subsequent ticket for more rigorous testing of margin behaviour.
# Conflicts: # newton/_src/sim/builder.py # newton/tests/test_model.py
Here is a ticket #1319 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
167-174: Clarify thatcontact_margin >= thicknessis a sufficient/conservative condition (docs currently read like “required” in all cases).
Right now the docstrings imply a hard requirement; in practice it’s the safe per-shape rule that guarantees broad-phase overlap regardless of the other shape (and likely matches the intent of this PR), but it’s not strictly necessary for every possible pair.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/sim/builder.pynewton/_src/solvers/mujoco/kernels.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/mujoco/kernels.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (1)
newton/_src/sim/builder.py (1)
5813-5851: Improve message formatting for orphan-joint errors and thickness warnings.The proposed formatting changes are sound:
- Add indices alongside keys in orphan joint error (line 5817):
f"{i}:{self.joint_key[i]}"prevents confusion when keys are auto-generated or collide.- Use
", ".join(example_shapes)instead of list repr for thickness warning (line 5848) for readability.Regarding threshold tests: The builder comments describe "sum of margins >= sum of thicknesses" (line 5828), but
check_aabb_overlapinbroad_phase_common.pycurrently usesmax(box1_cutoff, box2_cutoff), not sum. Confirm whether the threshold test request targets the current max-margin logic or anticipates an upcoming behavioral change to sum-margin logic, then add corresponding boundary tests (distance just below vs. just above the threshold).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @newton/_src/sim/builder.py:
- Around line 6061-6071: The orphan-joint check should iterate over the expected
joint range (use self.joint_count) instead of enumerating
self.joint_articulation to avoid silent under-reporting when joint_articulation
is shorter; compute orphan indices as [i for i in range(self.joint_count) if i
>= len(self.joint_articulation) or self.joint_articulation[i] < 0], then when
raising the ValueError include both the joint indices and their keys (from
self.joint_key) for the first N (e.g., 5) to make the error actionable and still
include the total orphan count and a reminder to call add_articulation() for
affected joints.
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (2)
167-175: Docstrings: consider explicitly stating pairwise margin behavior is additive (margin_a + margin_b).Given the PR-wide semantics change, spelling this out here would reduce ambiguity between (a) per-shape AABB expansion and (b) pairwise contact thresholds.
Proposed doc tweak
contact_margin: float | None = None """The contact margin for collision detection. If None, uses builder.rigid_contact_margin as default. AABBs are expanded by this value for broad phase detection. Must be >= thickness to ensure - collisions are not missed when thickened surfaces approach each other.""" + collisions are not missed when thickened surfaces approach each other. + + Note: + Pairwise contact detection uses the sum of both shapes' margins + (contact_margin_a + contact_margin_b)."""
6072-6099: Thickness-vs-margin warning: improve message readability (avoid list repr) and align comment wording with “missed broad-phase collisions”.The behavior is good, but the warning currently prints
Affected shapes: [...]using a Python list repr; joining is easier to read/copy-paste. Also the comment says “unstable contact behavior” while the rest of the block (and warning) is about broad-phase misses.Proposed refinement
- # warn if any shape has thickness > contact_margin (causes unstable contact behavior) + # warn if any shape has thickness > contact_margin (can cause missed broad-phase collisions) # Thickness is an outward offset from each shape's surface. AABBs are expanded by contact_margin. # For proper broad phase detection, each shape must have contact_margin >= thickness. @@ if shapes_with_bad_margin: - example_shapes = shapes_with_bad_margin[:5] + example_shapes = ", ".join(shapes_with_bad_margin[:5]) warnings.warn( f"Found {len(shapes_with_bad_margin)} shape(s) with thickness > contact_margin. " f"This can cause missed collisions in broad phase since AABBs are only expanded by contact_margin. " f"Set contact_margin >= thickness for each shape. " - f"Affected shapes: {example_shapes}" + ("..." if len(shapes_with_bad_margin) > 5 else ""), + f"Affected shapes: {example_shapes}" + ("..." if len(shapes_with_bad_margin) > 5 else ""), stacklevel=2, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/sim/builder.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/tests/test_mujoco_solver.py (1)
3071-3086: Make the exception assertion a bit more robust.The new test flow (raising from
builder.finalize()) matches the updated validation semantics. To reduce brittleness, consider lowercasing the exception string beforeassertIn.Proposed tweak
with self.assertRaises(ValueError) as ctx: builder.finalize() - self.assertIn("not belonging to any articulation", str(ctx.exception)) + self.assertIn("not belonging to any articulation", str(ctx.exception).lower())
🧹 Nitpick comments (1)
newton/_src/sim/builder.py (1)
6092-6119: Finalize-time warning is a good guardrail; consider making the warning message easier to read.
The condition (only shapes that collide; warn whenthickness > contact_margin) matches the broad-phase risk described. The current formatting prints a Python list repr.Suggested tweak: join the sample shapes into a string
if shapes_with_bad_margin: - example_shapes = shapes_with_bad_margin[:5] + example_shapes = ", ".join(shapes_with_bad_margin[:5]) warnings.warn( f"Found {len(shapes_with_bad_margin)} shape(s) with thickness > contact_margin. " f"This can cause missed collisions in broad phase since AABBs are only expanded by contact_margin. " f"Set contact_margin >= thickness for each shape. " - f"Affected shapes: {example_shapes}" + ("..." if len(shapes_with_bad_margin) > 5 else ""), + f"Affected shapes: {example_shapes}" + ("..." if len(shapes_with_bad_margin) > 5 else ""), stacklevel=2, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/geometry/narrow_phase.pynewton/_src/geometry/sdf_contact.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.pynewton/_src/sim/collide_unified.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/geometry/sdf_contact.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/geometry/narrow_phase.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/geometry/narrow_phase.pynewton/_src/sim/collide_unified.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/geometry/narrow_phase.pynewton/_src/sim/collide_unified.pynewton/_src/geometry/sdf_hydroelastic.pynewton/_src/sim/builder.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/geometry/narrow_phase.pynewton/_src/sim/collide_unified.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/geometry/narrow_phase.pynewton/_src/sim/collide_unified.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/builder.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/sim/builder.py
🧬 Code graph analysis (2)
newton/tests/test_mujoco_solver.py (1)
newton/_src/sim/builder.py (1)
finalize(6032-6706)
newton/_src/sim/builder.py (1)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (8)
newton/_src/sim/collide_unified.py (1)
117-120: LGTM! Margin summation aligns with thickness handling.The change from
max(margin_a, margin_b)tomargin_a + margin_bcorrectly mirrors how thickness is combined on lines 96-98. This ensures the contact margin threshold is consistent with the total separation calculation.Note: This is a breaking change that increases the effective contact margin (sum ≥ max for non-negative values), which may generate contacts at greater separation distances than before. Please verify that the migration guide update mentioned in the PR objectives documents this behavioral change for users upgrading existing simulations.
newton/_src/sim/builder.py (1)
167-175: Docstrings align with the new “sum” semantics (thickness + per-shape AABB expansion).
Nice clarification that thickness contributes asthickness_a + thickness_b, and that broad-phase expansion is per-shape viacontact_margin.newton/_src/geometry/sdf_hydroelastic.py (1)
1197-1201: Good consistency switch to sum-of-margins; consider clamping to non-negative and add/keep threshold tests.The summed margin is applied consistently across the SDF-hydro pipeline. As a small defensive improvement, consider
margin = wp.max(margin_a + margin_b, 0.0)at each site to avoid surprising behavior if margins can ever be negative.Also applies to: 1389-1393, 1669-1673
newton/_src/geometry/narrow_phase.py (5)
296-299: LGTM! Margin summing aligns with thickness handling.The change from
max(margin_a, margin_b)tomargin_a + margin_bis consistent with how thickness values are already combined (summed) inwrite_contact_simpleat lines 82-84. This ensures the margin calculation is compatible with the thickness model.Note: This is a behavioral change—contacts will now be detected at greater separation distances when both shapes have non-zero margins. The PR mentions the migration guide has been updated accordingly.
382-385: LGTM!Consistent margin summing applied to the mesh-convex midphase overlap detection.
464-467: LGTM!Consistent margin summing applied to mesh-triangle contact processing.
558-561: LGTM!Consistent margin summing applied to the non-reduction mesh-plane kernel. The contact threshold at line 580 correctly combines
margin + total_thicknesswhere both values are now sums of per-shape contributions.
685-688: LGTM!Consistent margin summing applied to the contact-reduction variant of the mesh-plane kernel, matching the non-reduction variant.
2 participants
Description
Add a warning that informs the user when the contact margin is smaller than the thickness. Also switches max(contact_margin_shape_a, contact_margin_shape_b) to their sum such that it is compatible with the thickness.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Bug Fixes
Validation
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.