refactor: fuse set_world_poses/set_scales into single _compose_fabric_transform

source/isaaclab/isaaclab/sim/views/frame_view.py

@@ -6,15 +6,25 @@
 """Backend-dispatching FrameView.
 
 ``FrameView(path, device=...)`` automatically selects the right backend:
-- PhysX: :class:`~isaaclab_physx.sim.views.FabricFrameView`
+- PhysX + Fabric enabled + supported device: :class:`~isaaclab_physx.sim.views.FabricFrameView`
+- PhysX without Fabric (or unsupported device): :class:`~isaaclab.sim.views.UsdFrameView`
+- OVPhysX: :class:`~isaaclab_ovphysx.sim.views.OvPhysxFrameView`
 - Newton: :class:`~isaaclab_newton.sim.views.NewtonSiteFrameView`
 """
 
 from __future__ import annotations
 
+import logging
+
 from isaaclab.utils.backend_utils import FactoryBase
 
 from .base_frame_view import BaseFrameView
+from .usd_frame_view import UsdFrameView
+
+logger = logging.getLogger(__name__)
+
+# Devices supported by Fabric GPU acceleration (USDRT SelectPrims + Warp).
+_FABRIC_SUPPORTED_DEVICES = ("cpu", "cuda", "cuda:0")
 
 
 class FrameView(FactoryBase, BaseFrameView):
@@ -23,8 +33,10 @@ class FrameView(FactoryBase, BaseFrameView):
     Callers use ``FrameView(prim_path, device=device)`` and get the
     correct implementation automatically:
 
-    - **PhysX / no backend**: :class:`~isaaclab_physx.sim.views.FabricFrameView`
-      (Fabric GPU acceleration with USD fallback).
+    - **PhysX + Fabric**: :class:`~isaaclab_physx.sim.views.FabricFrameView`
+      (GPU-accelerated transforms via Warp + USDRT).
+    - **PhysX without Fabric**: :class:`~isaaclab.sim.views.UsdFrameView`
+      (standard USD operations).
     - **OVPhysX**: :class:`~isaaclab_ovphysx.sim.views.OvPhysxFrameView`
       (Warp-native, reads body poses via an OVPhysX ``RIGID_BODY_POSE``
       tensor binding).
@@ -36,22 +48,50 @@ class FrameView(FactoryBase, BaseFrameView):
         "physx": "FabricFrameView",
         "ovphysx": "OvPhysxFrameView",
         "newton": "NewtonSiteFrameView",
+        # "usd" is registered eagerly below — no dynamic import needed.
     }
 
     @classmethod
     def _get_backend(cls, *args, **kwargs) -> str:
+        from isaaclab.app.settings_manager import SettingsManager  # noqa: PLC0415
         from isaaclab.sim.simulation_context import SimulationContext  # noqa: PLC0415
 
         ctx = SimulationContext.instance()
         if ctx is None:
-            return "physx"
+            return "usd"
+
         manager_name = ctx.physics_manager.__name__.lower()
         if "newton" in manager_name:
             return "newton"
         if "ovphysx" in manager_name:
             return "ovphysx"
-        return "physx"
+
+        # PhysX path — check if Fabric is enabled and the device is supported.
+        settings = SettingsManager.instance()
+        fabric_enabled = bool(settings.get("/physics/fabricEnabled", False))
+
+        device = kwargs.get("device", "cpu")
+        if len(args) >= 2:
+            device = args[1]
+
+        if fabric_enabled and device in _FABRIC_SUPPORTED_DEVICES:
+            return "physx"
+
+        if fabric_enabled and device not in _FABRIC_SUPPORTED_DEVICES:
+            logger.warning(
+                f"Fabric mode is not supported on device '{device}'. "
+                "USDRT SelectPrims and Warp fabric arrays are currently "
+                f"only supported on {', '.join(_FABRIC_SUPPORTED_DEVICES)}. "
+                "Falling back to UsdFrameView."
+            )
+
+        return "usd"
 
     def __new__(cls, *args, **kwargs) -> BaseFrameView:
         """Create a new FrameView for the active physics backend."""
         return super().__new__(cls, *args, **kwargs)
