Skip to content

feat(schemas): rigid-body schema-fragment API + migration (Plan 1)#5975

Closed
vidurv-nvidia wants to merge 1 commit into
isaac-sim:developfrom
vidurv-nvidia:vidurv/schema-fragments
Closed

feat(schemas): rigid-body schema-fragment API + migration (Plan 1)#5975
vidurv-nvidia wants to merge 1 commit into
isaac-sim:developfrom
vidurv-nvidia:vidurv/schema-fragments

Conversation

@vidurv-nvidia

Copy link
Copy Markdown

Summary

First slice of the physics schema-cfg refactor: replace the inheritance-based rigid-body schema config classes with single-namespace fragments so a prim can carry rigid-body properties from multiple USD namespaces (physics:, physxRigidBody:, mjc:) at once, with core naming no backend.

Framework (core):

  • SchemaFragment base + RigidBodyFragment marker + UsdPhysicsRigidBodyCfg, each carrying _usd_namespace / _usd_applied_schema metadata and a string-resolved func applier.
  • apply_namespaced (generic per-fragment writer) and apply_rigid_body_properties (applies a fragment list with UsdPhysics.RigidBodyAPI as the implicit anchor).
  • Spawner rigid_props slot now accepts a fragment list; legacy single cfgs keep working through a transition bridge in the shapes/meshes/from_files/mesh_converter writers.

Backends:

  • isaaclab_physx: add PhysxRigidBodyCfg; RigidBodyPropertiesCfg becomes a deprecation factory returning the equivalent fragment list.
  • isaaclab_newton: add MujocoRigidBodyCfg (mjc:gravcomp).

Migration: all RigidBodyPropertiesCfg usages across isaaclab_assets, isaaclab_tasks, isaaclab_mimic and the test suites migrated to fragment lists, with consumer sites that mutated/read rigid_props as a single object updated (disable_gravity mutations, .replace(), attribute reads).

This is Plan 1 of a staged refactor (collision / mass / joint-drive / articulation-root / mesh-collision / tendons / deformables / materials / codegen follow in later PRs). Closes the backend isinstance leak in from_files.py.

Test Plan

  • test_schema_fragments.py (new) — 8 passed
  • test_schemas.py — 39 passed
  • test_schemas_shim.py (deprecation/forwarding contract) — 145 passed
  • test_spawn_shapes.py — 12 passed
  • test_spawn_meshes.py — 10 passed
  • pre-commit (ruff, codespell, insert-license, rst, …) clean
  • ./isaaclab.sh -d docs build clean (no warnings on new API)
  • test_spawn_from_files.py (pulls Nucleus assets; run in CI)
  • full task/training regression in CI

Notes

  • Breaking: RigidBodyPropertiesCfg no longer returns a single cfg object — pass a fragment list; code mutating attributes on it must set the field on the PhysxRigidBodyCfg fragment. Deprecated, removal in 5.0.
  • Draft: opening for review of the fragment API shape before the remaining families are migrated.

Replace the inheritance-based rigid-body physics schema config classes
with single-namespace "fragments" that a prim can carry from multiple USD
namespaces at once.

Framework (core):
- Add SchemaFragment base, the RigidBodyFragment marker, and
  UsdPhysicsRigidBodyCfg, each carrying _usd_namespace / _usd_applied_schema
  metadata plus a string-resolved func applier.
- Add apply_namespaced (generic per-fragment writer) and
  apply_rigid_body_properties (applies a fragment list with
  UsdPhysics.RigidBodyAPI as the implicit anchor).
- Widen the spawner rigid_props slot to accept a fragment list and route
  it through the new path; legacy single cfgs keep working via a
  transition bridge in the shape/mesh/from_files/mesh_converter writers.

Backends:
- isaaclab_physx: add PhysxRigidBodyCfg; turn RigidBodyPropertiesCfg into
  a deprecation factory returning the equivalent fragment list.
