Skip to content

Fix fragment writers to reach nested rigid-body and mass prims#6635

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

Fix fragment writers to reach nested rigid-body and mass prims#6635
vidurv-nvidia wants to merge 1 commit into
isaac-sim:developfrom
vidurv-nvidia:vidurv/schema-frag-descend-nested

Conversation

@vidurv-nvidia

Copy link
Copy Markdown

Description

Follow-up reconciling the fragment writers with #6377, which merged the same day as #6501.

#6377 established that nested rigid-body hierarchies are legitimate asset topology: the URDF importer in Isaac Sim 6.0 and later authors child links under their parent link prims. It fixed the legacy modify_rigid_body_properties / modify_mass_properties path via apply_nested(stop_on_success=False) so every link is reached. The fragment target resolver introduced in #6501 still stops descending at the first schema-bearing prim on each branch, so on such assets the fragment path modifies only the outermost link — silently reproducing the exact 1-of-N-links bug #6377 fixed on the legacy path.

This PR restores legacy parity:

  • _resolve_fragment_targets gains a stop_at_carrier flag mirroring apply_nested's stop_on_success. The rigid-body and mass fragment writers now collect every carrier in the subtree; the collision writer keeps stop-at-carrier traversal, matching the legacy collision writer, which [Fix] Apply rigid-body and mass properties to every link of nested-hierarchy assets #6377 left on the default.
  • Docstrings no longer claim nested applications of the same physics schema are not allowed.
  • The nested-target regression tests author a nested child link in the test asset, so the spawn-path tests, the fragment-vs-legacy parity test, and a new direct-writer test all cover the nested topology. Without the fix, four of them fail (nested link untouched by the fragment path while legacy reaches it); with it, all pass, and the legacy schema tests still pass (41/41).

Fixes the fragment-path counterpart of #6377.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

The fragment target resolver stopped descending at the first
schema-bearing prim on each branch, so 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) only the
outermost link received rigid-body and mass fragment properties. The
legacy writers gained full-subtree traversal for these families in
isaac-sim#6377, so the fragment writers diverged from legacy parity.

Add a stop_at_carrier flag to the resolver, mirroring apply_nested's
stop_on_success: rigid-body and mass writers now collect every carrier
in the subtree, while collision keeps the legacy stop-at-carrier
traversal. Extend the nested-target regression tests with a nested
child link so the flat-asset, parity, and direct-writer tests all cover
the nested topology.
@vidurv-nvidia
vidurv-nvidia requested a review from a team July 20, 2026 19:34
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Jul 20, 2026
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a traversal bug in the fragment-based physics writers (apply_rigid_body_properties, apply_mass_properties) where the BFS stopped descending at the first schema-bearing prim on each branch, silently skipping nested rigid-body or mass carriers that the URDF importer legally produces by nesting child-link prims under their parent-link prims.

  • Adds a stop_at_carrier boolean to _resolve_fragment_targets; rigid-body and mass writers pass False to collect every carrier in the subtree, while the collision writer keeps True, preserving legacy stop-on-success traversal.
  • Adds a nested child link (link2/link3) to the shared test asset and expands all spawn-path tests plus a new direct-writer test to assert every body/mass prim is reached; removes the old test_rigid_body_fragments_do_not_descend_past_existing_body test, which asserted the now-fixed (wrong) stop behaviour.

Confidence Score: 5/5

Safe to merge. The traversal logic change is minimal, well-contained in a private helper, and each public writer explicitly opts in or out of the new behaviour.

The fix is a single guarded continue inside a BFS loop. The private helper's callers are all in the same file, so the new required parameter cannot be missed. The collision writer deliberately keeps the old stop-at-first-carrier traversal, and the test for the nested-topology asset verifies that all three colliders are still reached because stop-at-carrier applies at the collider prim level, not the link level.

No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/schemas/schemas.py Adds stop_at_carrier parameter to _resolve_fragment_targets; rigid-body and mass callers pass False, collision passes True. BFS logic is correct: the continue that previously always skipped a carrier's children is now guarded by stop_at_carrier, so nested carriers are collected when needed.
source/isaaclab/test/sim/test_schema_writer_nested_targets.py Test asset gains a nested child link (link2/link3); all spawn-path assertion loops now iterate over LINK_REL_PATHS including the nested path. The old test that verified the buggy stop-at-carrier behaviour is replaced with test_rigid_body_and_mass_fragments_modify_nested_bodies. Coverage is thorough.
source/isaaclab/changelog.d/vidurv-descend-nested-fragments.rst New changelog fragment accurately describes the fix and names the affected public API functions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_resolve_fragment_targets"] --> B["BFS queue = [root prim]"]
    B --> C{Next candidate in queue?}
    C -- No --> E{targets empty AND apply_fresh?}
    C -- Yes --> D{candidate.HasAPI api_type?}
    D -- No --> H["Add GetFilteredChildren to queue"]
    H --> C
    D -- Yes --> F{IsInstance or IsInstanceProxy?}
    F -- Yes --> G["skipped.append(candidate)"]
    F -- No --> I["targets.append(candidate)"]
    G --> J{stop_at_carrier?}
    I --> J
    J -- True --> K["continue skip children"]
    K --> C
    J -- False --> H
    E -- Yes --> L["Apply api_type to prim_path"]
    E -- No --> M["Return targets, bool skipped"]
    L --> M
    N["apply_rigid_body_properties"] -->|stop_at_carrier=False| A
    O["apply_mass_properties"] -->|stop_at_carrier=False| A
    P["apply_collision_properties"] -->|stop_at_carrier=True| A
Loading
%%{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"}}}%%
flowchart TD
    A["_resolve_fragment_targets"] --> B["BFS queue = [root prim]"]
    B --> C{Next candidate in queue?}
    C -- No --> E{targets empty AND apply_fresh?}
    C -- Yes --> D{candidate.HasAPI api_type?}
    D -- No --> H["Add GetFilteredChildren to queue"]
    H --> C
    D -- Yes --> F{IsInstance or IsInstanceProxy?}
    F -- Yes --> G["skipped.append(candidate)"]
    F -- No --> I["targets.append(candidate)"]
    G --> J{stop_at_carrier?}
    I --> J
    J -- True --> K["continue skip children"]
    K --> C
    J -- False --> H
    E -- Yes --> L["Apply api_type to prim_path"]
    E -- No --> M["Return targets, bool skipped"]
    L --> M
    N["apply_rigid_body_properties"] -->|stop_at_carrier=False| A
    O["apply_mass_properties"] -->|stop_at_carrier=False| A
    P["apply_collision_properties"] -->|stop_at_carrier=True| A
Loading

Reviews (1): Last reviewed commit: "Fix fragment writers to reach nested rig..." | Re-trigger Greptile

@vidurv-nvidia

Copy link
Copy Markdown
Author

Superseded by #6640: the fragment writers are moving from subtree traversal to explicit prim-path-expression targeting, which removes the traversal policy this PR patched. The nested-hierarchy regression tests and the fix itself are carried inside #6640 (this branch is part of its history), so nested URDF-importer assets are covered there. Closing to keep a single review surface — the fragment path is still experimental, so there is no released behavior needing an interim fix.

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

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant