Tighten FabricFrameView lifecycle and verify transpose math

Three review fixes plus the missing coverage for the transpose
storage convention.

* Hierarchy cache eviction: cache key is now (stage_id, fabric_id) so a
  recycled stage_id paired with a new Fabric attachment never returns a
  stale handle.  Added FabricFrameView.clear_static_caches() classmethod
  for explicit teardown and wired it into the test fixture so cached
  handles do not accumulate across the suite.

* PrepareForReuse no longer fires twice per sync.  Both _sync_local_from_world
  and _sync_world_from_local_if_dirty refresh trans_sel_ro exactly once,
  then read _world_ifa_ro / _local_ifa_ro / _parent_world_ifa_ro directly
  from the fields instead of going through the accessors.

* Class docstring rewritten to describe the actual PrepareForReuse policy
  (every accessor calls it; idempotent and cheap in the steady state).
  The prior wording claimed reads avoided PrepareForReuse, which has not
  been true since the indexedfabricarray rewrite.

* New regression tests for the transpose storage convention.  The standard
  fixture parents are translation-only, so the rotation block is identity
  and equal to its transpose - which means a wrong transpose convention
  would still pass every existing test.  Two new tests place a parent
  rotated 90 degrees around Z and verify the world<->local round-trip:
  - test_set_local_then_get_world_with_rotated_parent exercises
    update_indexed_world_matrix_from_local
  - test_set_world_then_get_local_with_rotated_parent exercises
    update_indexed_local_matrix_from_world
  Confirmed locally that flipping the multiply order in either kernel
  makes the matching test fail.

45 tests pass on cpu and cuda:0.

Author: pv-nvidia

source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py

@@ -80,12 +80,18 @@ class FabricFrameView(BaseFrameView):
     ``omni:fabric:localMatrix``.  All other operations delegate to the
     internal USD view.
 
-    After every Fabric write (``set_world_poses``, ``set_local_poses``,
-    ``set_scales``), :meth:`PrepareForReuse` is called on the
-    ``PrimSelection`` to notify the FSD renderer that Fabric data has
-    changed and to detect topology changes that require rebuilding
-    internal mappings.  Read operations do not call PrepareForReuse to
-    avoid unnecessary renderer invalidation.
+    Every accessor for a selection's indexed fabric array
+    (:meth:`_get_world_ro_array`, :meth:`_get_local_ro_array`,
+    :meth:`_get_world_rw_array`, :meth:`_get_local_rw_array`,
+    :meth:`_get_parent_world_ro_array`) calls :meth:`PrepareForReuse` on its
+    backing ``PrimSelection`` to detect Fabric topology changes that would
+    require rebuilding the view→fabric index mapping.  ``PrepareForReuse``
+    is idempotent and cheap in the steady state (returns ``False`` with no
+    rebuild), so the per-access cost is negligible.  The two sync helpers
+    (:meth:`_sync_local_from_world`, :meth:`_sync_world_from_local_if_dirty`)
+    refresh ``trans_sel_ro`` at most once per call to avoid the redundant
+    USDRT round-trip that would otherwise occur when ``_world_ifa_ro``,
+    ``_local_ifa_ro`` and ``_parent_world_ifa_ro`` are read consecutively.
 
     Pose getters return :class:`~isaaclab.utils.warp.ProxyArray`.  Setters accept ``wp.array``.
     """
@@ -94,12 +100,29 @@ class FabricFrameView(BaseFrameView):
     _LOCAL_MATRIX_NAME = "omni:fabric:localMatrix"
 
     # Stage-level shared state.  Multiple FabricFrameView instances on the same stage
-    # share one IFabricHierarchy handle.  ``ClassVar`` plus the ``_static_`` prefix
-    # make it explicit (and enforceable by type checkers) that this is class-level —
-    # never shadow it with ``self.<name> = ...``.  The cache is not protected by a
-    # lock; Isaac Lab's simulation loop is single-threaded, and adding locking would
-    # negate the per-stage hit-cache cost it exists to avoid.
-    _static_hierarchy_cache: ClassVar[dict[int, object]] = {}
+    # share one ``IFabricHierarchy`` handle, keyed by ``(stage_id, fabric_id)`` so
+    # that a recycled ``stage_id`` paired with a fresh ``fabric_id`` never reuses a
+    # stale handle.  ``ClassVar`` plus the ``_static_`` prefix make it explicit (and
+    # enforceable by type checkers) that this is class-level — never shadow it with
+    # ``self.<name> = ...``.  The cache is not protected by a lock; Isaac Lab's
+    # simulation loop is single-threaded, and adding locking would negate the
+    # per-stage hit-cache cost it exists to avoid.  Call
+    # :meth:`clear_static_caches` on stage teardown if you tear down many stages
+    # in one process.
+    _static_hierarchy_cache: ClassVar[dict[tuple[int, int], object]] = {}
+
+    @classmethod
+    def clear_static_caches(cls) -> None:
+        """Drop all cached ``IFabricHierarchy`` handles.
+
+        Call this after tearing down a USD stage if the process will continue
+        opening new stages.  Cached handles are otherwise retained for the
+        lifetime of the process, which can leak memory in long test suites and,
+        in theory, allow a recycled ``stage_id`` paired with a new Fabric
+        attachment to read a stale handle (though the ``(stage_id, fabric_id)``
+        cache key already guards against the latter).
+        """
+        cls._static_hierarchy_cache.clear()
 
     def __init__(
         self,
@@ -451,14 +474,15 @@ def _sync_world_from_local_if_dirty(self) -> None:
         """
         if not self._world_dirty:
             return
