Skip to content

Commit 1235be6

Browse files
committed
Enhancements
* Drop redundant `fabric_device` local in `_initialize_fabric` — every assignment was `fabric_device = self._device`, so just use `self._device` directly. `self._fabric_device` (the persistent attribute) is preserved. * Drop the explicit `_fabric_usd_sync_done = True` at the end of `_sync_fabric_from_usd_once`. Both `set_world_poses` and `set_scales` already set it on success, so the trailing assignment is dead under the happy path and would mask a partial failure if either write raised. * Tighten the no-writeback assertion in `test_fabric_set_world_does_not_write_back_to_usd` from `atol=0.1` to `atol=0.0`. USD literally must not have moved — any drift indicates a residual writeback path, and the loose tolerance would hide a 0.099-unit regression. * Add `test_fabric_rebuild_after_topology_change` covering the `_prepare_for_reuse` → `_rebuild_fabric_arrays` branch, which is the load-bearing replacement for the removed `sync_usd_on_fabric_write`. The test monkeypatches `_prepare_for_reuse` to always take the rebuild branch (real Fabric topology changes are hard to provoke from a unit test) and verifies that a subsequent write/read still produces correct data, proving `_view_to_fabric` and `_fabric_world_matrices` are still consistent after the rebuild.
1 parent 9a8b15a commit 1235be6

3 files changed

Lines changed: 63 additions & 13 deletions

File tree

source/isaaclab/isaaclab/sensors/camera/camera.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@
1818
import isaaclab.sim as sim_utils
1919
import isaaclab.utils.sensors as sensor_utils
2020
from isaaclab.app.settings_manager import get_settings_manager
21-
from isaaclab.renderers import BaseRenderer, Renderer
21+
from isaaclab.renderers import BaseRenderer, CameraRenderSpec
2222
from isaaclab.sim.views import FrameView
23-
from isaaclab.utils import has_kit, to_camel_case
23+
from isaaclab.utils import to_camel_case
2424
from isaaclab.utils.math import (
2525
convert_camera_frame_orientation_convention,
2626
create_rotation_matrix_from_view,

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -394,24 +394,22 @@ def _initialize_fabric(self) -> None:
394394
# The constructor should have taken care of this, but double check here to avoid regressions
395395
assert self._device in _fabric_supported_devices
396396

397-
fabric_device = self._device
398-
399397
self._fabric_selection = fabric_stage.SelectPrims(
400398
require_attrs=[
401399
(usdrt.Sdf.ValueTypeNames.UInt, self._view_index_attr, usdrt.Usd.Access.Read),
402400
(usdrt.Sdf.ValueTypeNames.Matrix4d, "omni:fabric:worldMatrix", usdrt.Usd.Access.ReadWrite),
403401
],
404-
device=fabric_device,
402+
device=self._device,
405403
)
406404

407-
self._view_to_fabric = wp.zeros((self.count,), dtype=wp.uint32, device=fabric_device)
405+
self._view_to_fabric = wp.zeros((self.count,), dtype=wp.uint32, device=self._device)
408406
self._fabric_to_view = wp.fabricarray(self._fabric_selection, self._view_index_attr)
409407

410408
wp.launch(
411409
kernel=fabric_utils.set_view_to_fabric_array,
412410
dim=self._fabric_to_view.shape[0],
413411
inputs=[self._fabric_to_view, self._view_to_fabric],
414-
device=fabric_device,
412+
device=self._device,
415413
)
416414
wp.synchronize()
417415

@@ -423,13 +421,17 @@ def _initialize_fabric(self) -> None:
423421
self._fabric_dummy_buffer = wp.zeros((0, 3), dtype=wp.float32, device=self._device)
424422
self._fabric_world_matrices = wp.fabricarray(self._fabric_selection, "omni:fabric:worldMatrix")
425423
self._fabric_stage = fabric_stage
426-
self._fabric_device = fabric_device
424+
self._fabric_device = self._device
427425

428426
self._fabric_initialized = True
429427
self._fabric_usd_sync_done = False
430428

431429
def _sync_fabric_from_usd_once(self) -> None:
432-
"""Sync Fabric world matrices from USD once, on the first read."""
430+
"""Sync Fabric world matrices from USD once, on the first read.
431+
432+
``set_world_poses`` and ``set_scales`` each set ``_fabric_usd_sync_done``
433+
themselves, so no explicit flag assignment is needed here.
434+
"""
433435
if not self._fabric_initialized:
434436
self._initialize_fabric()
435437

@@ -441,8 +443,6 @@ def _sync_fabric_from_usd_once(self) -> None:
441443
self.set_world_poses(positions_usd, orientations_usd)
442444
self.set_scales(scales_usd)
443445

444-
self._fabric_usd_sync_done = True
445-
446446
def _resolve_indices_wp(self, indices: wp.array | None) -> wp.array:
447447
"""Resolve view indices as a Warp uint32 array."""
448448
if indices is None or indices == slice(None):

source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,62 @@ def test_fabric_set_world_does_not_write_back_to_usd(device, view_factory):
174174
f"Fabric should have new position, got {pos_torch}"
175175
)
176176

177-
# Verify USD still has the ORIGINAL position (no writeback)
177+
# Verify USD still has the ORIGINAL position (no writeback). Equality, not
178+
# approximate — USD should literally not have moved, so any drift would
179+
# indicate a residual writeback path.
178180
xform_cache_after = UsdGeom.XformCache()
179181
usd_tf_after = xform_cache_after.GetLocalToWorldTransform(prim)
180182
usd_t_after = usd_tf_after.ExtractTranslation()
181183
usd_pos_after = torch.tensor([float(usd_t_after[0]), float(usd_t_after[1]), float(usd_t_after[2])])
182-
assert torch.allclose(usd_pos_after, orig_usd_pos, atol=0.1), (
184+
assert torch.allclose(usd_pos_after, orig_usd_pos, atol=0.0), (
183185
f"USD should still have original position {orig_usd_pos}, but got {usd_pos_after}. "
184186
f"sync_usd_on_fabric_write may not have been fully removed."
185187
)
188+
189+
190+
@pytest.mark.parametrize("device", ["cpu", "cuda:0"])
191+
def test_fabric_rebuild_after_topology_change(device, view_factory, monkeypatch):
192+
"""Forcing the topology-changed branch on a write triggers
193+
:meth:`_rebuild_fabric_arrays` and leaves the view in a state where
194+
subsequent writes/reads still produce correct data.
195+
196+
Real ``PrimSelection.PrepareForReuse`` reports topology change only when
197+
Fabric reallocates internally, which is hard to provoke from a unit test.
198+
Instead we monkeypatch ``_prepare_for_reuse`` on the instance to always
199+
take the rebuild branch and verify the view remains usable.
200+
"""
201+
bundle = view_factory(2, device)
202+
view = bundle.view
203+
204+
# First write — initializes Fabric and binds _fabric_selection.
205+
initial = wp.zeros((2, 3), dtype=wp.float32, device=device)
206+
wp.launch(kernel=_fill_position, dim=2, inputs=[initial, 1.0, 2.0, 3.0], device=device)
207+
view.set_world_poses(positions=initial)
208+
209+
rebuild_calls = []
210+
real_rebuild = view._rebuild_fabric_arrays
211+
212+
def spy_rebuild():
213+
rebuild_calls.append(True)
214+
real_rebuild()
215+
216+
def force_topology_changed():
217+
if view._fabric_selection is not None:
218+
view._fabric_selection.PrepareForReuse()
219+
spy_rebuild()
220+
221+
monkeypatch.setattr(view, "_prepare_for_reuse", force_topology_changed)
222+
223+
# Trigger another write — goes through the forced topology-change branch.
224+
new = wp.zeros((2, 3), dtype=wp.float32, device=device)
225+
wp.launch(kernel=_fill_position, dim=2, inputs=[new, 4.0, 5.0, 6.0], device=device)
226+
view.set_world_poses(positions=new)
227+
228+
assert rebuild_calls, "Forced topology-change branch did not invoke _rebuild_fabric_arrays"
229+
230+
# Read back — proves the rebuilt _view_to_fabric and _fabric_world_matrices
231+
# are still consistent.
232+
ret_pos, _ = view.get_world_poses()
233+
pos_torch = wp.to_torch(ret_pos)
234+
expected = torch.tensor([[4.0, 5.0, 6.0], [4.0, 5.0, 6.0]], device=device)
235+
assert torch.allclose(pos_torch, expected, atol=1e-7), f"Read after rebuild failed on {device}: {pos_torch}"

0 commit comments

Comments
 (0)