Fix PhysX scene data for joint-labeled bodies

[ooctipus]

Summary

• skip USD joint prims when building the global PhysX scene-data rigid-body view
• resolve Newton visualization body labels that point at USD joints through their rigid-body targets
• add focused regression coverage for the ShadowHand-style joint/body leaf-name collision

Validation

• python3 -m py_compile source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py source/isaaclab_physx/isaaclab_physx/physics/physx_manager.py source/isaaclab/test/sim/test_newton_manager_visualization_state.py source/isaaclab_physx/test/sim/test_physx_scene_data_backend.py
• git diff --check
• focused pytest attempted locally, but this worktree uses /usr/bin/python3.12 without Isaac Lab runtime deps (lazy_loader) and USD bindings (pxr) installed

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — PR #6041

Fix PhysX scene data for joint-labeled bodies by @ooctipus

---

Summary

This PR fixes two related issues when USD assets have rigid-body-annotated joint prims (e.g., ShadowHand-style robots where joints share leaf names with bodies):

1. PhysX scene-data backend (physx_manager.py): Excludes joint prims from the global rigid-body view even if they have RigidBodyAPI applied.
2. Newton visualization sync (newton_manager.py): Resolves body labels that point at joint prims to the actual rigid-body target via the joint's Body1Rel/Body0Rel relationships.

---

✅ Findings

#	Category	Finding	Severity
1	Correctness	The prim.IsA(UsdPhysics.Joint) check in physx_manager.py correctly filters joints that erroneously have RigidBodyAPI. This is the right USD predicate — it covers FixedJoint, RevoluteJoint, PrismaticJoint, etc.	✅ Good
2	Correctness	The _resolve_scene_data_body_paths method correctly uses a for/else pattern to break out of nested loops when the first valid rigid-body target is found.	✅ Good
3	Correctness	The resolution order (Body1 first, then Body0) is correct — Body1 is conventionally the child body in USD physics joints, which is the body that moves.	✅ Good
4	Edge cases	None entries in body_paths are properly skipped. Invalid prims and non-joint prims pass through unchanged. stage is None early-returns the original list.	✅ Good
5	Test coverage	Both fixes have focused regression tests using in-memory USD stages. Tests verify the exact filtering/resolution behavior.	✅ Good
6	Documentation	CHANGELOGs updated for both isaaclab_newton and isaaclab_physx.	✅ Good

---

⚠️ Observations (non-blocking)

1. CI Status: Check changelog fragments and pre-commit checks are currently failing. These should be resolved before merge. The changelog fragment check may require a specific file naming convention — please verify the entries match the expected format.