+
+
+# Eagerly register UsdFrameView — it lives in isaaclab, not a backend package,
+# so FactoryBase's dynamic import (isaaclab_{backend}.sim.views) can't find it.
+FrameView.register("usd", UsdFrameView)

source/isaaclab_physx/changelog.d/refactor-fabric-fused-compose.rst

@@ -0,0 +1,24 @@
+Changed
+^^^^^^^
+
+* Combined the initial USD→Fabric sync in
+  :class:`~isaaclab_physx.sim.views.FabricFrameView` into a single Fabric
+  write so ``PrepareForReuse`` is invoked exactly once per logical update
+  (positions, orientations, and scales are composed in one kernel launch).
+  This avoids the possibility of a second non-idempotent
+  ``PrepareForReuse`` call masking a topology-change signal that should
+  have triggered a fabricarray rebuild.
+
+* Extracted :meth:`~isaaclab_physx.sim.views.FabricFrameView._compose_fabric_transform`
+  to deduplicate the kernel-launch logic shared by ``set_world_poses`` and
+  ``set_scales``.
+
+Fixed
+^^^^^
+
+* Fixed the topology-change invariant guard in
+  :class:`~isaaclab_physx.sim.views.FabricFrameView` not surviving
+  ``python -O``.  The check now raises :class:`RuntimeError` instead of
+  using ``assert`` so the prim-count mismatch between view and Fabric is
+  reported at every optimisation level rather than silently producing
+  wrong poses or out-of-bounds kernel indices.

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

@@ -15,20 +15,13 @@
 from pxr import Usd
 
 import isaaclab.sim as sim_utils
-from isaaclab.app.settings_manager import SettingsManager
 from isaaclab.sim.views.base_frame_view import BaseFrameView
 from isaaclab.sim.views.usd_frame_view import UsdFrameView
 from isaaclab.utils.warp import ProxyArray
 from isaaclab.utils.warp import fabric as fabric_utils
 
 logger = logging.getLogger(__name__)
 
-# TODO: extend this to ``cuda:N`` once we wire up multi-GPU support for the view.
-# Recent Kit / USDRT releases do support multi-GPU ``SelectPrims``, but the
-# rest of the FabricFrameView wiring (selections, indexed arrays, etc.) still
-# assumes a single device — to be tackled in a follow-up.
-_fabric_supported_devices = ("cpu", "cuda", "cuda:0")
-
 
 def _to_float32_2d(a: wp.array | torch.Tensor) -> wp.array | torch.Tensor:
     """Ensure array is compatible with Fabric kernels (2-D float32).
@@ -49,19 +42,20 @@ def _to_float32_2d(a: wp.array | torch.Tensor) -> wp.array | torch.Tensor:
 class FabricFrameView(BaseFrameView):
     """FrameView with Fabric GPU acceleration for the PhysX backend.
 
-    Uses composition: holds a :class:`UsdFrameView` internally for USD
-    fallback and non-accelerated operations (local poses, visibility, scales
-    when Fabric is disabled).
+    This class is only instantiated when Fabric is enabled and the device is
+    supported.  The :class:`~isaaclab.sim.views.FrameView` factory dispatches
+    to :class:`~isaaclab.sim.views.UsdFrameView` otherwise.
 
-    When Fabric is enabled, world-pose and scale operations use Warp kernels
-    operating on ``omni:fabric:worldMatrix``.  All other operations delegate
-    to the internal USD view.
+    Uses composition: holds a :class:`UsdFrameView` internally for operations
+    that don't have a Fabric-accelerated path (local poses, visibility).
 