- isaaclab_newton: add MujocoRigidBodyCfg (mjc:gravcomp).

Migration:
- Migrate every RigidBodyPropertiesCfg construction across isaaclab_assets,
  isaaclab_tasks, isaaclab_mimic and the test suites to fragment lists, and
  update the consumer sites that mutated/read rigid_props as a single object
  (disable_gravity mutations, .replace(), attribute reads).

why: lets a prim compose properties from multiple backends without core
naming any backend, and closes the backend isinstance leak in from_files.
@github-actions github-actions Bot added asset New asset feature or request isaac-mimic Related to Isaac Mimic team labels Jun 4, 2026

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Summary

This PR introduces a schema-fragment API for rigid-body physics properties — the first slice (Plan 1) of a staged refactor. It replaces the inheritance-based RigidBodyPropertiesCfg with composable, single-namespace fragments so a prim can carry rigid-body properties from multiple USD namespaces (physics:, physxRigidBody:, mjc:) without core naming any backend.

Key changes:

  • New SchemaFragment base class with _usd_namespace / _usd_applied_schema class metadata
  • RigidBodyFragment marker + UsdPhysicsRigidBodyCfg (core), PhysxRigidBodyCfg (isaaclab_physx), MujocoRigidBodyCfg (isaaclab_newton)
  • apply_namespaced generic writer + apply_rigid_body_properties family dispatcher
  • RigidBodyPropertiesCfg → deprecation factory returning fragment list
  • Transition routing in all spawners (shapes, meshes, from_files, mesh_converter)
  • Full migration of 129 files across assets, tasks, and tests

Architecture verdict: Clean separation of concerns. Core defines the protocol; backends provide their fragments. The func string-resolved dispatch avoids core→backend imports.


Findings

1. ⚠️ StopIteration risk from unguarded next() calls (Warning)

Files: isaaclab_assets/robots/franka.py, test_operational_space.py, test_differential_ik.py, and 12+ other locations

The migration introduces a fragile pattern for mutating disable_gravity:

next(f for f in robot_cfg.spawn.rigid_props if isinstance(f, PhysxRigidBodyCfg)).disable_gravity = True

If a fragment list ever lacks a PhysxRigidBodyCfg (e.g., a Newton-only configuration, or user-defined configs using only UsdPhysicsRigidBodyCfg), this raises an unhelpful StopIteration with no context. This pattern appears ~14 times in production asset configs and tests.

Suggested fix: Use a helper utility:

def get_fragment(frags, frag_type, default=None):
    return next((f for f in frags if isinstance(f, frag_type)), default)

Or at minimum provide a default sentinel: next((...), None) with explicit handling.

2. ⚠️ apply_rigid_body_properties always applies RigidBodyAPI even if already present (Warning)

File: source/isaaclab/isaaclab/sim/schemas/schemas.py, line ~435

if not UsdPhysics.RigidBodyAPI(prim):
    UsdPhysics.RigidBodyAPI.Apply(prim)

In the USD Python bindings, UsdPhysics.RigidBodyAPI(prim) constructs a wrapper that is always truthy regardless of whether the schema is applied. The existing define_rigid_body_properties uses the same pattern, so this is a pre-existing issue being carried forward — not a regression. However, since this is new code being added, it's an opportunity to use the correct check:

if not prim.HasAPI(UsdPhysics.RigidBodyAPI):
    UsdPhysics.RigidBodyAPI.Apply(prim)

This ensures idempotency on prims that already have the schema applied through USD composition.

3. 💡 Migration inconsistency: some sites use only PhysxRigidBodyCfg without UsdPhysicsRigidBodyCfg (Suggestion)

Files: Multiple asset configs (e.g., agibot.py, galbot.py, allegro.py)

Some migrated configs use rigid_props=[PhysxRigidBodyCfg(...)] without an explicit UsdPhysicsRigidBodyCfg fragment:

rigid_props=[
    PhysxRigidBodyCfg(
        disable_gravity=False,
        max_depenetration_velocity=5.0,
    ),
],

