Refactor schema cfgs to separate solver-common from PhysX-specific fields#5275
Conversation
Greptile SummaryThis PR refactors IsaacLab's USD-physics cfg classes into solver-common base classes (
Confidence Score: 4/5Safe to merge with one fix: the deprecated usd_api/physx_api backward-compat accessors on MeshCollisionBaseCfg silently return None for all callers. The two-pass schema routing is well-designed and the configclass fix is correct. The one concrete defect is in MeshCollisionBaseCfg.getattr: both usd_api and physx_api look up _usd_applied_schema in self.dict, but ClassVar attributes are only on the type, not the instance — so every call returns None. Any external or test code that reads cfg.usd_api / cfg.physx_api after upgrading would silently get None instead of the schema name string it expected from the old direct-attribute design. source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py — MeshCollisionBaseCfg.getattr (lines 469-490) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["modify_*_properties(prim, cfg)"] --> B["cfg.to_dict() → cfg_dict\n(strip '_' prefixed keys)"]
B --> C["Pop writer-side fields\n(fix_root_link, ensure_drives_exist,\ndrive_type, max_force, stiffness, damping)"]
C --> D["_apply_namespaced_schemas(prim, cfg, cfg_dict)"]
D --> E["Pass 1: Per-field exceptions\nfor each schema in _usd_field_exceptions"]
E --> F{"Any listed field non-None?"}
F -- "Yes" --> G["AddAppliedSchema(exc_schema)\nwrite field under exc_namespace:attrName"]
F -- "No" --> H["Skip schema; fields already popped"]
G --> I["Pass 2: Per-declaring-class routing\nGroup remaining non-None fields by MRO walk"]
H --> I
I --> J["Read _usd_namespace/_usd_applied_schema\nfrom declaring class __dict__"]
J --> K{"_usd_namespace is None?"}
K -- "Yes" --> L["raise ValueError"]
K -- "No" --> M["AddAppliedSchema if set\nwrite namespace:camelCaseAttr"]
Reviews (2): Last reviewed commit: "Route cfg fields per declaring class; co..." | Re-trigger Greptile |
| @@ -76,6 +81,28 @@ class RigidBodyPropertiesCfg: | |||
| For more information on kinematic bodies, please refer to the `documentation <https://openusd.org/release/wp_rigid_body_physics.html#kinematic-bodies>`_. | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
RigidBodyPropertiesCfg previously exposed 15 public fields including disable_gravity, linear_damping, angular_damping, solver_position_iteration_count, etc. Removing them in a single step without first deprecating them in a prior release breaks any external code that constructs RigidBodyPropertiesCfg(disable_gravity=True, ...) or similar. The same applies to JointDrivePropertiesCfg.max_velocity.
Per AGENTS.md: "Breaking changes require a deprecation first. Do not remove or rename public API symbols without deprecating them in a prior release." The expected flow is to keep the old fields with a DeprecationWarning in __post_init__ first, then remove them in a subsequent release.
| if applied_schema: | ||
| if applied_schema not in rigid_body_prim.GetAppliedSchemas(): | ||
| rigid_body_prim.AddAppliedSchema(applied_schema) | ||
| for attr_name, value in cfg_dict.items(): | ||
| safe_set_attribute_on_usd_prim( | ||
| rigid_body_prim, f"physxRigidBody:{to_camel_case(attr_name, 'cC')}", value, camel_case=False | ||
| rigid_body_prim, f"{namespace}:{to_camel_case(attr_name, 'cC')}", value, camel_case=False | ||
| ) |
There was a problem hiding this comment.
None namespace if subclass omits _usd_namespace
If a future subclass of RigidBodyPropertiesCfg adds fields but forgets to define _usd_namespace, namespace is None and the write loop constructs "None:someAttr" — an invalid prim attribute name — with no error raised. Consider asserting namespace is not None before the write loop.
| ) -> RigidObjectCfg: | ||
| """Create RigidObjectCfg for a shape type.""" | ||
| rigid_props = sim_utils.RigidBodyPropertiesCfg( | ||
| rigid_props = sim_utils.PhysxRigidBodyPropertiesCfg( |
There was a problem hiding this comment.
PhysxRigidBodyPropertiesCfg with PhysX-specific fields
This uses PhysxRigidBodyPropertiesCfg, which applies PhysxRigidBodyAPI and writes physxRigidBody:* attributes that Newton silently ignores. Functionally harmless, but it undercuts the refactoring's goal of isolating solver-specific configuration. A # TODO(newton): comment would help track the planned NewtonRigidBodyPropertiesCfg follow-up.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR cleanly separates solver-common USD Physics properties from PhysX-specific properties in RigidBodyPropertiesCfg and JointDrivePropertiesCfg by introducing a two-level class hierarchy. The base classes retain only solver-agnostic fields, while new PhysxRigidBodyPropertiesCfg and PhysxJointDrivePropertiesCfg subclasses carry PhysX-specific fields plus class-level metadata that drives generic attribute writing. This is a well-motivated refactor that prepares the schema layer for additional solver backends (e.g. MuJoCo/Newton) without modifying the modify functions. The ~90-file migration of consumer call sites is mechanical and correct.
Design Assessment
Design is sound. The metadata-driven approach (_usd_applied_schema, _usd_namespace, _usd_attr_name_map) elegantly eliminates backend-specific branching from the modify functions. Spawner type annotations correctly retain the base class types, preserving polymorphism. The inheritance hierarchy is clean: base classes represent the USD Physics standard, subclasses represent solver-specific extensions. Adding a future backend (e.g. NewtonRigidBodyPropertiesCfg) would only require defining a new subclass with appropriate metadata — no changes to modify_rigid_body_properties or modify_joint_drive_properties.
Findings
🟡 Warning: No guard against None namespace when solver-specific fields remain in cfg_dict — source/isaaclab/isaaclab/sim/schemas/schemas.py:342
In modify_rigid_body_properties, after popping the solver-common fields (rigid_body_enabled, kinematic_enabled), the remaining cfg_dict entries are written using f"{namespace}:{to_camel_case(...)}". If namespace is None (i.e. someone passes a plain RigidBodyPropertiesCfg), the loop is a no-op because the base class has no additional fields — so this is safe today. However, if a future subclass adds fields but forgets to set _usd_namespace, attributes would be written as "None:someAttr", producing a silent malformation rather than a clear error.
A defensive guard would make this future-proof:
if cfg_dict and namespace is None:
raise ValueError(
f"{type(cfg).__name__} has solver-specific fields {list(cfg_dict)} but no _usd_namespace metadata. "
"Subclasses that add fields beyond the base must define _usd_namespace."
)
for attr_name, value in cfg_dict.items():
The same pattern applies in modify_joint_drive_properties (~line 730) where namespace is used in the attr_name_map loop.
🟡 Warning: _validate_joint_drive_properties_on_prim does not skip ensure_drives_exist — source/isaaclab/test/sim/test_schemas.py:431
The test validation iterates joint_cfg.__dict__ and skips _-prefixed names and "func", but ensure_drives_exist is a regular field that is never written to USD (it's popped by the modify function). The validation would attempt to look up drive:{model}:physics:ensureDrivesExist on the prim. This is a pre-existing issue (present on develop), but since this PR touches the skip-list logic in this function, it would be a good opportunity to add it:
if attr_name.startswith("_") or attr_name in ["func", "ensure_drives_exist"]:
🔵 Suggestion: Consider adding _usd_attr_name_map to PhysxRigidBodyPropertiesCfg for symmetry — source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py:98
PhysxRigidBodyPropertiesCfg has _usd_applied_schema and _usd_namespace but no _usd_attr_name_map, while PhysxJointDrivePropertiesCfg has all three. The rigid body modify function uses to_camel_case() directly instead of consulting a map. This works because all rigid body PhysX field names map 1:1 via camelCase conversion, but adding an empty _usd_attr_name_map = {} would make the metadata protocol consistent across both subclass families and document the expectation for future subclass authors.
🔵 Suggestion: Consistent use of .get() vs direct [] access for optional fields in angular conversion — source/isaaclab/isaaclab/sim/schemas/schemas.py:716–724
max_velocity uses cfg_dict.get("max_velocity") (safe for base class where it doesn't exist), while stiffness and damping use cfg_dict["stiffness"] (direct access). Both patterns are correct here since stiffness/damping always exist in the base class, but the inconsistency could confuse readers. A brief comment explaining why .get() is used for max_velocity specifically (it may not exist in the base class) would improve readability.
Test Coverage
- Coverage is adequate for this change type. This is primarily a refactoring PR — the existing
test_schemas.pytests are updated to use the new PhysX subclasses and continue to validate that properties are correctly written to USD prims. - The test fixture now uses
PhysxRigidBodyPropertiesCfgandPhysxJointDrivePropertiesCfg, exercising the full metadata-driven code path. - The validation helpers correctly filter
_-prefixed class metadata attributes. - Gap: No test exercises the base class path (passing a plain
RigidBodyPropertiesCfgwith onlyrigid_body_enabled/kinematic_enabled). This would validate the solver-agnostic path works in isolation, which is important since the motivation is multi-backend support. However, given this is groundwork and no non-PhysX backend exists yet, this is low priority (5/10).
CI Status
✅ labeler — pass
Verdict
Minor fixes needed
This is a well-designed, clean refactor that correctly separates concerns and prepares the codebase for multi-backend solver support. The implementation is sound — the metadata-driven approach eliminates backend branching elegantly, the inheritance hierarchy is appropriate, and the migration is comprehensive. The two warnings are defensive improvements (guarding against None namespace, and skipping ensure_drives_exist in test validation) rather than bugs in the current code. Ship after considering the defensive guards.
| if applied_schema: | ||
| if applied_schema not in rigid_body_prim.GetAppliedSchemas(): | ||
| rigid_body_prim.AddAppliedSchema(applied_schema) | ||
| for attr_name, value in cfg_dict.items(): |
There was a problem hiding this comment.
🟡 Warning: If namespace is None (plain RigidBodyPropertiesCfg passed) but cfg_dict is non-empty (e.g. a future subclass that forgets _usd_namespace), this would write attributes as "None:someAttr" — a silent malformation rather than a clear error.
Currently safe because the base class has no fields beyond the two that are popped above, but a defensive guard would future-proof this:
if cfg_dict and namespace is None:
raise ValueError(
f"{type(cfg).__name__} has solver-specific fields {list(cfg_dict)} "
"but no _usd_namespace metadata."
)Same pattern applies to the attr_name_map loop in modify_joint_drive_properties below.
| # skip names we know are not present | ||
| if attr_name == "func": | ||
| # skip class metadata and names we know are not present | ||
| if attr_name.startswith("_") or attr_name == "func": |
There was a problem hiding this comment.
🟡 Warning: ensure_drives_exist is not skipped here but is never written to USD (it's popped by modify_joint_drive_properties). The validation loop would try to look up drive:{model}:physics:ensureDrivesExist on the prim. Pre-existing on develop, but since this PR touches this skip-list, consider adding it:
| if attr_name.startswith("_") or attr_name == "func": | |
| if attr_name.startswith("_") or attr_name in ["func", "ensure_drives_exist"]: |
53678cd to
0484e95
Compare
a47e67d to
f977a3a
Compare
There was a problem hiding this comment.
Fresh Review of PR #5275 — Schema Refactor
Good refactoring work! The separation of solver-common and PhysX-specific configs is a solid architectural improvement that will help when adding new solver backends like MuJoCo/Newton.
Summary
Overall: This PR is well-structured and addresses a real maintainability concern. The class hierarchy is clean, the deprecation warnings are properly implemented, and the second commit addressed earlier review feedback nicely.
Key observations:
- ✅ Backwards compatibility preserved via deprecated aliases with
DeprecationWarning - ✅ Class-level metadata pattern (
_usd_namespace,_usd_applied_schema) is clean and extensible - ✅ Proper error handling added for missing
_usd_namespace - ✅ Tests updated to skip metadata fields
⚠️ A few minor documentation and consistency issues noted below
See inline comments for specific suggestions.
| the properties and leave the rest as-is. | ||
|
|
||
| .. _PhysxRigidBodyAPI: https://docs.omniverse.nvidia.com/kit/docs/omni_usd_schema_physics/104.2/class_physx_schema_physx_rigid_body_a_p_i.html | ||
| """ |
There was a problem hiding this comment.
📝 Suggestion: The class metadata fields (_usd_applied_schema, _usd_namespace) use the Python convention of underscore-prefixed "private" attributes, but they're actually class-level constants, not instance attributes.
Consider using ClassVar from typing to make this explicit and prevent them from being included in dataclass operations:
from typing import ClassVar
# -- Class metadata (not dataclass fields) --
_usd_applied_schema: ClassVar[str] = "PhysxRigidBodyAPI"
_usd_namespace: ClassVar[str] = "physxRigidBody"This would:
- Make the intent clearer
- Allow type checkers to understand these aren't instance fields
- Eliminate the need for the underscore-prefix filtering in
to_dict()
Not blocking, but would improve code clarity.
| and :class:`PhysxRigidBodyPropertiesCfg` (PhysX-specific). This alias preserves backwards | ||
| compatibility but will be removed in a future release. | ||
| """ | ||
|
|
There was a problem hiding this comment.
🟡 Minor issue: The stacklevel=2 may not point to the correct caller location.
In a typical instantiation like cfg = RigidBodyPropertiesCfg(disable_gravity=True), the stack frames are:
__post_init__(frame 0)__init__generated by@configclass/dataclass (frame 1)- User code calling
RigidBodyPropertiesCfg(...)(frame 2)
So stacklevel=2 should be correct for direct instantiation. However, if there's additional wrapping (e.g., the @configclass decorator adding its own __init__ logic), this might point to the wrong line.
Worth testing that the warning shows the correct source location in practice.
|
|
||
| # convert to dict, filtering out class metadata (underscore-prefixed keys) | ||
| cfg_dict = {k: v for k, v in cfg.to_dict().items() if not k.startswith("_")} | ||
|
|
There was a problem hiding this comment.
✅ Good: The validation added here (if namespace is None: raise ValueError(...)) addresses the earlier review feedback about silent None namespace failures. This is a solid defensive guard.
| applied_schemas = rigid_body_prim.GetAppliedSchemas() | ||
| if "PhysxRigidBodyAPI" not in applied_schemas: | ||
| rigid_body_prim.AddAppliedSchema("PhysxRigidBodyAPI") | ||
|
|
There was a problem hiding this comment.
💡 Observation: The filtering {k: v for k, v in cfg.to_dict().items() if not k.startswith("_")} is done at runtime on every call.
If to_dict() is called frequently (e.g., in hot paths during simulation setup for many prims), this could add up. However, for USD schema operations which are typically one-time setup, this is likely fine.
If performance becomes a concern in the future, consider caching the filtered dict or having to_dict() accept a skip_private=True parameter.
There was a problem hiding this comment.
This should only be ran once.
| cfg_dict["stiffness"] = 1e-3 | ||
|
|
||
| # check if linear drive | ||
| is_linear_drive = prim.IsA(UsdPhysics.PrismaticJoint) |
There was a problem hiding this comment.
📝 Consistency: The comment here says "note: max_velocity uses .get() because it only exists on PhysxJointDrivePropertiesCfg" — this is good documentation!
However, for stiffness and damping (lines 713-717), you use cfg_dict["stiffness"] with direct access. This is correct since these fields exist on JointDriveBaseCfg, but the asymmetry might confuse future maintainers.
Consider adding a brief comment explaining why stiffness/damping use [] (always present on base) vs max_velocity using .get() (only on subclass). The current comment partially covers this but could be clearer.
| Changelog | ||
| --------- | ||
|
|
||
| 4.6.2 (2026-04-15) |
There was a problem hiding this comment.
📝 Nit: The date format 2026-04-15 is fine, but the project seems to use ISO 8601 dates. Just verifying this is intentional and not a placeholder that should be updated before merge.
| """Mass properties.""" | ||
|
|
||
| rigid_props: schemas.RigidBodyPropertiesCfg | None = None | ||
| rigid_props: schemas.RigidBodyBaseCfg | None = None |
There was a problem hiding this comment.
✅ Good API design: Changing the type annotation from RigidBodyPropertiesCfg to RigidBodyBaseCfg allows users to pass either the base class or any PhysX/future-solver subclass. This maintains backwards compatibility while enabling extensibility.
The docstring update to reference RigidBodyBaseCfg.kinematic_enabled is also appropriate.
| If the values are None, they are not modified. This is useful when you want to set only a subset of | ||
| the properties and leave the rest as-is. | ||
|
|
||
| .. _PhysxJointAPI: https://docs.omniverse.nvidia.com/kit/docs/omni_usd_schema_physics/104.2/class_physx_schema_physx_joint_a_p_i.html |
There was a problem hiding this comment.
📝 Suggestion: Similar to PhysxRigidBodyPropertiesCfg, consider using ClassVar type hints for the class metadata:
from typing import ClassVar
_usd_applied_schema: ClassVar[str] = "PhysxJointAPI"
_usd_namespace: ClassVar[str] = "physxJoint"
_usd_attr_name_map: ClassVar[dict[str, str]] = {"max_velocity": "maxJointVelocity"}This would make the code more self-documenting and work better with type checkers.
| for attr_name, attr_value in rigid_cfg.__dict__.items(): | ||
| # skip names we know are not present | ||
| if attr_name in ["func", "rigid_body_enabled", "kinematic_enabled"]: | ||
| # skip class metadata and names we know are not present |
There was a problem hiding this comment.
✅ Good fix: Adding attr_name.startswith("_") to skip class metadata fields is the right approach. This ensures the test validation doesn't try to look up private metadata as USD attributes.
| for attr_name, attr_value in joint_cfg.__dict__.items(): | ||
| # skip names we know are not present | ||
| if attr_name == "func": | ||
| # skip class metadata and names we know are not present on the USD prim |
There was a problem hiding this comment.
✅ Good fix: Adding ensure_drives_exist to the skip list addresses the issue where this field is consumed by the modify function but never written to USD. This was noted in a previous review and correctly fixed here.
kellyguo11
left a comment
There was a problem hiding this comment.
how would this change look like for the asset cfgs? would we need different physx and newton variations of the assets? or somehow pick between the cfgs based on the engine used?
| class RigidBodyPropertiesCfg(PhysxRigidBodyPropertiesCfg): | ||
| """Deprecated: use :class:`PhysxRigidBodyPropertiesCfg` or :class:`RigidBodyBaseCfg`. | ||
|
|
||
| .. deprecated:: 4.6.2 |
There was a problem hiding this comment.
let's use 3.0.0 for the version
AntoineRichard
left a comment
There was a problem hiding this comment.
I think the PR overall is nice, It's great that it work as is without touching any other files. One concern is how user friendly this will be in the long run but at least it's making things explicit.
| applied_schemas = rigid_body_prim.GetAppliedSchemas() | ||
| if "PhysxRigidBodyAPI" not in applied_schemas: | ||
| rigid_body_prim.AddAppliedSchema("PhysxRigidBodyAPI") | ||
|
|
There was a problem hiding this comment.
This should only be ran once.
…elds Introduce RigidBodyBaseCfg and JointDriveBaseCfg as solver-common base classes. PhysX-specific fields move to PhysxRigidBodyPropertiesCfg and PhysxJointDrivePropertiesCfg subclasses that carry _usd_namespace and _usd_applied_schema metadata so modify_rigid_body_properties and modify_joint_drive_properties write attributes generically without backend-specific branching. The old class names (RigidBodyPropertiesCfg, JointDrivePropertiesCfg) are preserved as deprecated aliases that emit DeprecationWarning on instantiation, so existing code continues to work unchanged.
Fix docstring typo: solver_velocity_iteration_count said "position". Skip ensure_drives_exist and underscore-prefixed metadata in test validation helpers. Add comment explaining .get() vs [] for max_velocity in angular unit conversion.
f977a3a to
7aa505d
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This is a follow-up review after the latest commit (7aa505d). The PR cleanly separates solver-common rigid body and joint drive properties from PhysX-specific fields, enabling future solver backends without polluting base configs.
Architecture Impact
Self-contained refactor within the schemas module. Type annotations updated across spawner configs to use base classes (RigidBodyBaseCfg, JointDriveBaseCfg), maintaining polymorphism while allowing backend-specific subclasses.
Implementation Verdict
Ship it — The implementation is clean and addresses the architectural goals well.
Test Coverage
Tests properly updated to skip underscore-prefixed metadata fields. Existing schema validation tests exercise both rigid body and joint drive property paths.
CI Status
✅ labeler passed. Full CI results pending.
Findings
Previous concerns from the earlier review have been addressed — the code now correctly filters metadata fields with not k.startswith("_") pattern, and the test validation functions skip class metadata appropriately.
No new issues found in this commit. The refactor is well-executed and ready to merge.
|
maybe baseCfg lives in core, Physx related schema lives in isaaclab_physx? |
Move PhysxRigidBodyPropertiesCfg, PhysxJointDrivePropertiesCfg, and the deprecated RigidBodyPropertiesCfg / JointDrivePropertiesCfg aliases out of isaaclab.sim.schemas and into isaaclab_physx.sim.schemas. The base classes (RigidBodyBaseCfg, JointDriveBaseCfg) and the metadata-driven modify functions stay in core, so isaaclab remains solver-agnostic. Install a forwarding __getattr__ shim in isaaclab.sim.schemas and isaaclab.sim that lazily resolves the four moved names to their new home on first access. Existing imports continue to work and the deprecation warning still fires from __post_init__. The shim is scheduled for removal in 5.0. Add test_schemas_shim.py covering forward resolution, identity across import paths, deprecation-warning semantics, and dir() listing. Update test_schemas.py to import the new classes directly from isaaclab_physx.sim.schemas. Bump source/isaaclab to 4.6.23 and source/isaaclab_physx to 0.5.28 with matching CHANGELOG entries in both packages.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit 591cf40. Previous concerns were addressed. One new finding in the added shim tests.
Implementation Verdict
Ship it — Minor test improvement suggested below.
Findings
🔵 Improvement: source/isaaclab/test/sim/test_schemas_shim.py:42-45 — Deprecation warning test may be fragile
The test test_deprecated_alias_emits_deprecation_warning catches warnings but doesn't verify the warning message content. If another deprecation warning is emitted from a different source during instantiation, the test would pass incorrectly. Consider asserting on the warning message:
assert any(
issubclass(w.category, DeprecationWarning) and "RigidBodyPropertiesCfg" in str(w.message)
for w in caught
)This is a minor robustness improvement, not a blocker.
The package-level __getattr__ on isaaclab.sim.schemas does not catch direct
imports from the schemas_cfg submodule, e.g.
from isaaclab.sim.schemas.schemas_cfg import RigidBodyPropertiesCfg
which is used by ~30 task env cfgs in isaaclab_tasks. Without a submodule-
level shim, those imports fail with ImportError and break test_env_cfg_no_
forbidden_imports config loading for any env touching them.
Add an analogous __getattr__ to schemas_cfg.py itself that lazily forwards
the four moved names to isaaclab_physx.sim.schemas.schemas_cfg. Extend the
shim regression test to cover the direct-submodule-import path so this
class of regression is caught locally.
|
bot do a full review |
Replace the documented-exception comments and PhysX namespace metadata on the schema-cfg base classes with a uniform _usd_field_exceptions table. The four universal-physics-but-PhysX-namespaced fields (disable_gravity, contact_offset, rest_offset, articulation_enabled, max_velocity) are now declared as per-field exceptions on their base class instead of dragging the PhysX schema metadata onto the base. PhysX subclasses (PhysxRigidBodyPropertiesCfg, PhysxCollisionPropertiesCfg, PhysxArticulationRootPropertiesCfg, PhysxJointDrivePropertiesCfg) now declare their own _usd_namespace and _usd_applied_schema for their additional fields. The per-writer schema-application loop is consolidated into a single _apply_namespaced_schemas helper. modify_articulation_root_properties, modify_rigid_body_properties, modify_collision_properties, modify_joint_drive_properties, modify_mesh_collision_properties, and spawn_rigid_body_material all delegate to the helper after writing their typed-API UsdPhysics fields. The canonical exception-table + main-namespace gating logic now lives in one place. Observable behavior on standard inputs is unchanged (test_schemas.py 38/38).
Adopt the project convention that python snake_case cfg field names map
identity-style to USD camelCase attribute names. The legacy max_velocity
field on JointDriveBaseCfg diverged from this rule (it wrote
physxJoint:maxJointVelocity) and was the only case requiring a
_usd_attr_name_map override.
Renamed the field to max_joint_velocity, which auto-camelCases to the correct
USD attribute name. Added max_velocity as a deprecation alias forwarded via
__post_init__ to the canonical field; emits DeprecationWarning, scheduled for
removal in 5.0. Introduced a _deprecate_field_alias helper for future renames
of the same kind.
With max_velocity gone as a divergence, dropped _usd_attr_name_map from the
helper and all base classes. Collapsed _usd_field_exceptions shape from
dict[applied_schema, (namespace, {field: attr})] to
dict[applied_schema, (namespace, [field, ...])] since attr names are derivable.
Updated tests:
* test_max_velocity_on_base_cfg renamed to test_max_joint_velocity_on_base_cfg.
* Added test_max_velocity_deprecation_alias to validate the alias forwards
and emits DeprecationWarning.
* test_valid_properties_cfg now skips deprecation aliases in the
all-non-None check.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit acb418f. The previous review already approved this PR. Examining the new changes, I see minor additions to changelog files and test coverage improvements. No new issues introduced.
Implementation Verdict
Ship it — Previous approval stands. The changes since the last review are documentation updates (changelog entries) and the code structure remains solid with comprehensive backward compatibility and test coverage.
| """ | ||
|
|
||
|
|
||
| def _apply_namespaced_schemas(prim, cfg, cfg_dict: dict) -> None: |
There was a problem hiding this comment.
Crux of the reworked spawner system. The schema and namespace is now dictated by the class. This function automatically detects the schema and the namespace the parameter belong to and applies it to the asset. This also has a small section on exceptions. The base class carries a dictionary of exceptions - parameters that do not belong to the base class but are present because of an architectural reasons detailed in the exceptional parameters' docstring. Would be able to use team's expertise to vet those exceptions in each base cfg class.
Apply the same snake_case = camelCase convention to the typed-API write path in modify_joint_drive_properties. Renamed JointDriveBaseCfg.max_effort to max_force (matches drive:<axis>:physics:maxForce on UsdPhysics.DriveAPI) with a deprecation alias forwarded via _deprecate_field_alias in __post_init__. The cfg_to_usd_map dict is gone: max_effort -> max_force entry was the only non-trivial mapping; drive_type -> type stays as a single-line carve-out because 'type' is a Python keyword-like name we cannot use as a cfg field. Updated tests: * Renamed all test usages of max_effort= to max_force=. * Added test_max_effort_deprecation_alias mirroring test_max_velocity_deprecation_alias. * test_valid_properties_cfg now treats max_effort as a deprecation alias. * Dropped the dead max_effort branch in the joint-drive validator helper.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit 0e8351e. The new changes since the previous review (acb418f) consist primarily of changelog updates and minor test adjustments. The core refactoring logic remains unchanged and well-structured.
Implementation Verdict
Ship it — The previous approval stands. The new commits add changelog entries documenting the schema cfg refactoring and don't introduce any new concerns. The comprehensive test coverage and backward compatibility shims are in place.
Replace the writer-side typed-API write loops for UsdPhysics standard fields
(rigid_body_enabled, kinematic_enabled, collision_enabled, mass, density,
static/dynamic_friction, restitution) with a single per-declaring-class
routing pass in _apply_namespaced_schemas.
The helper now walks the cfg's MRO to find each field's declaring class and
uses that class's _usd_namespace / _usd_applied_schema for the write. Base
class fields (e.g. RigidBodyBaseCfg.rigid_body_enabled) write under
'physics:*' even when the cfg instance is a PhysX subclass, because the
subclass's _usd_namespace = 'physxRigidBody' applies only to fields it itself
declares -- not to inherited base fields.
Class metadata (_usd_namespace, _usd_applied_schema, _usd_field_exceptions)
moves to ClassVar annotations so dataclass treats them as class-level rather
than instance fields.
Joint drive standard fields (UsdPhysics.DriveAPI) stay on the typed-API path
because the namespace is multi-instance ('drive:linear:physics:*' vs
'drive:angular:physics:*') and resolved at write time. The mesh-collision
approximation token also stays typed because it requires TfToken with
allowedTokens.
Side change in configclass: detect string-form ClassVar annotations under
PEP 563 ('from __future__ import annotations') so _process_mutable_types
does not wrap ClassVar dict/list defaults in field(default_factory=...).
This matches Python stdlib dataclass semantics for string annotations.
Tests pass: test_schemas.py 40/40.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit c31a770. The new changes since the previous review add a max_effort → max_force deprecation alias and rename the canonical field to match USD naming conventions. The changelog entries document the refactoring comprehensively.
Implementation Verdict
Ship it — Previous approval stands. The new deprecation alias for max_effort follows the same well-tested pattern as max_velocity, and the test coverage for the new alias is thorough (test_max_effort_deprecation_alias).
Brief Notes
- The
max_effort→max_forcerename with deprecation forwarding is correctly implemented in_deprecate_field_aliasand tested. - Changelog entries are comprehensive and well-documented.
- No new concerns introduced.
| if name == "usd_api": | ||
| warnings.warn( | ||
| "'usd_api' attribute is deprecated and will be removed in 5.0. Use class-level" | ||
| " metadata via getattr(cfg, '_usd_applied_schema').", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| schema = self.__dict__.get("_usd_applied_schema", None) | ||
| # Every PhysX cooking subclass legacy-mapped to ``"MeshCollisionAPI"``; the base | ||
| # class also wrote that token. Return ``None`` only when no schema is declared. | ||
| return "MeshCollisionAPI" if schema is not None else None | ||
| if name == "physx_api": | ||
| warnings.warn( | ||
| "'physx_api' attribute is deprecated and will be removed in 5.0. Use class-level" | ||
| " metadata via getattr(cfg, '_usd_applied_schema').", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| schema = self.__dict__.get("_usd_applied_schema", None) | ||
| if schema and schema.startswith("Physx"): | ||
| return schema | ||
| return None |
There was a problem hiding this comment.
usd_api and physx_api backward-compat accessors always return None because _usd_applied_schema is a ClassVar — it lives in the class __dict__, not the instance __dict__. Any legacy code that calls cfg_instance.usd_api or cfg_instance.physx_api will silently receive None instead of the expected schema string (e.g. "MeshCollisionAPI" for the base, "PhysxConvexHullCollisionAPI" for the hull subclass). The fix is to look up the attribute on the type, not the instance.
| if name == "usd_api": | |
| warnings.warn( | |
| "'usd_api' attribute is deprecated and will be removed in 5.0. Use class-level" | |
| " metadata via getattr(cfg, '_usd_applied_schema').", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| schema = self.__dict__.get("_usd_applied_schema", None) | |
| # Every PhysX cooking subclass legacy-mapped to ``"MeshCollisionAPI"``; the base | |
| # class also wrote that token. Return ``None`` only when no schema is declared. | |
| return "MeshCollisionAPI" if schema is not None else None | |
| if name == "physx_api": | |
| warnings.warn( | |
| "'physx_api' attribute is deprecated and will be removed in 5.0. Use class-level" | |
| " metadata via getattr(cfg, '_usd_applied_schema').", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| schema = self.__dict__.get("_usd_applied_schema", None) | |
| if schema and schema.startswith("Physx"): | |
| return schema | |
| return None | |
| if name == "usd_api": | |
| warnings.warn( | |
| "'usd_api' attribute is deprecated and will be removed in 5.0. Use class-level" | |
| " metadata via getattr(cfg, '_usd_applied_schema').", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| schema = getattr(type(self), "_usd_applied_schema", None) | |
| # Every PhysX cooking subclass legacy-mapped to ``"MeshCollisionAPI"``; the base | |
| # class also wrote that token. Return ``None`` only when no schema is declared. | |
| return "MeshCollisionAPI" if schema is not None else None | |
| if name == "physx_api": | |
| warnings.warn( | |
| "'physx_api' attribute is deprecated and will be removed in 5.0. Use class-level" | |
| " metadata via getattr(cfg, '_usd_applied_schema').", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| schema = getattr(type(self), "_usd_applied_schema", None) | |
| if schema and schema.startswith("Physx"): | |
| return schema | |
| return None |
There was a problem hiding this comment.
Did a careful walk through the placement layer and the writer/helper with a agent. Most of it lands cleanly against the consumption-gated rule — the per-field audit comes out matching for ~16 of the 18 cases I checked, and the "don't stamp PhysX schemas onto Newton-targeted prims" invariant has solid test coverage (test_*_no_physx_schema_when_*). Two placement calls worth confirming, and a handful of framework-shape ideas that I think would make the cfg layer even nicer to live with.
Two placement calls
disable_gravity on RigidBodyBaseCfg. The strict reading of the consumption-gated rule would push this to the PhysX subclass. Newton's importer reads physxRigidBody:disableGravity from the scene prim (newton/_src/utils/import_usd.py:1655, prim_type=PrimType.SCENE) and uses it to drive scene-wide gravity — so a Newton user who writes RigidBodyBaseCfg(disable_gravity=True) per-body either no-ops or globally toggles, depending on which prim the importer happens to consult. The field's docstring is honest about this, which I genuinely appreciate, but the user-facing API will mislead Newton users today. Two options: keep on the base with the documented caveat, or move to PhysxRigidBodyPropertiesCfg until Newton ships per-body kernel support ("not yet supported on Newton" is a cleaner failure mode than "supported but lies"). I lean toward keeping it where it is, but would like a sanity check from someone with Newton-kernel context.
enabled_self_collisions on PhysxArticulationRootPropertiesCfg. This is a divergence from the rule as written. Newton has a native newton:selfCollisionEnabled (resolver checks it first) and consumes it per-articulation at import_usd.py:2294-2299. The PR introduces a separately stated "Rule 2" — when both backends have native namespaces, place on the backend-specific subclass and let a future NewtonArticulationRootPropertiesCfg carry the Newton side. That framing is internally consistent and forward-looking, but it's a real reframing, and it sets a precedent for future dual-namespace fields. Worth a moment to confirm this is the intended evolution.
Framework-shape ideas
None of these are blockers — the PR works. But the cfg layer carries some machinery I think we can lean out, and the deprecation boilerplate is repeating itself enough that I think it deserves a small framework lift.
1. Move routing onto the field via Annotated
Intent: the cfg body should read like a schema definition, not a mix of class-level metadata and fields. Per-field exceptions live on the field, the class default lives in a decorator above the class, and the body becomes 100% field declarations. Field renames stop desyncing from a separate string-keyed registry, and the MRO walk in _apply_namespaced_schemas (a workaround for "subclass _usd_namespace shouldn't apply to base fields") goes away.
Before (source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py:1325):
class RigidBodyBaseCfg:
_usd_namespace: ClassVar[str | None] = "physics"
_usd_applied_schema: ClassVar[str | None] = None
_usd_field_exceptions: ClassVar[dict] = {
"PhysxRigidBodyAPI": ("physxRigidBody", ["disable_gravity"]),
}
rigid_body_enabled: bool | None = None
kinematic_enabled: bool | None = None
disable_gravity: bool | None = NoneAfter:
@usd_route(namespace="physics")
@configclass
class RigidBodyBaseCfg:
rigid_body_enabled: bool | None = None
kinematic_enabled: bool | None = None
disable_gravity: Annotated[
bool | None,
Route("physxRigidBody", "PhysxRigidBodyAPI"),
] = NonePhysX subclasses compose the same way — decorator override for the subclass's own fields, inherited base routing untouched:
@usd_route(namespace="physxRigidBody", applied_schema="PhysxRigidBodyAPI")
@configclass
class PhysxRigidBodyPropertiesCfg(RigidBodyBaseCfg):
linear_damping: float | None = None
angular_damping: float | None = None
# ...The decorator is two lines:
def usd_route(*, namespace: str | None = None, applied_schema: str | None = None):
def _apply(cls):
cls._usd_class_route = Route(namespace, applied_schema)
return cls
return _applyResolved via get_type_hints(cls, include_extras=True), cached on the class. The triplet (_usd_namespace, _usd_applied_schema, _usd_field_exceptions) collapses to one decorator + per-field Annotated markers; _usd_field_exceptions disappears entirely. Classes that route nothing at the class level (e.g. JointDriveBaseCfg — every field is either typed-UsdPhysics.DriveAPI or Annotated-routed) just omit the decorator. Same Annotated machinery powers idea #3, and @usd_route composes cleanly with the @deprecated_subclass factory in idea #2.
2. Hoist class-deprecation boilerplate out of cfg files
Intent: cfg classes go back to being pure data declarations, no def in user-facing class bodies. Single source of truth for the deprecation list lives next to the writer that consumes it. Side benefit: messages don't drift (the current PR has both "scheduled for removal in 5.0" and "and will be removed in 5.0" wordings).
Before (source/isaaclab_physx/isaaclab_physx/sim/schemas/schemas_cfg.py:3585, ×14):
@configclass
class RigidBodyPropertiesCfg(PhysxRigidBodyPropertiesCfg):
"""Deprecated: ..."""
def __post_init__(self):
warnings.warn(
"'RigidBodyPropertiesCfg' is deprecated and will be removed in 5.0. Use ...",
DeprecationWarning, stacklevel=2,
)
super().__post_init__()After — empty cfg + registry next to the writer:
class RigidBodyPropertiesCfg(PhysxRigidBodyPropertiesCfg):
pass# in schemas.py
DEPRECATED_CFGS: dict[type, tuple[str, str]] = {
RigidBodyPropertiesCfg: ("PhysxRigidBodyPropertiesCfg | RigidBodyBaseCfg", "5.0"),
JointDrivePropertiesCfg: ("PhysxJointDrivePropertiesCfg | JointDriveBaseCfg", "5.0"),
# ... 12 more
}
def _warn_if_deprecated(cfg) -> None:
entry = DEPRECATED_CFGS.get(type(cfg))
if entry is None:
return
repl, removed = entry
warnings.warn(
f"{type(cfg).__name__!r} is deprecated and will be removed in {removed}. Use {repl} instead.",
DeprecationWarning, stacklevel=3,
)Each writer calls _warn_if_deprecated(cfg) at the top. Warnings fire on use rather than instantiation, which means config-loading-at-import stays quiet, and simplefilter("once") dedupes naturally. ~110 lines of repeated boilerplate become ~14 dict entries.
3. Field aliases on JointDriveBaseCfg — same trick
Intent: the only __post_init__ on a non-deprecated cfg, expressed as field-level data so the cfg has zero def. The alias rule self-documents next to the field it aliases.
Before (source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py:1519):
@configclass
class JointDriveBaseCfg:
def __post_init__(self):
_deprecate_field_alias(self, "max_velocity", "max_joint_velocity")
_deprecate_field_alias(self, "max_effort", "max_force")
max_force: float | None = None
max_effort: float | None = None
max_joint_velocity: float | None = None
max_velocity: float | None = NoneAfter:
@configclass
class JointDriveBaseCfg:
max_force: float | None = None
max_effort: Annotated[float | None, DeprecatedField(canonical="max_force")] = None
max_joint_velocity: float | None = None
max_velocity: Annotated[float | None, DeprecatedField(canonical="max_joint_velocity")] = NoneA _resolve_field_aliases(cfg) helper in schemas.py walks the markers, warns, and forwards old → new before the routing pass. Same Annotated infrastructure as #1.
4. Ask the schema registry for the Sdf type instead of guessing
Intent: the schema already knows the right wire type — just ask it. Removes the Python-value isinstance ladder and incidentally fixes the latent TfToken bug for combine-mode fields. Pre-existing on main, so the PR doesn't introduce it, but this feels like the right moment.
Before (source/isaaclab/isaaclab/sim/utils/prims.py:355):
if isinstance(value, bool): sdf_type = Sdf.ValueTypeNames.Bool
elif isinstance(value, int): sdf_type = Sdf.ValueTypeNames.Int
elif isinstance(value, float): sdf_type = Sdf.ValueTypeNames.Float
elif isinstance(value, str): sdf_type = Sdf.ValueTypeNames.String # ← combine modes land herephysxMaterial:frictionCombineMode and restitutionCombineMode are TfTokens with allowedTokens; the Literal[str] falls into the String branch.
After:
@functools.cache
def _attr_type(applied_schema: str, attr: str) -> Sdf.ValueTypeName:
spec = Usd.SchemaRegistry().FindAppliedAPIPrimDefinition(applied_schema)
return spec.GetSchemaAttributeSpec(attr).typeNameCached per (schema, attr). Future schemas with float3, asset paths, etc. work without expanding the ladder.
Small things alongside
MeshCollisionBaseCfg.__getattr__forusd_api/physx_api(schemas_cfg.py:1711): the dispatch that consumed these is gone. Consider deleting the shim —AttributeErrorwith a docstring pointer to_usd_applied_schemareads cleaner than a working-but-deprecated accessor that reconstructs a string. Also removes theself.__dict__.get(...)ClassVar lookup that only works because_custom_post_initdeepcopies ClassVars into instance dicts — fragile to read without configclass internals in mind.MeshCollisionBaseCfg._usd_attr_name_map: ClassVar[dict] = {}(schemas_cfg.py:1677) is declared but never read.- Worth one explicit
Changedchangelog line: the newPhysxRigidBodyMaterialCfgdefaultscompliant_contact_*/*_combine_modetoNone, where the legacy class had concrete defaults. Probably no observable change because PhysX engine defaults match, but the shift from "always authored" to "authored only when user sets it" is meaningful enough to call out. - PR description's
PhysxRigidBodyMaterialCfgfield table mentionsimprove_patch_friction, but I don't see the field in the code. cfg_dict = {k: v for k, v in cfg.to_dict().items() if not k.startswith("_")}appears in 6 writers — replacing with adataclasses.fields(cfg)walk drops the underscore filter and the recursiveto_dict()deepcopy. Pairs with #1.
If only one of the framework lifts lands, I'd push for #1 — it's the foundation that makes #3 cheap and removes the rename hazard structurally. #2 is the largest readability win at ~110 lines of cfg-file boilerplate.
Overall this is solid work — the consumption-gating tests are exactly the invariant I'd want to see covered, and the architecture is honest about its trade-offs in docstrings. Happy to see it merge once the two placement calls land.
* Replace 7 occurrences of `cfg_dict = {k: v for k, v in cfg.to_dict().items()
if not k.startswith("_")}` with `{f.name: getattr(cfg, f.name) for f in
dataclasses.fields(cfg)}`. The configclass `to_dict()` recursively deepcopies
nested cfg objects and surfaces `_usd_*` ClassVar metadata that the writers
then filter out; `dataclasses.fields(cfg)` is the typed source-of-truth for
the cfg's writable fields and skips ClassVar entries by construction.
Spawner-side `func` field is excluded explicitly in the material spawner.
* Add a `Changed` changelog line documenting that
`PhysxRigidBodyMaterialCfg` defaults `compliant_contact_*` and
`*_combine_mode` to None instead of the legacy concrete values
("average" / 0.0). PhysX engine defaults match the previous concrete
values, so simulation behavior is unchanged; the difference is consumption-
gated authoring on the prim.
8cb5198 to
e0312cc
Compare
|
On #1 — #3 — same concern as #1: same #4 — agree, right call. Pre-existing on main and fixes the latent TfToken bug for The cfg_dict → |
Per the new fragment-per-PR changelog workflow (PR isaac-sim#5434): <slug>.minor.rst signals a minor version bump. This PR adds new public API (RigidBodyBaseCfg, CollisionBaseCfg, ArticulationRootBaseCfg, JointDriveBaseCfg, RigidBodyMaterialBaseCfg, and PhysX subclasses in isaaclab_physx) and deprecates existing names -- minor bump for both isaaclab and isaaclab_physx.
Per PR isaac-sim#5434 (towncrier-style fragment workflow), CHANGELOG.rst and extension.toml are now owned by the nightly CI job. Human edits to these files are replaced by fragment files under changelog.d/. Reverts the direct edits to develop-branch state; the three .minor.rst fragments carry the changelog content instead.
fbde2f9 to
2aaef24
Compare
Test-only change (max_effort -> max_force rename in test_articulation.py); no user-facing changelog entry needed.
ca9122e to
fbde2f9
Compare
Adds MuJoCo (Newton MuJoCo kernel) and Newton-native schema cfg classes to isaaclab_newton.sim.schemas using the framework from isaac-sim#5275: * MujocoRigidBodyPropertiesCfg (gravcomp -> mjc:gravcomp) * MujocoJointDrivePropertiesCfg (actuatorgravcomp -> mjc:actuatorgravcomp via MjcJointAPI) * NewtonCollisionPropertiesCfg (contact_margin, contact_gap via NewtonCollisionAPI) * NewtonMeshCollisionPropertiesCfg (max_hull_vertices via NewtonMeshCollisionAPI) * NewtonMaterialPropertiesCfg (torsional_friction, rolling_friction via NewtonMaterialAPI) * NewtonArticulationRootPropertiesCfg (self_collision_enabled via NewtonArticulationRootAPI) All cfgs follow the per-declaring-class MRO routing from isaac-sim#5275 with ClassVar metadata. Field names follow the snake_case = camelCase convention; old names (gravity_compensation_scale, gravity_compensation) kept as deprecation aliases forwarded via __post_init__. Forwarding shims on isaaclab.sim.schemas preserve existing imports. Spawner auto-enables body-level gravcomp when joint-level actuatorgravcomp is requested without it.
) # Description Adds Newton-native and MuJoCo-specific schema cfg classes to `isaaclab_newton.sim.schemas`, following the base/subclass framework from #5275. All new cfgs use the per-declaring-class MRO routing in `_apply_namespaced_schemas` — no backend-specific branching in any writer. Depends on #5275. ## New cfgs ### MuJoCo (Newton MuJoCo kernel, `mjc:*` namespace) | Class | Field | USD attribute | Applied schema | |---|---|---|---| | `MujocoRigidBodyPropertiesCfg` | `gravcomp` | `mjc:gravcomp` | None (raw attr) | | `MujocoJointDrivePropertiesCfg` | `actuatorgravcomp` | `mjc:actuatorgravcomp` | `MjcJointAPI` | Body-level `gravcomp` must be set for joint-level `actuatorgravcomp` to have any effect. The spawner auto-enables `MujocoRigidBodyPropertiesCfg(gravcomp=1.0)` when joint-level actuator gravcomp is requested without body-level gravcomp. ### Newton-native (`newton:*` namespace) | Class | Fields | USD attributes | Applied schema | |---|---|---|---| | `NewtonCollisionPropertiesCfg` | `contact_margin`, `contact_gap` | `newton:contactMargin`, `newton:contactGap` | `NewtonCollisionAPI` | | `NewtonMeshCollisionPropertiesCfg` | `max_hull_vertices` | `newton:maxHullVertices` | `NewtonMeshCollisionAPI` | | `NewtonMaterialPropertiesCfg` | `torsional_friction`, `rolling_friction` | `newton:torsionalFriction`, `newton:rollingFriction` | `NewtonMaterialAPI` | | `NewtonArticulationRootPropertiesCfg` | `self_collision_enabled` | `newton:selfCollisionEnabled` | `NewtonArticulationRootAPI` | ## Design constraints Same single-cfg-per-spawner-slot rule as #5275. Newton cfgs subclass the same base classes as PhysX cfgs; each declares `_usd_namespace`/`_usd_applied_schema` (ClassVar) and fields that auto-camelCase to their USD attr names. Per-declaring-class MRO routing handles mixed PhysX+Newton cfg hierarchies correctly. ## Field renames (with deprecation aliases through 5.0) | Old | New | Reason | |---|---|---| | `gravity_compensation_scale` | `gravcomp` | Single word identity: `gravcomp` → `mjc:gravcomp` | | `gravity_compensation` | `actuatorgravcomp` | Single word identity: `actuatorgravcomp` → `mjc:actuatorgravcomp` | ## Type of change - New feature (non-breaking) Forwarding shims on `isaaclab.sim.schemas` keep existing imports working. Deprecation aliases keep old field names working through 5.0. ## Test plan - [x] MuJoCo tests: `mjc:gravcomp` / `mjc:actuatorgravcomp` written when set, not written when None - [x] Newton collision, material, articulation-root: attrs written, schemas applied only when non-None - [x] Deprecation alias tests for renamed fields - [x] `test_schemas.py` 46/46 pass — no regressions - [x] Pre-commit clean ## Supersedes Together with #5275, supersedes #4847 and #5203. --------- Co-authored-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Description
Splits IsaacLab's USD-physics cfg classes into solver-common base classes and backend-specific subclasses, and refactors the writers (
modify_*_properties,spawn_rigid_body_material) so that schema application is data-driven rather than hard-coded per-class. Prepares the schema layer for multi-backend support (PhysX today, Newton/Mjc next) without polluting base classes with silently-ignored fields or stamping backend-specific schemas onto prims that didn't opt in.Architecture
Two layered concepts:
physics:*; subclass fields write under their own namespace (physxRigidBody:*, etc.). When aPhysxRigidBodyPropertiesCfginstance is written, base fields still go underphysics:*because_usd_namespaceis read from the declaring class via__dict__, not viagetattr(which would hit the subclass override).disable_gravityonly exists atphysxRigidBody:disableGravity). These are declared as_usd_field_exceptions = {applied_schema: (namespace, [fields...])}on the base class; the writer applies the exception schema only when one of the listed fields is non-None.The single helper
_apply_namespaced_schemas(prim, cfg, cfg_dict)inschemas.pydoes both passes for every writer (rigid body, collision, articulation root, joint drive, mesh collision, rigid-body material).Design constraints
One cfg class per spawner slot. Spawners (
UsdFileCfg,MeshCuboidCfg, etc.) carry a single field for each property group:rigid_props: RigidBodyBaseCfg | None,collision_props: CollisionBaseCfg | None,joint_drive_props: JointDriveBaseCfg | None, etc. The user cannot pass two cfgs into the same slot, so the cfg class hierarchy must be single-rooted per spawner field — one base class per group, with backend-specific subclasses below.This rules out a "PhysX cfg sits next to a Newton cfg as siblings" design and drives several placement decisions:
RigidBodyBaseCfg(disable_gravity=True)without importingisaaclab_physx.disable_gravity)_usd_field_exceptions. The base stays backend-clean; the writer dispatches the PhysX write only when the field is non-None._usd_namespace,_usd_applied_schema,_usd_field_exceptions) drives behavior; the writer is a pure data interpreter._usd_namespace/_usd_applied_schema. No spawner-side changes, no writer-side changes, no base-cfg-side changes.NewtonArticulationRootPropertiesCfgwill own the same conceptual field on the Newton side. ("Rule 2" — e.g.,enabled_self_collisions.)_usd_field_exceptionsentry. ("Rule 1" — e.g.,disable_gravity,articulation_enabled,contact_offset,rest_offset,max_joint_velocity.) When Newton ships its own native attribute, the exception namespace switches transparently with no API change.Field placement
Base (solver-common) classes —
physics:*namespace viaUsdPhysics.*APIRigidBodyBaseCfgrigid_body_enabledphysics:rigidBodyEnabledRigidBodyBaseCfgkinematic_enabledphysics:kinematicEnabledCollisionBaseCfgcollision_enabledphysics:collisionEnabledMassPropertiesCfgmassphysics:massMassPropertiesCfgdensityphysics:densityRigidBodyMaterialBaseCfgstatic_frictionphysics:staticFrictionRigidBodyMaterialBaseCfgdynamic_frictionphysics:dynamicFrictionRigidBodyMaterialBaseCfgrestitutionphysics:restitutionJointDriveBaseCfgdrive_typedrive:<axis>:physics:typeJointDriveBaseCfgmax_forcedrive:<axis>:physics:maxForceJointDriveBaseCfgstiffnessdrive:<axis>:physics:stiffnessJointDriveBaseCfgdampingdrive:<axis>:physics:dampingMeshCollisionBaseCfgmesh_approximation_namephysics:approximation(token)ArticulationRootBaseCfgfix_root_linkUsdPhysics.FixedJoint)JointDriveBaseCfgandMeshCollisionBaseCfguse the typedUsdPhysics.DriveAPI/UsdPhysics.MeshCollisionAPIaccessors at the writer level (multi-instance namespace andTfTokenwithallowedTokens, respectively); all other base fields flow through the helper's per-class routing.PhysX subclasses —
physx*:*namespaces,Physx*APIschemas_usd_namespace_usd_applied_schemaPhysxRigidBodyPropertiesCfgphysxRigidBodyPhysxRigidBodyAPIlinear_damping,angular_damping,max_linear_velocity,max_angular_velocity,max_depenetration_velocity,max_contact_impulse,enable_gyroscopic_forces,retain_accelerations, solver iter counts, sleep / stabilization thresholdsPhysxCollisionPropertiesCfgphysxCollisionPhysxCollisionAPItorsional_patch_radius,min_torsional_patch_radiusPhysxArticulationRootPropertiesCfgphysxArticulationPhysxArticulationAPIenabled_self_collisions, solver iter counts, sleep / stabilization thresholdsPhysxJointDrivePropertiesCfgphysxJointPhysxJointAPIPhysxRigidBodyMaterialCfgphysxMaterialPhysxMaterialAPIcompliant_contact_stiffness,compliant_contact_damping,friction_combine_mode,restitution_combine_modePhysxConvexHullPropertiesCfgphysxConvexHullCollisionPhysxConvexHullCollisionAPIhull_vertex_limit,min_thicknessPhysxConvexDecompositionPropertiesCfgphysxConvexDecompositionCollisionPhysxConvexDecompositionCollisionAPIPhysxTriangleMeshPropertiesCfgphysxTriangleMeshCollisionPhysxTriangleMeshCollisionAPIweld_tolerancePhysxTriangleMeshSimplificationPropertiesCfgphysxTriangleMeshSimplificationCollisionPhysxTriangleMeshSimplificationCollisionAPIsimplification_metric,weld_tolerancePhysxSDFMeshPropertiesCfgphysxSDFMeshCollisionPhysxSDFMeshCollisionAPIsdf_margin,sdf_narrow_band_thickness,sdf_resolution, etc._usd_field_exceptionstableThese fields are declared on a base class but the only USD path today goes through a non-base namespace. Each entry says: "if any listed field on this cfg is non-None, apply the exception schema and write that one attribute under the exception namespace." All other fields on the cfg follow the per-declaring-class routing rule.
RigidBodyBaseCfgPhysxRigidBodyAPIphysxRigidBodydisable_gravityCollisionBaseCfgPhysxCollisionAPIphysxCollisioncontact_offset,rest_offsetModel.shape_collision_radius/_thickness(import_usd.py:2104, 2111)ArticulationRootBaseCfgPhysxArticulationAPIphysxArticulationarticulation_enabledrigid_object.py:1035. Universal user-facing intentJointDriveBaseCfgPhysxJointAPIphysxJointmax_joint_velocityModel.joint_velocity_limitin Newton (nonewton:*equivalent today). The exception namespace switches transparently when Newton shipsnewton:maxJointVelocityas a registered applied APIWhen any exception field is non-None, the corresponding
Physx*APIschema is applied to the prim. When all exception fields are None, no PhysX schema is stamped — Newton-targeted prims authored from*BaseCfgstay free of PhysX schemas they didn't opt in to.Field renames (with deprecation aliases)
To enforce the convention that python
snake_casecfg field names map identity-style to USDcamelCaseattribute names, two legacy fields were renamed. Both keep the old name as a deprecation alias forwarded via__post_init__(emitsDeprecationWarning, scheduled for removal in 5.0).JointDriveBaseCfg.max_velocitymax_joint_velocityphysxJoint:maxJointVelocityJointDriveBaseCfg.max_effortmax_forcedrive:<axis>:physics:maxForceType of change
The split is non-breaking at the spawner-cfg level — every base-class type accepts any subclass via polymorphism, and every legacy
RigidBodyPropertiesCfg/JointDrivePropertiesCfg/CollisionPropertiesCfg/ArticulationRootPropertiesCfg/MeshCollisionPropertiesCfg/RigidBodyMaterialCfg/FixedTendonPropertiesCfg/SpatialTendonPropertiesCfgimport path continues to work via deprecation-alias subclasses and__getattr__shims onisaaclab.sim,isaaclab.sim.schemas, andisaaclab.sim.schemas.schemas_cfg. Direct attribute access to the renamed fields still works through deprecation aliases. Removal scheduled for 5.0.The breaking aspect: cfg classes in
isaaclab_physx.sim.schemasandisaaclab_physx.sim.spawners.materialsare physically relocated. Anyone importing from internal paths (rather thanisaaclab.sim) needs to update.Migration
Spawner type annotations remain unchanged — they accept any subclass via polymorphism.
Internal helper
Used by all five
modify_*_propertieswriters andspawn_rigid_body_material. Replaced ~125 lines of duplicated gating logic with a single ~30-line helper.Side change: configclass
source/isaaclab/isaaclab/utils/configclass.py:_process_mutable_typesnow detects string-formClassVarannotations under PEP 563 (from __future__ import annotations) so it doesn't wrapClassVar[dict]defaults infield(default_factory=...). Matches Python stdlibdataclassessemantics. No pre-existing IsaacLab class usedClassVarinside a@configclassblock, so the change has no effect on existing code; it enables theClassVarmetadata pattern this PR introduces.Test plan
test_schemas.py(38 → 40 tests): all schema-cfg classes write correct attributes under the right namespace; PhysX schemas are NOT applied when only base/UsdPhysics fields are set; deprecation aliases (max_velocity→max_joint_velocity,max_effort→max_force) forward correctly and emitDeprecationWarning. 40 passed.test_schemas_shim.py: legacy import paths (isaaclab.sim.schemas.RigidBodyPropertiesCfgetc.) resolve via__getattr__shims. All passing.test_articulation.py,test_rigid_object_iface.py,test_valid_configs.py,test_spawn_*— no regressions../isaaclab.sh -t): 8768/9205 pass, 437 unrelated baseline failures (rendering,omni.physics.tensors.apimissing, OSC controller,install_ci,pyglet, Newton env-path, Anymal-C determinism). Zero new regressions; +123 passing tests vs. earlier state../isaaclab.sh -f(pre-commit) clean.Supersedes
Together with #5276, supersedes #4847 and #5203 with a cleaner schema-layer design.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/isaaclab/changelog.d/)config/extension.tomlfileCONTRIBUTORS.mdor my name already exists there