Add explicit rigid contact kinematics - #3803
Conversation
📝 WalkthroughWalkthroughAdds ChangesRigid contact kinematics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant eval_rigid_contact_kinematics
participant rigid_contact_kinematics_kernel
participant Contacts
Caller->>eval_rigid_contact_kinematics: provide model, state, contacts, and output arrays
eval_rigid_contact_kinematics->>eval_rigid_contact_kinematics: validate arrays and devices
eval_rigid_contact_kinematics->>rigid_contact_kinematics_kernel: evaluate selected outputs
rigid_contact_kinematics_kernel->>Caller: write distances and world-space points
Caller->>Contacts: read frozen rigid_contact_normal
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
newton/tests/test_differentiable_contacts.py (1)
55-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explanatory docstring body for this multi-scenario test.
This test exercises five distinct behaviors: full-output computation, distance-only output, point0-only output, missing-output validation, and gradient propagation. As per path instructions, "add a Google-style explanatory body for particularly complex tests." Add a short body listing the scenarios covered, so a future reader does not need to parse the whole test body to know what it verifies.
🤖 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/tests/test_differentiable_contacts.py` around lines 55 - 118, Expand the docstring of test_compute_rigid_contact_kinematics with a concise Google-style explanatory body listing its five covered scenarios: full-output computation, distance-only output, point0-only output, missing-output validation, and gradient propagation. Keep the test behavior unchanged.Source: Path instructions
newton/_src/sim/contacts.py (1)
507-518: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify that
rigid_contact_diff_normalreturns the same array object, not a copy.The getter returns
self.rigid_contact_normaldirectly when no override is set (line 517). The docstring says this attribute "contains the same values," which reads as value-equality. It is in fact the identical array object. A caller who mutates the array returned fromrigid_contact_diff_normalin place (for example.zero_()orwp.copyinto it) silently corruptsrigid_contact_normal, the buffer solvers read for contact resolution. Before this change,rigid_contact_diff_normalwas a separate buffer, so in-place writes were safe; that is no longer true.State explicitly in the docstring that the same underlying array object is returned, so callers do not mutate it in place.
📝 Proposed docstring clarification
def rigid_contact_diff_normal(self) -> wp.array[wp.vec3] | None: """Frozen world-space contact normal. .. deprecated:: 1.5 - Use :attr:`rigid_contact_normal`, which contains the same values. + Use :attr:`rigid_contact_normal`. When no override was assigned, this + property returns that same array object (not a copy); do not write into + it in place, since doing so mutates :attr:`rigid_contact_normal` itself. """🤖 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/sim/contacts.py` around lines 507 - 518, Update the docstring of rigid_contact_diff_normal to state explicitly that, when no override is set, it returns the same underlying array object as rigid_contact_normal rather than a copy. Preserve the existing getter behavior and deprecation guidance.
🤖 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/differentiable_contacts.py`:
- Around line 195-201: Update the state.body_q device validation in
compute_rigid_contact_kinematics to explicitly handle state.body_q being None
before accessing .device, raising a clear ValueError for body-less models;
retain the existing device mismatch validation when body_q is present.
---
Nitpick comments:
In `@newton/_src/sim/contacts.py`:
- Around line 507-518: Update the docstring of rigid_contact_diff_normal to
state explicitly that, when no override is set, it returns the same underlying
array object as rigid_contact_normal rather than a copy. Preserve the existing
getter behavior and deprecation guidance.
In `@newton/tests/test_differentiable_contacts.py`:
- Around line 55-118: Expand the docstring of
test_compute_rigid_contact_kinematics with a concise Google-style explanatory
body listing its five covered scenarios: full-output computation, distance-only
output, point0-only output, missing-output validation, and gradient propagation.
Keep the test behavior unchanged.
🪄 Autofix
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: 8316b32d-9abc-4dbd-9bd8-a03d23792ebb
📒 Files selected for processing (9)
CHANGELOG.mddocs/api/newton_geometry.rstdocs/concepts/collisions.rstnewton/_src/geometry/differentiable_contacts.pynewton/_src/sim/collide.pynewton/_src/sim/contacts.pynewton/_src/solvers/coupled/solver_coupled.pynewton/geometry.pynewton/tests/test_differentiable_contacts.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Let callers provide only the differentiable rigid-contact outputs they need, instead of coupling derived quantities to Contacts storage. Deprecate the legacy attributes and reuse rigid_contact_normal for the redundant differentiable-normal compatibility alias.
Match simulation evaluation APIs such as eval_fk by exporting the contact helper as newton.eval_rigid_contact_kinematics. Keep the caller-provided output contract while moving the public wrapper into the simulation layer.
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)
CHANGELOG.md (1)
50-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winState the migration action for this changed behavior.
The
Changedcategory requires migration guidance. AddNo migration is requiredor state that callers should useContacts.rigid_contact_normaldirectly when they do not need the deprecated alias.As per coding guidelines, "
Deprecated,Changed, andRemovedentries must include migration guidance."🤖 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 `@CHANGELOG.md` at line 50, Update the changelog entry for Contacts.rigid_contact_diff_normal to include migration guidance, stating that no migration is required or directing callers who do not need the deprecated alias to use Contacts.rigid_contact_normal directly.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
newton/_src/sim/contact_kinematics.py (1)
16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed Warp array annotation.
Replace
wp.array | Nonewithwp.array[Any] | None. ImportAnyfromtyping.Proposed fix
+from typing import Any + def _validate_output( name: str, - output: wp.array | None, + output: wp.array[Any] | None,As per coding guidelines, annotate Warp arrays with bracket syntax.
🤖 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/sim/contact_kinematics.py` around lines 16 - 22, Update the _validate_output parameter annotation from wp.array | None to wp.array[Any] | None, and import Any from typing. Keep the function behavior unchanged while using Warp’s bracketed array annotation syntax.Source: Coding guidelines
🤖 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/sim/contact_kinematics.py`:
- Around line 83-86: Update eval_rigid_contact_kinematics to validate that
contacts and state.body_q belong to the supplied model, not merely that their
devices match. Use an available model-identity or preallocated-buffer
association check, and reject inputs from different models before indexing
model.shape_body; if identity cannot be tracked, document and enforce the
required ownership contract.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Line 50: Update the changelog entry for Contacts.rigid_contact_diff_normal to
include migration guidance, stating that no migration is required or directing
callers who do not need the deprecated alias to use
Contacts.rigid_contact_normal directly.
---
Nitpick comments:
In `@newton/_src/sim/contact_kinematics.py`:
- Around line 16-22: Update the _validate_output parameter annotation from
wp.array | None to wp.array[Any] | None, and import Any from typing. Keep the
function behavior unchanged while using Warp’s bracketed array annotation
syntax.
🪄 Autofix
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: 69389ed4-5f3e-40f7-bf8e-316511a6f24c
📒 Files selected for processing (10)
CHANGELOG.mddocs/api/newton.rstdocs/concepts/collisions.rstnewton/__init__.pynewton/_src/geometry/differentiable_contacts.pynewton/_src/sim/__init__.pynewton/_src/sim/collide.pynewton/_src/sim/contact_kinematics.pynewton/_src/sim/contacts.pynewton/tests/test_differentiable_contacts.py
💤 Files with no reviewable changes (1)
- newton/_src/geometry/differentiable_contacts.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/tests/test_differentiable_contacts.py
- newton/_src/sim/contacts.py
- newton/_src/sim/collide.py
4625216 to
ef1aed0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/sim/contact_kinematics.py (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a parameterized
wp.arrayannotation.Line 18 uses bare
wp.array. Usewp.array[Any] | Nonebecause_validate_outputaccepts arrays with multiple element types.As per coding guidelines, “Annotate Warp arrays with bracket syntax.”
🤖 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/sim/contact_kinematics.py` at line 18, Update the output parameter annotation in the surrounding contact kinematics function from bare wp.array to wp.array[Any] | None, matching the multiple element types accepted by _validate_output and the required bracket syntax.Source: Coding guidelines
🤖 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/sim/contact_kinematics.py`:
- Around line 88-90: Update eval_rigid_contact_kinematics and its output
validation around _validate_output to reject out_distance, out_point0_world, and
out_point1_world when they alias any contact input buffer consumed by
_launch_rigid_contact_kinematics, including the margin and point arrays.
Preserve existing dtype, shape, and device validation, add regression coverage
for margin and point aliases, and document the changed accepted API behavior in
the changelog.
---
Nitpick comments:
In `@newton/_src/sim/contact_kinematics.py`:
- Line 18: Update the output parameter annotation in the surrounding contact
kinematics function from bare wp.array to wp.array[Any] | None, matching the
multiple element types accepted by _validate_output and the required bracket
syntax.
🪄 Autofix
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: 12ed80ed-dfc9-4845-b41f-a7c9d7395925
📒 Files selected for processing (11)
CHANGELOG.mddocs/api/newton.rstdocs/concepts/collisions.rstnewton/__init__.pynewton/_src/geometry/differentiable_contacts.pynewton/_src/sim/__init__.pynewton/_src/sim/collide.pynewton/_src/sim/contact_kinematics.pynewton/_src/sim/contacts.pynewton/_src/solvers/coupled/solver_coupled.pynewton/tests/test_differentiable_contacts.py
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/api/newton.rst
- newton/_src/sim/init.py
- newton/_src/sim/collide.py
- docs/concepts/collisions.rst
- newton/_src/geometry/differentiable_contacts.py
- newton/_src/sim/contacts.py
- newton/tests/test_differentiable_contacts.py
- newton/init.py
- CHANGELOG.md
- newton/_src/solvers/coupled/solver_coupled.py
2 participants
Description
Add
newton.eval_rigid_contact_kinematics()so callers can provide only the rigid-contact distance and world-point arrays they need. The simulation-layer placement andeval_*name match related APIs such asnewton.eval_fk()andnewton.eval_inverse_dynamics_passive().Deprecate the
Contacts.rigid_contact_diff_*compatibility outputs while retaining the distance and point arrays for the required deprecation window. ReuseContacts.rigid_contact_normalfor the deprecated differentiable-normal alias, eliminating that duplicate allocation immediately.This separates derived contact kinematics from
Contactsstorage and prepares the remaining compatibility arrays for removal after the deprecation period.eval_rigid_contact_kinematicsEvaluates selected kinematic quantities for the active rigid contacts in a populated
Contactsobject. It reconstructs the world-space support points from the body-local points produced by collision detection and computes the signed distance using those points, the frozen world-space contact normal, and the contact margins.Arguments
model(newton.Model): Supplies the shape-to-body mapping and determines the expected device.state(newton.State): Supplies the current body transforms throughstate.body_q. Its body transforms must be on the same device asmodel.contacts(newton.Contacts): Populated contacts whose rigid-contact geometry will be evaluated. The contacts must be on the same device asmodel.out_distance(wp.array[float] | None): Optional output for signed contact distances in meters. It must have shape(contacts.rigid_contact_max,), dtypefloat, and reside on the model device. Positive values represent gaps and negative values represent penetration.out_point0_world(wp.array[wp.vec3] | None): Optional output for world-space support points on shape 0 in meters. It must have shape(contacts.rigid_contact_max,), dtypewp.vec3, and reside on the model device.out_point1_world(wp.array[wp.vec3] | None): Optional output for world-space support points on shape 1 in meters. It must have shape(contacts.rigid_contact_max,), dtypewp.vec3, and reside on the model device.Only entries in the active contact range
[0, contacts.rigid_contact_count)are written. PassNonefor quantities that are not needed, but at least one output must be provided. Invalid output types or dtypes raiseTypeError; missing outputs, invalid shapes, and device mismatches raiseValueError.Calls made inside a
wp.Tapescope participate in autodiff when the output arrays andstate.body_qrequire gradients. Gradients flow through the body transforms, but not through the frozen contact normal or the discrete contact set. The existingcontacts.rigid_contact_normalarray provides the world-space normal directly.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
uv run python -m newton.tests.test_differentiable_contactsuvx pre-commit run -aNew feature / API change
Summary by CodeRabbit
New Features
eval_rigid_contact_kinematicsto compute rigid-contact distances and world-space support points into caller-provided output arrays.Documentation
Deprecations
Tests