Move TiledCamera implementation into Camera and deprecate TiledCamera. #5162
Move TiledCamera implementation into Camera and deprecate TiledCamera. #5162nvsekkin wants to merge 14 commits intoisaac-sim:developfrom
Conversation
Greptile SummaryThis PR unifies
Confidence Score: 4/5Safe to merge after addressing the rgba output-key regression; all other prior concerns have been resolved. One P1 finding remains: source/isaaclab/isaaclab/sensors/camera/camera.py — specifically the
|
| 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
There was a problem hiding this comment.
🤖 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:
- Keep both implementations, share code via inheritance → Rejected correctly because it would leave confusing API surface with unclear when to use which
- Extract common logic to a base class → Would add complexity without clear benefit; full consolidation is cleaner
- Current approach: Unify into
Camera, deprecateTiledCamera→ 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:
render_product_pathsproperty removed — PR author confirmed zero external usages (only internal references incamera.py,isaac_rtx_renderer.py,ovrtx_renderer.py).TiledCamera.data.infoformat change from flatdicttolist[dict]— This fixes a documented API contract violation. Users who accessedinfo["semantic_segmentation"]must now useinfo[0]["semantic_segmentation"].
Downstream consumers that use TiledCamera:
VisuoTactileSensor(isaaclab_contrib) — Will emit deprecation warning but continue to workobservations.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.py→test_camera.py(multi-regex init, all annotators, segmentation non-colorize, normals unit length, data types ordering, frame offset) - Slims
test_tiled_camera.pyto 4 focused deprecation/compatibility tests - Fixes
infoassertions intest_tiled_camera.pyandtest_multi_tiled_camera.pyto use the correctedlist[dict]format - Tests cover both
cuda:0andcpudevices
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-compatibleThis 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.
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py
Outdated
Show resolved
Hide resolved
2895d4a to
29804f1
Compare
…h Replicator annotator output
|
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] |
Description
Unify
CameraandTiledCameraby moving TiledCamera's renderer-based implementation intoCameraand makingTiledCameraa deprecated subclass alias.What changed
camera.py:_initialize_implnow usesRenderer(cfg.renderer_cfg)+renderer.prepare_stage()+renderer.create_render_data()instead of replicator imports and per-camera render product loops_create_buffersuses eager pre-allocation +renderer.set_outputs()instead of lazy allocation_rep_registry,_render_product_paths,_create_annotator_data(),_process_annotator_output(),render_product_pathsproperty,omni.replicator.coreimport,SyntheticDataimporttiled_camera.py:class TiledCamera(Camera)subclass that emits aDeprecationWarningon instantiationtiled_camera_cfg.py:DeprecationWarningin__post_init__infotype change:CameraData.infochanged fromlist[dict[str, Any]]todict[str, Any], matching the Replicator annotator output structure. The oldlist[dict]was an artifact of the originalCamera'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 nowcamera.data.info["semantic_segmentation"]directly.Test suite:
test_tiled_camera.py→test_camera.py(multi-regex init, all annotators, segmentation non-colorize, normals unit length, data types ordering, frame offset)test_tiled_camera.pyto 4 focused deprecation/compatibility testsinfoassertions across all test files to use flatdictaccess pattern (info["key"]instead ofinfo[0]["key"])Breaking changes
render_product_pathsproperty removed (confirmed zero external usage across entire repo — only referenced internally incamera.py,isaac_rtx_renderer.py, andovrtx_renderer.py)CameraData.infochanged fromlist[dict[str, Any]]todict[str, Any].Camerausers (old): replacecamera.data.info[cam_idx][data_type]withcamera.data.info[data_type].TiledCamerausers (old): access patterncamera.data.info[data_type]is unchanged.Type of change
Screenshots
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there