Fix scale-dependent MPR contacts on large mesh triangles - #3766
Conversation
|
|
GitHub Actions
Error
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
GitHub Actions
Error
📝 WalkthroughWalkthroughMPR triangle center initialization now applies bounded centroid nudging and triangle-normal fallbacks for coincident centers. CPU regression tests cover scale-dependent contacts, coplanar cases, and shared-edge witnesses. The changelog records the fix. ChangesMPR contact correction
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
newton/_src/geometry/mpr.py (1)
239-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the inline rationale.
Lines 239-257 contain detailed failure analysis that makes this local branch difficult to scan. Keep the reason for the nudge and its
0.01bound here. Move the extended manifold explanation to a design note or issue.As per path instructions, comments must be brief and explain non-obvious reasons rather than narrate implementation details.
🤖 Prompt for 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. In `@newton/_src/geometry/mpr.py` around lines 239 - 257, Shorten the inline comments around the triangle-centroid nudge to state only that it improves manifold quality and limits tangential displacement to 1% (0.01) of the normal center-to-plane distance. Remove the detailed shared-edge, manifold-builder, force-distribution, torque, and MPR convergence analysis from this branch, relocating it to the appropriate design note or issue.Sources: Coding guidelines, Path instructions
🤖 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/geometry/mpr.py`:
- Around line 267-270: Update the face-normal guard in the centroid-nudge logic
near center_a so it uses the degenerate-triangle threshold from
closest_point_on_triangle: require face_normal_length to exceed 1.0e-10, or
equivalently compare squared length against 1.0e-20. Add a regression case
covering the boundary and verifying near-degenerate triangles retain proj rather
than receiving a nudge.
In `@newton/tests/test_mpr.py`:
- Around line 108-115: Update the test docstrings in newton/tests/test_mpr.py at
lines 108-115 and 144-150 to begin with concise imperative one-line summaries:
make test_small_cylinder_on_large_triangle start with the requested face-normal
resolution summary, and make the other test start with the requested shared-edge
witness summary; retain the existing explanatory details after each summary.
---
Nitpick comments:
In `@newton/_src/geometry/mpr.py`:
- Around line 239-257: Shorten the inline comments around the triangle-centroid
nudge to state only that it improves manifold quality and limits tangential
displacement to 1% (0.01) of the normal center-to-plane distance. Remove the
detailed shared-edge, manifold-builder, force-distribution, torque, and MPR
convergence analysis from this branch, relocating it to the appropriate design
note or issue.
🪄 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: 818f3172-0e8b-4de8-9558-67fbf78d7573
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/geometry/mpr.pynewton/tests/test_mpr.py
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/geometry/mpr.py (1)
233-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated triangle-projection logic.
Both blocks recompute
tri_a,tri_b,tri_c,face_normal,face_normal_length_sq, the degenerate-threshold check, and the centroid/to_centroidtangential projection almost identically. This duplication is the reason the two blocks previously diverged on the degenerate-normal threshold (1.0e-12vs1.0e-20), an issue already flagged and fixed in this PR. Extracting a shared@wp.funchelper (e.g., returningtri_a, tri_b, tri_c, face_normal_unit, face_normal_valid, centroid) would remove this recurring drift risk betweengeometric_centerandsolve_mpr_core.This is not required before merge, since both sites are currently consistent, but it reduces future maintenance risk given the prior incident.
♻️ Sketch of a shared helper
`@wp.func` def _triangle_face_geometry(geom_a: Any) -> tuple[wp.vec3, wp.vec3, wp.vec3, wp.vec3, bool]: """Return triangle vertices, unit face normal, and validity for shape A.""" tri_a = wp.vec3(0.0, 0.0, 0.0) tri_b = geom_a.scale tri_c = geom_a.auxiliary face_normal = wp.cross(tri_b - tri_a, tri_c - tri_a) face_normal_length_sq = wp.length_sq(face_normal) valid = face_normal_length_sq >= 1.0e-20 face_normal_unit = wp.vec3(0.0, 0.0, 0.0) if valid: face_normal_unit = face_normal / wp.sqrt(face_normal_length_sq) return tri_a, tri_b, tri_c, face_normal_unit, validBoth
geometric_centerandsolve_mpr_corecan then call this helper instead of repeating the derivation.Also applies to: 358-395
🤖 Prompt for 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. In `@newton/_src/geometry/mpr.py` around lines 233 - 283, Extract the shared triangle face-geometry derivation from geometric_center and solve_mpr_core into a single `@wp.func` helper, returning the triangle vertices, normalized face normal, and validity flag using the consistent 1.0e-20 threshold. Update both callers to use the helper while preserving their existing projection and centroid-nudge behavior.
🤖 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.
Nitpick comments:
In `@newton/_src/geometry/mpr.py`:
- Around line 233-283: Extract the shared triangle face-geometry derivation from
geometric_center and solve_mpr_core into a single `@wp.func` helper, returning the
triangle vertices, normalized face normal, and validity flag using the
consistent 1.0e-20 threshold. Update both callers to use the helper while
preserving their existing projection and centroid-nudge behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9b22bcb-99a9-499c-b2fb-bd2dd66f70b9
📒 Files selected for processing (2)
newton/_src/geometry/mpr.pynewton/tests/test_mpr.py
1 participant
Description
Problem
In some poses, MPR turns a local contact between a small cylinder and a large triangle into a false, tens-of-metres penetration with witness points far from the cylinder. In the deterministic reproduction, the correct penetration is
0.061719 m, but Newtonee84505areturns58.826744 m; only the overlap boolean is correct.The vulnerable case is near-coplanar: the convex center is close to the triangle plane, while its projection lies inside a large face but far from the triangle centroid. The existing initialization shifts that projection toward the centroid by
1%of their separation. Here that adds0.474106 mtangentially against only0.000361 mnormally, making the initial MPR direction nearly tangent to the face. An instrumented trace in #3773 shows the resulting pose-sensitive portal branch and nonlocal witness reconstruction.Changes
This PR preserves the shared-edge manifold intent of #2276 without letting the initialization scale with triangle size:
-d * nfrom signed plane distancedand face normaln. This avoids a false tangent residual from subtracting large nearby float32 positions.|d| > 0, bound the centroid-directed tangent bias to0.01 * min(|G-P|, |d|). Its tangent/normal ratio is therefore at most0.01, independent of triangle size.d=0, use a triangle-specific face-normal fallback with the same centroid direction instead of the generic world-axis probe.Simply removing the centroid bias is not sufficient: it fixes the large-face case but makes adjacent triangles produce seam-biased witnesses. The bounded rule and triangle-specific fallback preserve those distinct witnesses, including at
d=0.Closes #3773. Related: #2276 and isaac-sim/IsaacLab#5071.
Checklist
CHANGELOG.mdhas been updated (user-facing collision fix)Test plan
Validated with Warp
1.16.0.dev20260716, the version locked by base commitee84505a, and Python3.12.3:All listed tests and hooks pass. Additional deterministic validation covered:
1001heights from0to2 mmin2 μmsteps, with no invalid depth, normal, or witness result;0.005xto10x, with penetration within2.9e-5relative error andnormal.z >= 0.99999994;58.826744 monee84505a,0.061719764 mwith this PR.Bug fix
Steps to reproduce:
ee84505a.overlap=True, penetration58.826744 m, a mostly tangential normal, and nonlocal witnesses.0.061719150 m.0.061719764 m.Minimal reproduction:
The standalone script is in #3773. The executable regression in
newton/tests/test_mpr.pycallscreate_solve_mpr(support_map).coredirectly, so broad phase, contact reduction, and dynamics solvers are not involved.Summary by CodeRabbit
Bug Fixes
Tests