Adding Articulation Root USD data classes and writers - #6272
Conversation
apply_articulation_root_properties applied UsdPhysics.ArticulationRootAPI unconditionally on the input prim. USD assets author the root on a child prim (the root link / fixed joint), so this stamped a SECOND root on the top prim and wrote the fragment properties + fix_root_link logic to the wrong prim -- a duplicate root that also violates the single-root asset requirement. Descend the subtree to the existing root and tune it in place (matching the legacy @apply_nested modify_articulation_root_properties writer); only define a fresh root on the input prim when the subtree has none (primitive or programmatic spawns). Fragments and the fix_root_link reparent logic now target the resolved root. Also guard an invalid input prim path. Add a regression test covering the child-root case (verified to fail before the fix).
Replace the hand-rolled subtree BFS with the existing get_first_matching_child_prim query helper (predicate = HasAPI(ArticulationRootAPI)). Same find-or-define behavior; the helper also validates the path, so the bespoke helper and the separate IsValid guard are dropped. Pass traverse_instance_prims=False to keep parity with the legacy @apply_nested writer, which does not author through instances.
The function docstring already explains the find-or-define rationale, so collapse the inline comments to terse intent. The fix_root_link block is left untouched -- it is reproduced verbatim from the legacy writer.
The create-new-joint branch of apply_articulation_root_properties was sim-unexercised (only the toggle-existing-joint case was tested). Add two tests: (1) fix_root_link=True with no existing joint creates a fixed joint and reparents the articulation root from the rigid-body root link to its parent; (2) fix_root_link=True on a non-rigid-body root raises NotImplementedError.
…-articulation # Conflicts: # source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py
The fix_root_link create path in apply_articulation_root_properties used omni.physx.scripts and a PhysxArticulationAPI root-relocation workaround (a PhysX-parser limitation) directly in core. Replace it with an inversion-of-control hook: core exposes register_fixed_root_joint_creator and delegates to the registered creator; the PhysX package registers _create_fixed_root_joint on import. Core's articulation-root writer now carries no PhysX-specific logic; the enable-existing-joint path stays in core (it is generic UsdPhysics). Legacy modify_articulation_root_properties is left untouched.
Cover the registry independent of PhysX: the writer delegates to a registered creator, and raises a clear error when none is registered. Uses monkeypatch to isolate the module-global creator.
Greptile SummaryAdds the articulation-root schema-fragment API on top of the existing
Confidence Score: 5/5Purely additive change — no existing call sites are touched and the legacy modify_* path is unchanged; safe to merge. The root-resolution and fragment-dispatch logic is correct and well-tested across 9 tests covering the key invariants: exactly-one-root, child-root tuning, joint toggle/create, backend-selection, and error paths. The fixed-joint lifecycle is consistent between PhysX and Newton backends, and no existing call sites are modified. No files require special attention; the only findings are minor style observations. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant S as Spawner (_spawn_from_usd_file)
participant W as apply_articulation_root_properties
participant Q as get_first_matching_child_prim
participant F as Fragment (apply_namespaced)
participant R as _FIXED_ROOT_JOINT_CREATORS registry
participant B as Backend creator (PhysX / Newton)
S->>W: fragments list, fix_root_link
W->>Q: search prim_path subtree for ArticulationRootAPI
Q-->>W: articulation_prim (existing child root OR None)
alt no existing root
W->>W: ArticulationRootAPI.Apply(prim_path)
end
loop each fragment
W->>F: func(cfg, root_path, stage)
F->>F: AddAppliedSchema + write namespaced attrs
end
alt fix_root_link is not None
W->>W: find_global_fixed_joint_prim(root_path)
alt existing fixed joint found
W->>W: joint.GetJointEnabledAttr().Set(fix_root_link)
else "fix_root_link=True, no joint"
W->>R: _resolve_fixed_root_joint_creator(type(sim.cfg.physics))
R-->>W: backend creator callable
W->>B: creator(articulation_prim, stage)
B->>B: create_fixed_root_joint (pure-USD)
Note over B: PhysX only: copy attrs to parent,<br/>relocate ArticulationRootAPI
end
end
W-->>S: True
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant S as Spawner (_spawn_from_usd_file)
participant W as apply_articulation_root_properties
participant Q as get_first_matching_child_prim
participant F as Fragment (apply_namespaced)
participant R as _FIXED_ROOT_JOINT_CREATORS registry
participant B as Backend creator (PhysX / Newton)
S->>W: fragments list, fix_root_link
W->>Q: search prim_path subtree for ArticulationRootAPI
Q-->>W: articulation_prim (existing child root OR None)
alt no existing root
W->>W: ArticulationRootAPI.Apply(prim_path)
end
loop each fragment
W->>F: func(cfg, root_path, stage)
F->>F: AddAppliedSchema + write namespaced attrs
end
alt fix_root_link is not None
W->>W: find_global_fixed_joint_prim(root_path)
alt existing fixed joint found
W->>W: joint.GetJointEnabledAttr().Set(fix_root_link)
else "fix_root_link=True, no joint"
W->>R: _resolve_fixed_root_joint_creator(type(sim.cfg.physics))
R-->>W: backend creator callable
W->>B: creator(articulation_prim, stage)
B->>B: create_fixed_root_joint (pure-USD)
Note over B: PhysX only: copy attrs to parent,<br/>relocate ArticulationRootAPI
end
end
W-->>S: True
Reviews (2): Last reviewed commit: "Merge develop to pick up docker install ..." | Re-trigger Greptile |
| Returns: | ||
| True if the properties were successfully set. | ||
|
|
||
| Raises: | ||
| ValueError: When the prim path is not valid. | ||
| NotImplementedError: When the root prim is not a rigid body and a fixed joint is to be created. | ||
| """ |
There was a problem hiding this comment.
The
Raises section is incomplete. When fix_root_link=True and no creator has been registered (i.e. isaaclab_physx was never imported), the function raises RuntimeError, not one of the two documented exceptions. A caller who catches only ValueError / NotImplementedError will get an unexpected uncaught exception.
| Returns: | |
| True if the properties were successfully set. | |
| Raises: | |
| ValueError: When the prim path is not valid. | |
| NotImplementedError: When the root prim is not a rigid body and a fixed joint is to be created. | |
| """ | |
| Returns: | |
| True if the properties were successfully set. | |
| Raises: | |
| ValueError: When the prim path is not valid. | |
| NotImplementedError: When the root prim is not a rigid body and a fixed joint is to be created. | |
| RuntimeError: When ``fix_root_link=True`` and no fixed-root-joint creator has been registered | |
| (i.e. no physics-backend extension such as ``isaaclab_physx`` has been imported). | |
| """ |
There was a problem hiding this comment.
Addressed in b0dcdd8. The per-backend "no creator registered" case is gone with the registry; the remaining RuntimeError is raised only when a fixed joint must be created but there is no active SimulationContext to resolve the backend from, and the Raises section now documents exactly that plus the propagated NotImplementedError.
Address greptile: the Raises section omitted the RuntimeError raised when fix_root_link=True and no fixed-root-joint creator is registered. Also clarify that NotImplementedError is propagated from the registered backend creator rather than raised by core directly.
Previously only isaaclab_physx registered a fixed-root-joint creator, so a Newton-only import hit a RuntimeError on fix_root_link=True even though Newton needs the joint (it defaults a jointless root to floating). Make the registry backend-aware: each backend registers a creator plus an is_active predicate, and the writer selects the creator matching the running backend. - isaaclab_physx: createJoint + the PhysX-parser root relocation (active when the sim uses PhysxCfg). - isaaclab_newton: authors the world<->root UsdPhysics.FixedJoint via the Kit omni.physx helper (no relocation; Newton reads the joint directly), active when the sim uses NewtonCfg. Nothing is hand-rolled (omni.physx.createJoint is a Kit utility present in any Isaac Sim app). Core stays backend-free; both backends work standalone, and a both-imported run picks the active backend's creator.
The PhysX and Newton schema packages registered their fixed-root-joint creators by importing the register function from the core schemas module at package import time. That core module imports pxr at module top, so importing a backend package eagerly pulled USD libraries into otherwise USD-free import paths (e.g. resolving a robot config), failing the omni/pxr-free import guard. Move the registry and registration API into a dedicated USD-free hooks module so a backend can register its hook at import time without dragging in pxr.
…-articulation # Conflicts: # source/isaaclab/isaaclab/sim/schemas/__init__.pyi # source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py
The fixed-root-joint registry stored (is_active, creator) pairs and resolved by calling every backend's is_active() probe. That had two problems: a backend being imported (hence registered) is not the same as it being active, and each probe imported its backend's cfg, so resolving during a Newton run could execute PhysX import code whenever both extensions were present. Key the registry by the backend's physics-cfg class instead and resolve with a direct lookup on the active simulation's cfg.physics type. "Active" now comes solely from the live cfg, and resolution never imports or executes a non-active backend.
The Newton fixed-root-joint creator authored the world<->root joint via omni.physx's createJoint, pulling a PhysX-flavored Kit dependency into the Newton backend. Add a backend-neutral create_fixed_root_joint helper in core (pure USD: world-anchored UsdPhysics.FixedJoint whose local frame is set to the prim's current world pose so the body is pinned in place) and use it from both backends. The Newton backend now depends on nothing PhysX-flavored. Add tests for the helper (world-anchored joint, world-pose pinning, unique naming), the active-backend resolution (creator selected by the live cfg.physics type, unregistered type and no-sim return None), and the Newton creator (authors the joint without the PhysX root relocation).
The articulation-root creators import pxr and the core schema helper at call time rather than module top. Add a comment explaining this is deliberate: the backend schema package is imported eagerly when a config references a backend cfg name, so its import path must stay free of the USD/Omniverse runtime; the creator itself only runs while a simulation is live, when those imports are available.
…-articulation # Conflicts: # source/isaaclab/isaaclab/sim/schemas/_backend_hooks.py # source/isaaclab/isaaclab/sim/schemas/schemas.py # source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py # source/isaaclab_physx/isaaclab_physx/sim/schemas/__init__.py
| """ | ||
| if physics_cfg is None: | ||
| return None | ||
| return _FIXED_ROOT_JOINT_CREATORS.get(physics_cfg) |
There was a problem hiding this comment.
Blocking: this exact-type lookup is not complete for the supported backend/config surface. OvPhysxCfg is a sibling of PhysxCfg, and nothing in isaaclab_ovphysx registers a creator, so fragment mode with fix_root_link=True reaches the RuntimeError. It also misses the in-tree DeformableNewtonCfg(NewtonCfg) and any user subclass. Hook availability additionally depends on importing the matching isaaclab_*.sim.schemas module, rather than on activating that backend, so a Newton run using only the PhysX fragment for articulation_enabled does not register the Newton creator.
The inversion-of-control direction is consistent with #6273, but the cfg class is a brittle dispatch key. SimulationContext.physics_manager already identifies the active implementation and supports inherited backend capabilities; a classmethod/capability there would avoid exact-type and import-order failures. If the registry remains, please resolve through the MRO, register OVPhysX explicitly, make registration occur with backend activation, and test each supported manager plus a cfg subclass.
There was a problem hiding this comment.
To make the alternative concrete: I do not think we need another factory here, and I would avoid putting this on physics_cfg. The cfg should remain declarative data; SimulationContext has already used cfg.physics.class_type to select the active runtime implementation as sim.physics_manager.
The call could therefore be:
sim = SimulationContext.instance()
final_root = sim.physics_manager.fix_articulation_root(articulation_prim, stage)with a classmethod capability on PhysicsManager, implemented by each backend. Newton would author the fixed joint and return the same prim; PhysX would author the joint, perform its parser-specific relocation, and return the parent/new root; OVPhysX would provide its own implementation. Returning the final root is important so the family writer can apply every fragment to the actual resulting root.
final_root = sim.physics_manager.fix_articulation_root(articulation_prim, stage)
for fragment in fragments:
apply_fragment(fragment, final_root, stage)The relocation path must also migrate/remove pre-existing articulation-root family schemas so the former child cannot remain a second root. This keeps runtime USD mutation on the manager, keeps property ownership in the family writer, inherits naturally for manager subclasses, and removes the parallel cfg-type registry plus its import-order dependency.
There was a problem hiding this comment.
Implemented as suggested in b0dcdd8. Fixing the base is now a capability on the manager:
final_root = sim.physics_manager.fix_articulation_root(articulation_prim, stage)
for cfg in fragments:
func(cfg, final_root.GetPath().pathString, stage)PhysicsManager.fix_articulation_root (base) authors the backend-neutral fixed joint and returns the same prim; PhysxManager overrides it to relocate the root to the parent and return the parent. The cfg stays declarative — dispatch is resolved from cfg.physics.class_type, so it inherits through the MRO with no parallel registry. The former child is no longer left as a second root because fragments are applied to the returned root (see the property-completeness thread).
One deviation: the enable/disable of an existing fixed joint stays in the writer (a backend-neutral JointEnabledAttr toggle, no relocation); only the create/relocate path is delegated to the manager.
There was a problem hiding this comment.
Fixed in b0dcdd8. The exact-type cfg registry is gone, replaced by PhysicsManager.fix_articulation_root resolved through cfg.physics.class_type. Because dispatch is by manager class and inheritance is by MRO, a subclassed cfg like DeformableNewtonCfg(NewtonCfg) resolves NewtonManager and inherits the base capability, OvPhysxManager inherits the base default instead of hitting a RuntimeError, and there is no import-order dependency (the active manager class always exists). Added test_fix_articulation_root_capability_inherited_by_manager_subclass covering a subclass inheriting an overriding backend, and a base-default test for the Newton/OVPhysx path.
|
|
||
| # remove the api from the (former) root | ||
| articulation_prim.RemoveAppliedSchema("PhysxArticulationAPI") | ||
| articulation_prim.RemoveAPI(UsdPhysics.ArticulationRootAPI) |
There was a problem hiding this comment.
Blocking property-completeness bug: this relocation moves only the USD/PhysX pieces. For the documented composition [PhysxArticulationCfg(...), NewtonArticulationCfg(...)] with fix_root_link=True, NewtonArticulationRootAPI and newton:* remain on the old child. Because NewtonArticulationRootAPI itself includes PhysicsArticulationRootAPI, removing the directly applied USD API here does not remove the composed root API: the parent and child both remain articulation roots.
That violates the exactly-one-root invariant and leaves newton:selfCollisionEnabled on the wrong root. The current composition test misses it because it never combines fragments with root fixing. Please make the backend operation return/identify the resulting root and let the family writer preserve or reapply every supplied articulation fragment, then add a PhysX + Newton + fix_root_link=True test asserting one root and the Newton value on that root. This is also important for URDF/MJCF assets that already carry the Newton API.
There was a problem hiding this comment.
Fixed in b0dcdd8. Root fixing now happens before any fragment is written, and each fragment is applied to the root returned by fix_articulation_root. So after PhysX relocates the root to the parent, both the PhysX and the Newton fragments land on that parent — nothing is stranded on the former child, and there is exactly one root. The PhysX override no longer copies attributes at all (there are none to copy at relocation time). Added test_physx_and_newton_fragments_fix_root_link_keeps_single_root asserting one root with the Newton value on it; verified it fails on the old ordering (2 == 1, the composed NewtonArticulationRootAPI kept the child a root).
There was a problem hiding this comment.
Follow-up in 3b9cf1c: the write-fragments-after-fix reorder covered composed fragments but not the URDF/MJCF case you mentioned — an asset that ships with NewtonArticulationRootAPI already authored on the root link would still have kept a second root through schema composition, and pre-authored physxArticulation:* values were dropped instead of moving with the root as the legacy writer did. The PhysX relocation now migrates any directly authored API schema that the USD schema registry reports as composing PhysicsArticulationRootAPI (backend-agnostic, no backend names) plus the PhysxArticulationAPI companion, together with their authored attributes. Two regression tests added (test_physx_fix_root_link_migrates_preauthored_newton_root_api, ..._preauthored_physx_attrs); both verified to fail without the migration (the Newton one with the same 2-roots signature).
| articulation_frags = ( | ||
| cfg.articulation_props if isinstance(cfg.articulation_props, (list, tuple)) else [cfg.articulation_props] | ||
| ) | ||
| if articulation_frags and all(isinstance(f, schemas.SchemaFragment) for f in articulation_frags): |
There was a problem hiding this comment.
The field has moved out of the USD fragment, which is the right ownership, but the behavior is still coupled to a non-empty property list here. articulation_props=[] fails this guard and is sent to the legacy writer as a cfg, where dataclasses.fields([]) raises TypeError. With articulation_props=None, the spawner-level fix_root_link is silently ignored. That means a caller needs a dummy backend fragment just to request a topology-only operation; there is no core articulation fragment to use instead. The direct writer tests at lines 268 and 298 already treat [] as valid, so the two entry points disagree.
Please route fragment collections by type even when empty, and preferably process the topology flag independently from whether schema properties were supplied. An end-to-end _spawn_from_usd_file regression test is important here; unlike #5976 and #6254, all new tests bypass this transition bridge.
There was a problem hiding this comment.
Fixed in b0dcdd8. articulation_props is now routed by type: a list (including []) goes to the fragment writer, only a legacy single cfg goes to modify_*, so an empty list no longer reaches dataclasses.fields([]). fix_root_link is read as a spawner-level flag independently of whether properties were supplied. Added end-to-end _spawn_from_usd_file regressions for both articulation_props=[] + fix_root_link=True and articulation_props=None + fix_root_link=True.
| # this logic is reproduced from the legacy ``modify_articulation_root_properties`` writer. | ||
| if fix_root_link is not None: | ||
| # check if a global fixed joint exists under the resolved root prim | ||
| existing_fixed_joint_prim = find_global_fixed_joint_prim(root_path) |
There was a problem hiding this comment.
Please pass stage=stage here. The root and fragment writes above honor the explicit stage argument, but this lookup falls back to the current global stage. When the supplied stage is not current, this can either raise because root_path is absent there or toggle a same-path joint on the wrong stage. The current tests all make the supplied stage current, so they do not cover the advertised explicit-stage behavior.
There was a problem hiding this comment.
Fixed in b0dcdd8 — find_global_fixed_joint_prim(root_path, stage=stage). Added test_apply_articulation_root_properties_honors_explicit_stage, which authors on a non-current in-memory stage and asserts nothing leaks onto the current stage.
ooctipus
left a comment
There was a problem hiding this comment.
I reviewed this against the runtime schemas and the same author history used for the earlier fragment reviews:
- #5275 established the core/base versus backend-package split. This PR follows that ownership correctly.
- #5276 established the explicit schema-property audit standard. Against Kit 110.1.11, the inventory here is complete: OpenUSD ArticulationRootAPI has no attributes;
PhysxArticulationCfghas all six PhysxArticulationAPI attributes;NewtonArticulationCfghas the sole Newton articulation attribute. - #5976 established fragment composition and tested the real spawner transition. The PhysX relocation here breaks that composition, and this PR has no end-to-end spawner test.
- #6273 is the closest precedent for moving a non-USD behavior flag to
UsdFileCfgand using backend hooks. Movingfix_root_linkout of schema fragments is correct; the remaining plumbing is not yet clean because it is still conditional on a non-empty property list and dispatches through an exact cfg type/import side effect.
I am requesting changes for two blocking correctness/completeness issues:
- PhysX root relocation leaves
NewtonArticulationRootAPIon the old child. Since that schema includesPhysicsArticulationRootAPI, a composed PhysX + Newton asset ends with two articulation roots afterfix_root_link=True, withnewton:selfCollisionEnabledon the wrong root. - Backend dispatch is incomplete: OVPhysX has no registration, cfg subclasses miss the exact lookup, and the matching creator may be absent solely because its schema subpackage was not imported.
The empty-list/spawner behavior and explicit-stage bug are also called out inline. I would add a real UsdFileCfg spawn test covering the fragment bridge, each supported physics manager, a cfg subclass, empty fragments/topology-only use, and combined PhysX + Newton fragments with root fixing.
The cleanest design in the current architecture is to make fixed-root authoring a capability of the active SimulationContext.physics_manager, and have that operation return the resulting articulation-root prim. Core can then retain ownership of fragment application/migration across every namespace, while each backend owns only its topology/parser difference. This removes cfg-type and registration-order coupling. A smaller acceptable fix is MRO-aware lookup, activation-time registration for every backend including OVPhysX, and a hook contract that lets core preserve all composed fragments.
Non-blocking cleanup: the PR description still promises a core UsdPhysicsArticulationRootCfg that is not present, and the new public symbols are absent from the API RST. The two failing CI jobs appear unrelated: OVPhysX failed while downloading a wheel due to a SHA mismatch, and rendering timed out in the Shadow Hand test.
| from isaaclab.sim import SimulationContext | ||
|
|
||
| sim = SimulationContext.instance() | ||
| creator = _resolve_fixed_root_joint_creator(type(sim.cfg.physics) if sim is not None else None) |
There was a problem hiding this comment.
there is one usage of this, maybe remove the shim helper
There was a problem hiding this comment.
Removed in b0dcdd8 — the shim (_resolve_fixed_root_joint_creator) and the whole creator registry are gone with the move to the PhysicsManager.fix_articulation_root capability.
Replace the cfg-type fixed-root-joint creator registry with a fix_articulation_root capability on PhysicsManager. The registry keyed on the exact cfg type missed cfg subclasses (e.g. DeformableNewtonCfg) and backends that register no creator (OVPhysx), and depended on which backend schema module happened to be imported. Resolving the capability from cfg.physics.class_type dispatches through the normal method resolution order instead, so every backend and subclass inherits the correct behaviour. The base implementation authors a backend-neutral fixed joint; PhysX overrides it to relocate the articulation root to the parent prim. Fix the root link before writing fragments so relocation is resolved up front and every fragment is applied to the single resulting root. Previously fragments were written first and the PhysX relocation copied only its own schema, stranding a composed Newton articulation-root API on the former child and leaving two articulation roots. Route the spawner articulation_props slot by type even when the list is empty (an empty list no longer falls through to the legacy writer and raises), and honor the spawner-level fix_root_link flag independently of whether any schema properties were supplied. Scope the fixed-joint lookup to the supplied stage.
|
Addressed the two non-blocking notes from the review summary:
The two failing CI jobs (OVPhysX wheel SHA mismatch, Shadow Hand render timeout) are unrelated to this change, as you noted. |
The fixed-root-joint authoring helper lived in sim.schemas, but after the fix operation moved onto the physics manager its only callers are the managers. That left the manager reaching back into sim.schemas for the primitive, so the runtime chain was schemas.apply_articulation_root_properties -> manager.fix_articulation_root -> schemas.create_fixed_root_joint, and sim.schemas was the physics manager's sole reason to import from schemas at all. Relocate the helper to sim.utils.prims next to the other USD authoring utilities (and its find_global_fixed_joint_prim sibling), so the dependency runs one way: both sim.schemas and the physics managers depend downward on sim.utils, and the physics package no longer imports sim.schemas. The public entry point isaaclab.sim.create_fixed_root_joint is unchanged; it is dropped from isaaclab.sim.schemas.
Writing fragments after the PhysX root relocation keeps supplied fragments on the resulting root, but asset-authored schemas were left behind. URDF/MJCF-imported assets can ship with a backend root API (e.g. NewtonArticulationRootAPI) already authored on the root link; that schema composes PhysicsArticulationRootAPI, so removing only the directly applied anchor left the former root link a second articulation root, with its authored values stranded on the wrong prim. Pre-authored PhysxArticulationAPI solver attributes were likewise dropped instead of moving with the root as the legacy writer did. Migrate them in the PhysX relocation: any directly authored API schema that the USD schema registry reports as composing PhysicsArticulationRootAPI moves to the parent together with its authored attributes (backend-agnostic detection, no backend names), and the PhysxArticulationAPI companion migrates the same way. Regression tests verified to fail without the migration. Also harden the fragment writer: reject non-fragment list items with a clear TypeError instead of an AttributeError deep in dispatch, gate the define-fresh ArticulationRootAPI anchor on fragments actually being supplied so a topology-only call cannot stamp a root as a side effect (the flag is ignored with a warning when no root exists), and warn when a spawner-level fix_root_link is ignored because articulation_props is a legacy cfg that owns its own flag.
ooctipus
left a comment
There was a problem hiding this comment.
I re-reviewed the latest head against the same author's merged schema-fragment precedents. The field split itself is complete (PhysX 6/6, Newton 1/1, and UsdPhysics.ArticulationRootAPI has no configurable attributes), and manager-based dispatch is the right separation boundary. I am requesting changes for the independently reproduced correctness gaps below. All 24 targeted tests pass locally, which means these cases need explicit regression coverage.
|
|
||
| cls._require_rigid_body_root(articulation_prim) | ||
| create_fixed_root_joint(articulation_prim, stage) | ||
| return articulation_prim |
There was a problem hiding this comment.
Blocking: this base implementation is not sufficient for OVPhysX. OvPhysxManager inherits it, but OVPhysX uses the PhysX parser and does not recognize the resulting topology as fixed-base when ArticulationRootAPI remains on the rigid link. I reproduced this with the official ovphysx==0.4.13 parser: root API on the parent gives is_fixed_base=True, while the topology produced here gives is_fixed_base=False.
The new “Newton/OVPhysX” test only invokes PhysicsManager directly and checks authored USD; it never runs the OVPhysX parser. Please give OVPhysX the PhysX-style relocation/migration behavior (a shared pure-USD helper would avoid duplication) and add a real backend test asserting is_fixed_base.
| # honor an explicit stage so the lookup does not fall back to the global current stage | ||
| existing_fixed_joint_prim = find_global_fixed_joint_prim(root_path, stage=stage) | ||
| if existing_fixed_joint_prim is not None: | ||
| # a joint already exists: just enable/disable it in place (no relocation) |
There was a problem hiding this comment.
Blocking: an existing joint does not make relocation unnecessary. If a pre-authored fixed joint exists while ArticulationRootAPI is still on the rigid link, this branch enables the joint but bypasses physics_manager.fix_articulation_root; PhysX/OVPhysX therefore retain the same floating/maximal-coordinate topology.
The backend capability should own the complete “ensure fixed” operation, including normalizing root placement when the joint already exists (without creating a duplicate). Please cover at least disabled-existing-joint -> fix_root_link=True with the real parser; the current test only covers True -> False.
| stage = get_current_stage() | ||
| # resolve the existing root (it may live on a child prim in USD assets); instance proxies can't be | ||
| # authored on, so don't traverse them | ||
| articulation_prim = get_first_matching_child_prim( |
There was a problem hiding this comment.
Blocking: resolving only the first matching child silently skips sibling articulations. I reproduced /World/A and /World/B, both carrying ArticulationRootAPI: the fragment was authored on A and absent from B. The legacy @apply_nested writer processes every non-nested sibling root, and multi-articulation USD files are valid inputs.
Please resolve and process all top-level/non-nested articulation roots, creating an anchor on prim_path only when no root exists, and aggregate the per-root result.
| new_attr = new_root.GetAttribute(prop_name) | ||
| if not new_attr: | ||
| new_attr = new_root.CreateAttribute(prop_name, attr.GetTypeName()) | ||
| new_attr.Set(attr.Get()) |
There was a problem hiding this comment.
Blocking: this does not preserve valid authored USD data. The source schema is removed before this read, and attr.Get() reads only the default time. I reproduced physxArticulation:sleepThreshold authored only at times 1 and 2: relocation fails with Type mismatch ... expected 'float', got 'void'. If a default is present, the operation succeeds but silently drops the time samples.
Please capture/copy before removing the schema and preserve the authored default, all time samples, metadata, and connections; property/spec-level copying is safer than reducing each attribute to one Get()/Set().
| " 'articulation_props' is a legacy cfg, which owns its own 'fix_root_link' field. Set" | ||
| " it on that cfg instead." | ||
| ) | ||
| schemas.modify_articulation_root_properties(prim_path, articulation_props) |
There was a problem hiding this comment.
Blocking for the advertised legacy transition: NewtonArticulationRootPropertiesCfg(articulation_enabled=False, self_collision_enabled=True) routed here raises ValueError instead of authoring both fields. The root cause is NewtonArticulationRootPropertiesCfg._usd_field_exceptions = {}, which shadows the inherited mapping required for articulation_enabled.
This breaks the base-field-through-backend-subclass invariant established in the same author's merged #5275. Please inherit/merge the base exception mapping and add a regression comparing this legacy cfg with the equivalent PhysX + Newton fragment list.
| final_root_path = final_root.GetPath().pathString | ||
| for cfg in fragments: | ||
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | ||
| func(cfg, final_root_path, stage) |
There was a problem hiding this comment.
The fragment result is discarded, so the family writer returns True even when an applier returns False; I reproduced that with a failing stub cfg.func. This contradicts the documented return contract and the aggregate-result semantics established by the same author's merged #5976 follow-up (2a8b8e7). Please accumulate bool(func(...)) across fragments/roots and add the same failing-applier regression used by the other family writers.
ooctipus
left a comment
There was a problem hiding this comment.
Re-reviewed head d55e0ed against the same property-inventory, backend-ownership, and fragment-composition precedents used in the earlier review. Approved: the fragment inventory remains complete (PhysX 6/6, Newton 1/1), and the direct fixes now cover every top-level root, instance proxies, return-value propagation, existing disabled joints, PhysX/OVPhysX parent-root normalization, lossless authored USD property migration, Newton legacy routing, and explicit-stage isolation. The joint authoring helper is private and the shared relocation boundary carries no backend imports. Local verification: pre-commit and changelog gates passed; 229 focused/neighboring tests passed. The OVPhysX result was also parsed directly as one fixed-base articulation; full local OV reset remains blocked by the pre-existing pinned-wheel set_cpu_mode mismatch, unrelated to this PR.
The manager classes are imported while environment configs load (manager_base pulls PhysicsManager at module import), and config loading must stay free of USD/omni modules before the simulation app starts -- the kitless/Warp config-loading tests enforce this. A module-level "from pxr import ..." here therefore fails config loading for every registered environment. Import pxr inside the two methods that use it instead, mirroring the existing local import that avoids the SimulationContext import cycle. The module-level get_current_stage import is safe to keep: the stage utilities now defer their Isaac Sim integration until Kit is available.
|
CI triage for the failures on 280c20a ( Both are the config-import purity tests ( The module-level
|
…-articulation # Conflicts: # source/isaaclab/isaaclab/physics/physics_manager.py
|
Install-CI note: the |
# Description Adds the **articulation-root** schema-fragment API. - `ArticulationRootFragment` marker (core) plus the PhysX / Newton articulation-root fragments — `PhysxArticulationCfg` (solver iteration counts, sleep / stabilization thresholds, self-collision toggles) and `NewtonArticulationCfg` (native self-collisions), each in its own namespace. There is no core neutral fragment: `UsdPhysics.ArticulationRootAPI` carries no attributes, so the writer applies it as a presence-gated anchor and each backend fragment contributes its own namespaced schema. - `apply_articulation_root_properties` family writer — resolves the existing `UsdPhysics.ArticulationRootAPI` root within the prim subtree and tunes *that* prim; only defines a fresh root on the target prim when the subtree has none. It never duplicates the root (exactly-one-root invariant). The `ArticulationRootAPI` anchor is applied by the writer, not by any fragment. - The non-USD `fix_root_link` flag is a **spawner-cfg field** (`UsdFileCfg.fix_root_link`), passed to the writer as a keyword argument — fragments carry USD attributes only. This is additive; the legacy `articulation_props.fix_root_link` path is untouched. - Fixing an articulation base is a capability of the active physics backend: `PhysicsManager.fix_articulation_root(prim, stage)`, resolved from `cfg.physics.class_type`, authors the world-to-root fixed joint and returns the resulting root prim. The base implementation authors a backend-neutral fixed joint and returns the same prim; PhysX overrides it to relocate the articulation root to the parent (parser workaround) and returns the parent. The writer fixes the root **before** writing fragments, so every fragment lands on the single resulting root regardless of backend — a composed PhysX + Newton fragment list with `fix_root_link=True` leaves exactly one root. Dispatch is resolved through the normal method-resolution order, so cfg subclasses and every backend inherit the correct behaviour without any per-type registration or import-order dependency. - The spawner `articulation_props` slot now also accepts a fragment list (transition bridge routes legacy single cfgs to the existing `define_`/`modify_` writers). The slot is routed by type even when the list is empty, and `fix_root_link` is honored independently of whether schema properties were supplied. Purely **additive** and self-contained: builds only on the schema-fragment base already in `develop`, existing call sites untouched, depends on no other open PR. Local `test_articulation_fragments.py`: 20/20 passing (includes PhysX + Newton root-fixing single-root, manager-subclass capability inheritance, empty/topology-only spawn, and end-to-end `_spawn_from_usd_file` regressions). ## Type of change - New feature (non-breaking change which adds functionality) ## Screenshots N/A — non-visual API change. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Co-authored-by: Octi Zhang <zhengyuz@nvidia.com>
Description
Adds the articulation-root schema-fragment API.
ArticulationRootFragmentmarker (core) plus the PhysX / Newton articulation-root fragments —PhysxArticulationCfg(solver iteration counts, sleep / stabilization thresholds, self-collision toggles) andNewtonArticulationCfg(native self-collisions), each in its own namespace. There is no core neutral fragment:UsdPhysics.ArticulationRootAPIcarries no attributes, so the writer applies it as a presence-gated anchor and each backend fragment contributes its own namespaced schema.apply_articulation_root_propertiesfamily writer — resolves the existingUsdPhysics.ArticulationRootAPIroot within the prim subtree and tunes that prim; only defines a fresh root on the target prim when the subtree has none. It never duplicates the root (exactly-one-root invariant). TheArticulationRootAPIanchor is applied by the writer, not by any fragment.fix_root_linkflag is a spawner-cfg field (UsdFileCfg.fix_root_link), passed to the writer as a keyword argument — fragments carry USD attributes only. This is additive; the legacyarticulation_props.fix_root_linkpath is untouched.PhysicsManager.fix_articulation_root(prim, stage), resolved fromcfg.physics.class_type, authors the world-to-root fixed joint and returns the resulting root prim. The base implementation authors a backend-neutral fixed joint and returns the same prim; PhysX overrides it to relocate the articulation root to the parent (parser workaround) and returns the parent. The writer fixes the root before writing fragments, so every fragment lands on the single resulting root regardless of backend — a composed PhysX + Newton fragment list withfix_root_link=Trueleaves exactly one root. Dispatch is resolved through the normal method-resolution order, so cfg subclasses and every backend inherit the correct behaviour without any per-type registration or import-order dependency.articulation_propsslot now also accepts a fragment list (transition bridge routes legacy single cfgs to the existingdefine_/modify_writers). The slot is routed by type even when the list is empty, andfix_root_linkis honored independently of whether schema properties were supplied.Purely additive and self-contained: builds only on the schema-fragment base already in
develop, existing call sites untouched, depends on no other open PR. Localtest_articulation_fragments.py: 20/20 passing (includes PhysX + Newton root-fixing single-root, manager-subclass capability inheritance, empty/topology-only spawn, and end-to-end_spawn_from_usd_fileregressions).Type of change
Screenshots
N/A — non-visual API change.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there