feat: add fabric_read/fabric_write context managers to FabricFrameView

[pv-nvidia]

Summary

Add RAII-style fabric_read() and fabric_write() context managers to FabricFrameView for safe raw Fabric access.

Problem

Users who need to launch custom Warp kernels against Fabric data must manually call PrepareForReuse(), update_world_xforms(), and manage sync state. Forgetting any of these steps leads to stale data or renderer notification bugs that are hard to debug.

This was Piotr's Issue #6 (reader/writer pattern).

Solution

Context Managers

# Safe Fabric write
with view.fabric_write() as fab:
    wp.launch(my_kernel, dim=N, inputs=[fab.world_matrices, fab.view_to_fabric, ...])
# PrepareForReuse on entry, update_world_xforms + sync on exit

# Safe Fabric read
with view.fabric_read() as fab:
    wp.launch(my_read_kernel, dim=N, inputs=[fab.world_matrices, ...])
# PrepareForReuse on entry (ensures valid pointers), no-op on exit

Properties

• view.world_matrices — raw fabricarray of omni:fabric:worldMatrix
• view.view_to_fabric_mapping — view-index to Fabric-index mapping

Test Results

# Fabric tests (including 2 new context manager tests)
19 passed, 15 skipped (CPU), 1 xfailed

Dependencies

Depends on PR #5380 (fix/fabric-prepare-for-reuse).

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds RAII-style fabric_read() and fabric_write() context managers to FabricFrameView, enabling safe raw Fabric access for custom Warp kernels. It also removes the deprecated sync_usd_on_fabric_write parameter in favor of calling PrepareForReuse() to notify the renderer of Fabric data changes. The implementation is generally correct and well-tested, but there are a few issues that should be addressed.

Architecture Impact

• Camera sensor (camera.py): Now relies on PrepareForReuse() instead of sync_usd_on_fabric_write. This is a behavioral change—Fabric writes no longer sync back to USD, which is intentional but changes observable behavior.
• FabricFrameView public API: Adds fabric_read(), fabric_write(), world_matrices, and view_to_fabric_mapping as new public API surface. These expose internal Fabric arrays, creating a contract that must be maintained.
• Dependencies: Adds omni.kit.viewport.window and omni.kit.hydra_texture to the headless rendering kit file, which may affect startup time and memory usage.

Implementation Verdict

Minor fixes needed

Test Coverage

Good coverage for the new context managers with test_fabric_write_context_manager and test_fabric_read_context_manager. The test_camera_pose_update_reflected_in_render test validates the PrepareForReuse integration with the renderer. However, tests for topology change handling in _rebuild_fabric_arrays are limited to verifying the return type of PrepareForReuse() without actually triggering a topology change.

CI Status

No CI checks available yet.

Findings

🟡 Warning: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:340-343 — _rebuild_fabric_arrays doesn't rebuild all cached buffers

When topology changes, _rebuild_fabric_arrays rebuilds _view_to_fabric, _fabric_to_view, and _fabric_world_matrices, but does NOT rebuild the cached position/orientation/scale buffers (_fabric_positions_buf, _fabric_orientations_buf, _fabric_scales_buf). If the prim count changes, these buffers would have the wrong size, causing kernel launch dimension mismatches.

def _rebuild_fabric_arrays(self) -> None:
    """Rebuild fabricarray and view↔fabric mappings after a topology change."""
    self._view_to_fabric = wp.zeros((self.count,), dtype=wp.uint32, device=self._fabric_device)
    # ...
    # Missing: _fabric_positions_buf, _fabric_orientations_buf, _fabric_scales_buf

🟡 Warning: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:525 — _FabricWriteContext.__exit__ doesn't call _prepare_for_reuse() after hierarchy update

The docstring says it calls "PrepareForReuse() on exit", but the actual implementation only calls update_world_xforms() without a subsequent _prepare_for_reuse(). This means the renderer isn't notified of the write completion in the same way as set_world_poses().

def __exit__(self, exc_type, exc_val, exc_tb):
    if exc_type is None:
        wp.synchronize()
        self._view._fabric_hierarchy.update_world_xforms()
        self._view._fabric_usd_sync_done = True
    return False  # Missing: self._view._prepare_for_reuse()

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:359-370 — Context manager properties access private members without validation

The world_matrices and view_to_fabric properties on the context managers access _fabric_world_matrices and _view_to_fabric directly. While __enter__ ensures initialization, a user could still access these properties before entering the context or after a topology change invalidates them during the context.

@property
def world_matrices(self) -> "wp.fabricarray":
    """The fabricarray of omni:fabric:worldMatrix."""
    return self._view._fabric_world_matrices  # Could be stale after topology change

🔵 Improvement: source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py:119-126 — test_set_world_updates_local shadow definition

The test imports the shared test via from frame_view_contract_utils import * (line ~19) and then redefines it with xfail. This works but relies on pytest's name-based collection. Consider using pytest.param with marks or a different function name to make the override relationship explicit.

🔵 Improvement: source/isaaclab/test/sensors/test_tiled_camera.py:248-267 — Test doesn't clean up camera on failure

The test creates a camera and uses del camera at the end, but if an assertion fails mid-test, the camera won't be cleaned up. Consider using a fixture or try/finally.

def test_camera_pose_update_reflected_in_render(setup_camera, device, camera_cls):
    # ...
    camera = camera_cls(cam_cfg)
    # ... assertions that could fail ...
    del camera  # Not reached on assertion failure

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py:376-377 — world_matrices and view_to_fabric_mapping use getattr with None default

These properties use getattr(self, "_fabric_world_matrices", None) which is fine, but the return type annotation says wp.fabricarray | None while the internal code assumes non-None after initialization. Consider documenting that users should only access these after calling get_world_poses() or within a context manager.

[greptile-apps]

Greptile Summary

This PR adds RAII-style fabric_read() and fabric_write() context managers to FabricFrameView, removes the sync_usd_on_fabric_write workaround in favor of PrepareForReuse()-based renderer notification, and adds topology-change detection that rebuilds fabricarrays when Fabric's internal memory layout shifts.

• P1 — _rebuild_fabric_arrays incomplete: When a topology change is detected, the method rebuilds _view_to_fabric, _fabric_to_view, and _fabric_world_matrices but leaves _default_view_indices, _fabric_positions_buf, _fabric_orientations_buf, and _fabric_scales_buf stale. If self.count changes after a topology event (the documented "prims added/removed" case), subsequent get_world_poses() / get_scales() calls using the cached buffers will produce wrong results or crash.
• The fabric_write() docstring says "PrepareForReuse() on exit" but _FabricWriteContext.__exit__ does not call it; the docstring should be corrected.
• test_camera_pose_update_reflected_in_render is missing the @pytest.mark.isaacsim_ci marker present on every other test in the file.

Confidence Score: 4/5

Safe to merge after addressing the incomplete _rebuild_fabric_arrays rebuild — the P1 defect is dormant in today's static-prim environments but is a correctness trap for the topology-change path the code explicitly documents.

One P1 finding: _rebuild_fabric_arrays omits count-dependent buffers (_default_view_indices, position/orientation/scale caches) that would produce wrong results or shape mismatches if self.count changes after a topology event. The feature otherwise looks well-designed: the PrepareForReuse integration is clean, both context managers are symmetrical, and the test coverage for the new paths is solid. The P2 items (docstring and missing CI marker) are non-blocking.

source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py — specifically the _rebuild_fabric_arrays method around line 336.

Important Files Changed

Filename	Overview
source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py	Core change: adds fabric_read()/fabric_write() context managers, _prepare_for_reuse() for renderer notification and topology detection, and removes sync_usd_on_fabric_write; _rebuild_fabric_arrays() has an incomplete rebuild missing position/orientation/scale/default-index buffers.
source/isaaclab/isaaclab/sensors/camera/camera.py	Removes sync_usd_on_fabric_write=True from FrameView construction; relies on PrepareForReuse-based renderer notification instead.
source/isaaclab/isaaclab/sim/views/usd_frame_view.py	Minor docstring cleanup removing the specific sync_usd_on_fabric_write example from the **kwargs description.
source/isaaclab/test/sensors/test_tiled_camera.py	Adds a new render-validation test for camera pose update via PrepareForReuse/Fabric path; missing @pytest.mark.isaacsim_ci marker that all other tests in the file carry.
source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py	Adds four new Fabric-specific tests: USD writeback removal, PrepareForReuse return type, and context manager round-trips; removes sync_usd_on_fabric_write=True from test fixture.
apps/isaaclab.python.headless.rendering.kit	Adds omni.kit.viewport.window and omni.kit.hydra_texture extensions to headless rendering kit for viewport infrastructure and annotator availability.

Sequence Diagram

sequenceDiagram
    participant User as User Code
    participant FW as fabric_write() ctx
    participant FFV as FabricFrameView
    participant WP as Warp Runtime
    participant Renderer as FSD/Storm Renderer

    User->>FW: with view.fabric_write() as fab
    FW->>FFV: _initialize_fabric() (if needed)
    FW->>FFV: _sync_fabric_from_usd_once() (if needed)
    FW->>FFV: _prepare_for_reuse()
    FFV->>Renderer: PrepareForReuse() — notify of pending changes
    Note over FFV: topology_changed? → _rebuild_fabric_arrays()
    FW-->>User: fab (world_matrices, view_to_fabric, count)

    User->>WP: wp.launch(my_kernel, inputs=[fab.world_matrices, ...])
    WP->>Renderer: (Fabric data modified in-place)

    User->>FW: __exit__ (no exception)
    FW->>WP: wp.synchronize()
    FW->>FFV: _fabric_hierarchy.update_world_xforms()
    FW->>FFV: _fabric_usd_sync_done = True

    Note over User,Renderer: Next render frame picks up updated transforms

Loading

_{Reviews (1): Last reviewed commit: "feat: add fabric_read/fabric_write conte..." | Re-trigger Greptile}

[greptile-apps]

Incomplete rebuild after topology change may leave stale/wrong-sized buffers

_rebuild_fabric_arrays re-creates the fabricarrays and the _view_to_fabric / _fabric_to_view mappings, but the docstring explicitly documents this as the handler for "prims added/removed" — a scenario that changes self.count. The following count-dependent buffers are not rebuilt:

• _default_view_indices (shape=(self.count,))
• _fabric_positions_buf (shape=(self.count, 3))
• _fabric_orientations_buf (shape=(self.count, 4))
• _fabric_scales_buf (shape=(self.count, 3))

If self.count changes after a topology event, subsequent get_world_poses() / get_scales() calls that use these cached buffers will either produce wrong results or crash with a shape mismatch. _initialize_fabric creates all of them; _rebuild_fabric_arrays should mirror that.

[greptile-apps]

Docstring incorrectly promises a second PrepareForReuse() call on exit

The docstring says: "and update_world_xforms() + PrepareForReuse() on exit", but _FabricWriteContext.__exit__ only calls wp.synchronize() and update_world_xforms() — there is no second PrepareForReuse() call. Either the exit should call self._view._prepare_for_reuse() after update_world_xforms(), or the docstring should be corrected to match the implementation.

[greptile-apps]

Missing @pytest.mark.isaacsim_ci marker

Every other test in this file carries @pytest.mark.isaacsim_ci, which gates execution in the CI pipeline. The new test_camera_pose_update_reflected_in_render is missing it, so it will be silently skipped by CI runs that filter on that marker — defeating the intent of validating the Fabric/PrepareForReuse pose propagation fix.

[pv-nvidia]

Code Coverage — fabric_frame_view.py

Name                   Stmts   Miss  Cover   Missing
------------------------------------------------------
fabric_frame_view.py     259     42    84%   ...

Coverage breakdown (this PR adds context managers):

• ✅ fabric_write() context manager: covered by test_fabric_write_context_manager
• ✅ fabric_read() context manager: covered by test_fabric_read_context_manager
• ✅ world_matrices property: covered
• ✅ view_to_fabric_mapping property: covered
• ✅ _FabricWriteContext.__enter__/__exit__: covered
• ✅ _FabricReadContext.__enter__/__exit__: covered
• ✅ get_scales Fabric path: covered by test_get_scales_fabric_path (new)
• ⬜ CPU fallback paths: not tested (CPU skips — Warp fabricarray limitation)
• ⬜ Topology rebuild branch: not triggered during tests

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

• All 16 contract tests pass on cuda:0
• test_fabric_write_context_manager ✅
• test_fabric_read_context_manager ✅
• test_get_scales_fabric_path ✅ (new)

[pbarejko]

Thanks @pv-nvidia , will this sync and kernel go away if we switch to indexedfabricarray? Is this a temporary solution?