2. Import location: In _resolve_scene_data_body_paths, from pxr import UsdPhysics is imported inside the method body. This is fine for lazy-loading (avoids import errors when pxr isn't available), but consider noting this pattern if it deviates from the module's conventions.

3. pathString attribute: Line resolved_paths[i] = target_path.pathString — this assumes target_path is an Sdf.Path object (which it is when coming from GetTargets()). Correct usage.

4. Performance: The resolution iterates over all body paths and does USD stage lookups for each. For robots with hundreds of bodies, this runs once at mapping creation time, so the cost is acceptable.

---

Verdict

✅ LGTM — The logic is correct, well-tested, and properly scoped. The fix addresses a real issue where joint prims with RigidBodyAPI corrupt the PhysX rigid-body view tensor shape. The only action items are fixing the failing CI checks (changelog fragments + pre-commit).

---

Update (176ecd1): Reviewed incremental changes. New commits add env destructor cleanup, MDL sibling-module import resolution, lift task consolidation (removing -v0 suffixes), AutoMate SoftDTW Numba→Torch rewrite, skrl JAX import fix, and optional wheeled-robot extensions. No new issues found. Previous P2 inline comments (test style, Body0Rel fallback coverage) remain unaddressed but are non-blocking.

---

Update (a6714f2): Reviewed the incremental diff (176ecd1→a6714f26). The new commits are a large-scale task-renaming and package-consolidation refactor (e.g. Isaac-Repose-* → Isaac-Reorient-*, inhand → reorient, shadow_hand_over → handover, cabinet direct-workflow extraction, device_split CI infrastructure). These changes are unrelated to the PhysX scene-data fix. My previous P2 inline comments (test pytest.importorskip placement, untested Body0Rel fallback path) were not addressed — the affected files were not modified in this push. No new issues found in the incremental changes relevant to this PR's scope.

[greptile-apps]

Greptile Summary

This PR fixes two related PhysX/Newton interop bugs that appear with ShadowHand-style USD assets where a joint prim has the same leaf name as the child rigid body. The physx_manager traversal now skips joint prims (even when RigidBodyAPI is applied to them), and Newton's visualization sync resolves body labels that point at a joint prim to the joint's actual rigid-body target via Body1Rel/Body0Rel.

• physx_manager.py: Adds and not prim.IsA(UsdPhysics.Joint) to the rigid-body discovery loop; the excluded joint's leaf name correctly falls through to non_rigid_body_names, preserving the existing wildcard-collision-avoidance logic for real rigid bodies with the same name.
• newton_manager.py: New _resolve_scene_data_body_paths static method wraps the mapping creation step; uses a for-else labeled-break to prefer Body1Rel over Body0Rel when looking up the rigid-body target of a joint prim.
• Tests: Focused regression tests added for both fixes; the physx test correctly uses module-level pytest.importorskip, while the newton test imports pytest and calls importorskip inside the function body (inconsistent style). The Body0Rel fallback and unresolved-path fallback in _resolve_scene_data_body_paths are not covered by any test.

Confidence Score: 4/5

Both fixes are small, well-scoped, and safe to merge; the only open questions are test coverage gaps for the Body0Rel fallback path.

The core logic changes are a single guard added to a traversal loop and a new static resolution method — both are straightforward. The for-else labeled-break in _resolve_scene_data_body_paths is correct Python and the usd_stage attribute is consistently defined on SceneDataProvider. The main gap is that the Body1Rel→Body0Rel priority and the 'no valid target → keep original path' fallback are untested, which leaves a refactoring risk around the most nuanced control flow in the new method.

newton_manager.py deserves a second look at the for-else fallback paths; the new test in test_newton_manager_visualization_state.py only exercises the Body1Rel happy path.

Important Files Changed

Filename	Overview
source/isaaclab_physx/isaaclab_physx/physics/physx_manager.py	One-line fix adding and not prim.IsA(UsdPhysics.Joint) to the rigid-body traversal guard; correctly excludes joint prims (even those with RigidBodyAPI) from the PhysX tensor view and, as a consequence, adds their leaf names to non_rigid_body_names so the collision-name detection path continues to work for matching rigid bodies.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	New _resolve_scene_data_body_paths static method and call-site wiring; uses the for-else labeled-break pattern correctly to prefer Body1Rel over Body0Rel when resolving a joint to its rigid-body target, with None-path and invalid-prim guards. Logic is sound.
source/isaaclab/test/sim/test_newton_manager_visualization_state.py	New test covers the Body1Rel happy path but imports pytest inside the function body (inconsistent with module-level skip pattern) and doesn't exercise the Body0Rel fallback or the unresolved-path fallback.
source/isaaclab_physx/test/sim/test_physx_scene_data_backend.py	New focused regression test that verifies joint prims with RigidBodyAPI are excluded from the captured rigid-body paths; correctly uses module-level pytest.importorskip("pxr") and monkeypatches omni.usd.get_context.
source/isaaclab_newton/docs/CHANGELOG.rst	CHANGELOG entry added for the Newton visualizer fix under the current unreleased section; correct placement and wording.
source/isaaclab_physx/docs/CHANGELOG.rst	CHANGELOG entry added for the PhysX rigid-body view discovery fix; correct placement under the current unreleased section.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[stage.Traverse prim] --> B{prim.HasAPI RigidBodyAPI?}
    B -- No --> C{matches env_\\d+ path?}
    B -- Yes --> D{prim.IsA Joint?}
    D -- No --> E[Add to rigid_body_paths]
    D -- Yes --> C
    C -- Yes --> F[Add leaf name to non_rigid_body_names]
    C -- No --> G[Skip]
    E --> H{leaf name in non_rigid_body_names?}
    H -- Yes --> I[Add to exact_paths]
    H -- No --> J[Add to wildcard patterns]
    I --> K[create_rigid_body_view]
    J --> K

    subgraph Newton resolve
        L[body_paths from model.body_label] --> M{prim.IsA Joint?}
        M -- No --> N[Keep original path]
        M -- Yes --> O{Body1Rel has RigidBodyAPI target?}
        O -- Yes --> P[Use Body1Rel target path]
        O -- No --> Q{Body0Rel has RigidBodyAPI target?}
        Q -- Yes --> R[Use Body0Rel target path]
        Q -- No --> N
    end

Loading

_{Reviews (1): Last reviewed commit: "Fix PhysX scene data for joint-labeled b..." | Re-trigger Greptile}

[greptile-apps]

The import pytest / pytest.importorskip("pxr") placed inside the test function body is inconsistent with the companion test file test_physx_scene_data_backend.py, which calls pytest.importorskip at module level (line 10) so the entire module is skipped when pxr is absent. Embedding it inside the function still works, but it means the rest of the module is imported and any top-level side effects run before the skip fires.

Suggested change

	def test_resolve_scene_data_body_paths_uses_joint_body_targets():
	"""PhysX visualization sync maps Newton joint labels to the actual body prim path."""
	import pytest
	pytest.importorskip("pxr")
	from pxr import Usd, UsdGeom, UsdPhysics
	def test_resolve_scene_data_body_paths_uses_joint_body_targets():
	"""PhysX visualization sync maps Newton joint labels to the actual body prim path."""
	pxr = pytest.importorskip("pxr")
	Usd = pxr.Usd
	UsdGeom = pxr.UsdGeom
	UsdPhysics = pxr.UsdPhysics

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!

[greptile-apps]

Body0Rel fallback path is untested

The Body1Rel→Body0Rel priority logic (the for-else labeled-break) is the only non-trivial control flow in _resolve_scene_data_body_paths, but the new test only exercises Body1Rel. A joint whose Body1Rel is empty (or has no RigidBodyAPI target) would fall through to Body0Rel; without a test, a future refactor of the nested for-else could silently break that fallback. Similarly, the "no valid target on either rel → path unchanged" case is uncovered.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — PR #6041 (Update)

Reviewed incremental changes: c00ce4a → a6714f2

---

Changes Since Last Review

The PR-relevant files were modified in this push. Here's what's new:

Newton Manager Refactoring (newton_manager.py)

#	Change	Assessment
1	Extracted _initialize_fabric_body_prims() as a static method	✅ Good refactor — improves testability and separation of concerns
2	Added _cl_fabric_body_bindings class variable for body-index mapping	✅ Cleaner than inline construction
3	clear() now resets _cl_fabric_body_bindings = None	✅ Proper cleanup
4	Uses fabric_hierarchy.update_world_xforms() instead of per-prim SetWorldXformFromUsd()	✅ More efficient batch update

Core Fix (unchanged logic, now in final position)

• _resolve_scene_data_body_paths(): The joint-to-body resolution logic is intact and correctly placed.
• physx_manager.py: The prim.IsA(UsdPhysics.Joint) skip is present at line 194.

Tests

Both test files are new additions that verify the fix:

• test_newton_manager_visualization_state.py: Tests joint→body path resolution
• test_physx_scene_data_backend.py: Tests joint prim exclusion from rigid body views

---

💭 Observations

1. Test placement of pytest.importorskip: In test_newton_manager_visualization_state.py, the importorskip is inside the test function (line 166). This works but means the skip message will show the test name rather than skipping at module load. The physx test file places it at module level (line 10-11), which is the more conventional pattern. Non-blocking style note.

2. Body0Rel fallback path: The _joint_body_path helper iterates Body1Rel then Body0Rel, returning on first valid target. If Body1 targets an invalid prim but Body0 is valid, it falls through correctly. However, there's no test for the Body0Rel fallback path. Consider adding one for completeness. Non-blocking.

3. Fabric body bindings: The new _cl_fabric_body_bindings is populated externally (by NewtonReplicateContext presumably). If it's None, the code constructs bindings from body_label directly. This dual-path is sensible for supporting both replicated and non-replicated scenes.

---

Verdict

✅ LGTM — The core fix is correct and well-tested. The refactoring of start_simulation is a clean improvement. Previous observations about CI failures (changelog fragments, pre-commit) still apply if not yet resolved.

---

Reviewed commit: a6714f2