Synchronized Multi-env Visualization

[zhx06]

Summary

Add synchronized multi-env visualization utility

Detailed description

• Added SynchronizedVisualizer: global auto-framed camera + per-env TiledCamera grid anchored to env_origins, with mp4/gif output
• Added related tests

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — PR #766 Review

Summary

This PR introduces SynchronizedVisualizer, a well-architected utility for capturing synchronized multi-environment visualization with a global (bird's-eye) camera and a per-env tiled camera grid. The implementation is clean, well-documented, and includes a solid two-phase test suite. Overall this is high-quality work — the findings below are aimed at robustness improvements for production use.

---

Architecture Assessment ✅

The two-camera approach (global + per-env tiled grid) is architecturally sound:

• TiledCamera integration correctly hooks into Isaac Lab's scene registration pre-build via add_env_camera_to_cfg, avoiding the well-known all-white-frames issue with late render-product registration.
• Standalone Camera fallback provides graceful degradation with a clear warning when the TiledCamera wasn't injected.
• Auto-framing from env-origin bounding boxes is a practical design that works regardless of scene layout. The margin parameter and max(span, 1.0) guard handle the single-env edge case correctly.
• OpenGL convention is used consistently in both the baked OffsetCfg path and the _lookat_offset helper.

---

Findings

1. 🔴 Memory accumulation in _CaptureBuffers — unbounded growth (Medium Severity)

File: isaaclab_arena/utils/synchronized_visualizer.py

_CaptureBuffers stores every captured frame as a numpy array with no cap or cleanup between saves. For realistic workloads:

64 envs × 480×360×3 bytes × 120 frames ≈ 3.8 GB RAM

With save_per_env=True, the per-env dict retains all individual frames in addition to the composed grid. For long rollouts or repeated capture-save cycles, this will OOM silently (no warning, no limit).

Suggestion: Add a clear_buffers: bool = True parameter to save(), or a standalone reset_buffers() method, and consider logging a warning when buffer memory exceeds a configurable threshold. This is especially important since this utility is designed to scale to 64+ envs.

---

2. 🟡 _write_video does not explicitly close the moviepy clip (Low-Medium Severity)

File: isaaclab_arena/utils/synchronized_visualizer.py (L547–550)

clip = ImageSequenceClip(frames, fps=fps)
clip.write_videofile(path, logger=None, audio=False)
del clip

del clip relies on garbage collection but moviepy's write_videofile spawns an ffmpeg subprocess internally. Without explicit clip.close(), the subprocess handle and file descriptors may linger — particularly relevant when writing many per-env videos in a loop (e.g., save_per_env=True with 64 envs could leave 64+ lingering ffmpeg handles).

Suggestion:

clip = ImageSequenceClip(frames, fps=fps)
try:
    clip.write_videofile(path, logger=None, audio=False)
finally:
    clip.close()

---

3. 🟡 initialize() calls sim.reset() — discoverability concern (Low-Medium Severity)

File: isaaclab_arena/utils/synchronized_visualizer.py (L307)

The method-level docstring correctly warns that sim.reset() discards placement/randomization state, but:

1. The class-level docstring (the usage example at the top of the module) does not mention this constraint.
2. Users reading only the constructor or class docs may call initialize() mid-rollout without realizing it resets the sim.
3. If a user forgets env.reset() after initialize(), the OrderEnforcing wrapper will reject subsequent step() calls with an unhelpful error.

Suggestion: Add the required call ordering to the class-level usage example (viz.initialize() → env.reset() → loop), and consider asserting not self._initialized to prevent accidental double-initialization.

---

4. 🟡 close() does not remove created USD prims (Low Severity)

File: isaaclab_arena/utils/synchronized_visualizer.py (L559)

close() nulls camera references but leaves USD prims under prim_root (e.g., /World/SyncViz/GlobalCam, /World/SyncViz/EnvCam_*). If a user creates multiple SynchronizedVisualizer instances in the same simulation session (e.g., different camera angles for A/B comparison), orphan prims accumulate in the stage.

Suggestion: Either document that close() is a soft release (prims persist until the sim ends), or add optional prim cleanup via a remove_prims: bool = False parameter.

---

5. 🟡 Standalone Camera fallback path has no integration test coverage (Low Severity)

File: isaaclab_arena/tests/test_synchronized_visualizer.py

The smoke test explicitly validates only the TiledCamera path (assert viz._using_tiled). The standalone multi-prim Camera fallback (use_tiled_camera=False or missing injection) has zero integration coverage. If the fallback's set_world_poses_from_view positioning diverges from the baked OffsetCfg path (or breaks on a future Isaac Lab update), it would go undetected.

Suggestion: Add a second smoke test variant (or parametrize the existing one) with EnvView(use_tiled_camera=False) to exercise the fallback codepath.

---

6. 🟢 capture() per-frame allocation overhead for large env counts (Informational)

File: isaaclab_arena/utils/synchronized_visualizer.py (L449–455)

per_env = []
for i in range(self.num_envs):
    img = self._to_uint8(env_rgb[i])
    per_env.append(img)
    self._buffers.per_env_frames.setdefault(i, []).append(img)

This creates a fresh list and iterates per-env every frame. For typical counts (2–16) this is fine, but for 64+ envs, consider using np.split on the batched tensor directly to avoid per-frame Python-loop overhead.

---

Positive Highlights

• Two-phase test strategy is exemplary: fast pure-logic tests (no GPU) + focused integration smoke test with real rendering. CI can gate on Phase 1 without GPU resources.
• _check_optional_deps at construction prevents the frustrating scenario of discovering missing moviepy/Pillow only after an expensive rollout.
• Comprehensive docstrings with usage examples, warnings, and parameter descriptions make this immediately usable.
• _downscale_to_width enforcing even dimensions for h264 compatibility shows attention to production encoding details.
• Example script with full CLI parametrization (env view offsets, global azimuth/elevation, output format options) serves as both documentation and a ready-to-use visualization entry point.
• Excellent use of run_simulation_app_function for test isolation — keeping heavy sim tests in a subprocess prevents GPU resource leaks.

---

Verdict

Approve with minor suggestions. The core implementation is solid and production-ready. The memory accumulation issue (Finding #1) is the most impactful for real-world usage and worth addressing before this sees heavy use with large env counts. The remaining findings are quality-of-life improvements.

---

Update (2fd60a5): The author pushed a follow-up commit addressing several findings:

Findings Addressed ✅

• Finding #1 (Memory accumulation): Partially addressed — close() now resets _buffers = _CaptureBuffers(), freeing accumulated frames. Test test_close_clears_state_and_buffers verifies this. The buffer growth during a long rollout before close() is still unbounded (no cap or auto-clear on save()), but this is a reasonable first step.

• Finding #2 (moviepy clip not closed): ✅ Fully addressed — _write_video now uses try/finally: clip.close() with a clear comment explaining the motivation.

• Finding #3 (initialize() double-init guard): ✅ Fully addressed — Added assert not self._initialized, "initialize() already called; it must run exactly once." and the usage example in the module docstring now shows env.reset() after initialize(). New test test_initialize_rejects_double_init covers this.

Still Open (unchanged)

• Finding #4 (close() does not remove USD prims) — acknowledged as low severity, not addressed.
• Finding #5 (Standalone Camera fallback path untested) — not addressed.

New Functionality

• Per-env GIF export: save(save_per_env=True, also_gif=True) now also writes per-env GIF files (previously only global/grid got GIFs). Test updated to verify env000_gif and env001_gif keys.

Updated Verdict

The most impactful concerns (memory cleanup on close, explicit clip resource release, double-init guard) are now resolved. The remaining open items are low-severity. No objections to merge.

---

Update (85b14b7): Significant refactoring push addressing architecture, robustness, and test coverage. This is a major quality improvement.

Findings Addressed ✅

• Finding #1 (Memory accumulation): Further improved — capture_per_env is now an opt-in flag (default False), so per-env frame retention only costs memory when explicitly requested. The grid is still built from per-env data each frame without retaining it. close() swaps in a fresh _CaptureBuffers (decoupled from the old reference). save() also warns if save_per_env=True but capture_per_env was not enabled. This is a much better default for production workloads.

• Finding #3 (initialize() / sim.reset() concern): ✅ Fully resolved — The entire global camera architecture changed. The visualizer now creates its own USD camera prim (/World/SyncVizGlobalCam) and routes the render product through it, eliminating the sim.reset() call and the viewport_camera_controller dependency. Posing is now via direct USD xform authoring (_pose_camera_prim), which is headless-safe. The old viewer_origin subtraction dance is gone.

• Finding #4 (close() lifecycle): Significantly improved — Added a _closed flag with guards: initialize() and capture() both assert not self._closed. close() is idempotent (safe to call twice). New tests: test_initialize_after_close_raises, test_capture_after_close_raises, test_double_close_is_safe. The visualizer is explicitly documented as non-reusable after close.

• Finding #5 (Standalone Camera fallback untested): The standalone fallback appears to have been removed entirely in this refactor — the visualizer now exclusively uses the scene-registered TiledCamera + the new world camera prim. This eliminates the untested codepath rather than testing it (valid approach: simpler architecture).

• Finding #6 (per-frame allocation): The capture loop structure is unchanged, but capture_per_env=False by default means the per_env_frames dict is no longer populated unless requested, reducing the allocation cost.

New Improvements in This Push

1. Dedicated global camera prim (_setup_global_camera_prim): Defines a UsdGeom.Camera with proper focal length, aperture, and clipping range. Routes cfg.viewer.cam_prim_path to it before the first env.render() call. This is a correct fix for the headless posing problem (the GUI viewport cam /OmniverseKit_Persp is unposable headless).

2. Direct USD camera posing (_pose_camera_prim): Computes camera-to-world basis (OpenGL -Z forward, +Y up) and writes it as a matrix xform op. Handles near-vertical view edge case with right-vector fallback.

3. Viewer resolution contract assertion: __init__ now asserts cfg.viewer.resolution == (global_view.width, global_view.height), catching a subtle misconfiguration where GlobalView dimensions would be silently ignored.

4. Robust to_uint8 with no-alias guarantee: The function now explicitly copies CPU tensors (.numpy() shares storage) and ndarrays, preventing frame buffer aliasing. New tests: test_to_uint8_does_not_alias_source_ndarray, test_to_uint8_does_not_alias_source_cpu_tensor.

5. Capture diagnostics: capture() warns once (not per frame) on env.render() → None or all-zeros global frames (annotator warm-up). Sentinels _warned_global_none and _warned_global_black prevent log spam.

6. camera_video.py flush safety: _flush() now resets self.buffers and self.recording before writing, so a write failure cannot leave stale buffers that double-flush.

7. Scene type safety: Scene.sensors is now typed dict[str, SensorBaseCfg] (was Any), add_sensor asserts isinstance(sensor_cfg, SensorBaseCfg), and AssetCfg union includes SensorBaseCfg.

8. Expanded validation: EnvView and GlobalView now reject non-positive focal length. GlobalView elevation/distance guards only fire for auto-frame (explicit eye bypasses them). grid_cols > 0 assertion. save() rejects name_prefix with path separators (prevents directory traversal).

9. Test coverage greatly expanded: ~150 new test lines covering edge cases — test_to_uint8_rescales_normalized_float, test_to_uint8_rejects_empty_frame, test_write_video_rejects_empty_frames, test_capture_skips_per_env_when_disabled, test_capture_warns_once_on_none_render, test_save_returns_empty_and_warns_when_no_frames, test_reposition_poses_global_camera_prim, etc.

10. Docstring cleanup: Reduced verbosity throughout (fewer backtick-heavy cross-references, more concise phrasing). The module-level docstring now accurately describes the architecture (dedicated world camera, not viewport controller).

Minor Observations (Non-blocking)

• _pose_camera_prim falls back to right = [1, 0, 0] for near-vertical views (norm < 1e-6). This is fine for practical angles but could produce a discontinuous basis flip if the camera sweeps through exactly vertical during auto-framing. Not a realistic scenario given the elevation guard.

• The to_uint8 normalize heuristic (max <= 1.0 → scale up) can misfire on a near-black scene with float dtype. The docstring now correctly calls this out and recommends passing uint8 to avoid ambiguity. Good documentation of a known limitation.

Updated Verdict

All original findings are either fully resolved or eliminated by the architectural refactor. The dedicated world camera approach is cleaner and more correct than the viewport controller path it replaces. The no-alias guarantee in to_uint8, lifecycle guards, and opt-in per-env capture make this production-ready for large-scale rollouts. Excellent iteration. No objections to merge.

[greptile-apps]

Greptile Summary

Adds SynchronizedVisualizer, a utility that simultaneously records a global viewport view (via env.render()) and a per-env TiledCamera grid anchored to env origins, writing both as mp4/gif. It also extracts shared frame-encoding helpers into a new video_io module and extends Scene with sensor registration support.

• SynchronizedVisualizer (synchronized_visualizer.py): dataclass configs (EnvView, GlobalView) with validation guards, a USD camera-to-world matrix built directly from cross products, warn-once sentinels for degenerate global frames, and an idempotent close() that swaps in a fresh _CaptureBuffers to release accumulated frames.
• video_io (video_io.py): consolidates the duplicated _to_uint8 helper into a single copy-safe to_uint8, write_video (with finally-guarded clip.close()), and write_gif; camera_video.py is updated to delegate to these helpers.
• scene.py: adds a sensors dict and add_sensor() with bidirectional name-collision guards against the existing assets dict; sensors flow through get_scene_cfg() into the built Isaac Lab SceneCfg.

Confidence Score: 5/5

Safe to merge — new utility with no changes to existing runtime paths beyond the camera_video refactor, which is a strict improvement.

The camera_video refactor correctly moves state reset before writes (preventing double-flush), video_io centralizes frame-encoding logic with copy-safe semantics, SynchronizedVisualizer has proper lifecycle guards and idempotent close(), and the test suite covers all major code paths including the buffer-swap and per-env GIF branches identified in earlier review rounds.

No files require special attention.

Important Files Changed

Filename	Overview
isaaclab_arena/utils/synchronized_visualizer.py	New 650-line SynchronizedVisualizer: clean lifecycle, correct USD camera-to-world matrix construction, warn-once sentinels, and proper buffer swap in close().
isaaclab_arena/utils/video_io.py	New shared helpers: to_uint8 handles GPU/CPU tensors and ndarrays with copy-on-retain, write_video uses a try/finally to close the clip, write_gif uses Pillow correctly.
isaaclab_arena/evaluation/camera_video.py	Refactored to use video_io helpers; _flush now resets state before writing to prevent double-flush on write failure.
isaaclab_arena/scene/scene.py	Added sensors dict and add_sensor(); bidirectional name-collision guards between assets and sensors correctly in place.
isaaclab_arena/tests/test_synchronized_visualizer.py	Comprehensive test suite covering pure-logic paths, lifecycle guards, buffer management, tiling, and an end-to-end smoke test with a real SimulationApp.
isaaclab_arena_examples/visualization/synchronized_visualization_example.py	Demo script with correct warm-up/capture/reset pattern.
isaaclab_arena_examples/visualization/init.py	New package init file, license header only.

Sequence Diagram

sequenceDiagram
    participant User
    participant Scene
    participant SyncViz as SynchronizedVisualizer
    participant Env as Gym Env
    participant TiledCam as TiledCamera
    participant VideoIO as video_io

    User->>Scene: add_sensor(name, build_env_camera_cfg(env_view))
    User->>Env: "make_registered(render_mode=rgb_array)"
    User->>SyncViz: __init__(env, env_view, global_view)
    SyncViz->>Env: _resolve_env_camera()
    SyncViz->>Env: _setup_global_camera_prim()
    User->>Env: env.reset()
    User->>SyncViz: initialize()
    SyncViz->>Env: _pose_camera_prim()
    loop num_steps
        User->>Env: env.step(actions)
        Note over TiledCam: updated by env.step
        User->>SyncViz: capture()
        SyncViz->>TiledCam: _read_rgb()
        SyncViz->>VideoIO: to_uint8() per env + tile
        SyncViz->>Env: env.render()
        SyncViz->>VideoIO: to_uint8(global_frame)
    end
    User->>SyncViz: save(output_dir)
    SyncViz->>VideoIO: write_video + write_gif
    SyncViz-->>User: dict of written paths
    User->>SyncViz: close()
    SyncViz->>SyncViz: "_buffers = _CaptureBuffers()"

Loading

_{Reviews (5): Last reviewed commit: "fix docstring" | Re-trigger Greptile}

[xyao-nv]

Can we attach the camera sensor to the Scene, like how contact sensor is added to Asset?

Scene.add_env_cam(env_view, name=...) that attaches a TiledCamCfg to SceneCfg.
Or even Scene.add_sensor(sensor_cfg, name=...)?