Remove deprecated Model.mujoco.dof_passive_damping#3623
Remove deprecated Model.mujoco.dof_passive_damping#3623adenzler-nvidia wants to merge 4 commits into
Conversation
Remove the dof_passive_damping custom attribute, its finalize-time merge into joint_damping, and the deprecated runtime alias on the mujoco namespace (deprecated in 1.3.0); use Model.joint_damping. USD mjc:damping now resolves through SchemaResolverMjc like the other direct-mapped MuJoCo attributes (armature, frictionloss), via a new damping_per_rad schema key: MuJoCo authors damping in SI units, so it bypasses the USD per-degree convention that applies to the plain damping key on angular DOFs. It is no longer parsed implicitly when MuJoCo custom attributes are registered; pass SchemaResolverMjc in schema_resolvers when importing MuJoCo-authored USD. The MJCF importer now parses damping on ball joints natively, mirroring its frictionloss handling; ball-joint damping previously reached joint_damping only through the removed custom-attribute merge (surfaced by the Cassie menagerie comparison). Hinge and slide damping was already parsed natively and is unaffected.
Remove AttributeNamespace.add_deprecated_alias and its alias lookup in __getattr__/__setattr__; the dof_passive_damping alias was its only user. Also drop the coupled solver's compaction filter and the namespace-attribute collision guard that existed solely to skip alias entries.
📝 WalkthroughWalkthroughMuJoCo damping import now supports explicit per-radian USD mappings and MJCF ball-joint damping propagation. The deprecated ChangesMuJoCo damping behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ModelBuilder
participant SchemaResolverMjc
participant USDImporter
participant Model
ModelBuilder->>SchemaResolverMjc: Resolve mjc:damping
SchemaResolverMjc->>USDImporter: Return damping_per_rad
USDImporter->>Model: Populate joint_damping
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.
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
1755-1766: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate SI-vs-legacy damping resolution logic.
The
damping/damping_per_radresolution and its paired conditional/DegreesToRadianconversion is duplicated almost verbatim between the single-DOF path (resolve_dof_params, lines 1571-1583 and 1642) and this D6 path. If the priority/conversion rule ever needs a fix, both copies must be updated in lockstep.♻️ Suggested extraction
+def _resolve_dof_damping( + R, + prim, + default_joint_damping: float, + verbose: bool, +) -> tuple[float, bool, bool]: + """Return (damping, authored, is_si) resolving damping_per_rad (SI) over legacy damping (per-degree).""" + _damping_usd = R.get_value(prim, prim_type=PrimType.JOINT, key="damping", default=None, verbose=verbose) + _damping_per_rad_usd = R.get_value( + prim, prim_type=PrimType.JOINT, key="damping_per_rad", default=None, verbose=verbose + ) + damping_si = _damping_per_rad_usd is not None + damping_authored = damping_si or _damping_usd is not None + if damping_si: + return _damping_per_rad_usd, damping_authored, damping_si + if damping_authored: + return _damping_usd, damping_authored, damping_si + return default_joint_damping, damping_authored, damping_siThen both
resolve_dof_paramsand the D6 branch call_resolve_dof_damping(...)and applydamping / DegreesToRadian if damping_authored and not damping_si else dampingfor angular DOFs.Also applies to: 1963-1965
🤖 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/utils/import_usd.py` around lines 1755 - 1766, Extract the shared SI-versus-legacy damping selection and authored-status logic from resolve_dof_params and the D6 branch into a helper such as _resolve_dof_damping. Update both callers, including the later D6 occurrence, to use the helper and retain the existing damping / DegreesToRadian conversion only for authored non-SI angular damping.
🤖 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/utils/import_usd.py`:
- Around line 1755-1766: Extract the shared SI-versus-legacy damping selection
and authored-status logic from resolve_dof_params and the D6 branch into a
helper such as _resolve_dof_damping. Update both callers, including the later D6
occurrence, to use the helper and retain the existing damping / DegreesToRadian
conversion only for authored non-SI angular damping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cb0517d-00ff-412d-b78f-2f0e158a5120
📒 Files selected for processing (10)
CHANGELOG.mddocs/solvers/mujoco.rstnewton/_src/sim/model.pynewton/_src/solvers/coupled/solver_coupled.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/usd/schemas.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_mjcf.pynewton/tests/test_import_usd.py
💤 Files with no reviewable changes (3)
- docs/solvers/mujoco.rst
- newton/_src/solvers/mujoco/solver_mujoco.py
- newton/tests/test_import_mjcf.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Re-register mujoco:dof_passive_damping with usd_attribute_name="mjc:damping" so USD files authored with the MuJoCo convention continue to have their passive joint damping picked up when SolverMuJoCo.register_custom_attributes is called, without requiring an explicit SchemaResolverMjc. The deprecated runtime alias (Model.mujoco.dof_passive_damping as a proxy for Model.joint_damping) stays removed. The attribute now lives as a plain Warp array on model.mujoco; its values are copied into joint_damping by the finalizer at model build time, matching the pre-1.3 behaviour. The MJCF path is unaffected: the importer already parses damping directly into joint_damping for hinge/slide joints (and ball joints since this PR), so no mjcf_attribute_name is registered here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
7-7: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove the deprecated damping staging path entirely.
Renaming the finalizer does not remove the deprecated behavior: it still performs the old conflict check and merge into
model.joint_damping. Lines 994-1009 also registerdof_passive_dampingwithassignment=MODEL, somodel.mujoco.dof_passive_dampingremains exposed. WithSchemaResolverMjcnow resolvingmjc:dampingthroughdamping_per_rad, this creates a duplicate import path that can falsely reject converted values. Remove the finalizer, registration, and unusedmathimport; let the schema resolver own this mapping and add a regression check that the deprecated field is absent.Per the PR objectives,
SchemaResolverMjcshould resolve USD damping directly intojoint_damping.Also applies to: 123-156, 994-1010
🤖 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/solvers/mujoco/solver_mujoco.py` at line 7, Remove the deprecated damping finalizer, its dof_passive_damping registration with assignment=MODEL, and the now-unused math import in solver_mujoco.py. Ensure SchemaResolverMjc resolves USD mjc:damping directly into joint_damping without the legacy conflict-check or merge path. Add a regression check confirming model.mujoco.dof_passive_damping is absent.
🤖 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.
Outside diff comments:
In `@newton/_src/solvers/mujoco/solver_mujoco.py`:
- Line 7: Remove the deprecated damping finalizer, its dof_passive_damping
registration with assignment=MODEL, and the now-unused math import in
solver_mujoco.py. Ensure SchemaResolverMjc resolves USD mjc:damping directly
into joint_damping without the legacy conflict-check or merge path. Add a
regression check confirming model.mujoco.dof_passive_damping is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed361e2f-8079-41f8-8618-747d799a914c
📒 Files selected for processing (1)
newton/_src/solvers/mujoco/solver_mujoco.py
| # MuJoCo damping is authored in SI units (per radian for angular | ||
| # DOFs), unlike the USD per-degree convention behind the plain | ||
| # "damping" key, so it resolves through the _per_rad variant. | ||
| "damping_per_rad": SchemaAttribute("mjc:damping", None), |
There was a problem hiding this comment.
🟠 Please also consume this mapping for spherical joints in newton/_src/utils/import_usd.py. The spherical branch currently calls add_joint_ball(**joint_params) without resolving damping_per_rad, so resolver-only imports silently discard authored mjc:damping. MuJoCo ball joints have three angular DOFs, and their scalar damping applies uniformly to all three. Resolve the source-aware value, broadcast it across the three DOFs, pass it through add_joint_ball(damping=...), and add a resolver-only spherical-joint regression test.
2 participants
Description
Remove
Model.mujoco.dof_passive_damping, deprecated in 1.3.0 in favor ofModel.joint_damping: the runtime alias on themujoconamespace, themujoco:dof_passive_dampingcustom attribute, and its finalize-time merge intojoint_dampingwith the conflict check.The custom attribute was doing double duty as a parsing channel, so the removal re-homes both import paths:
mjc:dampingnow resolves throughSchemaResolverMjcintojoint_damping, like the other direct-mapped MuJoCo attributes (mjc:armature,mjc:frictionloss). It resolves via a newdamping_per_radschema key because MuJoCo authors damping in SI units (per radian for angular DOFs), unlike the USD per-degree convention behind the plaindampingkey. Behavior change:mjc:dampingis no longer parsed implicitly when MuJoCo custom attributes are registered — passschema_resolvers=[..., SchemaResolverMjc()]toModelBuilder.add_usd()when importing MuJoCo-authored USD (noted in the changelog).dampingwas already parsed natively and is unaffected. Ball-jointdamping, however, had only ever reachedjoint_dampingthrough the removed custom-attribute merge — surfaced by the Cassie menagerie comparison (achilles-rod and loop-closure ball joints losing theirdamping="0.01"). The MJCF importer now parses it natively in the ball branch, mirroring itsfrictionlosshandling.The second commit removes the now-orphaned namespace deprecated-alias machinery (
AttributeNamespace.add_deprecated_alias, the alias branches in__getattr__/__setattr__, the namespace-attribute collision guard, and the coupled solver's compaction filter that existed solely to skip alias entries); the removed alias was its only user.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
All pass, including
test_mjc_damping_from_usd_via_schema_resolver(now exercising the resolver path end to end) and theTestMenagerie_AgilityCassiemodel comparison that guards the ball-joint damping fix.Summary by CodeRabbit
damping_per_rad) from USD.dof_passive_dampingalias; usejoint_dampinginstead.mjc:dampingintojoint_damping.