-        # Make sure the parent indexed array is up-to-date with the current trans selection.
-        if self._parent_world_ifa_ro is None:
-            self._parent_world_ifa_ro = self._build_parent_indexed_array(self._trans_sel_ro)
+        # Refresh trans_sel_ro once, then read _local_ifa_ro and _parent_world_ifa_ro
+        # directly to avoid calling PrepareForReuse twice on the same selection.
+        if self._trans_sel_ro.PrepareForReuse() or self._parent_world_ifa_ro is None:
+            self._rebuild_trans_ro_arrays()
         wp.launch(
             kernel=fabric_utils.update_indexed_world_matrix_from_local,
             dim=self.count,
             inputs=[
-                self._get_local_ro_array(),
+                self._local_ifa_ro,
                 self._parent_world_ifa_ro,
                 self._get_world_rw_array(),
                 self._view_indices,
@@ -476,12 +500,15 @@ def _sync_local_from_world(self, indices_wp: wp.array) -> None:
         not provide a built-in world → local sync, so we do it via a Warp kernel
         using the parent indexed fabric array.
         """
+        # Refresh trans_sel_ro once; _world_ifa_ro and _parent_world_ifa_ro share it.
+        if self._trans_sel_ro.PrepareForReuse() or self._parent_world_ifa_ro is None:
+            self._rebuild_trans_ro_arrays()
         wp.launch(
             kernel=fabric_utils.update_indexed_local_matrix_from_world,
             dim=indices_wp.shape[0],
             inputs=[
-                self._get_world_ro_array(),
-                self._get_parent_world_ro_array(),
+                self._world_ifa_ro,
+                self._parent_world_ifa_ro,
                 self._get_local_rw_array(),
                 indices_wp,
             ],
@@ -611,18 +638,22 @@ def _initialize_fabric(self) -> None:
         self._stage = usdrt.Usd.Stage.Attach(self._stage_id)
         self._stage.SynchronizeToFabric()
 
-        # Reuse (or create) a hierarchy handle for this stage.  Enable change-tracking
+        # Reuse (or create) a hierarchy handle for this stage.  The cache key is
+        # ``(stage_id, fabric_id)`` so a recycled ``stage_id`` paired with a fresh
+        # Fabric attachment never returns a stale handle.  Enable change-tracking
         # BEFORE we author any local matrices, so the per-prim
-        # ``SetLocalXformFromUsd`` calls below mark themselves dirty and the next
-        # ``update_world_xforms()`` walks the parent chain to populate worldMatrix.
-        if self._stage_id not in FabricFrameView._static_hierarchy_cache:
+        # ``SetLocalXformFromUsd`` calls below mark themselves dirty.
+        fabric_id = self._stage.GetFabricId()
+        self._fabric_id = fabric_id
+        cache_key = (self._stage_id, fabric_id)
+        if cache_key not in FabricFrameView._static_hierarchy_cache:
             hierarchy = usdrt.hierarchy.IFabricHierarchy().get_fabric_hierarchy(
-                self._stage.GetFabricId(), self._stage.GetStageIdAsStageId()
+                fabric_id, self._stage.GetStageIdAsStageId()
             )
             hierarchy.track_local_xform_changes(True)
             hierarchy.track_world_xform_changes(True)
-            FabricFrameView._static_hierarchy_cache[self._stage_id] = hierarchy
-        self._fabric_hierarchy = FabricFrameView._static_hierarchy_cache[self._stage_id]
+            FabricFrameView._static_hierarchy_cache[cache_key] = hierarchy
+        self._fabric_hierarchy = FabricFrameView._static_hierarchy_cache[cache_key]
 
         # Ensure each child prim AND its parent have BOTH Fabric world and local matrix
         # attributes.  Our ``trans_ro`` selection requires both, so prims missing either

source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py

@@ -41,6 +41,9 @@ def test_setup_teardown():
     yield
     sim_utils.clear_stage()
     sim_utils.SimulationContext.clear_instance()
+    # Each test creates a fresh stage; drop cached IFabricHierarchy handles so
+    # the next test does not reuse a handle attached to the disposed stage.
+    FrameView.clear_static_caches()
 
 
 def _skip_if_unavailable(device: str):
@@ -276,3 +279,81 @@ def test_get_scales_fabric_path(device, view_factory):
     # Default scale should be (1, 1, 1)
     expected = torch.tensor([[1.0, 1.0, 1.0]], dtype=torch.float32, device=device)
     torch.testing.assert_close(scales_t, expected, atol=1e-4, rtol=0)
+
+
+# ------------------------------------------------------------------
+# Transpose-convention verification: world ↔ local kernels rely on the
+# identity ``(A·B)ᵀ = Bᵀ·Aᵀ`` to drop explicit transposes when operating
+# on Fabric's column-transposed matrix storage.  The translation-only
+# parents used by the standard fixture cannot distinguish the right
+# convention from the wrong one — the rotation block is identity and
+# equals its own transpose.  These tests use a parent rotated 90° around
+# Z so that an incorrect storage convention would produce a clearly
+# wrong child pose.
+# ------------------------------------------------------------------
+
+
+# Parent at (0, 0, 1) rotated +90° around Z (so the parent X axis points
+# along world +Y).  Quaternion components in (x, y, z, w) order.
+_ROTATED_PARENT_POS = (0.0, 0.0, 1.0)
+_ROTATED_PARENT_QUAT_XYZW = (0.0, 0.0, 0.70710678, 0.70710678)
+
+
+def _build_rotated_parent_view(device: str) -> "FrameView":
+    """Build a 1-env FabricFrameView whose parent is rotated 90° around Z."""
+    stage = sim_utils.get_current_stage()
+    sim_utils.create_prim(
+        "/World/Parent_0",
+        "Xform",
+        translation=_ROTATED_PARENT_POS,
+        orientation=_ROTATED_PARENT_QUAT_XYZW,
+        stage=stage,
+    )
+    sim_utils.create_prim("/World/Parent_0/Child", "Camera", translation=(0.0, 0.0, 0.0), stage=stage)
+    sim_utils.SimulationContext(sim_utils.SimulationCfg(dt=0.01, device=device, use_fabric=True))
+    view = FrameView("/World/Parent_.*/Child", device=device)
+    view.get_world_poses()  # force Fabric init and USD→Fabric seed
+    return view
+
+
+@pytest.mark.parametrize("device", ["cpu", "cuda:0"])
+def test_set_local_then_get_world_with_rotated_parent(device):
+    """Verify ``update_indexed_world_matrix_from_local`` under non-identity parent rotation.
+
+    With parent rotated +90° around Z, a child local translation of (1, 0, 0)
+    must produce world translation (0, 1, 1) — parent_pos + R · local.  If the
+    transpose convention in the kernel were wrong, the rotation would flip
+    direction and the world position would land at (0, -1, 1) instead.
+    """
+    _skip_if_unavailable(device)
+    view = _build_rotated_parent_view(device)
+
+    new_local = wp.zeros((1, 3), dtype=wp.float32, device=device)
+    wp.launch(kernel=_fill_position, dim=1, inputs=[new_local, 1.0, 0.0, 0.0], device=device)
+    identity_quat = wp.from_torch(torch.tensor([[0.0, 0.0, 0.0, 1.0]], dtype=torch.float32, device=device))
+    view.set_local_poses(translations=new_local, orientations=identity_quat)
+
+    world_pos, _ = view.get_world_poses()
+    expected = torch.tensor([[0.0, 1.0, 1.0]], dtype=torch.float32, device=device)
+    torch.testing.assert_close(world_pos.torch, expected, atol=1e-5, rtol=0)
+
+
+@pytest.mark.parametrize("device", ["cpu", "cuda:0"])
+def test_set_world_then_get_local_with_rotated_parent(device):
+    """Verify ``update_indexed_local_matrix_from_world`` under non-identity parent rotation.
+
+    With parent rotated +90° around Z and at (0, 0, 1), writing child world
+    translation (5, 0, 2) must yield child local translation Rᵀ · (5, 0, 1) =
+    (0, -5, 1).  A wrong transpose convention would invert the rotation in the
+    wrong direction and produce (0, 5, 1) instead.
+    """
+    _skip_if_unavailable(device)
+    view = _build_rotated_parent_view(device)
+
+    new_world = wp.zeros((1, 3), dtype=wp.float32, device=device)
+    wp.launch(kernel=_fill_position, dim=1, inputs=[new_world, 5.0, 0.0, 2.0], device=device)
+    view.set_world_poses(positions=new_world)
+
+    local_pos, _ = view.get_local_poses()
+    expected = torch.tensor([[0.0, -5.0, 1.0]], dtype=torch.float32, device=device)
+    torch.testing.assert_close(local_pos.torch, expected, atol=1e-5, rtol=0)