diff --git a/source/isaaclab/isaaclab/sim/views/frame_view.py b/source/isaaclab/isaaclab/sim/views/frame_view.py index 95c5f0bb6850..eaa172529d52 100644 --- a/source/isaaclab/isaaclab/sim/views/frame_view.py +++ b/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) diff --git a/source/isaaclab_physx/changelog.d/refactor-fabric-fused-compose.rst b/source/isaaclab_physx/changelog.d/refactor-fabric-fused-compose.rst new file mode 100644 index 000000000000..e132fd992d2c --- /dev/null +++ b/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. diff --git a/source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py b/source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py index 1bcff86d57ac..7c44bf7a9983 100644 --- a/source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py +++ b/source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py @@ -15,7 +15,6 @@ 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 @@ -23,12 +22,6 @@ 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.""" diff --git a/source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py b/source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py index f0c18ccb98c7..fc3a32bb0147 100644 --- a/source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py +++ b/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}"