Skip to content

Conversation

@kellyguo11
Copy link
Contributor

Description

Attempts to make the SimulationApp optional in the AppLauncher, where we only launch Kit via SimulationApp if omniverse visualizer (or in the future, RTX rendering) is required.

Also replaces carb settings with an internal settings manager for storing global settings.

This PR is made on top of the visualizer changes in #4099

This will currently break things because we still need to refactor SimulationContext to avoid any Kit libraries

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 27, 2025

Greptile Overview

Greptile Summary

Decouples SimulationApp from AppLauncher to enable running Isaac Lab with lightweight visualizers (Newton, Rerun) without the full Omniverse stack. Introduces a new SettingsManager to replace carb.settings, a SceneDataProvider for unified scene data access, and a flexible visualizer system with --visualizer CLI flag.

Key Changes:

  • AppLauncher now conditionally launches SimulationApp only when Omniverse visualizer, cameras, or livestreaming is required
  • New SettingsManager provides unified interface for both standalone and Omniverse modes
  • SimulationContext.carb_settings.settings (SettingsManager)
  • SimulationCfg.render.render_cfg (breaking change)
  • SimulationCfg.enable_newton_rendering.visualizer_cfgs list (breaking change)
  • Removed --newton_visualizer flag; replaced with --visualizer newton|rerun|omniverse
  • New SceneDataProvider handles fabric transform syncing only when OV visualizer is active

Critical Issue:

  • simulation_context.py passes SettingsManager to set_carb_setting/get_carb_setting helper functions that expect actual carb.settings object. This will fail in standalone mode when these rendering methods are called.

Confidence Score: 2/5

  • This PR has critical bugs that will cause runtime failures in standalone mode
  • The architecture changes are well-designed, but there's a critical type mismatch bug where SettingsManager is passed to set_carb_setting/get_carb_setting functions that expect the actual carb.settings object. This will cause immediate failures when rendering settings are applied in standalone mode. Additionally, several methods like _apply_render_settings_from_cfg access Omniverse-specific APIs (carb.tokens, omni.replicator) without proper guards for standalone mode.
  • Pay close attention to simulation_context.py - it has critical bugs around settings manager usage that will break standalone mode

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/simulation_context.py 2/5 replaced carb_settings with settings (SettingsManager), but passes it to set_carb_setting/get_carb_setting functions that expect actual carb.settings object - will fail in standalone mode
source/isaaclab/isaaclab/app/app_launcher.py 4/5 introduces conditional SimulationApp launching based on visualizer type, with standalone mode for non-Omniverse visualizers; logic appears sound but complex
source/isaaclab/isaaclab/app/settings_manager.py 5/5 new settings manager provides unified interface for both standalone and Omniverse modes; implementation is clean and straightforward
source/isaaclab/isaaclab/sim/scene_data_provider.py 5/5 new provider unifies scene data access for visualizers; handles fabric transform syncing only when OV visualizer is active
source/isaaclab/isaaclab/sim/simulation_cfg.py 4/5 renames render to render_cfg and replaces enable_newton_rendering with flexible visualizer_cfgs list; breaking change but well-documented

Sequence Diagram

sequenceDiagram
    participant User
    participant AppLauncher
    participant SettingsManager
    participant SimulationApp
    participant SimulationContext
    participant SceneDataProvider
    participant Visualizers

    User->>AppLauncher: __init__(visualizer=['newton'])
    AppLauncher->>AppLauncher: _config_resolution()
    AppLauncher->>AppLauncher: _check_if_omniverse_required()
    
    alt Omniverse Required (OV visualizer, cameras, or livestream)
        AppLauncher->>SimulationApp: create SimulationApp
        SimulationApp-->>AppLauncher: app instance
        AppLauncher->>SettingsManager: initialize_carb_settings()
        Note over SettingsManager: Switches to Omniverse mode<br/>(uses carb.settings)
    else Standalone Mode (Rerun/Newton only)
        Note over AppLauncher: Skip SimulationApp creation
        Note over SettingsManager: Stays in standalone mode<br/>(uses Python dict)
    end
    
    AppLauncher->>SettingsManager: store settings
    
    User->>SimulationContext: __init__(cfg)
    SimulationContext->>SettingsManager: get_settings_manager()
    SimulationContext->>SimulationContext: _apply_physics_settings()
    SimulationContext->>SimulationContext: _apply_render_settings_from_cfg()
    
    User->>SimulationContext: initialize_visualizers()
    SimulationContext->>SettingsManager: get('/isaaclab/visualizer')
    SettingsManager-->>SimulationContext: requested visualizers
    
    alt No configs in SimulationCfg
        SimulationContext->>SimulationContext: _create_default_visualizer_configs()
    else Configs exist
        SimulationContext->>SimulationContext: filter by requested types
    end
    
    SimulationContext->>SceneDataProvider: __init__(visualizer_cfgs)
    SceneDataProvider-->>SimulationContext: provider instance
    
    loop For each visualizer config
        SimulationContext->>Visualizers: create_visualizer()
        Visualizers-->>SimulationContext: visualizer instance
        SimulationContext->>Visualizers: initialize(scene_data)
    end
    
    loop Simulation step
        User->>SimulationContext: step()
        SimulationContext->>SimulationContext: forward()
        SimulationContext->>SceneDataProvider: update()
        
        alt OV Visualizer Active
            SceneDataProvider->>SceneDataProvider: _sync_fabric_transforms()
            Note over SceneDataProvider: Sync Newton transforms to USD Fabric
        end
        
        SimulationContext->>SimulationContext: step_visualizers(dt)
        loop For each visualizer
            SimulationContext->>Visualizers: step(dt, state)
        end
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. source/isaaclab/isaaclab/sim/simulation_context.py, line 346 (link)

    logic: In standalone mode (no SimulationApp), carb.tokens won't be available. This will throw an import error when _apply_render_settings_from_cfg is called without Omniverse mode active.

33 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

# enable hydra scene-graph instancing
# note: this allows rendering of instanceable assets on the GUI
set_carb_setting(self.carb_settings, "/persistent/omnihydra/useSceneGraphInstancing", True)
set_carb_setting(self.settings, "/persistent/omnihydra/useSceneGraphInstancing", True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: set_carb_setting from isaacsim.core.utils.carb expects an actual carb.settings object, but self.settings is now a SettingsManager instance. In standalone mode (no SimulationApp), this will fail because the function signature is incompatible.

The SettingsManager has its own set_bool/set_int/etc methods, so you should either:

  1. Call self.settings.set_bool(path, value) directly, OR
  2. Only call these render settings methods when in Omniverse mode

Similar issue at lines 355, 368, 378, 380, 392-393.

if self.cfg.render.antialiasing_mode is not None:
if self.cfg.render_cfg.antialiasing_mode is not None:
try:
import omni.replicator.core as rep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: In standalone mode, omni.replicator.core won't be available. The try-except handles this, but the entire _apply_render_settings_from_cfg method should probably be guarded to only run in Omniverse mode.

# Consider wrapping entire method:
def _apply_render_settings_from_cfg(self):
    if not get_settings_manager().is_omniverse_mode:
        return
    # ... rest of method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants