[wip] Add initial design for Visualizers #3979
Conversation
matthewtrepte
requested review from
ClemensSchwarke,
Mayankm96,
Toni-SM,
hhansen-bdai,
jtigue-bdai,
kellyguo11,
masoudmoghani,
ooctipus,
ossamaAhmed,
pascal-roth,
peterd-NV and
xyao-nv
as code owners
github-actions
bot
added
documentation
Greptile OverviewGreptile SummaryThis PR introduces a multi-visualizer architecture for Isaac Lab 3.0, enabling support for Newton OpenGL, Omniverse, and Rerun visualizers through a factory pattern with scene data providers. The Newton visualizer implementation is functional, but the Rerun and Omniverse visualizers contain critical bugs. Key Changes
Critical Issues Found
Architecture Strengths
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Script as RL Script
participant SimCtx as SimulationContext
participant Provider as SceneDataProvider
participant VizCfg as VisualizerCfg
participant Viz as Visualizer
participant Newton as NewtonManager
Script->>SimCtx: __init__(cfg)
SimCtx->>Provider: create NewtonSceneDataProvider()
Script->>SimCtx: reset()
SimCtx->>Newton: start_simulation()
SimCtx->>Newton: initialize_solver()
SimCtx->>SimCtx: initialize_visualizers()
SimCtx->>VizCfg: for each cfg in visualizers
VizCfg->>Viz: create_visualizer()
Note over VizCfg,Viz: Factory pattern via registry
SimCtx->>Provider: get_scene_data()
Provider-->>SimCtx: {model, state, usd_stage, metadata}
SimCtx->>Viz: initialize(scene_data)
Viz->>Viz: validate physics backend
Viz->>Viz: create viewer (Newton/Rerun/OV)
loop Training Loop
Script->>SimCtx: step(render=True)
SimCtx->>Newton: step()
SimCtx->>SimCtx: step_visualizers(dt)
SimCtx->>Viz: is_running()?
alt Visualizer closed
SimCtx->>Viz: close()
SimCtx->>SimCtx: remove from list
else Visualizer paused (training)
loop While paused
SimCtx->>Viz: step(0.0, provider)
SimCtx->>Viz: is_training_paused()?
end
else Visualizer paused (rendering)
Note over SimCtx,Viz: Skip step, continue
else Normal step
SimCtx->>Provider: get_state()
Provider->>Newton: get current state
Provider-->>SimCtx: state
SimCtx->>Viz: step(dt, provider)
Viz->>Provider: get_state()
Viz->>Viz: render frame
end
end
Script->>SimCtx: close()
SimCtx->>SimCtx: close_visualizers()
SimCtx->>Viz: close()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Frame lifecycle is now properly handled by try-finally | ||
| pass # Silently ignore to avoid log spam - the viewer will recover |
There was a problem hiding this comment.
style: Silently suppressing exceptions can hide bugs
The pass statement after the try/finally block will hide any rendering errors. While the comment explains this is to avoid log spam, completely ignoring exceptions can make debugging difficult and hide serious issues.
| # Frame lifecycle is now properly handled by try-finally | |
| pass # Silently ignore to avoid log spam - the viewer will recover | |
| except Exception as e: | |
| # Log at debug level to avoid spam but still track issues | |
| import omni.log | |
| omni.log.debug(f"Rendering error in Newton visualizer: {e}") |
| while visualizer.is_training_paused() and visualizer.is_running(): | ||
| if isinstance(visualizer, NewtonVisualizer): | ||
| # Update state before rendering during pause | ||
| visualizer.update_state(NewtonManager._state_0) | ||
| visualizer.step(0.0) # Step with 0 dt during pause |
There was a problem hiding this comment.
logic: Potential infinite loop if visualizer hangs
The while loop blocks indefinitely if the visualizer is paused. If is_running() never returns False and the user never unpauses, this will hang the entire simulation thread. The inner step(0.0) call could also raise exceptions that aren't caught here.
Consider adding a timeout or making this non-blocking to prevent simulation hangs.
| if visualizer.is_rendering_paused(): | ||
| continue |
There was a problem hiding this comment.
logic: Rendering continues when paused
When rendering is paused via is_rendering_paused(), this continue skips the visualization step but the simulation still runs. However, the visualizer's ImGui state still needs to be updated every frame (as noted in the comment on line 347 of newton_visualizer.py). This could cause the UI to become unresponsive.
The Newton visualizer's step() should probably still be called with a flag to update UI but skip rendering.
| else: | ||
| # Skip unsupported visualizer types for now | ||
| omni.log.warn( | ||
| f"Visualizer type '{type(viz_cfg).__name__}' is not yet implemented. Skipping." | ||
| ) | ||
| continue | ||
|
|
There was a problem hiding this comment.
style: Accessing private class variable directly
The code directly accesses NewtonManager._model and NewtonManager._state_0 which are private class variables (indicated by the leading underscore). This creates tight coupling and could break if the NewtonManager implementation changes.
Consider adding public getter methods in NewtonManager like get_model() and get_state() for better encapsulation.
|
|
||
| # Initialize visualizers after simulation is set up (only on first reset) | ||
| if not soft and not self._visualizers: | ||
| self.initialize_visualizers() |
There was a problem hiding this comment.
style: Check visualizer recreation behavior after cleanup
The condition if not soft and not self._visualizers means visualizers are only initialized if the list is empty. If visualizers were previously created and then closed (making the list empty again), they won't be recreated on the next reset.
Verify this matches the intended behavior for simulation resets.
|
|
||
| from __future__ import annotations | ||
|
|
||
| from .ov_visualizer_cfg import RerunVisualizerCfg |
There was a problem hiding this comment.
syntax: incorrect import - importing RerunVisualizerCfg from ov_visualizer_cfg instead of rerun_visualizer_cfg
| from .ov_visualizer_cfg import RerunVisualizerCfg | |
| from .rerun_visualizer_cfg import RerunVisualizerCfg |
|
|
||
|
|
||
| class RerunVisualizer(Visualizer): | ||
| """Omniverse Visualizer implementation.""" |
There was a problem hiding this comment.
syntax: docstring copy-pasted from OmniverseVisualizer instead of describing Rerun visualizer
| """Omniverse Visualizer implementation.""" | |
| """Rerun Visualizer implementation.""" |
| # | ||
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| """Omniverse Visualizer implementation.""" |
There was a problem hiding this comment.
syntax: module docstring copy-pasted from Omniverse instead of describing Rerun
| """Omniverse Visualizer implementation.""" | |
| """Rerun Visualizer implementation.""" |
| class OmniverseVisualizer(Visualizer): | ||
| """Omniverse Visualizer implementation.""" | ||
| def __init__(self, cfg: OVVisualizerCfg): | ||
| super().__init__(cfg) | ||
| # stub for now No newline at end of file |
There was a problem hiding this comment.
logic: stub visualizer missing abstract method implementations - base class requires initialize(), step(), close(), and is_running() to be implemented
| class RerunVisualizer(Visualizer): | ||
| """Omniverse Visualizer implementation.""" | ||
| def __init__(self, cfg: RerunVisualizerCfg): | ||
| super().__init__(cfg) | ||
| # stub for now No newline at end of file |
There was a problem hiding this comment.
logic: stub visualizer missing abstract method implementations - base class requires initialize(), step(), close(), and is_running() to be implemented
matthewtrepte
changed the title
| while visualizer.is_training_paused() and visualizer.is_running(): | ||
| # Step with 0 dt during pause, pass scene provider for state updates | ||
| visualizer.step(0.0, self._scene_provider) |
There was a problem hiding this comment.
logic: This while loop will block the simulation thread indefinitely if visualizer is paused and never unpauses. If is_running() becomes false during pause, the visualizer is removed but simulation remains blocked. The inner step(0.0) call could also raise exceptions that aren't handled.
Add a timeout or make this non-blocking to prevent simulation hangs.
| def update_state(self, state) -> None: | ||
| """Update the simulation state for visualization. | ||
| This method should be called before step() to provide the latest simulation state. | ||
| Args: | ||
| state: The Newton State object to visualize. | ||
| """ | ||
| self._state = state |
There was a problem hiding this comment.
style: Unused method - update_state() is never called. The visualizer pulls state from scene_provider in step() (line 365). Consider removing if not needed.
| self.cfg = cfg | ||
| self._is_initialized = False | ||
| self._is_closed = False |
There was a problem hiding this comment.
style: _is_initialized and _is_closed flags must be manually set by subclasses - error-prone. Consider setting them in base class methods or document this requirement.
| def enable_visualizers(env_cfg, train_mode: bool = True) -> None: | ||
| """Enable visualizers for an environment configuration. | ||
| If no visualizers are configured, defaults to Newton OpenGL visualizer. | ||
| If visualizers are already configured, enables them. | ||
| This is a utility function for use in scripts that want to enable visualization | ||
| based on command-line arguments. | ||
| Args: | ||
| env_cfg: Environment configuration (DirectRLEnvCfg or ManagerBasedRLEnvCfg) to modify. | ||
| train_mode: Whether to run visualizers in training mode (True) or play/inference mode (False). | ||
| Default is True. | ||
| Example: | ||
| >>> import isaaclab.sim as sim_utils | ||
| >>> if args_cli.visualize: | ||
| ... sim_utils.enable_visualizers(env_cfg) # For training | ||
| ... sim_utils.enable_visualizers(env_cfg, train_mode=False) # For play/inference | ||
| """ | ||
| if env_cfg.sim.visualizers : | ||
| # Enable configured visualizer(s) | ||
| if isinstance(env_cfg.sim.visualizers, list): | ||
| for viz_cfg in env_cfg.sim.visualizers: | ||
| viz_cfg.enabled = True | ||
| viz_cfg.train_mode = train_mode | ||
| else: | ||
| env_cfg.sim.visualizers.enabled = True | ||
| env_cfg.sim.visualizers.train_mode = train_mode |
There was a problem hiding this comment.
logic: docstring says function "defaults to Newton OpenGL visualizer" when no visualizers configured, but implementation only enables existing visualizers. if env_cfg.sim.visualizers is None, function does nothing instead of creating default Newton visualizer
| def enable_visualizers(env_cfg, train_mode: bool = True) -> None: | |
| """Enable visualizers for an environment configuration. | |
| If no visualizers are configured, defaults to Newton OpenGL visualizer. | |
| If visualizers are already configured, enables them. | |
| This is a utility function for use in scripts that want to enable visualization | |
| based on command-line arguments. | |
| Args: | |
| env_cfg: Environment configuration (DirectRLEnvCfg or ManagerBasedRLEnvCfg) to modify. | |
| train_mode: Whether to run visualizers in training mode (True) or play/inference mode (False). | |
| Default is True. | |
| Example: | |
| >>> import isaaclab.sim as sim_utils | |
| >>> if args_cli.visualize: | |
| ... sim_utils.enable_visualizers(env_cfg) # For training | |
| ... sim_utils.enable_visualizers(env_cfg, train_mode=False) # For play/inference | |
| """ | |
| if env_cfg.sim.visualizers : | |
| # Enable configured visualizer(s) | |
| if isinstance(env_cfg.sim.visualizers, list): | |
| for viz_cfg in env_cfg.sim.visualizers: | |
| viz_cfg.enabled = True | |
| viz_cfg.train_mode = train_mode | |
| else: | |
| env_cfg.sim.visualizers.enabled = True | |
| env_cfg.sim.visualizers.train_mode = train_mode | |
| if env_cfg.sim.visualizers: | |
| # Enable configured visualizer(s) | |
| if isinstance(env_cfg.sim.visualizers, list): | |
| for viz_cfg in env_cfg.sim.visualizers: | |
| viz_cfg.enabled = True | |
| viz_cfg.train_mode = train_mode | |
| else: | |
| env_cfg.sim.visualizers.enabled = True | |
| env_cfg.sim.visualizers.train_mode = train_mode | |
| else: | |
| # Default to Newton OpenGL visualizer if none configured | |
| from .visualizers import NewtonVisualizerCfg | |
| env_cfg.sim.visualizers = NewtonVisualizerCfg(enabled=True, train_mode=train_mode) |
| Args: | ||
| markers: VisualizationMarkers instance to log each frame. | ||
| """ | ||
| if self._visualize_markers: |
There was a problem hiding this comment.
logic: _visualize_markers attribute undefined - should be _enable_markers
| if self._visualize_markers: | |
| if self._enable_markers: |
| Args: | ||
| plots: Dictionary mapping plot names to LivePlot instances. | ||
| """ | ||
| if self._visualize_plots: |
There was a problem hiding this comment.
logic: _visualize_plots attribute undefined - should be _enable_live_plots
| if self._visualize_plots: | |
| if self._enable_live_plots: |
| - Optimize batch logging for large marker counts | ||
| - Add color/material support for better visual distinction | ||
| """ | ||
| if not self._visualize_markers or len(self._registered_markers) == 0: |
There was a problem hiding this comment.
logic: _visualize_markers attribute undefined - should be _enable_markers
| if not self._visualize_markers or len(self._registered_markers) == 0: | |
| if not self._enable_markers or len(self._registered_markers) == 0: |
| - Maintain proper timeline synchronization | ||
| - Support different plot types (line, bar, etc.) | ||
| """ | ||
| if not self._visualize_plots or len(self._registered_plots) == 0: |
There was a problem hiding this comment.
logic: _visualize_plots attribute undefined - should be _enable_live_plots
| if not self._visualize_plots or len(self._registered_plots) == 0: | |
| if not self._enable_live_plots or len(self._registered_plots) == 0: |
| keep_scalar_history: bool = True | ||
| """Keep scalar/plot history in timeline.""" | ||
|
|
||
| record_to_rrd: str | None = test.rrd |
There was a problem hiding this comment.
syntax: syntax error - test.rrd is not quoted and will cause NameError
| record_to_rrd: str | None = test.rrd | |
| record_to_rrd: str | None = None |
| self._viewer.log_state(self._state) | ||
|
|
||
| # Actively log markers (if enabled) | ||
| if self.cfg.visualize_markers: |
There was a problem hiding this comment.
logic: cfg.visualize_markers attribute doesn't exist in RerunVisualizerCfg - should use cfg.enable_markers
| if self.cfg.visualize_markers: | |
| if self.cfg.enable_markers: |
| self._viewer.log_markers() | ||
|
|
||
| # Actively log plot data (if enabled) | ||
| if self.cfg.visualize_plots: |
There was a problem hiding this comment.
logic: cfg.visualize_plots attribute doesn't exist in RerunVisualizerCfg - should use cfg.enable_live_plots
| if self.cfg.visualize_plots: | |
| if self.cfg.enable_live_plots: |
| physics_backend = self._metadata.get("physics_backend", "unknown") | ||
| metadata_text += f"**Physics Backend:** {physics_backend}\n" | ||
|
|
||
| # Environment info | ||
| num_envs = self._metadata.get("num_envs", 0) | ||
| metadata_text += f"**Total Environments:** {num_envs}\n" | ||
|
|
||
| if self._env_indices is not None: | ||
| metadata_text += f"**Visualized Environments:** {len(self._env_indices)} (indices: {self._env_indices[:5]}...)\n" | ||
| else: | ||
| metadata_text += f"**Visualized Environments:** All ({num_envs})\n" | ||
|
|
||
| # Physics backend info | ||
| physics_backend = self._metadata.get("physics_backend", "unknown") | ||
| metadata_text += f"**Physics:** {physics_backend}\n" |
There was a problem hiding this comment.
style: duplicate physics_backend logging - already logged on line 87 as "Physics Backend", then again on line 100 as "Physics"
| physics_backend = self._metadata.get("physics_backend", "unknown") | |
| metadata_text += f"**Physics Backend:** {physics_backend}\n" | |
| # Environment info | |
| num_envs = self._metadata.get("num_envs", 0) | |
| metadata_text += f"**Total Environments:** {num_envs}\n" | |
| if self._env_indices is not None: | |
| metadata_text += f"**Visualized Environments:** {len(self._env_indices)} (indices: {self._env_indices[:5]}...)\n" | |
| else: | |
| metadata_text += f"**Visualized Environments:** All ({num_envs})\n" | |
| # Physics backend info | |
| physics_backend = self._metadata.get("physics_backend", "unknown") | |
| metadata_text += f"**Physics:** {physics_backend}\n" | |
| # Physics info | |
| physics_backend = self._metadata.get("physics_backend", "unknown") | |
| metadata_text += f"**Physics Backend:** {physics_backend}\n" | |
| # Environment info | |
| num_envs = self._metadata.get("num_envs", 0) | |
| metadata_text += f"**Total Environments:** {num_envs}\n" | |
| if self._env_indices is not None: | |
| metadata_text += f"**Visualized Environments:** {len(self._env_indices)} (indices: {self._env_indices[:5]}...)\n" | |
| else: | |
| metadata_text += f"**Visualized Environments:** All ({num_envs})\n" |
| if env_cfg.sim.visualizers : | ||
| # Enable configured visualizer(s) | ||
| if isinstance(env_cfg.sim.visualizers, list): | ||
| for viz_cfg in env_cfg.sim.visualizers: | ||
| viz_cfg.enabled = True | ||
| viz_cfg.train_mode = train_mode | ||
| else: | ||
| env_cfg.sim.visualizers.enabled = True | ||
| env_cfg.sim.visualizers.train_mode = train_mode |
There was a problem hiding this comment.
logic: function doesn't create default visualizer when env_cfg.sim.visualizers is None, contradicting docstring on line 1036 which says "defaults to Newton OpenGL visualizer"
the implementation only enables existing visualizers but never creates a default one
| if env_cfg.sim.visualizers : | |
| # Enable configured visualizer(s) | |
| if isinstance(env_cfg.sim.visualizers, list): | |
| for viz_cfg in env_cfg.sim.visualizers: | |
| viz_cfg.enabled = True | |
| viz_cfg.train_mode = train_mode | |
| else: | |
| env_cfg.sim.visualizers.enabled = True | |
| env_cfg.sim.visualizers.train_mode = train_mode | |
| if env_cfg.sim.visualizers: | |
| # Enable configured visualizer(s) | |
| if isinstance(env_cfg.sim.visualizers, list): | |
| for viz_cfg in env_cfg.sim.visualizers: | |
| viz_cfg.enabled = True | |
| viz_cfg.train_mode = train_mode | |
| else: | |
| env_cfg.sim.visualizers.enabled = True | |
| env_cfg.sim.visualizers.train_mode = train_mode | |
| else: | |
| # Default to Newton OpenGL visualizer if none configured | |
| from .visualizers import NewtonVisualizerCfg | |
| env_cfg.sim.visualizers = NewtonVisualizerCfg(enabled=True, train_mode=train_mode) |
| keep_scalar_history: bool = True | ||
| """Keep scalar/plot history in timeline.""" | ||
|
|
||
| record_to_rrd: str | None = "/tmp/test.rrd" |
There was a problem hiding this comment.
syntax: unquoted string causes NameError at runtime - /tmp/test.rrd is not a valid Python identifier
| record_to_rrd: str | None = "/tmp/test.rrd" | |
| record_to_rrd: str | None = "/tmp/test.rrd" |
| if self.cfg.enable_markers: | ||
| self._viewer.log_markers() |
There was a problem hiding this comment.
logic: cfg.enable_markers doesn't exist in RerunVisualizerCfg - should use self.cfg.enable_markers which is correctly defined
| if self.cfg.enable_markers: | |
| self._viewer.log_markers() | |
| # Actively log markers (if enabled) | |
| if self.cfg.enable_markers: | |
| self._viewer.log_markers() |
| # Actively log plot data (if enabled) | ||
| if self.cfg.enable_live_plots: | ||
| self._viewer.log_plot_data() |
There was a problem hiding this comment.
logic: cfg.enable_live_plots doesn't exist - should use self.cfg.enable_live_plots
| # Actively log plot data (if enabled) | |
| if self.cfg.enable_live_plots: | |
| self._viewer.log_plot_data() | |
| # Actively log plot data (if enabled) | |
| if self.cfg.enable_live_plots: | |
| self._viewer.log_plot_data() |
| # Handle training pause - block until resumed | ||
| while visualizer.is_training_paused() and visualizer.is_running(): | ||
| # Step with 0 dt during pause, pass scene provider for state updates | ||
| visualizer.step(0.0, self._scene_provider) |
There was a problem hiding this comment.
logic: infinite loop blocks simulation if visualizer never unpauses - no timeout or escape mechanism
if is_running() returns False during the loop, the visualizer is removed but the loop continues blocking the simulation thread
| # Handle training pause - block until resumed | |
| while visualizer.is_training_paused() and visualizer.is_running(): | |
| # Step with 0 dt during pause, pass scene provider for state updates | |
| visualizer.step(0.0, self._scene_provider) | |
| # Handle training pause with timeout | |
| pause_start_time = time.time() | |
| max_pause_duration = 300 # 5 minutes timeout | |
| while visualizer.is_training_paused() and visualizer.is_running(): | |
| if time.time() - pause_start_time > max_pause_duration: | |
| omni.log.warn(f"Visualizer pause timeout after {max_pause_duration}s, resuming simulation") | |
| break | |
| # Step with 0 dt during pause, pass scene provider for state updates | |
| visualizer.step(0.0, self._scene_provider) |
1 participant
Description
Initial design to support multiple visualizers with Isaac Lab 3.0
The visualizers
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there