Refactor viz and recording cfg classes: passive recorder, render callbacks, ViewerCfg removal#6598
Conversation
github-actions
Bot
added
documentation
Greptile SummaryThis PR performs a large architectural cleanup of the visualization and recording stack, replacing
Confidence Score: 3/5The core recording pipeline and render-callback refactor are sound, but the UI camera control panel has functional regressions that affect interactive use. The recording refactor is well-tested and architecturally clean. The base_env_window.py camera UI has two regressions: the origin-type dropdown no longer repositions the viewport camera immediately for world/env modes, and eye/lookat slider changes during asset-tracking mode are silently undone on the next render step because viz.cfg.eye/lookat are not written back. source/isaaclab/isaaclab/envs/ui/base_env_window.py — both the origin-type callback and the location callback need fixes before the UI camera controls behave correctly. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Env as DirectRLEnv
participant SimCtx as SimulationContext
participant PhysxMgr as PhysxManager
participant KitViz as KitVisualizer
participant VidRec as VideoRecorder
Note over Env,VidRec: Initialization
Env->>SimCtx: __init__() sets _render_callbacks
SimCtx->>PhysxMgr: initialize(sim)
PhysxMgr->>SimCtx: add_render_callback(physx_headless_video_pump)
Env->>KitViz: initialize()
KitViz->>KitViz: _setup_initial_camera_view()
Env->>VidRec: VideoRecorder(cfg, scene)
Note over Env,VidRec: Per-step render loop
Env->>SimCtx: render()
SimCtx->>KitViz: step(dt)
KitViz->>KitViz: _update_asset_tracking_camera()
loop sorted _render_callbacks
SimCtx->>PhysxMgr: pump_kit_app_for_headless_video_render_if_needed
end
Env->>VidRec: render_rgb_array()
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Env as DirectRLEnv
participant SimCtx as SimulationContext
participant PhysxMgr as PhysxManager
participant KitViz as KitVisualizer
participant VidRec as VideoRecorder
Note over Env,VidRec: Initialization
Env->>SimCtx: __init__() sets _render_callbacks
SimCtx->>PhysxMgr: initialize(sim)
PhysxMgr->>SimCtx: add_render_callback(physx_headless_video_pump)
Env->>KitViz: initialize()
KitViz->>KitViz: _setup_initial_camera_view()
Env->>VidRec: VideoRecorder(cfg, scene)
Note over Env,VidRec: Per-step render loop
Env->>SimCtx: render()
SimCtx->>KitViz: step(dt)
KitViz->>KitViz: _update_asset_tracking_camera()
loop sorted _render_callbacks
SimCtx->>PhysxMgr: pump_kit_app_for_headless_video_render_if_needed
end
Env->>VidRec: render_rgb_array()
Reviews (1): Last reviewed commit: "Fix Kit+Newton recording gap: skip Kit R..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _set_viewer_origin_type_fn(self, value: str): | ||
| """Sets the origin of the viewport's camera. This is based on the drop-down menu in the UI.""" | ||
| # Extract the viewport camera controller from environment | ||
| vcc = self.env.viewport_camera_controller | ||
| if vcc is None: | ||
| raise ValueError("Viewport camera controller is not initialized! Please check the rendering mode.") | ||
| viz = self._get_kit_visualizer() | ||
| if viz is None: | ||
| return | ||
|
|
||
| # Based on origin type, update the camera view | ||
| if value == "World": | ||
| vcc.update_view_to_world() | ||
| viz.cfg.origin_type = "world" | ||
| elif value == "Env": | ||
| vcc.update_view_to_env() | ||
| viz.cfg.origin_type = "env" | ||
| else: | ||
| # find which index the asset is | ||
| fancy_names = [name.replace("_", " ").title() for name in self._viewer_assets_options] | ||
| # store the desired env index | ||
| viewer_asset_name = self._viewer_assets_options[fancy_names.index(value)] | ||
| # update the camera view | ||
| vcc.update_view_to_asset_root(viewer_asset_name) | ||
| viz.cfg.origin_type = "asset_root" | ||
| viz.cfg.asset_name = viewer_asset_name |
There was a problem hiding this comment.
Changing to "World" or "Env" now only mutates viz.cfg.origin_type but never calls _setup_initial_camera_view() or _apply_viewer_origin_to_camera(), so the viewport camera stays frozen at its previous position. The old ViewportCameraController.update_view_to_world() / update_view_to_env() immediately computed the new viewer_origin and called update_view_location(). The same is true for the env-index spinner in _set_viewer_env_index_fn (line 418): setting viz.cfg.env_index has no effect until the next asset-tracking step because KitVisualizer.step() only updates the camera for "asset_root" / "asset_body" origins.
Each branch should additionally call the camera-setup helpers (or a new public method that re-runs the origin resolution and applies it) so that the dropdown and spinner produce the same immediate repositioning users expected before.
| def _set_viewer_location_fn(self, model: omni.ui.SimpleFloatModel): | ||
| """Sets the viewport camera location based on the UI.""" | ||
| # access the viewport camera controller (for brevity) | ||
| vcc = self.env.viewport_camera_controller | ||
| if vcc is None: | ||
| raise ValueError("Viewport camera controller is not initialized! Please check the rendering mode.") | ||
| # obtain the camera locations and set them in the viewpoint camera controller | ||
| viz = self._get_kit_visualizer() | ||
| if viz is None: | ||
| return | ||
| eye = [self.ui_window_elements["viewer_eye"][i].get_value_as_float() for i in range(3)] | ||
| lookat = [self.ui_window_elements["viewer_lookat"][i].get_value_as_float() for i in range(3)] | ||
| # update the camera view | ||
| vcc.update_view_location(eye, lookat) | ||
| viz.set_camera_view(eye, lookat) |
There was a problem hiding this comment.
viz.set_camera_view(eye, lookat) moves the viewport camera to the given absolute position, but viz.cfg.eye and viz.cfg.lookat (the relative offsets used by _apply_viewer_origin_to_camera) are never updated. On the very next call to KitVisualizer.step() in "asset_root" or "asset_body" mode, _update_asset_tracking_camera() recomputes the position from the old cfg.eye/cfg.lookat and overwrites whatever the user just set.
The old ViewportCameraController.update_view_location() avoided this by storing the new values in self.default_cam_eye / self.default_cam_lookat before applying them. The fix is to also write back: viz.cfg.eye = tuple(eye); viz.cfg.lookat = tuple(lookat) before calling set_camera_view.
| from . import mdp, ui | ||
| from .common import VecEnvObs, VecEnvStepReturn, ViewerCfg | ||
| from .common import VecEnvObs, VecEnvStepReturn |
There was a problem hiding this comment.
VideoRecorderCfg is now exported from __init__.py (the test file test_recording_backends.py imports it from isaaclab.envs), but it is absent from the .pyi type stub. Type-checkers and IDE completions will not see it.
| from . import mdp, ui | |
| from .common import VecEnvObs, VecEnvStepReturn, ViewerCfg | |
| from .common import VecEnvObs, VecEnvStepReturn | |
| from . import mdp, ui | |
| from .common import VecEnvObs, VecEnvStepReturn | |
| from .utils.video_recorder_cfg import VideoRecorderCfg |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
This looks like it'll make simulation control so much smoother! 🗺️✨😌 Maintainer? Turn off weaves from non-maintainers → |
There was a problem hiding this comment.
AI review: Basic camera positioning remains available through VisualizerCfg and SimulationContext.set_camera_view(), but this refactor currently removes or regresses several related capabilities. I left inline comments for the runtime UI/tracking regressions, independent recorder camera positioning, and the undeprecated public controller removal. Please also preserve or document a replacement for ViewerCfg.cam_prim_path, since the new renderer synchronization hardcodes /OmniverseKit_Persp.
| # update the camera view | ||
| vcc.update_view_to_asset_root(viewer_asset_name) | ||
| viz.cfg.origin_type = "asset_root" | ||
| viz.cfg.asset_name = viewer_asset_name |
There was a problem hiding this comment.
AI review: Changing the origin type now only mutates viz.cfg.origin_type; it does not recompute _viewer_origin or apply a new camera pose. KitVisualizer.step() only refreshes asset_root and asset_body, so switching to World or Env leaves the viewport at its previous pose. The env-index callback has the same issue in Env mode. Could KitVisualizer expose a public origin/tracking setter that validates the selection, recomputes the origin, and updates the camera immediately, with these UI callbacks calling that method?
| lookat = [self.ui_window_elements["viewer_lookat"][i].get_value_as_float() for i in range(3)] | ||
| # update the camera view | ||
| vcc.update_view_location(eye, lookat) | ||
| viz.set_camera_view(eye, lookat) |
There was a problem hiding this comment.
AI review: set_camera_view() applies these values once but does not persist them to viz.cfg.eye and viz.cfg.lookat. During asset tracking, the next KitVisualizer.step() calls _apply_viewer_origin_to_camera() with the old configured offsets and immediately undoes the UI edit. The previous controller stored new eye/look-at offsets before applying them. Please persist the relative offsets here, ideally through a public visualizer API so UI and programmatic changes share the same semantics.
| Set automatically by the environment base classes; do not set manually. | ||
| """ | ||
|
|
||
| eye: tuple[float, float, float] = (7.5, 7.5, 7.5) |
There was a problem hiding this comment.
AI review: Removing eye and lookat makes the recorder passive, but it also removes the ability to configure a recording camera independently from the interactive viewer. A named sensor is a possible workaround, although that requires modifying the scene and is not equivalent to a recorder-owned perspective camera. Is this feature loss intentional? If so, the migration guidance should show how to create and select an independently positioned camera source; otherwise these fields need an equivalent on a recording-source configuration.
| from isaaclab.envs import DirectRLEnv, ManagerBasedEnv, ViewerCfg | ||
|
|
||
|
|
||
| class ViewportCameraController: |
There was a problem hiding this comment.
AI review: ViewportCameraController is an exported public class with runtime methods for selecting an environment and switching among world, env, asset-root, and asset-body tracking. I could not find a prior deprecation for this class, and the replacement tracking operations in KitVisualizer are private. Under the repository policy that public API removals require deprecation in a prior release, please retain a deprecated compatibility shim and add a public replacement API before removing this class.
|
@matthewtrepte please look into: #6605. Would this PR help with this? |
… Newton/Rerun/Viser)
…er accesses Fix critical crash in ManagerBasedRLEnv.__init__ where cfg.viewer.eye/lookat was accessed after the viewer field was removed from the env cfg base classes. The recorder is now passive and no longer needs camera position forwarded. Migrate all envs that silently lost their ViewerCfg without replacement: g1 locomanipulation SDG, dexsuite reorient, g129 trocar assembly, Spot flat, cartpole direct, cartpole direct-camera, and cartpole-showcase camera envs all now set sim.default_visualizer_cfg = VisualizerCfg(eye=..., lookat=...) in __post_init__, following the pattern of the other migrated tasks. Fix visualizer_integration_utils.py stale env_cfg.viewer.eye/lookat access that would crash all cartpole visualizer integration tests; migrated to env_cfg.sim.default_visualizer_cfg = VisualizerCfg(...) instead.
Export VideoRecorderCfg from isaaclab.envs so the public namespace reflects the new recorder API; the class lived in utils/ and was not reachable via lazy_export, breaking the install CI smoke test. Add five unit tests for add_render_callback / remove_render_callback in test_simulation_context.py — the new public API introduced by the re-arch had zero test coverage. Update stale docstrings in the two headless-video-pump regression tests that still referenced the deleted recording_hooks module as the implementation mechanism; the same contract is now wired through add_render_callback by the physics-backend managers.
… paths Four integration tests covering every VideoRecorder capture path: kit-visualizer-source, newton-visualizer-source, physx-renderer-source, newton-renderer-source. Each creates a CartpoleEnv with render_mode="rgb_array", steps 10 physics ticks, calls env.render(), and asserts the frame is a non-trivial (_H, _W, 3) uint8 ndarray with at least 1% non-black pixels. Previously test_record_video.py only exercised the old gymnasium RecordVideo wrapper without touching VideoRecorderCfg, and test_video_recorder.py only used mocks. These tests exercise the real capture stack end-to-end.
The previous Newton visualizer test used NewtonVisualizerCfg (abstract base, visualizer_type=None) instead of NewtonGLVisualizerCfg (visualizer_type="newton"). This made it accidentally test the physics manager fallback rather than the Newton GL visualizer direct path. Rewrite to cover all three _select_video_backend dispatch paths: - Path 1 (type="kit"): Kit visualizer with PhysX and with Newton physics (cross-backend — Kit capture is renderer-agnostic) - Path 2 (type="newton"): NewtonGLVisualizerCfg direct framebuffer capture - Path 3 (fallthrough): NewtonRTXVisualizerCfg (type="newton_rtx") is not recognised so falls through to the physics manager; also standalone renderer-source tests for PhysX and Newton backends
Three issues found by running the tests:
- Kit Replicator render products are zero-initialized and need RTX warmup
frames before the buffer populates. Add a poll-until-non-black loop
(_WARMUP_RENDER_BUDGET=40 sim.render() calls) matching the same pattern
used by OVRTX tests in rendering_test_utils.py.
- Cartpole against a black background is sparse; lower _MIN_NONZERO_RATIO
from 0.01 to 0.005 so a valid but thin scene does not false-fail.
- Kit Replicator recording produces a zero buffer when Newton is the active
physics backend. The PhysX manager registers an ensure_isaac_rtx_render_update
render callback that primes the render product; Newton does not register an
equivalent, so the buffer stays zeroed even with Kit visualizer pumping
app.update(). Mark test_kit_visualizer_newton_source_records_rgb as
strict xfail to document this known gap.
- NewtonRTXVisualizerCfg is not a factory-registered visualizer type
("newton_rtx" raises ValueError). Remove the broken fallthrough test for
it; path-3 fallthrough logic is already unit-tested in test_video_recorder.py.
Final result: 4 passed, 1 xfailed in 35s.
Newton's Fabric transform writes (via wp.fabricarray Warp kernel) do not trigger RTX's scene delegate change notifications, so Replicator annotator buffers stay zeroed regardless of how many app.update() calls are issued. Fix _select_video_backend to detect Newton physics (video_capture_backend() == "newton_gl") and skip Kit Replicator in that case. If a Newton GL visualizer is configured, use its framebuffer directly. If only a Kit visualizer is configured alongside Newton, log a warning and fall back to standalone Newton GL capture, which renders Newton's scene correctly. Kit recording continues to work unchanged with PhysX. Add two unit tests for the new fallback dispatch and remove the xfail from test_kit_visualizer_newton_source_records_rgb — all 5 integration tests now pass (previously 4 passed, 1 xfailed).
…ure classes
Remove IsaacsimKitPerspectiveVideo/Cfg and NewtonGlPerspectiveVideo/Cfg.
Recording is now fully internal to env.step() — no gym RecordVideo wrapper
needed. VideoRecorderCfg is backend-agnostic and specifies only what to
record and where to write it, not how to capture.
VideoRecorderCfg fields:
source: str = "visualizer" source string (visualizer[:<type>[/tiled]],
sensor:<name>[/<data_type>])
output_dir: str clip output directory
fps: int output video frame rate
clip_length: int env steps per clip
clip_trigger_step: int start a new clip every N steps (0 = once)
env cfgs gain video_recorders: list[VideoRecorderCfg] = [] (empty = off)
replacing the single video_recorder: VideoRecorderCfg field. Multiple
entries produce independent simultaneous streams from different sources.
KitVisualizer gains render_rgb_array() via a lazily-created Replicator
annotator on its controlled camera prim. Newton GL visualizer already had
render_rgb_array(). Sensor sources read from env.scene.sensors[name].
VideoRecorder.step() is called in DirectRLEnv, ManagerBasedRLEnv, and
DirectMARLEnv step loops after physics/render and before observations.
test_video_recorder.py: complete rewrite as pure-Python unit tests (no AppLauncher, no sim). Tests cover _parse_source(), clip trigger logic, visualizer frame routing (auto, typed, tiled, missing), sensor frame routing (rgb default, specific data type, missing), and clip writing via a mocked ImageSequenceClip. test_recording_backends.py: complete rewrite for internal recording. Tests configure env_cfg.video_recorders with short clips (clip_length=5) and mock ImageSequenceClip to capture frames without requiring ffmpeg. Covers kit+PhysX, kit+Newton fallback, newton GL, auto source, multiple simultaneous streams, and sensor:tiled_camera source.
…r messages source='visualizer:kit' with Newton physics now warns and falls back to the Newton GL visualizer if one is also active, instead of silently returning black frames. Add error log (with active-type list) when the requested visualizer type is not configured at all, vs a silent None return. Add three unit tests: Kit→Newton fallback routes to Newton GL, Kit→Newton with no Newton fallback logs a warning and returns None, missing typed visualizer logs an error listing active types. Update the integration test to configure both Kit and Newton visualizers for the fallback test — the fallback requires Newton GL to be active.
701b657 to
2f2679a
Compare
…sualizer source='visualizer:X' now strictly uses visualizer type X. If X is not active, log an error listing active types. If source='visualizer:kit' is used with Newton physics, log a clear error explaining the limitation and pointing to source='visualizer:newton' — no silent fallback, no black video. Auto source='visualizer' is unaffected (still picks the first capable visualizer). Unit tests updated: remove the two fallback tests, add one test for the Kit+Newton error path. Integration test updated: Kit+Newton now asserts no clip is written and the error message mentions visualizer:newton.
Sensor error now lists available sensors (truncated to 8) and adds a hint about adding CameraCfg when the scene has none at all. Two new unit tests cover both cases. Remove dead _assert_clip_written helper and stale 'Kit fallback' note from the integration test module docstring. Rewrite docs/source/how-to/record_video.rst for the new internal recording API: source strings, common use cases (Kit/Newton/sensor/multi-stream), clip control table, requirements. Remove the stale recording section from visualization.rst (referenced old gym wrapper + ViewerCfg + backend_source API) and replace it with a two-sentence summary + link to the new doc.
…eo wrapper Add apply_video_recording(env_cfg, log_dir, args_cli, *, subdir) to common.py. It injects a VideoRecorderCfg into env_cfg.video_recorders before env creation so recording is driven by env.step() internally. CLI arg mapping: --video → enables recording (source="visualizer", auto-selects active visualizer) --video_length → VideoRecorderCfg.clip_length --video_interval → VideoRecorderCfg.clip_trigger_step (train scripts only; play uses 0) All 4 train scripts (rsl_rl, skrl, rl_games, sb3) call apply_video_recording after log_dir is set and before create_env. All 4 play scripts replace their inline gym.wrappers.RecordVideo block with apply_video_recording(..., subdir="play"). Remove render_mode="rgb_array" from all gym.make() calls — it was only needed to activate the old gym wrapper path. wrap_record_video() becomes a no-op stub that logs a deprecation warning.
clip_length → video_length (matches --video_length) clip_trigger_step → video_interval (matches --video_interval) The fields now read identically to the CLI args that populate them, making apply_video_recording() a direct 1:1 mapping.
3 participants
Summary
This PR replaces the
ViewerCfg/ViewportCameraController/recording_hooksstack with a cleaner, backend-agnostic architecture:ViewerCfgfrom all env cfg bases (DirectRLEnvCfg,ManagerBasedEnvCfg,DirectMARLEnvCfg) and migrates all task envs tosim.default_visualizer_cfg = VisualizerCfg(eye=..., lookat=...)orsim.visualizer_cfgs = [KitVisualizerCfg(...)]ViewportCameraController(243 lines) — camera tracking is now handled directly byKitVisualizervia itsorigin_type/asset_name/body_namecfg fieldsrecording_hooksmodule from bothisaaclabandisaaclab_newton— physics-backend recording callbacks are now registered via the newSimulationContext.add_render_callback(name, fn, order)public APIVideoRecorder:VideoRecorderCfg.eye/lookatremoved; the recorder captures whatever the active visualizer renders without repositioning the cameraPhysicsManager.video_capture_backend(): new classmethod replaces name-sniffing heuristic for backend selectionVideoRecorderCfgexported fromisaaclab.envspublic namespace_select_video_backendskips Kit Replicator (Newton Fabric writes don't notify RTX's scene delegate) and falls back to standalone Newton GL capture with a logged warningNet diff: 69 files changed, +750 / −745 (≈ net zero lines despite the significant cleanup, because new
KitVisualizercamera tracking code replaces the deletedViewportCameraController).Test plan
source/isaaclab/test/envs/test_video_recorder.py— 17/17 unit tests pass (covers all_select_video_backenddispatch paths including new Newton fallback)source/isaaclab/test/sim/test_simulation_context.py— 29 passed before pre-existing hang ontest_timeline_callbacks_with_weakref(unrelated)source/isaaclab/test/envs/test_env_rendering_logic.py— 17/17 passsource/isaaclab/test/sim/test_simulation_context_visualizers.py— 28/28 passsource/isaaclab_tasks/test/core/test_recording_backends.py— 5/5 integration tests pass (Kit/PhysX, Kit/Newton fallback, Newton GL, PhysX renderer, Newton renderer)🤖 Generated with Claude Code