-    After every Fabric write (``set_world_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.
+    World-pose and scale operations use Warp kernels operating on
+    ``omni:fabric:worldMatrix``.  After every Fabric write
+    (``set_world_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.
 
     Pose getters return :class:`~isaaclab.utils.warp.ProxyArray`.  Setters accept ``wp.array``.
     """
@@ -89,18 +83,6 @@ def __init__(
         self._usd_view = UsdFrameView(prim_path, device=device, validate_xform_ops=validate_xform_ops, stage=stage)
         self._device = device
 
-        settings = SettingsManager.instance()
-        self._use_fabric = bool(settings.get("/physics/fabricEnabled", False))
-
-        if self._use_fabric and self._device not in _fabric_supported_devices:
-            logger.warning(
-                f"Fabric mode is not supported on device '{self._device}'. "
-                "USDRT SelectPrims and Warp fabric arrays are currently "
-                f"only supported on {', '.join(_fabric_supported_devices)}. "
-                "Falling back to standard USD operations. This may impact performance."
-            )
-            self._use_fabric = False
-
         self._fabric_initialized = False
         self._fabric_usd_sync_done = False
         self._fabric_selection = None
@@ -146,51 +128,9 @@ def set_visibility(self, visibility, indices=None):
     # ------------------------------------------------------------------
 
     def set_world_poses(self, positions=None, orientations=None, indices=None):
-        if not self._use_fabric:
-            self._usd_view.set_world_poses(positions, orientations, indices)
-            return
-
-        if not self._fabric_initialized:
-            self._initialize_fabric()
-
-        self._prepare_for_reuse()
-
-        indices_wp = self._resolve_indices_wp(indices)
-        count = indices_wp.shape[0]
-
-        dummy = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
-        positions_wp = _to_float32_2d(positions) if positions is not None else dummy
-        orientations_wp = (
-            _to_float32_2d(orientations)
-            if orientations is not None
-            else wp.zeros((0, 4), dtype=wp.float32, device=self._device)
-        )
-
-        wp.launch(
-            kernel=fabric_utils.compose_fabric_transformation_matrix_from_warp_arrays,
-            dim=count,
-            inputs=[
-                self._fabric_world_matrices,
-                positions_wp,
-                orientations_wp,
-                dummy,
-                False,
-                False,
-                False,
-                indices_wp,
-                self._view_to_fabric,
-            ],
-            device=self._fabric_device,
-        )
-        wp.synchronize()
-
-        self._fabric_hierarchy.update_world_xforms()
-        self._fabric_usd_sync_done = True
+        self._compose_fabric_transform(positions=positions, orientations=orientations, indices=indices)
 
     def get_world_poses(self, indices: wp.array | None = None) -> tuple[ProxyArray, ProxyArray]:
-        if not self._use_fabric:
-            return self._usd_view.get_world_poses(indices)
-
         if not self._fabric_initialized:
             self._initialize_fabric()
         if not self._fabric_usd_sync_done:
@@ -241,10 +181,19 @@ def get_local_poses(self, indices: wp.array | None = None) -> tuple[ProxyArray,
     # ------------------------------------------------------------------
 
     def set_scales(self, scales, indices=None):
-        if not self._use_fabric:
-            self._usd_view.set_scales(scales, indices)
-            return
+        self._compose_fabric_transform(scales=scales, indices=indices)
+
+    def _compose_fabric_transform(self, positions=None, orientations=None, scales=None, indices=None):
+        """Write the given subset of (position, orientation, scale) into Fabric in one kernel launch.
+
+        Components left as ``None`` are skipped via empty input arrays — the kernel reads them
+        from the existing Fabric matrix.  Always invokes :meth:`_prepare_for_reuse` exactly once
+        per write, even when multiple components are updated together.
 
+        Side effect: sets ``self._fabric_usd_sync_done = True`` after the write, marking Fabric
+        as the authoritative source going forward.  This suppresses the lazy USD→Fabric sync
+        in subsequent getters (see :meth:`get_world_poses`, :meth:`get_scales`).
+        """
         if not self._fabric_initialized:
             self._initialize_fabric()
 
@@ -253,17 +202,19 @@ def set_scales(self, scales, indices=None):
         indices_wp = self._resolve_indices_wp(indices)
         count = indices_wp.shape[0]
 
-        dummy3 = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
-        dummy4 = wp.zeros((0, 4), dtype=wp.float32, device=self._device)
-        scales_wp = _to_float32_2d(scales)
+        empty3 = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
+        empty4 = wp.zeros((0, 4), dtype=wp.float32, device=self._device)
+        positions_wp = _to_float32_2d(positions) if positions is not None else empty3
+        orientations_wp = _to_float32_2d(orientations) if orientations is not None else empty4
+        scales_wp = _to_float32_2d(scales) if scales is not None else empty3
 
         wp.launch(
             kernel=fabric_utils.compose_fabric_transformation_matrix_from_warp_arrays,
             dim=count,
             inputs=[
                 self._fabric_world_matrices,
-                dummy3,
-                dummy4,
+                positions_wp,
+                orientations_wp,
                 scales_wp,
                 False,
                 False,
@@ -279,9 +230,6 @@ def set_scales(self, scales, indices=None):
         self._fabric_usd_sync_done = True
 
     def get_scales(self, indices=None):
-        if not self._use_fabric:
-            return self._usd_view.get_scales(indices)
-
         if not self._fabric_initialized:
             self._initialize_fabric()
         if not self._fabric_usd_sync_done:
@@ -347,10 +295,11 @@ def _rebuild_fabric_arrays(self) -> None:
         pattern (via ``_usd_view.count``) and does not change when Fabric rearranges its
         internal memory layout.  The assertion below guards this invariant.
         """
-        assert self.count == self._default_view_indices.shape[0], (
-            f"Prim count changed ({self.count} vs {self._default_view_indices.shape[0]}). "
-            "Fabric topology change added/removed tracked prims — full re-initialization required."
-        )
+        if self.count != self._default_view_indices.shape[0]:
+            raise RuntimeError(
+                f"Prim count changed ({self.count} vs {self._default_view_indices.shape[0]}). "
+                "Fabric topology change added/removed tracked prims — full re-initialization required."
+            )
         self._view_to_fabric = wp.zeros((self.count,), dtype=wp.uint32, device=self._fabric_device)
         self._fabric_to_view = wp.fabricarray(self._fabric_selection, self._view_index_attr)
 
@@ -404,9 +353,6 @@ def _initialize_fabric(self) -> None:
         )
         wp.synchronize()
 
-        # The constructor should have taken care of this, but double check here to avoid regressions
-        assert self._device in _fabric_supported_devices
-
         self._fabric_selection = fabric_stage.SelectPrims(
             require_attrs=[
                 (usdrt.Sdf.ValueTypeNames.UInt, self._view_index_attr, usdrt.Usd.Access.Read),
@@ -442,19 +388,20 @@ def _initialize_fabric(self) -> None:
     def _sync_fabric_from_usd_once(self) -> None:
         """Sync Fabric world matrices from USD once, on the first read.
 
-        ``set_world_poses`` and ``set_scales`` each set ``_fabric_usd_sync_done``
-        themselves, so no explicit flag assignment is needed here.
+        Combines position/orientation/scale into a single Fabric write so
+        :meth:`_prepare_for_reuse` (and its underlying ``PrepareForReuse``) is invoked
+        exactly once across the full sync.
         """
         if not self._fabric_initialized:
             self._initialize_fabric()
 
         positions_usd_ta, orientations_usd_ta = self._usd_view.get_world_poses()
-        positions_usd = positions_usd_ta.warp
-        orientations_usd = orientations_usd_ta.warp
         scales_usd = self._usd_view.get_scales()
-
-        self.set_world_poses(positions_usd, orientations_usd)
-        self.set_scales(scales_usd)
+        self._compose_fabric_transform(
+            positions=positions_usd_ta.warp,
+            orientations=orientations_usd_ta.warp,
+            scales=scales_usd,
+        )
 
     def _resolve_indices_wp(self, indices: wp.array | None) -> wp.array:
         """Resolve view indices as a Warp uint32 array."""

source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py

@@ -169,7 +169,7 @@ def test_fabric_set_world_does_not_write_back_to_usd(device, view_factory):
 
     # Verify Fabric has the new position
     fab_pos, _ = view.get_world_poses()
-    pos_torch = wp.to_torch(fab_pos)
+    pos_torch = fab_pos.torch
     assert torch.allclose(pos_torch, torch.tensor([[99.0, 99.0, 99.0]], device=device), atol=0.1), (
         f"Fabric should have new position, got {pos_torch}"
     )
@@ -230,6 +230,6 @@ def force_topology_changed():
     # Read back — proves the rebuilt _view_to_fabric and _fabric_world_matrices
     # are still consistent.
     ret_pos, _ = view.get_world_poses()
-    pos_torch = wp.to_torch(ret_pos)
+    pos_torch = ret_pos.torch
     expected = torch.tensor([[4.0, 5.0, 6.0], [4.0, 5.0, 6.0]], device=device)
     assert torch.allclose(pos_torch, expected, atol=1e-7), f"Read after rebuild failed on {device}: {pos_torch}"