Skip to content

Move TiledCamera implementation into Camera and deprecate TiledCamera. #5162

Open
nvsekkin wants to merge 14 commits intoisaac-sim:developfrom
nvsekkin:esekkin/camera-refactor
Open

Move TiledCamera implementation into Camera and deprecate TiledCamera. #5162
nvsekkin wants to merge 14 commits intoisaac-sim:developfrom
nvsekkin:esekkin/camera-refactor

Conversation

@nvsekkin
Copy link
Copy Markdown

@nvsekkin nvsekkin commented Apr 3, 2026

Description

Unify Camera and TiledCamera by moving TiledCamera's renderer-based implementation into Camera and making TiledCamera a deprecated subclass alias.

What changed

camera.py:

  • _initialize_impl now uses Renderer(cfg.renderer_cfg) + renderer.prepare_stage() + renderer.create_render_data() instead of replicator imports and per-camera render product loops
  • _create_buffers uses eager pre-allocation + renderer.set_outputs() instead of lazy allocation
  • Removed: _rep_registry, _render_product_paths, _create_annotator_data(), _process_annotator_output(), render_product_paths property, omni.replicator.core import, SyntheticData import

tiled_camera.py:

  • Replaced entire class with a thin class TiledCamera(Camera) subclass that emits a DeprecationWarning on instantiation

tiled_camera_cfg.py:

  • Added DeprecationWarning in __post_init__

info type change:

  • CameraData.info changed from list[dict[str, Any]] to dict[str, Any], matching the Replicator annotator output structure. The old list[dict] was an artifact of the original Camera's per-camera-annotator architecture. With tiled rendering, all cameras share a single annotator pipeline, so the per-camera list dimension carried no information. Access is now camera.data.info["semantic_segmentation"] directly.

Test suite:

  • Ported 6 unique tests from test_tiled_camera.pytest_camera.py (multi-regex init, all annotators, segmentation non-colorize, normals unit length, data types ordering, frame offset)
  • Slimmed test_tiled_camera.py to 4 focused deprecation/compatibility tests
  • Updated info assertions across all test files to use flat dict access pattern (info["key"] instead of info[0]["key"])

Breaking changes

  • render_product_paths property removed (confirmed zero external usage across entire repo — only referenced internally in camera.py, isaac_rtx_renderer.py, and ovrtx_renderer.py)
  • CameraData.info changed from list[dict[str, Any]] to dict[str, Any]. Camera users (old): replace camera.data.info[cam_idx][data_type] with camera.data.info[data_type]. TiledCamera users (old): access pattern camera.data.info[data_type] is unchanged.

Type of change

  • Breaking change (existing functionality will not work without user modification)

Screenshots

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

@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Apr 3, 2026
@nvsekkin nvsekkin changed the title Move TiledCamera implementation into Camera and deprecate TiledCamera. Draft: Move TiledCamera implementation into Camera and deprecate TiledCamera. Apr 3, 2026
@nvsekkin nvsekkin changed the title Draft: Move TiledCamera implementation into Camera and deprecate TiledCamera. Move TiledCamera implementation into Camera and deprecate TiledCamera. Apr 3, 2026
@nvsekkin nvsekkin marked this pull request as draft April 3, 2026 04:06
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 3, 2026

Greptile Summary

This PR unifies Camera and TiledCamera by moving all rendering logic into Camera via the Renderer abstraction and reducing TiledCamera to a thin deprecated subclass. The approach is sound and prior review concerns (semantic filter, info format, env_ids broadcast) have been addressed.

  • P1 — rgba leaks into output when only rgb is requested: _create_buffers unconditionally inserts \"rgba\" into self._data.output whenever \"rgb\" is in data_types, changing output.keys() for existing Camera users who never requested rgba. The old replicator-based Camera never exposed \"rgba\" unless explicitly requested, and _data.info remains keyed only by cfg.data_types, creating an inconsistency between the two structures.

Confidence Score: 4/5

Safe to merge after addressing the rgba output-key regression; all other prior concerns have been resolved.

One P1 finding remains: rgba is unconditionally added to camera.data.output when only rgb is requested, changing the public contract of CameraData.output for existing Camera users. All previously flagged concerns (semantic filter, info format fix, env_ids broadcast, super().post_init) are resolved or intentionally deferred with justification.

source/isaaclab/isaaclab/sensors/camera/camera.py — specifically the _create_buffers rgba allocation and the _update_buffers_impl write_output loop.

Vulnerabilities

No security concerns identified. The PR does not introduce new network access, credential handling, user-controlled inputs, or injection vectors.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sensors/camera/camera.py Core change: replaces per-camera replicator annotators with a Renderer abstraction; introduces a P1 regression where rgba appears in output when only rgb is requested, and exposes renderer/render_data as public attributes.
source/isaaclab/isaaclab/sensors/camera/tiled_camera.py Replaced full ~360-line implementation with a minimal deprecated subclass that emits DeprecationWarning and delegates to Camera; clean and correct.
source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Migrated tiled-camera renderer logic from TiledCamera into a standalone BaseRenderer implementation; handles annotator lifecycle, segmentation filters, simple-shading, depth clipping, and cleanup correctly.
source/isaaclab/isaaclab/sensors/camera/camera_data.py Added a docstring note clarifying that output key order is not guaranteed to match data_types; no logic changes.

Reviews (2): Last reviewed commit: "Document dictionary behavior" | Re-trigger Greptile

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

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

🤖 Isaac Lab Review Bot

Summary

This PR unifies Camera and TiledCamera by moving TiledCamera's renderer-based implementation into Camera and deprecating TiledCamera as a thin subclass alias. The new Camera class delegates rendering to the Renderer abstraction (via IsaacRtxRenderer), removing the old per-camera replicator annotator approach. The PR also fixes a silent API contract violation where TiledCamera.data.info returned a flat dict instead of the documented list[dict].

Design Assessment

This is a well-designed refactoring that improves the framework's architecture.

Problem being solved: Redundant implementations — Camera and TiledCamera maintained separate codepaths for essentially the same functionality, with TiledCamera using the cleaner Renderer abstraction while Camera used legacy per-camera replicator annotators.

Alternative approaches considered:

  1. Keep both implementations, share code via inheritance → Rejected correctly because it would leave confusing API surface with unclear when to use which
  2. Extract common logic to a base class → Would add complexity without clear benefit; full consolidation is cleaner
  3. Current approach: Unify into Camera, deprecate TiledCamera → Best choice for consistency and maintainability

Framework layering: ✅ Correct. The PR keeps rendering logic in the Renderer abstraction layer where it belongs, and the sensor layer (Camera) just orchestrates initialization and buffer management. No boundary violations.

Will this survive refactors? Yes — the Renderer abstraction is cleanly separated, so Camera doesn't depend on RTX internals.

Design is sound.

Architecture Impact

Breaking changes:

  1. render_product_paths property removed — PR author confirmed zero external usages (only internal references in camera.py, isaac_rtx_renderer.py, ovrtx_renderer.py).
  2. TiledCamera.data.info format change from flat dict to list[dict] — This fixes a documented API contract violation. Users who accessed info["semantic_segmentation"] must now use info[0]["semantic_segmentation"].

Downstream consumers that use TiledCamera:

  • VisuoTactileSensor (isaaclab_contrib) — Will emit deprecation warning but continue to work
  • observations.py — Type hints updated, behavior unchanged
  • Various task envs — All use TiledCameraCfg, will work through the deprecation shim

No silent breakage — the deprecation warnings provide clear migration guidance.

Implementation Verdict

Ship it — Clean implementation with comprehensive test coverage. One minor note below.

Test Coverage

Excellent. The PR:

  • Ports 6 unique tests from test_tiled_camera.pytest_camera.py (multi-regex init, all annotators, segmentation non-colorize, normals unit length, data types ordering, frame offset)
  • Slims test_tiled_camera.py to 4 focused deprecation/compatibility tests
  • Fixes info assertions in test_tiled_camera.py and test_multi_tiled_camera.py to use the corrected list[dict] format
  • Tests cover both cuda:0 and cpu devices

The tests properly validate the unified behavior and the deprecation mechanism.

CI Status

Only labeler check has run so far (passed). Full test suite hasn't executed yet — recommend waiting for CI to complete before merge.

Findings

🔵 Improvement: tiled_camera_cfg.py:28 — Missing super().__post_init__() call

While CameraCfg and SensorBaseCfg don't currently have __post_init__ methods, calling super().__post_init__() is a defensive practice that prevents silent bugs if a parent class adds __post_init__ in the future. Consider:

def __post_init__(self):
    warnings.warn(
        "TiledCameraCfg is deprecated. Use CameraCfg directly.",
        DeprecationWarning,
        stacklevel=2,
    )
    super().__post_init__()  # Forward-compatible

This is a minor improvement, not blocking.


Overall: This is a clean, well-tested refactoring that improves framework consistency and fixes a documented API violation. The deprecation path is properly implemented with clear warnings. Ready to merge once CI completes.

@nvsekkin nvsekkin marked this pull request as ready for review April 8, 2026 18:22
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

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

Typo in docker/.env.baseISAACSSIM_VERSION (double S) should be ISAACSIM_VERSION. Docker builds referencing ${ISAACSIM_VERSION} (e.g. docker-compose.yaml:95) will resolve to empty.

@nvsekkin nvsekkin force-pushed the esekkin/camera-refactor branch from 2895d4a to 29804f1 Compare April 9, 2026 22:29
@nvsekkin nvsekkin requested a review from jtigue-bdai as a code owner April 9, 2026 23:10
@kellyguo11
Copy link
Copy Markdown
Contributor

One of the doc pages will also need an update

/home/runner/work/IsaacLab/IsaacLab/docs/source/how-to/save_camera_output.rst:37: WARNING: end-at pattern not found: single_cam_info = camera.data.info[camera_index] [docutils]

@nvsekkin nvsekkin requested a review from Mayankm96 as a code owner April 10, 2026 18:27
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 10, 2026
@nvsekkin nvsekkin requested review from bareya and kellyguo11 April 10, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants