[Backport release/3.0.0] Drive fragment schema writers by prim path expressions - #7441
Conversation
# Description Moves the physics schema **fragment** surface onto prim path expressions, so a writer targets prims by pattern instead of traversing the subtree of a single input prim. Since this PR was opened, isaac-sim#6841 landed the plain-regex prim path matcher on `develop`. This PR has been rebuilt on top of it: it no longer touches `sim/utils/queries.py` at all, and the bespoke `**` recursive token it originally introduced is gone. Targeting is now expressed entirely in standard Python regular expressions matched against whole prim paths. ## What changes **Family writers take an expression.** `apply_rigid_body_properties`, `apply_collision_properties`, `apply_mass_properties`, `apply_articulation_root_properties`, `apply_joint_drive_properties`, and the two tendon writers now take `prim_path_expr` and resolve targets through `find_matching_prims`. A bare prim path matches only that prim; `f"{prim_path}/.*"` reaches its descendants and `f"{prim_path}(/.*)?"` reaches the prim together with its subtree. **Spawner fields take a mapping.** The fragment fields (`rigid_props`, `collision_props`, `mass_props`, `articulation_props`, `joint_drive_props`, `fixed_tendons_props`, `spatial_tendons_props`) accept `dict[str, list[<Family>Fragment]]`. A key is a regular-expression **suffix appended to the prim the spawner anchors that family on**, so it carries its own leading `/` when it targets descendants: | Key | Selects | |---|---| | `""` | the anchor prim itself | | `"/[^/]+"` | its direct children | | `"/.*"` | all of its descendants | | `"(/.*)?"` | the anchor together with its descendants | Entries apply in insertion order, so on overlapping targets later entries override earlier ones per attribute. Legacy single-cfg values are unaffected and still route to the legacy writers. ```python spawn = sim_utils.UsdFileCfg( usd_path=..., rigid_props={ "(/.*)?": [UsdPhysicsRigidBodyCfg(rigid_body_enabled=True), PhysxRigidBodyCfg(...)], "/.*_hand(/.*)?": [PhysxRigidBodyCfg(max_depenetration_velocity=1.0)], }, ) ``` **Explicit API creation.** The writers no longer apply their defining USD API implicitly on a bare prim. `create_if_missing=True` (and the spawner flags `mass_props_create_if_missing`, `articulation_props_create_if_missing`, `joint_drive_props_create_if_missing`) applies the defining API to matched prims that lack it. **Articulation roots.** `apply_articulation_root_properties` authors on every matched root and warns when roots nest, rather than silently pruning nested ones. Asset validity is the author's responsibility. **Tendon backends de-traverse.** The PhysX and Newton tendon fragment functions now author on the given prim only; target selection is owned by the core family writers. **Deformable meshes accept the mapping too.** On the mesh spawner's deformable branch, `collision_props` given as a mapping previously failed the fragment type check and raised. It also passed the bare body prim to the collision writer, which under expression matching no longer reaches the simulation mesh authored beneath it. Mapping entries now anchor at the body prim (e.g. `{"/sim_mesh": [...]}`) and a bare fragment list uses a subtree pattern to reach the collider. ## Fixes Rigid-body and mass fragments previously reached only the outermost schema-bearing prim on assets with nested rigid-body hierarchies (child links authored under their parent link prims, as produced by the URDF importer in Isaac Sim 6.0 and later). A whole-subtree pattern now reaches every carrier. ## Type of change - New feature (non-breaking change which adds functionality) - This change requires a documentation update **Breaking within the fragment surface only.** The fragment path is experimental and has no in-tree users yet; legacy `define_*` / `modify_*` writers and legacy cfg classes are untouched. Migration for anyone already on fragments: wrap lists as `{"(/.*)?": [...]}`, and pass `create_if_missing=True` where an API was previously created implicitly. ## Screenshots N/A ## Checklist - [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 updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there ## Testing Per-file, via the wrapped Python: | Suite | Result | |---|---| | `test_utils_queries` | 12 passed | | `test_schema_fragments` | 12 passed | | `test_schema_writer_nested_targets` | 13 passed | | `test_mass_fragments` | 9 passed | | `test_collision_fragments` | 19 passed | | `test_articulation_fragments` | 28 passed, 1 xfailed | | `test_joint_drive_fragments` | 22 passed | | `test_tendon_fragments` | 23 passed | | `test_mesh_converter` | 15 passed | | `test_spawn_meshes` (physx) | 3 passed | ### Note on the xfail `test_physx_fix_root_link_migrates_preauthored_newton_root_api` is marked `xfail`. It is not caused by this PR. `develop`'s experience files were restructured so the schema plugin that registered `NewtonArticulationRootAPI` no longer loads, making it an unregistered token schema. `PhysicsManager._relocate_articulation_root` only migrates applied schemas it can resolve through `Usd.SchemaRegistry.FindAppliedAPIPrimDefinition`, so on root relocation the token and its authored `newton:selfCollisionEnabled` value are left stranded on the former root link. Migrating it needs a backend-registered schema-to-namespace mapping, which is out of scope here — flagging it for a follow-up. --------- Co-authored-by: ooctipus <zhengyuz@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com> (cherry picked from commit 76e9cdd)
github-actions
Bot
added
documentation
Greptile SummaryThis backport changes schema-fragment writers and spawners to use whole-path prim expressions, adds explicit missing-API creation controls, and makes backend tendon/root handling per prim.
Confidence Score: 4/5The root-relocation import-order defect should be fixed before merging because pre-authored Newton root schemas can remain stranded when an asset is processed under PhysX. Companion handling for an unregistered Newton token is installed only by importing the optional Newton schemas package, while normal PhysX relocation does not guarantee that import and otherwise skips the schema. Files Needing Attention: source/isaaclab/isaaclab/physics/physics_manager.py; source/isaaclab_newton/isaaclab_newton/sim/schemas/init.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Spawner fragment mapping] --> B[Build whole-path expression]
B --> C[Match USD prims]
C --> D{Family target API present?}
D -- Yes --> E[Dispatch fragments per prim]
D -- No and creation enabled --> F[Apply defining API]
F --> E
E --> G[Shared USD attributes]
E --> H[PhysX attributes]
E --> I[Newton attributes]
J[Root relocation] --> K[USD schema registry]
J --> L[Registered backend companions]
K --> M[Move schemas and properties]
L --> M
Reviews (1): Last reviewed commit: "Drive fragment schema writers by prim pa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| schemas_to_move = [] | ||
| for schema_name in articulation_prim.GetPrimTypeInfo().GetAppliedAPISchemas(): | ||
| definition = registry.FindAppliedAPIPrimDefinition(schema_name) | ||
| companion_namespace_override = _articulation_root_companion_namespace(schema_name) |
There was a problem hiding this comment.
When PhysX relocates an imported articulation carrying NewtonArticulationRootAPI before isaaclab_newton.sim.schemas has been imported, the companion lookup returns None and relocation skips the unregistered token, leaving the Newton root schema and its newton:* attributes on the former root link.
Knowledge Base Used:
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The expression-driven fragment targeting and backend/per-prim writer split are coherent, but the backport introduces a positional API compatibility hazard, an unsafe topology-only articulation creation path, and contradictory FileCfg shorthand documentation.
- Design and architecture: Target matching is cleanly separated from backend-specific per-prim application, including tendon handling and articulation-root companion registration. However, the topology-only articulation path combines the broad subtree expression with create_if_missing, potentially creating and fixing articulation roots on every matched descendant despite having no fragments to author.
- API: The rigid-body, collision, and mass writer signatures insert create_if_missing into the former third-position stage slot. Existing positional callers can therefore silently pass a Stage as the creation flag and operate on the current stage instead. FileCfg also documents bare articulation, tendon, and joint-drive fragments as anchor-only even though implementation expands them across the spawn subtree.
- Implementation: Spawner routing generally follows the new mapping model, but the topology-only branch must not forward articulation_props_create_if_missing when articulation_props is absent or empty. The three affected writer signatures should retain stage as the third parameter, and the FileCfg shorthand documentation should describe the implemented {"(/.*)?": [...]} behavior.
Significant concerns. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| prim_path: str, fragments: Iterable[schemas_cfg.RigidBodyFragment], stage: Usd.Stage | None = None | ||
| prim_path_expr: str, | ||
| fragments: Iterable[schemas_cfg.RigidBodyFragment], | ||
| create_if_missing: bool = False, |
There was a problem hiding this comment.
🟡 Warning · Api — create_if_missing inserted ahead of stage parameter
apply_rigid_body_properties, apply_collision_properties and apply_mass_properties now take create_if_missing in the third position, which previously held stage. An existing positional call apply_rigid_body_properties(path, frags, stage) silently binds the truthy Stage to create_if_missing, applying the defining API to every matched prim while authoring on the current stage. apply_articulation_root_properties and apply_joint_drive_properties keep stage third. Place create_if_missing after stage.
There was a problem hiding this comment.
@vidurv-nvidia @ooctipus this seems like a good comment that we can address? would be good to align the ordering, and help avoid breakage of existing user code as much as we can.
| props_expr(prim_path, "(/.*)?"), | ||
| [], | ||
| fix_root_link=articulation_fix_root_link, | ||
| create_if_missing=cfg.articulation_props_create_if_missing, |
There was a problem hiding this comment.
🟡 Warning · Implementation — Topology-only path forwards articulation creation flag
This branch runs when articulation_props is None or an empty mapping, yet still forwards articulation_props_create_if_missing with the widest expression "(/.*)?". With that flag set, apply_articulation_root_properties applies UsdPhysics.ArticulationRootAPI to every matched descendant lacking it and then fixes each resulting root. The field documents itself as "only consumed when articulation_props is given as fragments". Pass create_if_missing=False here, since no fragments are authored.
| Keys are regular-expression suffixes appended to the spawn prim, so a key carries its own leading ``/`` when it | ||
| targets descendants (``""`` the anchor itself, ``"/[^/]+"`` its direct children, ``"/.*"`` everything beneath it). | ||
| Entries apply in insertion order, so on overlapping targets later entries override earlier ones per attribute. As | ||
| a shorthand for the common case, a bare fragment or a list of fragments is read as ``{"": [...]}``, i.e. the |
There was a problem hiding this comment.
🟡 Warning · Api — FileCfg shorthand documented with wrong target
These docstrings (articulation_props, both tendon fields, joint_drive_props) state a bare fragment or list is read as {"": [...]}, the spawn prim itself. _apply_articulation_schema_properties calls fragment_mapping(..., "(/.*)?"), so the shorthand targets the spawn prim together with its descendants, as the new concept page and changelog state. Users following these docstrings will mis-scope tendon and joint-drive rules. Correct them to {"(/.*)?": [...]}.
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
2 participants
Description
Backports #6640 onto
release/3.0.0.This moves the experimental physics schema-fragment surface to prim-path expressions, adds per-family target mappings to spawner configs, makes schema API creation explicit, and keeps the PhysX/Newton tendon backends per-prim. It also fixes nested rigid-body and mass fragment targeting.
The prerequisite whole-path regular-expression matcher from #6841 is already present on the release branch. This is an exact
cherry-pick -xof the merged commit.Risk assessment
developbase and the current release tip, so the commit applied without conflicts or release-specific edits; source and backport patch IDs match.define_*/modify_*writers are unchanged, and there are no in-tree fragment users.{"(/.*)?": [...]}and passcreate_if_missing=Truewhere API creation was previously implicit.Type of change
Validation
git diff --checkrelease/3.0.0The full project test and docs commands were not rerun locally because the project lockfile supports Linux and Windows, not this macOS host. Release-branch CI should provide the final platform validation. The source PR's unrelated Franka Pour dataset-contract failure and ARM runner-queue cancellation did not involve these paths.
Checklist
CONTRIBUTORS.md