Migrate viewport recording to Isaac Lab

[qianl-nv]

Summary

Fix viewport video recording

Detailed description

• Replace the removed rgb_array/Gymnasium recording path with Isaac Lab's native Kit visualizer recorder
• Enable camera support before SimulationApp startup when viewport recording is requested
• Cover step- and episode-limited native recorder configuration

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge, with no concrete correctness, security, or repository-rule violations identified.

Summary

• Enables camera support before SimulationApp startup when viewport recording is requested.
• Configures native viewport recording before environment construction using step- or episode-derived limits.
• Retains the existing wrapper only for embodiment-mounted camera observations.
• Adds focused tests for native recorder configuration and recording length.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Parse recording options] --> B[Enable cameras before SimulationApp startup]
    B --> C[Build policy and resolve rollout limit]
    C --> D[Build environment configuration]
    D --> E{Viewport recording requested?}
    E -- Yes --> F[Append native Kit VideoRecorderCfg]
    E -- No --> G[Leave native recorder configuration unchanged]
    F --> H[Create environment]
    G --> H
    H --> I{Camera-observation recording requested?}
    I -- Yes --> J[Apply camera-observation wrapper]
    I -- No --> K[Use environment directly]
    J --> L[Run policy rollout]
    K --> L

Loading

[arena-review-bot]

🔴 This breaks the sim-preview viewport recording

isaaclab_arena_examples/agentic_environment_generation/review_gui/simapp/sim_preview.py still calls builder.make_registered(..., render_mode=video_cfg.render_mode) (line 143) and then relies on wrap_env_for_video to record the viewport (line 146). With VideoRecordingCfg.render_mode deleted that first call is an AttributeError, and even past it wrap_env_for_video now returns the env untouched, so the expected one viewport video, found 0 check at line 155 would fire. It's a live path (simapp/server.py dispatches run_sim_preview). Could it be switched over to configure_env_for_video(env_cfg, video_cfg, num_steps, None) before make_registered too?

[arena-review-bot]

🟡 Rebuilds overwrite each other's viewport video

The prefix is fixed, and Isaac Lab's recorder restarts its clip index at 0 for each new env (_clip_path → <prefix>_0000.mp4). In build_and_run every rebuild writes into the same output_dir, so rebuild 1's viewport_0000.mp4 silently replaces rebuild 0's. camera_name_prefix already dodges this with a per-rebuild suffix — could the viewport prefix do the same, e.g. a viewport_name_prefix field on VideoRecordingCfg set alongside camera_name_prefix?

Suggested change

	output_filename_prefix="viewport",
	output_filename_prefix=video_cfg.viewport_name_prefix,

[arena-review-bot]

🔵 Leftovers from the old viewport path

Three bits are dead now: these two parameters (all three call sites are in-repo, so they can just be dropped), the VideoRecordingCfg.enabled property (no remaining users), and the inner if video_cfg.record_camera_video: at line 108, which is always true after the guard on line 101. Worth deleting all three so the function reads as what it is — the camera recorder wrapper.

[arena-review-bot]

🔵 Comment doesn't match the line below it

This line builds the arena builder, not the policy — the policy is built at line 220. The ordering the comment describes belongs up by that call.

Suggested change

	# Build the policy before the environment so its intrinsic length can configure native video recording.
	# Build the arena builder; the environment itself is created after the rollout length is known.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Swaps the gymnasium RecordVideo viewport path for Isaac Lab's native VideoRecorderCfg, moving viewport setup from "wrap the env after construction" to "mutate env_cfg before construction". The mechanics check out — video_length is in env steps, env.close() flushes partial clips so episode-limited rollouts still produce a file, and video_recorders is a configclass field with a deepcopy default factory so appending is per-instance. The problem is that one of the three viewport call sites was left behind.

Design, Boundaries & Scope

Viewport recording used to be a single call (wrap_env_for_video) that every caller already made. It is now a two-step contract — build_registered() → configure_env_for_video(env_cfg, …) → make_registered(env_cfg, env_kwargs) — that each caller must reassemble by hand. policy_runner.py and run_execution._build_environment_from_cfg now carry the same three lines, and sim_preview.py was missed entirely (see the 🔴 inline). Would it be worth pushing this into ArenaEnvBuilder instead — e.g. make_registered(..., video_cfg=..., num_steps=..., num_episodes=...) — so viewport recording works wherever an environment is built rather than only where someone remembered to wire it up?

Findings

🔴 Critical: isaaclab_arena/video/video_recording.py:101 — removing VideoRecordingCfg.render_mode and the viewport branch of wrap_env_for_video breaks isaaclab_arena_examples/.../simapp/sim_preview.py (AttributeError at line 143, then the expected one viewport video check at line 155). It's reachable from simapp/server.py.

🟡 Warning: isaaclab_arena/video/video_recording.py:77 — the hardcoded "viewport" prefix plus a per-env clip index reset means multi-rebuild runs silently overwrite earlier rebuilds' viewport videos. camera_name_prefix already solves this per rebuild.

🔵 Improvement: isaaclab_arena/video/video_recording.py:99 — num_steps/num_episodes, the VideoRecordingCfg.enabled property, and the redundant inner if video_cfg.record_camera_video: (line 108) are all dead now.

🔵 Improvement: isaaclab_arena/evaluation/policy_runner.py:209 — the comment describes the policy build but sits above the arena-builder call.

🔵 Improvement: isaaclab_arena/evaluation/policy_runner_cli.py:99 — --record_viewport_video still advertises "(uses gymnasium.wrappers.RecordVideo)", which this PR makes untrue. Not a diff line, but it's the user-facing description of the thing being migrated.

Test Coverage

test_video_recording.py is a clean phase-1 pure-Python test — no sim needed, and the import chain to VideoRecorderCfg is light enough (isaaclab.envs.__init__ is lazy_export + common + the cfg module) that it will import without a SimulationApp. Copyright year 2026 on the new file is right. Two gaps: nothing covers the camera-only path (configure_env_for_video must be a no-op when only record_camera_video is set — cheap to add to the same file), and nothing would have caught the sim_preview.py breakage.

Verdict

Minor fixes needed