While apply_rigid_body_properties applies UsdPhysics.RigidBodyAPI as the implicit anchor regardless, the absence of UsdPhysicsRigidBodyCfg means physics:rigidBodyEnabled and physics:kinematicEnabled are never explicitly authored. For the from_files spawner path (which calls apply_rigid_body_properties on an already-existing USD prim), this is fine — those attributes exist from the USD file. But for shapes/meshes (creating prims from scratch), verify that the anchor application alone is sufficient or whether rigid_body_enabled=True should be explicitly set.

4. 💡 Type annotation allows a bare RigidBodyFragment singleton (Suggestion)

File: source/isaaclab/isaaclab/sim/spawners/spawner_cfg.py

rigid_props: schemas.RigidBodyBaseCfg | schemas.RigidBodyFragment | list[schemas.RigidBodyFragment] | None = None

Allowing a single RigidBodyFragment (not in a list) means the transition routing code needs to handle three shapes: None, a single legacy cfg, a single fragment, or a list. The isinstance(cfg.rigid_props, (list, tuple)) checks correctly handle this, but a bare fragment would be wrapped in a list and passed to apply_rigid_body_properties — which always applies the RigidBodyAPI anchor. This is fine functionally, but consider whether the type annotation should require a list to simplify the contract:

rigid_props: schemas.RigidBodyBaseCfg | list[schemas.RigidBodyFragment] | None = None

5. 💡 RigidBodyPropertiesCfg deprecation factory loses @configclass semantics (Suggestion)

File: source/isaaclab_physx/isaaclab_physx/sim/schemas/schemas_cfg.py

The factory function RigidBodyPropertiesCfg(**kwargs) returns a list[RigidBodyFragment] rather than a configclass instance. Code that previously used .replace() or attribute access on the returned object will fail at runtime rather than at type-check time. The PR body documents this as a known breaking change (removal in 5.0), and the test test_rigidbody_properties_factory_returns_fragments validates the new behavior. However, any third-party code outside this repo that does cfg = RigidBodyPropertiesCfg(...) and later calls cfg.disable_gravity will get an AttributeError on a list. Consider adding a note in the deprecation warning message pointing to the migration pattern.

6. ✅ CI: Changelog fragment check failing

The "Check changelog fragments" CI job is failing. All 7 changelog fragments are present in the diff, so this may be a formatting/naming issue with one of them (e.g., isaaclab_assets/changelog.d/vidurv-schema-fragments.rst lacks the .minor suffix while isaaclab/changelog.d/vidurv-schema-fragments.minor.rst has it). Verify the CI job's expected naming convention.


Test Coverage Assessment

Strengths:

  • Dedicated test_schema_fragments.py with 8 tests covering the full fragment lifecycle
  • Tests verify namespace writes, partial updates (None fields not authored), multi-namespace composition
  • Deprecation factory test validates the backward-compat path
  • Existing spawn tests migrated and passing (shapes: 12, meshes: 10)

Gaps:

  • No test for error handling when prim_path is invalid in apply_rigid_body_properties (the existing define_rigid_body_properties raises ValueError)
  • No test for the from_files.py gravcomp auto-injection logic with the new fragment list path
  • The StopIteration scenarios from next() calls are untested

Verdict

Well-designed architectural foundation for the multi-namespace schema composition pattern. The fragment protocol is clean, the transition routing is backward-compatible, and the test coverage for the new APIs is solid. The main concerns are the ergonomic next() pattern (which will be a maintenance burden as configs evolve) and the CI changelog failure that needs resolution before merge.

Reviewed commit: b5cdb6a

@vidurv-nvidia

Copy link
Copy Markdown
Author

Superseded by a 2-PR stack: #5976 (additive fragment API) + the migration PR stacked on it (vidurv-nvidia#4). Splitting for easier review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request isaac-mimic Related to Isaac Mimic team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant