Adding RigidBody USD data classes and writers #5976
Conversation
Introduce single-namespace schema "fragments" so a prim can carry rigid-body properties from multiple USD namespaces at once, while core names no backend. This change is purely additive: existing cfgs and the deprecated RigidBodyPropertiesCfg class are untouched. - core: SchemaFragment base, RigidBodyFragment marker, UsdPhysicsRigidBodyCfg; 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 accepts a fragment list; legacy single cfgs keep working via a transition bridge in the shapes/meshes/from_files/mesh_converter writers. - isaaclab_physx: add PhysxRigidBodyCfg. isaaclab_newton: add MujocoRigidBodyCfg. - exports + a new test_schema_fragments.py. First of a two-PR stack; the migration of call sites and the RigidBodyPropertiesCfg deprecation follow in a dependent PR.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔄 Incremental Review Update (commits c61c237...1a809a3)
Changes Since Last Review
The new commits update comments in 4 files:
mesh_converter.pyfrom_files.pymeshes.pyshapes.py
Change Summary: Replaced "transition routing" with "transition shim, remove later" in comments marking the dual-path logic for schema fragments vs legacy configs.
Assessment
✅ LGTM for this increment — these are documentation/comment improvements only:
- More explicit language ("shim" + "remove later") makes the temporary nature clearer
- Helps future maintainers understand this code is transitional
- No functional changes
Previous Review Findings Still Apply
The architectural observations from my initial review remain valid:
- The
StopIterationrisk from barenext()calls inschemas.pystill exists - The
if not prim.GetPrim().HasAPI(...)truthiness pattern in_apply_to_primcould mask issues - CI changelog job failure (unrelated to this PR)
No new issues introduced by this commit. Ready for merge once CI passes.
🔄 Incremental Review Update (commits 1a809a3...559c57a)
Changes Since Last Review
Type annotation improvements in schemas.py:
- Added
from collections.abc import Iterableimport - Added type hint to
apply_namespaced():cfg: schemas_cfg.SchemaFragment - Added type hint to
apply_rigid_body_properties():fragments: Iterable[schemas_cfg.RigidBodyFragment]
Assessment
✅ LGTM — These are strictly type annotation additions. Improves IDE support and static analysis without any behavioral change. Good housekeeping.
No new issues. Previous findings (bare next(), HasAPI truthiness) remain unchanged in this diff.
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits 559c57a...8c2b75d)
Changes Since Last Review
The new commits (559c57a → 8c2b75d) contain no changes to the schema/fragment files that are the focus of this PR. The diff consists entirely of unrelated work merged into the branch:
- Circular buffer
stack_dimmode and performance improvements - Image normalization utilities (
isaaclab.utils.images, Warp fused kernel) - Locomotion task consolidation (ant/humanoid refactoring, Gym ID renames)
- Template generator improvements and tests
- Newton version bumps (v1.2.0 → v1.2.1rc2),
ls_paralleldeprecation - OVPhysX version pin, OVRTX error messaging
- Minor fixes: Fabric logging level, cartpole camera frame-stack perf
Assessment
✅ No impact on the schema fragment code under review — the core PR files (schemas.py, schemas_cfg.py, etc.) are untouched in this increment.
Previous Review Findings Still Apply
- The
StopIterationrisk from barenext()calls inschemas.pystill exists - The
if not prim.GetPrim().HasAPI(...)truthiness pattern in_apply_to_primcould mask issues
No new issues introduced.
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits 8c2b75d...2a8b8e7)
Changes Since Last Review
This commit directly addresses all three inline review comments from earlier reviews:
✅ P1: Missing prim validity check — RESOLVED
Both apply_namespaced() and apply_rigid_body_properties() now validate the prim at the given path and raise ValueError if invalid, preventing silent no-ops.
✅ P2: apply_rigid_body_properties always returns True — RESOLVED
The function now aggregates per-fragment return values with success = bool(func(...)) and success, honoring the boolean contract established by modify_* writers.
✅ P2: Fragile func-field exclusion by name — RESOLVED
Introduced _non_usd_fields: ClassVar[frozenset] on SchemaFragment. The apply loop now checks f.name in non_usd_fields (which always includes "func") rather than hardcoding a single name. Subclasses can declare additional non-USD bookkeeping fields explicitly.
New Tests Added
Four targeted tests in test_schema_fragments.py:
test_apply_namespaced_raises_on_invalid_prim— validates the ValueError guardtest_apply_rigid_body_properties_raises_on_invalid_prim— same for the aggregate appliertest_apply_rigid_body_properties_aggregates_fragment_results— verifies False propagationtest_apply_namespaced_skips_declared_non_usd_fields— confirms_non_usd_fieldsopt-out
Assessment
✅ All previous review concerns are addressed. The implementation is clean and well-tested. Minor observations:
- The
non_usd_fieldsunion (getattr(...) | {"func"}) ensures backward compat even if a subclass forgets to inherit the base frozenset — good defensive design. - Tests use a custom
@configclasssubclass to verify the mechanism generically.
No new issues introduced. This PR is ready for merge once CI is green.
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits 2a8b8e7...fb864c0)
Changes Since Last Review
This commit simplifies the non-USD field handling by removing the _non_usd_fields mechanism introduced in the previous iteration and replacing it with a stricter invariant:
Key Changes:
schemas.py: Removed_non_usd_fieldsset; the apply loop now only skipsfunc(hardcoded). Added a namespace-presence guard: if_usd_namespaceisNone,apply_namespaced()raisesValueErrorimmediately — enforcing that every fragment must declare where its fields go.schemas_cfg.py: Removed_non_usd_fields: ClassVarand its docstring. Added a comprehensive.. important::docstring explaining the design invariant: all dataclass fields (exceptfunc) are USD attributes, and non-USD state belongs on the spawner cfg or as writer kwargs.test_schema_fragments.py: Replacedtest_apply_namespaced_skips_declared_non_usd_fieldswithtest_apply_namespaced_raises_without_namespace— verifying the new ValueError guard fires when_usd_namespaceisNone.
Assessment
✅ LGTM — this is a good simplification. Instead of providing an opt-out mechanism for non-USD fields (which invites misuse), the framework now forbids them entirely with a clear error message. This is a better design because:
- Fail-fast: misconfigured fragments are caught immediately at apply time
- Simpler API: no
_non_usd_fieldsto maintain or remember - Clear guidance: the error message tells users where non-USD state should live (spawner cfg or writer kwargs)
- The
funcfield remains the single hardcoded exception, which is fine since it is framework machinery
Previous Review Findings
StopIterationrisk from barenext()HasAPItruthiness
No new issues introduced. PR remains ready for merge once CI is green.
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits fb864c0...419e9ef)
Changes Since Last Review
This update is a branch rebase/merge from develop bringing in a large volume of unrelated upstream changes. The diff contains ~80 files spanning:
- Cloner refactoring:
scene.clone_environments()replaced by explicitcloner.ClonePlan.from_env_0()+cloner.replicate()across all direct-workflow envs and templates - PhysX replication:
physx_replicate.py→replicate.pywith newPhysxReplicateContextclass andqueue_physx_replication()pattern - Task renaming (breaking): Gym IDs drop
-v0suffix (Isaac-Reach-Franka-v0→Isaac-Reach-Franka, etc.); lift tasks consolidated underisaaclab_tasks.core.lift - Reach MDP removal:
isaaclab_tasks.core.reach.mdpdeleted; reward terms moved toisaaclab.envs.mdp - Deformable soft-lift consolidation:
lift_franka_softmerged intolift.config.franka_soft; shared deformable MDP terms moved toisaaclab_tasks.core.lift.mdp - Preset CLI simplification:
fold_preset_tokens()removed; typed selectors (physics=/renderer=) now parsed directly by Hydra resolver with type validation - Visualizer improvements: Newton contact rendering fallback, infinite plane expansion, Viser grid caching,
set_camera_view()API - Dependency updates:
newton[sim]pinned to1.2.1(from git RC),isaaclab_ppispmade optional,isaaclab_physxversion bump to 1.1.4 - ISP import fix:
isaaclab_ppispimport now wrapped in try/except with descriptive error - skrl JAX fix: preloads
jax.experimental.multihost_utilssubmodule - AutoMate collision stack:
gpu_collision_stack_size=2**27added; placeholder assembly ID validation
Assessment
✅ No changes to the PR's core schema fragment files (schemas.py, schemas_cfg.py, test_schema_fragments.py, the RigidBody USD data classes and writers). This is purely a branch synchronization bringing in upstream develop changes.
No new issues introduced to the schema fragment framework by this merge. The upstream changes are well-structured and follow established patterns (e.g., the cloner refactoring is consistent across all envs).
Previous Review Findings
All prior observations remain unchanged (addressed or out-of-scope for this PR):
- Bare
next()StopIteration risk — unchanged HasAPItruthiness pattern — unchanged
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits fb864c0...419e9ef)
Changes Since Last Review
This update is a branch rebase/merge from develop bringing in 28 commits of upstream changes. The schema fragment files received only minor comment simplifications:
schemas.py: Editorial cleanup of comments inapply_namespaced()andapply_rigid_body_properties()— more concise language, no logic changes
The bulk of the diff (~80+ files) is unrelated upstream work merged into the branch (cloner refactoring, task renaming, visualizer improvements, dependency updates, etc.).
Previous Review Concerns
- ✅ P1: Missing prim validity check — Fixed in earlier commit, verified still present
- ✅ P2:
apply_rigid_body_propertiesreturns True — Fixed in earlier commit, verified still present - ✅ P2: Fragile
func-field exclusion — Addressed by simpler design (fail if no namespace)
Assessment
✅ No new issues introduced. The schema fragment framework code is unchanged in substance — only comments were tightened. All prior review concerns were addressed in earlier commits and remain in place.
PR remains ready for merge once CI is green.
Automated incremental review by IsaacLab Review Bot 🤖
🔄 Incremental Review Update (commits 419e9ef...b7456fc)
Changes Since Last Review
2 commits touching 2 files:
-
CONTRIBUTORS.md: Added "Vidur Vij" to the contributors list (alphabetically correct placement between Tyler Lum and Virgilio Gómez Lambo). -
source/isaaclab/test/sim/test_schema_fragments.py: Removed "Task N --" prefixes from all test section headers (e.g.,Task 1 -- SchemaFragment base→SchemaFragment base). This is a cosmetic cleanup — test logic is entirely unchanged.
Assessment
✅ LGTM — These are purely housekeeping changes:
- Contributor addition is appropriate for the PR author
- Test header cleanup removes stale task-numbering artifacts that are no longer relevant now that the implementation is complete
- No functional code changes whatsoever
Previous Review Status
All prior review concerns were addressed in earlier commits and remain in place:
- ✅ Prim validity checks — still present
- ✅
apply_rigid_body_propertiesreturn value aggregation — still present - ✅ Namespace-presence guard (simplified design) — still present
PR remains ready for merge once CI is green.
Automated incremental review by IsaacLab Review Bot 🤖
|
Thanks for the review! Responses below. #1 — So the anchor is applied for a bare #3 — type annotation on #2 — DRY the 4 spawner transition shims: intentional for now. The #4 — #5 — noted, no action. |
vidurv-nvidia
changed the title
| # apply rigid body properties | ||
| if cfg.rigid_props is not None: | ||
| schemas.define_rigid_body_properties(prim_path, cfg.rigid_props, stage=stage) | ||
| # transition shim, remove later: new fragment list -> apply_*; legacy single cfg -> define_* |
There was a problem hiding this comment.
Transition shim — remove at the end of the migration. A fragment list routes to apply_rigid_body_properties; a legacy single cfg falls through to the existing define_/modify_ writer, so current behavior is unchanged (this PR is purely additive). Collapses to one unconditional apply_* call once call sites move to fragments and the legacy cfgs are dropped.
| schemas.define_mass_properties(prim_path, cfg.mass_props, stage=stage) | ||
| # apply rigid properties | ||
| schemas.define_rigid_body_properties(prim_path, cfg.rigid_props, stage=stage) | ||
| # apply rigid properties (transition shim, remove later: fragment list -> apply_*; legacy cfg -> define_*) |
There was a problem hiding this comment.
Transition shim — remove at the end of the migration. A fragment list routes to apply_rigid_body_properties; a legacy single cfg falls through to the existing define_/modify_ writer, so current behavior is unchanged (this PR is purely additive). Collapses to one unconditional apply_* call once call sites move to fragments and the legacy cfgs are dropped.
| # modify rigid body properties | ||
| if cfg.rigid_props is not None: | ||
| schemas.modify_rigid_body_properties(prim_path, cfg.rigid_props) | ||
| # transition shim, remove later: new fragment list -> apply_*; legacy single cfg -> modify_* |
There was a problem hiding this comment.
Transition shim — remove at the end of the migration. A fragment list routes to apply_rigid_body_properties; a legacy single cfg falls through to the existing define_/modify_ writer, so current behavior is unchanged (this PR is purely additive). Collapses to one unconditional apply_* call once call sites move to fragments and the legacy cfgs are dropped.
| if cfg.mass_props is not None: | ||
| schemas.define_mass_properties(prim_path=xform_prim.GetPath(), cfg=cfg.mass_props, stage=stage) | ||
| # apply rigid body properties | ||
| # apply rigid body properties (transition shim, remove later: fragment list -> apply_*; legacy cfg -> define_*) |
There was a problem hiding this comment.
Transition shim — remove at the end of the migration. A fragment list routes to apply_rigid_body_properties; a legacy single cfg falls through to the existing define_/modify_ writer, so current behavior is unchanged (this PR is purely additive). Collapses to one unconditional apply_* call once call sites move to fragments and the legacy cfgs are dropped.
| stage = get_current_stage() | ||
| prim = stage.GetPrimAtPath(prim_path) | ||
| if not UsdPhysics.RigidBodyAPI(prim): | ||
| UsdPhysics.RigidBodyAPI.Apply(prim) |
There was a problem hiding this comment.
Implicit anchor: no fragment owns RigidBodyAPI (the PhysX/mjc fragments are inert add-ons without it), so the family writer applies it once regardless of which fragments are passed — which is why UsdPhysicsRigidBodyCfg._usd_applied_schema is None.
vidurv-nvidia
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
ooctipus
as code owners
Greptile SummaryIntroduces a schema-fragment API for rigid-body USD authoring, allowing a prim to carry properties from multiple namespaces (
Confidence Score: 4/5Safe to merge for new projects using the fragment API on single prims, but the dispatching shim in _spawn_from_usd_file produces silently incomplete writes for USD assets that carry RigidBodyAPI on multiple nested links. The transition shim in from_files.py routes the new fragment path to apply_rigid_body_properties, which targets only the root prim. The legacy path it replaced called modify_rigid_body_properties, whose @apply_nested decorator walks the full prim hierarchy and writes to every child that already has RigidBodyAPI. For a robot USD asset with several articulation links, a user who switches to the new fragment API will only see the property written to the root — all child links are silently left with their prior values. source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py — the fragment-path dispatch in _spawn_from_usd_file does not propagate fragment writes to nested rigid-body prims. Important Files Changed
Reviews (2): Last reviewed commit: "test(schemas): drop internal plan task n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| stage is used. | ||
|
|
||
| Returns: | ||
| True if the properties were successfully set. | ||
| """ | ||
| if stage is None: | ||
| stage = get_current_stage() | ||
| prim = stage.GetPrimAtPath(prim_path) | ||
| if not UsdPhysics.RigidBodyAPI(prim): | ||
| UsdPhysics.RigidBodyAPI.Apply(prim) | ||
| for cfg in fragments: | ||
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | ||
| func(cfg, prim_path, stage) | ||
| return True |
There was a problem hiding this comment.
Both apply_namespaced and apply_rigid_body_properties skip the prim.IsValid() guard that every analogous define_*/modify_* function in this file uses. If prim_path does not exist in the stage, stage.GetPrimAtPath returns an invalid UsdPrim; prim.GetAppliedSchemas() returns [], prim.AddAppliedSchema(...) and safe_set_attribute_on_usd_prim silently no-op (or emit a TF error), and the function still returns True. define_rigid_body_properties (line 476) raises ValueError in this case. The missing check means the new fragment path gives callers no indication that nothing was written.
| for cfg in fragments: | ||
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | ||
| func(cfg, prim_path, stage) | ||
| return True |
There was a problem hiding this comment.
apply_rigid_body_properties always returns True regardless of per-fragment failures
func(cfg, prim_path, stage) may return False (e.g. a custom fragment applier signals a failure). The return is currently discarded and the outer function unconditionally returns True. modify_rigid_body_properties returns False when the API is not applied. Propagating the aggregate result keeps the contract consistent.
| for cfg in fragments: | |
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | |
| func(cfg, prim_path, stage) | |
| return True | |
| ok = True | |
| for cfg in fragments: | |
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | |
| ok = func(cfg, prim_path, stage) and ok | |
| return ok |
| for f in dataclasses.fields(cfg): | ||
| if f.name == "func": | ||
| continue | ||
| value = getattr(cfg, f.name) |
There was a problem hiding this comment.
func-field exclusion by hard-coded name
Skipping the field via f.name == "func" is a maintenance hazard: any future non-USD metadata field added to a SchemaFragment subclass will be silently written as a USD attribute unless the exclusion list in apply_namespaced is also updated. A class-level _non_usd_fields set makes the policy explicit.
| for f in dataclasses.fields(cfg): | |
| if f.name == "func": | |
| continue | |
| value = getattr(cfg, f.name) | |
| _non_usd_fields = getattr(type(cfg), "_non_usd_fields", frozenset()) | {"func"} | |
| for f in dataclasses.fields(cfg): | |
| if f.name in _non_usd_fields: | |
| continue | |
| value = getattr(cfg, f.name) |
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!
…e non-USD fields Address review feedback on the rigid-body schema-fragment API: - apply_namespaced and apply_rigid_body_properties now raise ValueError on an invalid prim path, matching the legacy define_/modify_ writers instead of silently no-op'ing and still returning True. - apply_rigid_body_properties aggregates each fragment applier's boolean result so a reported failure is not masked by the always-applied anchor. - apply_namespaced skips fields named in a fragment's _non_usd_fields set (in addition to func), so future non-USD bookkeeping fields opt out explicitly rather than leaking as namespaced USD attributes.
…t set Per design review, fragments carry only USD-attribute fields plus func -- non-USD/bookkeeping state lives on the spawner cfg or as a writer kwarg (e.g. fix_root_link, ensure_drives_exist). Replace the _non_usd_fields opt-out skip-list with the invariant enforced loudly: - Drop the _non_usd_fields ClassVar; restore the simple func-only skip. - apply_namespaced raises ValueError when a fragment has no _usd_namespace, turning a misconfigured (non-USD) field from a silent None: write into a loud failure. Non-scalar value types already raise in safe_set_attribute_on_usd_prim. - Document the invariant on SchemaFragment. Field-name validation against the USD schema registry is intentionally deferred to the codegen/CI sync-check: a runtime registry check would false-positive on namespace-only fragments (e.g. mjc:gravcomp) that own no applied schema.
The SchemaFragment docstring carries the invariant rationale, so collapse the inline comments in apply_namespaced / apply_rigid_body_properties to terse intent. No behavior change.
| assert abs(prim.GetAttribute("physxRigidBody:linearDamping").Get() - 0.2) < 1e-6 | ||
| assert abs(prim.GetAttribute("mjc:gravcomp").Get() - 1.0) < 1e-6 | ||
|
|
||
|
|
There was a problem hiding this comment.
-------------------------------------------------------------------------------------
Task 6 -- spawner slot accepts a fragment list + transition routing
-------------------------------------------------------------------------------------
remember to remove ai comments like this :D
The section banners referenced internal planning task numbers; replace with self-contained descriptions.
2 participants
Description
Summary (PR 1 of 2 — additive, no behavior change)
Introduces single-namespace schema fragments so a prim can carry rigid-body properties from multiple USD namespaces at once (
physics:,physxRigidBody:,mjc:), with core naming no backend. Purely additive — existing cfgs and the deprecatedRigidBodyPropertiesCfgclass are untouched.SchemaFragment,RigidBodyFragment,UsdPhysicsRigidBodyCfg;apply_namespaced(generic per-fragment writer) andapply_rigid_body_properties(applies a fragment list withUsdPhysics.RigidBodyAPIas the implicit anchor).rigid_propsaccepts a fragment list; legacy single cfgs keep working via a transition bridge (shapes/meshes/from_files/mesh_converter).PhysxRigidBodyCfg(physx),MujocoRigidBodyCfg(newton).test_schema_fragments.py+ exports.PR 2 (stacked) flips
RigidBodyPropertiesCfgto a deprecation factory and migrates all call sites: vidurv-nvidia#4 (review after this lands).Test Plan
test_schema_fragments.py— 7 passed./isaaclab.sh -ddocs build cleanFirst slice of the staged physics schema-cfg refactor (collision / mass / joint-drive / … follow later). Draft for review of the fragment API shape.
Type of change
Screenshots
N/A — no user-facing visual changes.
Checklist
ruff+ruff-format) on the changed files./isaaclab.sh -d; not separately authored)source/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.md