Skip to content

Test for Camera and Tiled Camera transforms on CPU and GPU#4923

Draft
bareya wants to merge 5 commits intoisaac-sim:developfrom
bareya:pbarejko/camera-update
Draft

Test for Camera and Tiled Camera transforms on CPU and GPU#4923
bareya wants to merge 5 commits intoisaac-sim:developfrom
bareya:pbarejko/camera-update

Conversation

@bareya
Copy link
Collaborator

@bareya bareya commented Mar 10, 2026

Fix camera update.

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@bareya bareya requested a review from ooctipus March 10, 2026 20:08
@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Mar 10, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR fixes a bug where repositioning a TiledCamera via set_world_poses_from_view was not reflected in subsequent renders. The root cause was that TiledCamera intentionally omits sync_usd_on_fabric_write=True on its XformPrimView for performance, so pose writes go to Fabric but not USD — yet Replicator reads transforms from USD. The fix implements IsaacRtxRenderer.update_camera (previously a no-op) to convert poses from the world quaternion convention back to OpenGL and write them directly to USD via _set_world_poses_usd immediately before each render. A new regression test covers both tiled and non-tiled cameras.

Key changes:

  • IsaacRtxRenderer.update_camera: no longer a no-op; writes world-frame positions and OpenGL-convention orientations directly to USD before each render so Replicator always sees the latest camera transform.
  • TiledCamera._update_poses: calls renderer.update_camera after reading the fresh Fabric poses, ensuring the USD is updated in the same camera.update() call, just before the RTX render is triggered.
  • tiled_camera.py comment: a comment was added to the XformPrimView construction, but it was copied verbatim from camera.py and is inaccurate — sync_usd_on_fabric_write is deliberately not enabled here, and the comment implies it is.
  • Potential performance concern: update_camera always writes the full pose tensor for all cameras to USD (via _set_world_poses_usd), even when only a subset of cameras was moved. For large environments this could add non-trivial per-step overhead.

Confidence Score: 4/5

  • This PR is safe to merge with minor documentation and performance caveats.
  • The core fix is sound: update_camera now correctly converts orientations and writes USD poses before each render, resolving the stale-transform bug. The two concerns are (1) an inaccurate comment in tiled_camera.py that implies sync_usd_on_fabric_write=True is active when it is not, and (2) all-camera USD writes on each call regardless of how many cameras actually moved, which could be a real cost in large parallel environments. Neither blocks correctness.
  • source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py — the update_camera write-all-poses behaviour, and source/isaaclab/isaaclab/sensors/camera/tiled_camera.py — the inaccurate comment at line 173.

Important Files Changed

Filename Overview
source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Core of the fix: update_camera is no longer a no-op; it now converts orientations to OpenGL convention and writes all camera poses directly to USD via _set_world_poses_usd so Replicator picks up the latest transforms. Minor concern: full pose tensors are always written even when only a subset of cameras moved.
source/isaaclab/isaaclab/sensors/camera/tiled_camera.py Comment added to XformPrimView construction, but it is copied verbatim from camera.py and is inaccurate for TiledCamerasync_usd_on_fabric_write=True is intentionally absent here, and the stale-pose fix is handled through the new update_camera path instead.
source/isaaclab/test/sensors/test_tiled_camera.py New regression test test_camera_pose_update_reflected_in_render validates that moving a camera is reflected in the rendered depth for both tiled and non-tiled cameras. The test logic is sound; asymmetric step counts (5 vs 2) between the two positions remain unexplained.

Sequence Diagram

sequenceDiagram
    participant User
    participant Camera as TiledCamera
    participant View as XformPrimView
    participant Fabric
    participant USD
    participant Renderer as IsaacRtxRenderer
    participant Replicator

    User->>Camera: set_world_poses_from_view(eyes, targets)
    Camera->>View: set_world_poses(positions, orientations)
    View->>Fabric: write pose (sync_usd_on_fabric_write=False)
    Note over USD: USD pose is STALE here

    User->>Camera: update(dt)
    Camera->>Camera: _update_buffers_impl(env_mask)
    Camera->>Camera: _update_poses(env_ids)
    Camera->>View: get_world_poses(env_ids)
    View->>Fabric: read pose
    Fabric-->>View: fresh positions/orientations
    View-->>Camera: pos_w, quat_w_world
    Camera->>Renderer: update_camera(render_data, pos_w, quat_w_world, intrinsics)
    Renderer->>Renderer: convert orientations world→opengl
    Renderer->>View: _set_world_poses_usd(positions, orientations_opengl)
    View->>USD: write ALL poses
    Note over USD: USD now has latest poses
    Camera->>Renderer: render(render_data)
    Renderer->>Replicator: ensure_isaac_rtx_render_update()
    Replicator->>USD: read camera transforms
    USD-->>Replicator: fresh transforms
    Replicator-->>Renderer: rendered frame
    Renderer-->>Camera: output buffers filled
Loading

Last reviewed commit: 67bd3e4

@bareya bareya marked this pull request as draft March 10, 2026 20:47
@ooctipus
Copy link
Collaborator

repost:

My worry is that for tiled camera we almost always directly read and write to fabric array
sync_usd_on_fabric_write could be an expensive operation
For non-tiled camera, I guess the user doesn't care about speed at all

@bareya
Copy link
Collaborator Author

bareya commented Mar 10, 2026

This requires longer investigation.

@bareya bareya closed this Mar 10, 2026
@bareya bareya reopened this Mar 11, 2026
@bareya bareya marked this pull request as ready for review March 11, 2026 02:40
Comment on lines +173 to +178
# Create a view for the sensor with Fabric enabled for fast pose queries, otherwise position will be stale.
self._view = XformPrimView(
self.cfg.prim_path,
device=self._device,
stage=self.stage,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inaccurate comment copied from camera.py

The comment says "with Fabric enabled for fast pose queries, otherwise position will be stale" — but sync_usd_on_fabric_write=True is intentionally not passed here. In camera.py that flag is what makes the comment true; here it is absent by design (the docstring in update_camera even explains the trade-off: "TiledCamera disables sync_usd_on_fabric_write for performance").

The stale-pose problem in TiledCamera is solved by the new update_camera() path that writes directly to USD, not by Fabric–USD sync. Copying the camera.py comment verbatim will mislead future readers into thinking the XformPrimView is configured identically to the base class.

Consider updating it to accurately describe the approach used here:

Suggested change
# Create a view for the sensor with Fabric enabled for fast pose queries, otherwise position will be stale.
self._view = XformPrimView(
self.cfg.prim_path,
device=self._device,
stage=self.stage,
)
# Create a view for the sensor. NOTE: sync_usd_on_fabric_write is intentionally NOT set here
# for performance. Instead, USD poses are written directly in update_camera() before each render
# so that Replicator picks up the latest transforms.
self._view = XformPrimView(
self.cfg.prim_path,
device=self._device,
stage=self.stage,
)

Comment on lines +194 to +195
orientations_opengl = convert_camera_frame_orientation_convention(orientations, origin="world", target="opengl")
sensor._view._set_world_poses_usd(positions, orientations_opengl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All camera USD poses rewritten on every _update_poses call

update_camera receives the full self._data.pos_w / self._data.quat_w_world tensors (all view.count cameras) even when _update_poses was called for only a subset of env_ids. _set_world_poses_usd then iterates over every prim via Sdf.ChangeBlock, so cameras whose pose didn't change still incur USD-write and stage-notification overhead.

In large-scale training runs with many parallel environments this could become a non-trivial cost per camera.update() call.

A targeted write would limit the overhead to only the cameras that were actually moved:

orientations_opengl = convert_camera_frame_orientation_convention(
    orientations, origin="world", target="opengl"
)
# Write only the subset passed to _update_poses (if indices are threaded through)
sensor._view._set_world_poses_usd(positions, orientations_opengl)

Consider threading the env_ids / indices down from _update_poses into update_camera and forwarding them to _set_world_poses_usd(positions[env_ids], orientations_opengl[env_ids], indices=env_ids). This keeps the fix correct while avoiding redundant USD writes for unchanged cameras.

@bareya bareya marked this pull request as draft March 11, 2026 02:52
@bareya bareya changed the title Fix Tiled Camera bug when set_world_poses_from_view is called Test for Camera and Tiled Camera transforms on CPU and GPU Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants