Parse contact response attributes from NewtonMaterialAPI - #3005
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates contact response parameters (ke, kd, kf, ka) from legacy per-shape USD custom attributes to material-level NewtonMaterialAPI mappings. Schema resolvers for Newton, PhysX, and MuJoCo are updated with deprecation-aware getters. The internal PhysicsMaterial model is extended to hold material-level contact parameters, and shape resolution logic implements two-stage precedence: material values with fallback to builder defaults, plus per-shape overrides only for ke/kd when material omits them. Comprehensive tests and documentation updates complete the migration. ChangesContact Response Migration
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
newton/tests/test_schema_resolver.py (2)
1766-1772: 💤 Low valueInconsistent warning capture pattern.
For consistency with the patterns used in
test_contact_response_attrs(line 1680) andtest_contact_response_legacy_shape(line 1718), this should convert warnings to strings:- deprecation_msgs = [x for x in w if issubclass(x.category, DeprecationWarning)] + deprecation_msgs = [str(x.message) for x in w if issubclass(x.category, DeprecationWarning)]The current code works correctly since only the length is checked, but matching the established pattern improves consistency.
🤖 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_schema_resolver.py` around lines 1766 - 1772, The test currently collects warning records in variable w and filters them by category; update it to follow the project's pattern by converting the matched DeprecationWarning records to strings: when using SchemaResolverManager with SchemaResolverNewton and SchemaResolverMjc and calling get_value(collider, PrimType.SHAPE, "ke"), build deprecation_msgs by filtering w for issubclass(x.category, DeprecationWarning) and mapping each matching record to a string (e.g., str(x.message)) before asserting the length equals 1 so the warning content is stored as strings consistently with other tests.
1636-1703: ⚡ Quick winConsider adding -inf sentinel test coverage.
The PR summary indicates that "explicitly authoring -inf on a material attribute blocks the legacy fallback." This test validates material-level attribute resolution and priority, but doesn't verify that a material with
newton:contactStiffness = -infblocks fallback to a shape's legacynewton:contact_keattribute. Consider adding a test case like:# Material has -inf (engine default), shape has legacy attr material.GetAttribute("newton:contactStiffness").Set(float("-inf")) collider.CreateAttribute("newton:contact_ke", Sdf.ValueTypeNames.Float).Set(1000.0) # Should return -inf, not 1000.0, blocking the legacy fallbackThis would ensure the sentinel behavior is explicitly validated.
🤖 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_schema_resolver.py` around lines 1636 - 1703, Add a test that verifies the -inf sentinel on a material blocks legacy fallback: create a collider prim that has the legacy attribute "newton:contact_ke" set (e.g., 1000.0), set the material's "newton:contactStiffness" to float("-inf"), then use SchemaResolverManager with SchemaResolverNewton (and any legacy resolver used in other tests) to call get_value for "ke" (on the material/PrimType.MATERIAL or via the shape resolution path used elsewhere) and assert the returned value is -inf (not the collider's legacy 1000.0); reference the existing test_contact_response_attrs setup, the material prim, the collider prim, and get_value/SchemaResolverManager/SchemaResolverNewton symbols to locate where to add this case.
🤖 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/utils/import_usd.py`:
- Around line 2650-2665: The change drops legacy per-shape fallback for kf/ka
causing silent breakage; restore the compatibility path by treating "kf" and
"ka" like "ke"/"kd": when material.kf or material.ka is None, attempt to read
per-shape values via R.get_value(prim, prim_type=PrimType.SHAPE, key=attr_key,
verbose=verbose) and use them if finite before falling back to
builder.default_shape_cfg (update the loop over attr_key to include "kf","ka"
and assign shape_kf/shape_ka accordingly), and add matching deprecated resolver
entries for newton:contact_kf and newton:contact_ka in
newton/_src/usd/schemas.py mirroring the ke/kd shims so callers get warnings
before removal.
---
Nitpick comments:
In `@newton/tests/test_schema_resolver.py`:
- Around line 1766-1772: The test currently collects warning records in variable
w and filters them by category; update it to follow the project's pattern by
converting the matched DeprecationWarning records to strings: when using
SchemaResolverManager with SchemaResolverNewton and SchemaResolverMjc and
calling get_value(collider, PrimType.SHAPE, "ke"), build deprecation_msgs by
filtering w for issubclass(x.category, DeprecationWarning) and mapping each
matching record to a string (e.g., str(x.message)) before asserting the length
equals 1 so the warning content is stored as strings consistently with other
tests.
- Around line 1636-1703: Add a test that verifies the -inf sentinel on a
material blocks legacy fallback: create a collider prim that has the legacy
attribute "newton:contact_ke" set (e.g., 1000.0), set the material's
"newton:contactStiffness" to float("-inf"), then use SchemaResolverManager with
SchemaResolverNewton (and any legacy resolver used in other tests) to call
get_value for "ke" (on the material/PrimType.MATERIAL or via the shape
resolution path used elsewhere) and assert the returned value is -inf (not the
collider's legacy 1000.0); reference the existing test_contact_response_attrs
setup, the material prim, the collider prim, and
get_value/SchemaResolverManager/SchemaResolverNewton symbols to locate where to
add this case.
🪄 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: 662643d0-5413-421d-88d5-dbcf0ab0a5f7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.pypyproject.toml
0d2d056 to
7ff487e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Line 39: Update the CHANGELOG entry to use the full deprecated attribute names
for clarity: replace the ambiguous `newton:contact_ke`/`kd` phrasing with both
explicit keys `newton:contact_ke` and `newton:contact_kd`, and expand the
migration guidance to instruct authors to instead put `newton:contactStiffness`
and `newton:contactDamping` on the bound `NewtonMaterialAPI` material; ensure
this `Deprecated` entry follows the repository's changelog guideline format for
migration guidance.
🪄 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: c6f802a5-a28a-4cd6-a27c-8561b0dfbdfc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.pypyproject.toml
✅ Files skipped from review due to trivial changes (1)
- docs/concepts/collisions.rst
🚧 Files skipped from review as they are similar to previous changes (5)
- pyproject.toml
- newton/tests/test_import_usd.py
- newton/tests/test_schema_resolver.py
- newton/_src/usd/schemas.py
- newton/_src/utils/import_usd.py
7ff487e to
8c6cd87
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
8c7a8b5 to
d17cfaf
Compare
jcarius-nv
left a comment
There was a problem hiding this comment.
Thanks for moving the canonical contact-response representation onto materials; that is the right direction. I think there are two compatibility/precedence issues to resolve before merging, plus a couple of tests that should pin down the intended behavior.
5b6199e to
d751599
Compare
andrewkaufman
left a comment
There was a problem hiding this comment.
Address review feedback on precedence, legacy kf/ka, and solref warning
5f1efe2 to
ee2369f
Compare
ee2369f to
9448771
Compare
Read ke/kd/kf/ka from NewtonMaterialAPI on bound materials during USD
import. The schema resolver resolves these across Newton, PhysX
(compliantContactStiffness/Damping), and MuJoCo (solref) at the
MATERIAL level, with configurable resolver priority.
Legacy newton:contact_ke/kd custom attributes on shape prims are
retained as a fallback when no material value is authored, emitting a
DeprecationWarning at resolve time directing users to the
NewtonMaterialAPI attrs. MuJoCo mjc:solref on material prims is
similarly deprecated. Explicitly authoring -inf on a material attr
("use engine default") blocks legacy fallback.
Requires newton-usd-schemas>=0.3.0.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
newton-usd-schemas 0.3.1 renamed this attribute. Update schema resolver, docs, tests, and bump dependency to >=0.3.1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per-shape mjc:solref (MjcGeomAPI) is unchanged; only material-level solref is deprecated in favor of NewtonMaterialAPI attrs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Correct precedence: non-legacy per-shape (MuJoCo solref) > material > legacy per-shape attr > builder default. Material now correctly wins over deprecated newton:contact_* attrs; MuJoCo solref still wins over material when authored on the shape. - Detect legacy vs non-legacy per-shape values via DeprecationWarning emitted by legacy getters; non-legacy (solref) emits none - Add legacy newton:contact_kf / newton:contact_ka to Newton SHAPE resolver with deprecation warning getters - Update mjc:solref material deprecation to recommend both NewtonMaterialAPI and per-shape mjc:solref (MjcGeomAPI) - Add test_contact_response_solref_over_material covering MuJoCo per-geom solref winning over material ke/kd Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per-shape mjc:solref should only win over material contact attrs when the MuJoCo resolver appears before Newton in the resolver list. Add reverse-precedence test verifying material wins when Newton is first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The rebase incorrectly picked up newton-physics#2964's [0.02, 1.0] default instead of preserving our None fallback, which ensures ke/kd only resolve when mjc:solref is actually authored on the prim. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
236bcf7 to
6443fb1
Compare
…ics#3005) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Description
Read contact response attributes (
ke,kd,kf,ka) fromNewtonMaterialAPIon bound materials during USD import instead of per-shape custom attributes. The schema resolver resolves these across Newton (newton:contactStiffness/contactDamping/contactFrictionStiffness/contactAdhesion), PhysX (physxMaterial:compliantContactStiffness/Damping), and MuJoCo (mjc:solref) at theMATERIALlevel, with configurable resolver priority.Legacy
newton:contact_ke/kdcustom attributes on shape prims are retained as a lowest-priority fallback when no material value is authored, emitting aDeprecationWarningat resolve time.mjc:solrefon material prims is similarly deprecated. Explicitly authoring-infon a material attr ("use engine default") blocks legacy fallback.Requires newton-usd-schemas>=0.3.0.
CHANGELOG.mdhas been updated (if user-facing change)Test plan
New feature / API change
Summary by CodeRabbit
New Features
Deprecated
Documentation