fix: add PrepareForReuse to FabricFrameView, remove sync_usd_on_fabric_write

[pv-nvidia]

Summary

Replace the sync_usd_on_fabric_write workaround in FabricFrameView with proper PrepareForReuse() calls on the Fabric PrimSelection. This tells the renderer (FSD/Storm) that Fabric data is about to be modified, so the next rendered frame reflects updated transforms — eliminating the need to copy Fabric writes back to USD.

Motivation

The existing sync_usd_on_fabric_write flag worked by mirroring every Fabric write back to USD, which defeated the performance benefits of Fabric. With Kit 110.1.1 (build 300709+), PrepareForReuse() properly notifies the rendering pipeline of Fabric data changes, including camera transforms.

This addresses two of the issues raised in Piotr Barejko's review of PR #4923:

• Issue [Bug Report] Program aborted the execution due to unhandled error when runing API Demos 1 and 2 #1 (USD write-back): Fabric writes no longer sync back to USD
• Issue [Question] Visual Observations Setup? #4 (PrepareForReuse): Renderer notification via PrepareForReuse() instead of USD writeback

Changes

Core (FabricFrameView)

• Call _prepare_for_reuse() before every Fabric read/write to notify the renderer
• Remove sync_usd_on_fabric_write parameter (accepted via **kwargs with deprecation warning for backward compat)
• Add _rebuild_fabric_arrays() for topology change recovery when PrepareForReuse() returns True

Camera

• Remove sync_usd_on_fabric_write=True from FrameView construction in camera.py

Headless rendering

• Add omni.kit.viewport.window and omni.kit.hydra_texture to isaaclab.python.headless.rendering.kit (required for camera render tests, also fixes fix: add missing viewport.window + hydra_texture deps for headless rendering #5375)

Tests Added

Test	What it validates
test_camera_pose_update_reflected_in_render	Camera pose changes propagate to rendered depth (close vs far) for CPU/GPU, tiled/non-tiled
test_fabric_set_world_does_not_write_back_to_usd	Fabric writes stay in Fabric, USD prim unchanged
test_prepare_for_reuse_detects_topology_change	PrepareForReuse API returns correct topology status
test_set_world_updates_local (xfail)	Documents Issue #5: set_world_poses doesn't update local pose in Fabric mode

Test Results

# Fabric contract tests
17 passed, 15 skipped (CPU), 1 xfailed

# Camera tests (including render validation)
12 passed

# USD contract tests (regression check)
45 passed

Dependencies

Requires Kit 110.1.1 build 300709+ (with PrepareForReuse camera fix).

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR replaces the sync_usd_on_fabric_write workaround in FabricFrameView with proper PrepareForReuse() calls to notify the RTX renderer of Fabric data changes. The change removes the performance-defeating USD writeback pattern while maintaining correct rendering behavior. The implementation is architecturally sound with good test coverage, though there are a few correctness concerns around the placement and frequency of _prepare_for_reuse() calls.

Architecture Impact

• Camera sensor: Removes sync_usd_on_fabric_write=True from FrameView construction, now relying on PrepareForReuse() for renderer synchronization.
• FabricFrameView: Internal API change - sync_usd_on_fabric_write parameter removed (with deprecation warning via **kwargs).
• Downstream callers: Any code passing sync_usd_on_fabric_write=True to FrameView will now get a deprecation warning but continue to function (the flag is simply ignored).
• Kit extensions: Adds omni.kit.viewport.window and omni.kit.hydra_texture as dependencies for headless rendering.

Implementation Verdict

Minor fixes needed — The core approach is correct, but _prepare_for_reuse() is called in getters where it shouldn't be, and there's a potential race condition in the initialization flow.

Test Coverage

Good coverage for this change:

• test_camera_pose_update_reflected_in_render: Validates that pose changes propagate to rendered depth (critical regression test)
• test_fabric_set_world_does_not_write_back_to_usd: Confirms USD writeback removal
• test_prepare_for_reuse_detects_topology_change: Basic API contract test
• test_set_world_updates_local: Properly marked as xfail documenting known limitation

Missing: No test for the topology-changed path (_rebuild_fabric_arrays). The test only verifies PrepareForReuse() returns False when topology hasn't changed.

CI Status

No CI checks available yet — cannot verify the changes work on the required Kit version.

Findings

🟡 Warning: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:189-190 — _prepare_for_reuse() called in get_world_poses() is semantically incorrect

PrepareForReuse() is a write notification mechanism per the PR description ("tells the renderer that Fabric data is about to be modified"). Calling it before a read operation is incorrect — the purpose is to notify before writes, not reads. This could cause unnecessary renderer invalidation on every pose query.

def get_world_poses(self, indices=None):
    ...
    self._prepare_for_reuse()  # <-- Should not be here for a getter

Same issue at line 281 in get_scales(). The calls should only be in set_world_poses() and set_scales().

🟡 Warning: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:146 — _prepare_for_reuse() called before Fabric arrays may be valid

In set_world_poses(), _prepare_for_reuse() is called at line 146, but _fabric_selection is only set during _initialize_fabric() which happens at line 143. On the first call, _initialize_fabric() runs, sets _fabric_selection, then _prepare_for_reuse() is called. However, if _initialize_fabric() fails partway through, _fabric_selection could be partially initialized. The guard in _prepare_for_reuse() (line 320: if self._fabric_selection is None) handles this, but the ordering is confusing.

Consider calling _prepare_for_reuse() after the writes complete, not before.

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:326-327 — Topology change handling logs at INFO but doesn't re-sync from USD

When topology changes, _rebuild_fabric_arrays() rebuilds the index mappings but doesn't re-sync transforms from USD. If prims were added, their initial Fabric world matrices may be stale. The existing _sync_fabric_from_usd_once() is a one-shot sync; a topology change might warrant another sync.

def _prepare_for_reuse(self) -> None:
    ...
    if topology_changed:
        logger.info("Fabric topology changed — rebuilding view-to-fabric index mapping.")
        self._rebuild_fabric_arrays()
        # Consider: self._fabric_usd_sync_done = False  # Force re-sync

🔵 Improvement: source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py:127-132 — The xfail override shadows the parametrized device fixture

The test function test_set_world_updates_local takes device and view_factory as parameters, but there's no @pytest.mark.parametrize("device", ...) decorator. This relies on fixture injection from the shared contract utils. If the shared test was parametrized with multiple devices, this override won't run for all of them.

@pytest.mark.xfail(...)
def test_set_world_updates_local(device, view_factory):  # Where does 'device' come from?

Verify that the device fixture is properly inherited or add explicit parametrization.

🔵 Improvement: source/isaaclab/test/sensors/test_tiled_camera.py:215-216 — Test creates camera without cleanup guard

The setup_camera fixture cleans up a specific camera, but test_camera_pose_update_reflected_in_render creates an additional camera at /World/PoseTestCam. While del camera is called at line 268, if an assertion fails before that line, the camera may leak.

Consider using a context manager or adding the camera to a cleanup list.

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:63-71 — Constructor signature changed without updating all callers

The old signature had sync_usd_on_fabric_write: bool = False as a positional-or-keyword parameter. Now it's removed and handled via **kwargs. The test file at line 97 shows the update, but any external code calling FabricFrameView(..., sync_usd_on_fabric_write=True) will get a deprecation warning rather than a hard error. This is fine for backward compatibility but should be documented in the changelog.

[greptile-apps]

Greptile Summary

This PR replaces the sync_usd_on_fabric_write workaround in FabricFrameView with proper PrepareForReuse() calls on the Fabric PrimSelection, eliminating the performance penalty of mirroring every Fabric write back to USD. The change requires Kit 110.1.1 build 300709+ and is accompanied by new contract tests that verify no USD writeback occurs, topology-change detection, and render-visible camera pose updates.

Confidence Score: 5/5

Safe to merge; all findings are P2 style/documentation suggestions with no impact on correctness.

The core logic is sound — sync_usd_on_fabric_write is fully removed, PrepareForReuse is wired in correctly for write paths, and the deprecation shim preserves backward compatibility. The three P2 findings (PrepareForReuse on reads, incomplete rebuild documentation, missing CI marker) are non-blocking quality observations that do not affect runtime correctness.

fabric_frame_view.py — review the PrepareForReuse-on-reads concern and add a comment in _rebuild_fabric_arrays explaining the count-invariant assumption.

Important Files Changed

Filename	Overview
source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py	Core change: replaces sync_usd_on_fabric_write with PrepareForReuse(); adds topology-change rebuild path. PrepareForReuse is called on both reads and writes, which may cause unnecessary renderer dirty signals on reads.
source/isaaclab/isaaclab/sensors/camera/camera.py	Removes sync_usd_on_fabric_write=True from FrameView construction; clean, minimal change.
source/isaaclab/isaaclab/sim/views/usd_frame_view.py	Minor docstring cleanup removing reference to sync_usd_on_fabric_write; no logic changes.
source/isaaclab/test/sensors/test_tiled_camera.py	Adds render-validation test for PrepareForReuse/Fabric camera pose path; missing @pytest.mark.isaacsim_ci marker may exclude it from CI runs.
source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py	Adds three Fabric-specific tests (no-USD-writeback, PrepareForReuse return type, xfail for Issue #5); well structured with proper xfail annotation.
apps/isaaclab.python.headless.rendering.kit	Adds omni.kit.viewport.window and omni.kit.hydra_texture extensions for headless camera render tests; well-commented additions.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant FabricFrameView
    participant FabricSelection
    participant WarpKernel
    participant Renderer

    Caller->>FabricFrameView: set_world_poses(positions, orientations)
    FabricFrameView->>FabricSelection: PrepareForReuse()
    FabricSelection-->>FabricFrameView: topology_changed (bool)
    alt topology changed
        FabricFrameView->>FabricFrameView: _rebuild_fabric_arrays()
    end
    FabricSelection-->>Renderer: [dirty signal — next frame picks up Fabric data]
    FabricFrameView->>WarpKernel: compose_fabric_transformation_matrix(...)
    WarpKernel->>FabricSelection: write worldMatrix
    FabricFrameView->>FabricFrameView: _fabric_hierarchy.update_world_xforms()

    Caller->>FabricFrameView: get_world_poses()
    FabricFrameView->>FabricSelection: PrepareForReuse()
    FabricSelection-->>FabricFrameView: topology_changed (bool)
    FabricFrameView->>WarpKernel: decompose_fabric_transformation_matrix(...)
    WarpKernel-->>FabricFrameView: positions_wp, orientations_wp
    FabricFrameView-->>Caller: (positions_wp, orientations_wp)

Loading

_{Reviews (1): Last reviewed commit: "fix: add PrepareForReuse to FabricFrameV..." | Re-trigger Greptile}

[greptile-apps]

PrepareForReuse called on pure reads

_prepare_for_reuse() is invoked inside both get_world_poses() (line 190) and get_scales() (line 282), operations that do not modify Fabric data. According to the PR description, PrepareForReuse() "tells the renderer that Fabric data is about to be modified" — signalling a modification on every read may cause the renderer to unnecessarily dirty its render state and force re-renders even when no transform has changed.

The topology-change-detection benefit (return value handling) is legitimate for reads, but the renderer-notification side effect is not needed here. Consider separating the two concerns: call PrepareForReuse() only in write paths for renderer notification, and poll a separate topology-change check in read paths, or at minimum document why the dirty signal on reads is acceptable.

[greptile-apps]

Incomplete rebuild after topology change

_rebuild_fabric_arrays() recreates _view_to_fabric, _fabric_to_view, and _fabric_world_matrices, but leaves _fabric_positions_buf, _fabric_orientations_buf, _fabric_scales_buf, _fabric_dummy_buffer, and _default_view_indices pointing to buffers sized at the original self.count. Since self.count is derived from _usd_view.count (fixed by the USD prim-path pattern) and Fabric topology changes only rearrange internal memory without adding/removing the tracked prims, the sizes stay correct in practice — but this invariant is not documented. A brief comment (or an assertion assert self.count == self._default_view_indices.shape[0]) would make the assumption explicit and guard against future changes that could break the rebuild path.

[greptile-apps]

Missing @pytest.mark.isaacsim_ci marker

Every other test in this file has @pytest.mark.isaacsim_ci, but test_camera_pose_update_reflected_in_render does not. If CI filters on that marker, the render-validation test — the primary integration guard for the PrepareForReuse path — will be silently skipped in CI runs. If this is intentional (e.g., it requires a full RTX renderer not available in CI), a comment explaining the omission would prevent future confusion.

[pv-nvidia]

Code Coverage — fabric_frame_view.py

Name                                           Stmts   Miss  Cover   Missing
-----------------------------------------------------------------------------
fabric_frame_view.py                             201     48    76%   ...

Coverage breakdown:

• ✅ set_world_poses / get_world_poses: fully covered
• ✅ set_local_poses / get_local_poses: covered (USD fallback path)
• ✅ _prepare_for_reuse(): covered
• ✅ _initialize_fabric(): covered
• ⬜ get_scales / set_scales: not tested (no scale tests in contract suite)
• ⬜ CPU fallback path: not tested (CPU skips expected — Warp fabricarray limitation)
• ⬜ _rebuild_fabric_arrays(): topology rebuild branch not triggered during test
• ⬜ _sync_fabric_from_usd_once(): deferred sync path

Test results (17 passed, 15 CPU-skipped, 1 xfail):

• All 16 contract tests pass on cuda:0
• test_set_world_updates_local is xfail (fixed in PR fix: implement Fabric-aware get/set_local_poses in FabricFrameView #5381)
• test_fabric_set_world_does_not_write_back_to_usd ✅
• test_prepare_for_reuse_detects_topology_change ✅

[pv-nvidia]

Updated Code Coverage — fabric_frame_view.py

Name                   Stmts   Miss  Cover
--------------------------------------------
fabric_frame_view.py     201     35    83%

Added test_get_scales_fabric_path — coverage now 83%.

Remaining uncovered lines (all expected/acceptable):

• CPU fallback/warning paths — Warp fabricarray limitation
• _rebuild_fabric_arrays() topology rebuild branch
• _sync_fabric_from_usd_once() — deferred sync edge case
• set_scales write path — no set_scales in contract suite
• set_local_poses / get_local_poses — USD fallback (Fabric path added in PR fix: implement Fabric-aware get/set_local_poses in FabricFrameView #5381)