Parse NewtonCollisionAPI contact penalty attributes - #2954
Closed
andrewkaufman wants to merge 13 commits into
Closed
Parse NewtonCollisionAPI contact penalty attributes#2954andrewkaufman wants to merge 13 commits into
andrewkaufman wants to merge 13 commits into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds NewtonCollisionAPI USD attributes for per-shape contact penalties, maps them to internal ke/kd/kf/ka (with legacy-name fallback and DeprecationWarning), applies resolved values in the USD importer (including per-shape material density), and adds unit/integration tests plus documentation and changelog entries. ChangesNewton USD Contact Penalty Attributes
Sequence DiagramsequenceDiagram
participant USDStage
participant SchemaResolverNewton
participant Importer
participant ModelBuilder
USDStage->>SchemaResolverNewton: read newton:contactStiffness/Damping/FrictionStiffness/Adhesion
SchemaResolverNewton->>USDStage: fallback to legacy newton:contact_* (emit DeprecationWarning)
SchemaResolverNewton->>Importer: return ke/kd/kf/ka values
Importer->>ModelBuilder: construct ShapeConfig and apply density/defaults
Estimated code review effort🎯 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/tests/test_import_usd.py (1)
5224-5248: ⚡ Quick winApply
NewtonCollisionAPIin this test to validate schema-based behavior, not just raw attribute presence.Right now the test only authors raw
newton:contact*attributes. AddingPrim.ApplyAPI("NewtonCollisionAPI")to these colliders makes the test match the intended schema contract and improves regression coverage.Proposed patch
col_all = UsdGeom.Cube.Define(stage, "/Articulation/AllAuthored/Collider") col_all_prim = col_all.GetPrim() + col_all_prim.ApplyAPI("NewtonCollisionAPI") UsdPhysics.CollisionAPI.Apply(col_all_prim) col_all_prim.CreateAttribute("newton:contactStiffness", Sdf.ValueTypeNames.Float).Set(5000.0) col_all_prim.CreateAttribute("newton:contactDamping", Sdf.ValueTypeNames.Float).Set(200.0) col_all_prim.CreateAttribute("newton:contactFrictionStiffness", Sdf.ValueTypeNames.Float).Set(2000.0) col_all_prim.CreateAttribute("newton:contactAdhesion", Sdf.ValueTypeNames.Float).Set(0.5) @@ col_partial = UsdGeom.Sphere.Define(stage, "/Articulation/PartialAuthored/Collider") col_partial_prim = col_partial.GetPrim() + col_partial_prim.ApplyAPI("NewtonCollisionAPI") UsdPhysics.CollisionAPI.Apply(col_partial_prim) col_partial_prim.CreateAttribute("newton:contactStiffness", Sdf.ValueTypeNames.Float).Set(9999.0) @@ col_none = UsdGeom.Capsule.Define(stage, "/Articulation/NoAuthored/Collider") col_none_prim = col_none.GetPrim() + col_none_prim.ApplyAPI("NewtonCollisionAPI") UsdPhysics.CollisionAPI.Apply(col_none_prim)🤖 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_import_usd.py` around lines 5224 - 5248, The colliders in the test are only setting raw newton:contact* attributes but not applying the NewtonCollisionAPI schema; update the test to call ApplyAPI("NewtonCollisionAPI") on each collider prim (col_all_prim, col_partial_prim, col_none_prim) before or after setting their contact attributes so the test validates schema-based behavior rather than just raw attributes.
🤖 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/usd/schemas.py`:
- Around line 88-91: The schema drop removed legacy attribute keys
(newton:contact_ke/kd/kf/ka) causing older USD files to silently use defaults;
add a compatibility shim in newton/_src/usd/schemas.py that accepts the legacy
names by mapping them to the current keys ("ke","kd","kf","ka") using
SchemaAttribute entries (the same SchemaAttribute("newton:contactStiffness",
None) etc.), and emit a one-time deprecation warning when any legacy key is
encountered (use the existing logging facility or a warnings.warn call) so users
see guidance; keep both names functional for this deprecation window and
document removal for the next release.
---
Nitpick comments:
In `@newton/tests/test_import_usd.py`:
- Around line 5224-5248: The colliders in the test are only setting raw
newton:contact* attributes but not applying the NewtonCollisionAPI schema;
update the test to call ApplyAPI("NewtonCollisionAPI") on each collider prim
(col_all_prim, col_partial_prim, col_none_prim) before or after setting their
contact attributes so the test validates schema-based behavior rather than just
raw attributes.
🪄 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: dabeb5e0-b04c-4d97-8698-9508d9230bfa
📒 Files selected for processing (6)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/usd/schemas.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/tests/test_schema_resolver.py (1)
1758-1760: ⚡ Quick winAssert that no deprecation warning is emitted when the new name is authored.
You already verify value precedence, but adding a no-warning assertion would lock in the intended behavior (new name should short-circuit legacy fallback).
Suggested patch
# New name takes priority over legacy when both are authored collider.CreateAttribute("newton:contactStiffness", Sdf.ValueTypeNames.Float).Set(9999.0) - self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ke"), 9999.0) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ke"), 9999.0) + self.assertFalse(any(issubclass(w.category, DeprecationWarning) for w in caught))🤖 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 1758 - 1760, Wrap the call that checks precedence (the collider.CreateAttribute(...) then resolver.get_value(collider, PrimType.SHAPE, "ke")) in a warnings capture and assert that no deprecation/warning was emitted: use Python's warnings.catch_warnings(record=True) with warnings.simplefilter("always"), call resolver.get_value inside that context, and assert the recorded warnings list is empty; ensure warnings is imported if not already and keep the existing value assertion (self.assertAlmostEqual(...)) alongside the no-warning assertion to lock behavior that the new name short-circuits legacy fallback.
🤖 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/usd/schemas.py`:
- Around line 108-129: The SchemaAttribute entries for "ke", "kd", "kf", and
"ka" use usd_value_getter=_newton_contact_attr_with_fallback(...) but do not
list the legacy attribute names in the SchemaAttribute.attribute_names, so
SchemaResolver.collect_prim_attrs() will not include the legacy keys in
schema_attrs; update each SchemaAttribute (the ones created for "ke", "kd",
"kf", "ka") to include the legacy attribute name (e.g., "newton:contact_ke",
"newton:contact_kd", "newton:contact_kf", "newton:contact_ka") in the
attribute_names parameter so that collect_prim_attrs() tracks both the current
spec.name and the legacy key used by the fallback getter.
---
Nitpick comments:
In `@newton/tests/test_schema_resolver.py`:
- Around line 1758-1760: Wrap the call that checks precedence (the
collider.CreateAttribute(...) then resolver.get_value(collider, PrimType.SHAPE,
"ke")) in a warnings capture and assert that no deprecation/warning was emitted:
use Python's warnings.catch_warnings(record=True) with
warnings.simplefilter("always"), call resolver.get_value inside that context,
and assert the recorded warnings list is empty; ensure warnings is imported if
not already and keep the existing value assertion (self.assertAlmostEqual(...))
alongside the no-warning assertion to lock behavior that the new name
short-circuits legacy fallback.
🪄 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: 4fa1b9af-d201-4386-b707-622a299b223f
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/usd/schemas.pynewton/tests/test_import_usd.pynewton/tests/test_schema_resolver.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/tests/test_schema_resolver.py (1)
1760-1766: ⚡ Quick winAssert new-name precedence is warning-free.
This block verifies returned values, but it doesn’t assert that
DeprecationWarningis not emitted when new names are authored alongside legacy names.Suggested test tightening
@@ import math import unittest +import warnings from pathlib import Path from typing import Any @@ - self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ke"), 9999.0) - self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "kd"), 999.0) - self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "kf"), 888.0) - self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ka"), 0.99) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ke"), 9999.0) + self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "kd"), 999.0) + self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "kf"), 888.0) + self.assertAlmostEqual(resolver.get_value(collider, PrimType.SHAPE, "ka"), 0.99) + self.assertFalse(any(issubclass(w.category, DeprecationWarning) for w in caught))🤖 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 1760 - 1766, Wrap the calls to resolver.get_value for the collider assertions in a warnings capture block and assert that no DeprecationWarning was recorded; e.g. use warnings.catch_warnings(record=True) with warnings.simplefilter("always") around the four resolver.get_value calls (which reference resolver.get_value, collider, PrimType.SHAPE and the attributes "newton:contactDamping"/"newton:contactFrictionStiffness"/"newton:contactAdhesion"), then assert that no captured warning has category DeprecationWarning (e.g. assert not any(w.category is DeprecationWarning for w in captured_warnings)).
🤖 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/tests/test_schema_resolver.py`:
- Around line 1760-1766: Wrap the calls to resolver.get_value for the collider
assertions in a warnings capture block and assert that no DeprecationWarning was
recorded; e.g. use warnings.catch_warnings(record=True) with
warnings.simplefilter("always") around the four resolver.get_value calls (which
reference resolver.get_value, collider, PrimType.SHAPE and the attributes
"newton:contactDamping"/"newton:contactFrictionStiffness"/"newton:contactAdhesion"),
then assert that no captured warning has category DeprecationWarning (e.g.
assert not any(w.category is DeprecationWarning for w in captured_warnings)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 25d57d56-d15c-428d-9292-0ae87fdc3bc7
📒 Files selected for processing (1)
newton/tests/test_schema_resolver.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Add schema resolver mappings and USD parsing for the four contact penalty attributes from NewtonCollisionAPI: newton:contactStiffness (ke), newton:contactDamping (kd), newton:contactFrictionStiffness (kf), newton:contactAdhesion (ka). Replace old custom attribute names (newton:contact_ke/kd/kf/ka) with the new schema names. Switch kf/ka parsing from get_float_with_fallback to the resolver pattern, matching ke/kd which already used it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Legacy newton:contact_ke/kd/kf/ka attributes now resolve with a DeprecationWarning, falling back to the new newton:contactStiffness etc. names. New name takes priority when both are authored. Apply NewtonCollisionAPI in integration test colliders. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… new-name priority Add attribute_names tuples so collect_prim_attrs tracks legacy names. Assert DeprecationWarning is not emitted when new names short-circuit the fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
newton-usd-schemas PR newton-physics#62 changes the schema defaults for contactStiffness/contactDamping/contactFrictionStiffness/contactAdhesion from baked Newton ShapeConfig values to the -inf sentinel (matching the contactGap precedent). Once that schema lands, the parser will receive float('-inf') for unauthored attributes instead of None, so the existing 'shape_kX is None' fallback would no longer trigger and -inf would propagate to the solver. Add an explicit '-inf' check alongside the None check, mirroring how gap_val is already handled a few lines above. Future-proofs the parser for the schema bump and also lets authors explicitly opt back into the builder default by writing -inf on a single attribute. Add test_newton_contact_penalty_inf_sentinel covering the explicit -inf case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two new tests document the cross-resolver semantics around the -inf sentinel: - test_newton_contact_penalty_unauthored_with_mjc_solref locks in existing fallback behavior: when NewtonCollisionAPI's contactStiffness/ contactDamping are unauthored, MuJoCo's mjc:solref takes effect via solref_to_stiffness/solref_to_damping. - test_newton_contact_penalty_explicit_inf_overrides_mjc_solref documents that explicit Newton -inf authoring is treated as an authored value and wins over MjcCollisionAPI's mjc:solref in the resolver priority order. The -inf sentinel then triggers the builder.default_shape_cfg.* substitution rather than the solref-derived value. Authors who want solref to take effect should simply leave the Newton attribute unauthored. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Schema resolver semantics belong in test_schema_resolver.py (matching the existing test_contact_penalty_attrs and test_contact_penalty_legacy_fallback tests on this PR), not test_import_usd.py. Replace the three test_newton_contact_penalty_* tests in test_import_usd.py with a single test_contact_penalty_inf_sentinel in test_schema_resolver.py covering: - Resolver returns the authored -inf sentinel as-is (parser substitutes builder.default_shape_cfg.* downstream). - Cross-resolver: explicit Newton -inf wins over MuJoCo solref fallback in priority order because the sentinel is authored, not unset. - Full import end-to-end: builder.default_shape_cfg.* is substituted for -inf on the resulting shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…solver test in test_schema_resolver.py Per review: the parser-level inf_sentinel test belongs alongside the other parse_usd integration tests. Only the cross-resolver portion (explicit Newton -inf vs MuJoCo solref priority) requires the resolver test file. Split accordingly: - test_import_usd.py::test_newton_contact_penalty_inf_sentinel: end-to-end parser substitution of builder.default_shape_cfg.* for -inf authoring. - test_schema_resolver.py::test_contact_penalty_inf_sentinel_cross_resolver: resolver-level priority where explicit -inf outranks an unauthored upstream mjc:solref fallback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
andrewkaufman
force-pushed
the
akaufman/contact-penalty-parsing
branch
from
May 27, 2026 19:27
8e6af0c to
9934baf
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Closed in favor of #3005 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add schema resolver mappings and USD parsing for the four contact penalty
attributes from
NewtonCollisionAPI:newton:contactStiffness(ke),newton:contactDamping(kd),newton:contactFrictionStiffness(kf),newton:contactAdhesion(ka).Replace old custom attribute names (
newton:contact_ke/kd/kf/ka) with thenew schema names. Switch kf/ka parsing from
get_float_with_fallbackto theresolver pattern, matching ke/kd which already used it. Legacy attribute names
still resolve with a
DeprecationWarning.Treat
-infas a sentinel meaning "use builder default" alongsideNone,matching the
gap_valpattern already inparse_usd. Required for forwardcompatibility with newton-physics/newton-usd-schemas#62, which changes the
schema-level defaults of these four attributes from baked Newton ShapeConfig
values to
-inf.Document
NewtonCollisionAPIandNewtonMeshCollisionAPIin the collisionUSD integration guide.
Depends on newton-physics/newton-usd-schemas#62.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
test_contact_penalty_inf_sentinelcovers explicit-infauthoring at theresolver level, the cross-resolver priority case (authored Newton
-infwinsover MuJoCo
solreffallback), and the full-import substitution intobuilder.default_shape_cfg.*.New feature / API change