Migrate camera/renderer/camera data to warp#5578
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR migrates Camera, CameraData, and renderer backends to use warp-backed ProxyArray instead of raw torch.Tensor. The changes align the camera sensor with other IsaacLab sensors (ContactSensor, RayCaster, IMU) that already use this pattern.
✅ Strengths
-
Consistent API: All sensor data now follows the same ProxyArray pattern, enabling zero-copy interop between warp kernels and torch operations.
-
Clean backward compatibility: The deprecation bridge via ProxyArray allows existing torch-based code to continue working with a one-time warning, giving users time to migrate.
-
Proper RGB/RGBA aliasing: The strided warp view for RGB into RGBA storage is well-implemented:
rgb_wp = wp.array( ptr=rgba_wp.ptr, shape=(num_views, height, width, 3), strides=(height * width * 4, width * 4, 4, 1), dtype=wp.uint8, device=rgba_wp.device, copy=False, )
-
Comprehensive test updates: All test files correctly updated to use
.torchfor shape/dtype assertions.
🔍 Observations
1. Device string handling in camera.py
The device conversion is handled correctly:
device_str = self._device if isinstance(self._device, str) else str(self._device)This ensures compatibility whether self._device is a string or torch.device object.
2. Quaternion convention conversion
In camera.py line 560, the call to convert_camera_frame_orientation_convention now receives a ProxyArray directly:
self._data.quat_w_world.torch[env_ids] = convert_camera_frame_orientation_convention(
quat_w, origin="opengl", target="world"
)This relies on the deprecation bridge to handle the conversion implicitly. Verify that convert_camera_frame_orientation_convention properly handles ProxyArray input—if it internally operates on tensors, the bridge should work, but explicit .torch might be cleaner for clarity.
3. Newton renderer's intrinsics access
In newton_warp_renderer.py line 112:
first_focal_length = intrinsics.torch[:, 1, 1][0:1]This correctly accesses the torch view for the focal length computation. Good attention to detail.
4. OVRTX renderer position handling
In ovrtx_renderer.py, the positions ProxyArray is passed directly to the warp kernel:
inputs=[positions, camera_orientations_wp, camera_transforms],Since ProxyArray supports warp interop, this should work correctly, but ensure the kernel signature expects wp.array(dtype=wp.vec3f).
📋 Minor Suggestions
-
Changelog typo: In
source/isaaclab/changelog.d/mh-warp_cam.minor.rst, the checkbox formatting appears corrupted:- [ x I have added my name...Should be
- [x]. -
Consider explicit
.torchin orientation conversion: For code clarity incamera.py:560, consider:self._data.quat_w_world.torch[env_ids] = convert_camera_frame_orientation_convention( quat_w.torch, origin="opengl", target="world" )
This makes the tensor conversion explicit rather than relying on the deprecation bridge.
✅ Overall
Well-structured migration that maintains backward compatibility while aligning the camera sensor with IsaacLab's warp-first architecture. The changes are thorough across all renderer backends (Newton, OVRTX, IsaacRtx) and test coverage has been appropriately updated.
🔄 Update (2026-05-13)
Re-reviewed commits since 632c29dc. Previous suggestions have been addressed:
-
✅ Orientation conversion: Now using a native warp kernel
convert_camera_frame_orientation_convention_wpthat operates directly on warp arrays. This is cleaner and more performant than my original suggestion of using explicit.torch. -
✅ Changelog: The content has been properly formatted with the actual change description.
-
✅ Renderer initialization: The new
initialize()lifecycle hook onBaseRendererallows proper sequencing of pre-physics and post-physics setup.
No new issues identified in the incremental changes. The PR has also been approved by maintainers.
Update (0f2d3b5): Reviewed incremental changes to renderer initialization lifecycle:
✅ BaseRenderer.initialize() hook: Good addition - allows post-physics setup separation from construction.
✅ RenderContext.ensure_initialize(): Properly idempotent and integrates with PHYSICS_READY callback.
✅ NewtonWarpRenderer split: Clean separation into pre-physics __init__ (registers requirements) and post-physics initialize() (reads Newton model). The self.newton_sensor = None initialization in __init__ is correct.
✅ OVRTXRenderer changes:
- Moving
Renderercreation to__init__aligns with OVRTX 0.3 requirements (must create before reset). - Replacing
assertwith explicitRuntimeErroris safer (works with-Oflag). - Renaming
initialize(spec)→_initialize_from_spec(spec)avoids shadowing the new lifecycle hook.
No new issues introduced. Previous review findings remain applicable.
Update (54aca63): Reviewed incremental changes for OVPhysX RigidObjectData and OvPhysxManager improvements:
This commit adds significant OVPhysX infrastructure alongside the camera warp migration:
✅ RigidObjectData for OVPhysX (ovphysx_rigid_object_data.py): Full implementation of rigid body state properties using ProxyArray wrappers and TimestampedBuffer caching. Well-structured property accessors with lazy initialization matching the articulation data pattern.
✅ OvPhysxManager process-global device lock (ovphysx_manager.py): Added _locked_device class variable and validation to surface a clear Python error when a different device is requested after the wheel's C++ layer has locked. Good defensive programming against the ovphysx<=0.3.7 device-mode limitation.
✅ Soft-reset refactor (_release_physx): Changed from dropping the Python reference (which triggered destructor race conditions) to calling physx.reset() and keeping the cached instance alive. The HACK comment clearly documents the dual-Carbonite static destructor issue.
✅ CPU/GPU scene prim configuration (_configure_physx_scene_prim): Now properly conditionalizes GPU-specific attributes based on device parameter, preventing unnecessary GPU attributes from being written for CPU simulations.
✅ Mock binding set updates (mock_ovphysx_bindings.py): Added asset_kind="rigid_object" support with appropriate tensor type validation.
✅ New tensor types (tensor_types.py): Added RIGID_BODY_* constants with graceful try/except for optional wheel aliases that may not be available yet.
✅ Comprehensive test suite (test_rigid_object.py): 1000+ lines of real-backend tests mirroring PhysX test patterns, with proper device-lock skip logic via _ovphysx_skip_other_device fixture.
Note: These changes appear to be from a different feature branch (RigidObject warp migration) that was merged/rebased onto this camera PR. The code quality is consistent with the rest of the PR.
No blocking issues found.
Update (de53ea1): Reviewed incremental changes for RayCasterCamera warp migration:
✅ multi_mesh_ray_caster_camera.py: Removed redundant torch buffer initialization for pos_w and quat_w_world. Now correctly uses .torch accessor on the ProxyArray wrappers allocated by CameraData.create_buffers().
✅ ray_caster_camera.py: Switched from manual torch buffer creation to the CameraData.create_buffers() factory method and ProxyArray pattern for output data. This aligns the RayCasterCamera with the TiledCamera/Camera migration completed earlier in this PR.
No new issues found. These changes complete the warp migration across all camera sensor types.
Update (0b90700): Reviewed incremental changes addressing look-at math robustness:
✅ create_rotation_matrix_from_view singularity fix (math.py): When the look-at direction is parallel to the up axis, the function now uses an alternate reference vector (world X) to compute a valid orthonormal frame instead of producing a singular matrix. Rows with undefined forward direction (eyes == targets or non-finite input) now return NaN to signal failure.
✅ quat_from_matrix validation (math.py): Added a safeguard that detects non-rotation input (singular, reflection, or scale-error matrices) by checking if the output quaternion norm deviates from 1.0 beyond a 2e-5 threshold. Invalid inputs now return NaN.
✅ set_world_poses_from_view graceful degradation (camera.py, ray_caster_camera.py): Both implementations now detect degenerate rows via NaN checking and skip them (with a logged warning), raising ValueError only if the entire batch is degenerate. This is a safer API than silent garbage poses.
✅ Test updates: Added _assert_quat_close helper to handle equivalent negated quaternion representations. Output dtype assertions correctly updated from torch.dtype to wp.dtype (e.g., wp.uint8, wp.float32). Comprehensive new test cases for the math edge cases.
✅ PVA debug visualization (isaaclab_newton, isaaclab_physx): Filters out bodies with effectively zero acceleration before computing arrow direction, avoiding undefined rotation matrices.
✅ Changelog CLI validation (tools/changelog/cli.py): Added check for orphan paragraphs (non-bullet lines at column 0) that would cause Sphinx Unexpected indentation errors when merged into CHANGELOG.rst.
All changes are well-tested and improve robustness without breaking backward compatibility. No new issues identified.
Update (b206201): Reviewed incremental changes since 0b907003:
✅ LEAPP export script refactoring (export.py): The script has been restructured into composable functions (parse_export_args(), export_rsl_rl_agent(), run_export_with_hydra(), main_cli()) with proper resource cleanup in finally blocks. This enables in-process testing and better modularity.
✅ AppLauncher CUDA startup fix (app_launcher.py): Defers torch.cuda.set_device() until after SimulationApp starts to avoid crashes from OpenBLAS at-fork handlers during Kit's platform-info fork.
✅ Import path cleanup: Changed from from isaaclab.utils import configclass to direct from isaaclab.utils.configclass import configclass imports in physics config files. This is a minor cleanup.
✅ Test optimizations:
- Removed unnecessary Isaac Sim imports from
test_noise.pyandtest_wrench_composer.py - Batched LEAPP export tests (
test_rsl_rl_export_flow.py) to share a single Kit process, reducing CI time - Added
-sflag to pytest for test output visibility
✅ CI improvements: Added OmniHub environment setup and LEAPP package resolution logging in test runner.
✅ Version bumps: isaaclab 5.2.1, isaaclab_newton 0.9.1, isaaclab_physx 0.7.1 with consolidated changelogs.
No new issues found. These changes are orthogonal to the camera warp migration and improve infrastructure, testing, and CI reliability.
Update (b08de57): Reviewed incremental changes since b2062011:
✅ isaaclab_tasks test updates for ProxyArray (rendering_test_utils.py, test_rendering_registered_tasks.py, test_shadow_hand_vision_presets.py): These changes propagate the ProxyArray migration to the isaaclab_tasks package test suite.
Key changes:
- Type hints updated from
torch.TensortoProxyArrayinvalidate_camera_outputs() - Camera output handling now uses conditional conversion:
tensor = output if isinstance(output, torch.Tensor) else output.torch - Shadow Hand vision preset tests correctly use
.torchaccessor
✅ Changelog skip marker (mh-warp_cam.skip): Added to prevent duplicate changelog entries since the main changelog is in the isaaclab core package.
These are clean, straightforward changes completing the ProxyArray adoption across the full test suite. No issues found.
Update (4933fff): Reviewed final commit removing LEAPP semantics:
✅ camera_data.py cleanup: Removed leapp_tensor_semantics decorators and imports from pos_w, quat_w_world, quat_w_ros, and quat_w_opengl properties. This is appropriate scope reduction - LEAPP integration should be handled in a separate PR rather than bundled with the warp migration.
No issues found. PR is ready for merge.
Update (3b4b2cd): Reviewed incremental changes since 4933fffd:
The new commits add documentation, tooling, and infrastructure changes unrelated to the camera warp migration:
✅ uv run workflow (pyproject.toml, uv_run.rst): Experimental support for uv run train and uv run play directly from repo root. Well-structured with proper CUDA index routing per architecture.
✅ Unified RL entrypoints (scripts/reinforcement_learning/): New train.py and play.py dispatch scripts with shared common.py utilities. Library-specific scripts now have deprecation warnings. This is a nice developer experience improvement.
✅ isaaclab CLI additions (cli/__init__.py, setup.py): Added train and play console-script entry points. The early dispatch in cli() for these commands is correct.
✅ Teleop replay automation (teleop_replay_agent.py, isaaclab_teleop.automation): Non-interactive CI entry point for XCR replay. Good defensive programming with timeout bounds and proper cleanup.
✅ Newton tendon label fix (newton_replicate.py): Fixed boundary-safe prefix matching for string label renaming. The new test coverage is comprehensive.
✅ skrl drone-ARL config fix: Changed input: STATES to input: OBSERVATIONS. Correct fix for single-agent environments where state_space is None.
✅ Doc updates: RLinf setup instructions, flash-attn version bump, RST formatting fixes.
No issues found in the incremental changes. The camera warp migration (original PR focus) remains unchanged since my last review. PR has already been approved by maintainers (@pbarejko, @fatimaanes, @ooctipus).
Greptile SummaryThis PR migrates
Confidence Score: 4/5Safe to merge; all functional paths are logically consistent with the ProxyArray contract and no data-loss or crash conditions were identified. The hardcoded strides in the RGB alias construction are arithmetically correct for current specs but fragile against future RGBA spec changes. The stale docstring and mixed warp/torch path in OVRTXRenderer are cosmetic. No blocking issues found. source/isaaclab/isaaclab/sensors/camera/camera_data.py (RGB stride construction), source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py (stale docstring) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self._pos_w_wp: wp.array | None = None | ||
| self._quat_w_world_wp: wp.array | None = None | ||
| self._intrinsic_matrices: wp.array | None = None |
There was a problem hiding this comment.
intrinsic_matrices uses the name _intrinsic_matrices while the other two backing fields follow the _<name>_wp pattern (_pos_w_wp, _quat_w_world_wp). Without the _wp suffix it is easy to confuse the raw warp buffer with the ProxyArray wrapper _intrinsic_matrices_pa.
| self._pos_w_wp: wp.array | None = None | |
| self._quat_w_world_wp: wp.array | None = None | |
| self._intrinsic_matrices: wp.array | None = None | |
| self._pos_w_wp: wp.array | None = None | |
| self._quat_w_world_wp: wp.array | None = None | |
| self._intrinsic_matrices_wp: wp.array | None = None |
| if rgb_alias: | ||
| # Zero-copy strided view into rgba: shape (N, H, W, 3), skipping the alpha channel. | ||
| # Byte strides for a contiguous (N, H, W, 4) uint8 array are (H*W*4, W*4, 4, 1). | ||
| # Using the same outer strides but limiting the last dim to 3 channels gives a | ||
| # non-contiguous view where each pixel reads RGB without the alpha byte. | ||
| rgba_wp = buffers[str(RenderBufferKind.RGBA)].warp | ||
| rgb_wp = wp.array( | ||
| ptr=rgba_wp.ptr, | ||
| shape=(num_views, height, width, 3), | ||
| strides=(height * width * 4, width * 4, 4, 1), | ||
| dtype=wp.uint8, | ||
| device=rgba_wp.device, | ||
| copy=False, | ||
| ) |
There was a problem hiding this comment.
strides=(height * width * 4, width * 4, 4, 1) and dtype=wp.uint8 without reading the RGBA spec. If a future renderer provides RGBA with a different element size the strides and dtype would silently be wrong. Deriving both from supported_specs[RenderBufferKind.RGBA] makes the invariant explicit.
| if rgb_alias: | |
| # Zero-copy strided view into rgba: shape (N, H, W, 3), skipping the alpha channel. | |
| # Byte strides for a contiguous (N, H, W, 4) uint8 array are (H*W*4, W*4, 4, 1). | |
| # Using the same outer strides but limiting the last dim to 3 channels gives a | |
| # non-contiguous view where each pixel reads RGB without the alpha byte. | |
| rgba_wp = buffers[str(RenderBufferKind.RGBA)].warp | |
| rgb_wp = wp.array( | |
| ptr=rgba_wp.ptr, | |
| shape=(num_views, height, width, 3), | |
| strides=(height * width * 4, width * 4, 4, 1), | |
| dtype=wp.uint8, | |
| device=rgba_wp.device, | |
| copy=False, | |
| ) | |
| if rgb_alias: | |
| # Zero-copy strided view into rgba: shape (N, H, W, 3), skipping the alpha channel. | |
| # Strides derived from the spec so a non-uint8 RGBA layout would still be correct. | |
| rgba_spec = supported_specs[RenderBufferKind.RGBA] | |
| rgba_channels = rgba_spec.channels # expected 4 | |
| rgba_wp = buffers[str(RenderBufferKind.RGBA)].warp | |
| rgb_wp = wp.array( | |
| ptr=rgba_wp.ptr, | |
| shape=(num_views, height, width, 3), | |
| strides=( | |
| height * width * rgba_channels, | |
| width * rgba_channels, | |
| rgba_channels, | |
| 1, | |
| ), | |
| dtype=rgba_spec.dtype, | |
| device=rgba_wp.device, | |
| copy=False, | |
| ) |
| num_envs = positions.shape[0] | ||
| camera_quats_opengl = convert_camera_frame_orientation_convention(orientations, origin="world", target="opengl") | ||
| camera_positions_wp = wp.from_torch(positions.contiguous(), dtype=wp.vec3) | ||
| camera_orientations_wp = wp.from_torch(camera_quats_opengl.contiguous(), dtype=wp.quatf) |
There was a problem hiding this comment.
update_camera
positions is passed directly to wp.launch via __cuda_array_interface__ (fast path), but orientations still goes through convert_camera_frame_orientation_convention → .contiguous() → wp.from_torch. Since orientations is already a wp.quatf-typed ProxyArray, this round-trip is inconsistent with how positions is handled and adds unnecessary overhead on every camera update.
| if rgb is None: | ||
| rgb = env._tiled_camera.data.output[env.cfg.tiled_camera.data_types[0]] | ||
| last_rgb = rgb | ||
| # TODO: We should make the assert not use torch tensors as we migrate to warp. |
There was a problem hiding this comment.
Is this left out on purpose?
There was a problem hiding this comment.
I have removed the comment, but in general as we migrate to warp, we need to begin removing all these torch testing utilities. We should opt for np.testing for array comparisons using the warp array with .numpy(). Removed the comment for now as this is actually a wider issue that we can address altogether later.
| dst[i] = quat_mul_xyzw(src[i], q_const) | ||
|
|
||
|
|
||
| @wp.kernel | ||
| def _convert_camera_orientation_indexed_kernel( | ||
| src: wp.array(dtype=wp.quatf), | ||
| dst: wp.array(dtype=wp.quatf), | ||
| indices: wp.array(dtype=wp.int32), | ||
| q_const: wp.quatf, | ||
| ): | ||
| """Apply constant-quaternion convention conversion to indexed elements. | ||
|
|
||
| Reads ``src[i]`` and writes to ``dst[indices[i]]``. Use this for partial | ||
| camera updates (e.g. environment resets targeting a subset of cameras). | ||
| """ | ||
| i = wp.tid() | ||
| dst[indices[i]] = quat_mul_xyzw(src[i], q_const) |
There was a problem hiding this comment.
If the above is removed, this and the other kernel simplify to:
dst[i] = src[i] * q_const
| # Note: We can't use warp functions since warp quaternion is wxyz format | ||
|
|
||
|
|
||
| @wp.func | ||
| def quat_mul_xyzw(q1: wp.quatf, q2: wp.quatf) -> wp.quatf: | ||
| """Hamilton product of two unit quaternions in (x, y, z, w) format. | ||
|
|
||
| Args: | ||
| q1: First quaternion ``(x, y, z, w)``. | ||
| q2: Second quaternion ``(x, y, z, w)``. | ||
|
|
||
| Returns: | ||
| Product quaternion ``(x, y, z, w)``. | ||
| """ | ||
| x = q1[3] * q2[0] + q1[0] * q2[3] + q1[1] * q2[2] - q1[2] * q2[1] | ||
| y = q1[3] * q2[1] - q1[0] * q2[2] + q1[1] * q2[3] + q1[2] * q2[0] | ||
| z = q1[3] * q2[2] + q1[0] * q2[1] - q1[1] * q2[0] + q1[2] * q2[3] | ||
| w = q1[3] * q2[3] - q1[0] * q2[0] - q1[1] * q2[1] - q1[2] * q2[2] | ||
| return wp.quatf(x, y, z, w) | ||
|
|
There was a problem hiding this comment.
This custom quat_mul_xyzw kernel might be redundant. warp's wp.quatf uses xyzw order (not wxyz as the comment states), so the native q1 * q2 computes the same Hamilton product. The Isaac Lab 3.0 migration switched to XYZW specifically to align with Warp, PhysX, and Newton. Can this function be removed?
There was a problem hiding this comment.
Thanks, I assumed wrongly it would be wxyz since that's what I'm used to in MuJoCo. Removed and fixed the related comment as well
|
🔄 Re-review Update (b03897f) Reviewed incremental changes from 54aca63...b03897f. Status of previous concerns:
New changes look good: The migration from |
🤖 Isaac Lab Review Bot - Incremental Review UpdateReviewing commits: Summary of ChangesThis update introduces significant new functionality around unified RL training/play entrypoints and teleop automation, plus various bug fixes. 🆕 New Features
🐛 Bug Fixes
📝 Documentation
Review NotesThe changes look well-structured:
Previous line comments still apply - the warp array naming inconsistency and RGB strided-view construction issues noted in the prior review remain relevant. Reviewed SHA: |
🔄 Incremental Review Update (b6a3073)New commit reviewed: Changes in this update:
Assessment:✅ Good fix - Properly handles the case where Previous comments status:The 3 suggestions from the initial review (naming consistency for |
✨ Review Update (c17cd38)Incremental diff reviewed: b6a3073 → c17cd38 (merge from develop + minor fix) Previous Comments Status
Summary2 of 3 previous comments have been addressed. The RGB strided-view comment remains a minor suggestion (P2) and does not block approval. The merge from develop brought in unrelated changes (Newton bump, OVRTX cloning, preset CLI, etc.) which are outside the scope of this PR review. Isaac Lab Review Bot • automated incremental review |
📋 Incremental Review Update (e913585)Reviewed commits: c17cd38...e913585 ✅ Previous Review Comments AddressedAll three suggestions from my previous review have been implemented:
🆕 New Changes in This PushThe incremental diff adds several well-implemented features: Docker Non-Root User Migration
Frame Stacking Feature
Bug Fixes
SummaryThis is a solid incremental update. All previous review items have been addressed, and the new features (Docker non-root, frame stacking) are well-implemented with good test coverage. 👍 |
5 participants
Description
Migrates
Camera,CameraData, and all renderer backends to warp-backedProxyArray,consistent with the rest of IsaacLab's sensors (ContactSensor, RayCaster, IMU, etc.).
CameraDatafields (pos_w,quat_w_world,intrinsic_matrices,output) now returnProxyArrayinstead oftorch.Tensor. Use.torchfor a zero-copy tensor view or passdirectly to warp kernels.
RenderBufferSpec.dtypeis now a warp dtype (e.g.wp.float32).The
ProxyArraydeprecation bridge means existing torch usage continues to work with aone-time
DeprecationWarning.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there