Skip to content

Conversation

@kellyguo11
Copy link
Contributor

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

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

@kellyguo11 kellyguo11 marked this pull request as draft December 4, 2025 01:18
@github-actions github-actions bot added isaac-sim Related to Isaac Sim team isaac-lab Related to Isaac Lab team labels Dec 4, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 4, 2025

Greptile Overview

Greptile Summary

Updates Isaac Lab rendering system from RT1 (Ray Tracing 1.0) to RT2 (Ray Tracing 2.0) for Isaac Sim 6.0 compatibility. The PR migrates rendering configuration files, adds new RT2-specific settings to the configuration schema, and updates version detection logic.

Key Changes:

  • Replaces RT1 settings (rtx.translucency, rtx.reflections, rtx.indirectDiffuse) with RT2 settings (rtx.rtpt.maxBounces, rtx.rtpt.splitGlass, etc.) in rendering mode files
  • Adds 10 new RT2 configuration fields to RenderCfg class with proper documentation
  • Renames version check from is_isaac_sim_version_4_5() to is_isaac_sim_version_5() and updates logic to check for version < 6
  • Creates new apps/isaacsim_5/ directory structure for Isaac Sim 5.x compatibility
  • Comments out deprecated rtx-transient.dldenoiser settings (RT2 only supports DLSS-RR)

Issues Found:

  • Critical logic error in test file: version check condition is inconsistent with the main implementation, which will cause test failures for Isaac Sim 5.x versions

Confidence Score: 2/5

  • This PR has a critical logic bug in the test file that will cause failures for Isaac Sim 5.x
  • The implementation is mostly correct and well-structured, with proper RT2 migration and comprehensive documentation. However, the logic error in test_simulation_render_config.py:111 is critical - it checks isaac_sim_version < 5 when it should check < 6, inconsistent with the correct implementation in simulation_context.py:749. This will cause the test to fail for Isaac Sim 5.x versions as it will try to load files from the wrong directory.
  • Pay close attention to source/isaaclab/test/sim/test_simulation_render_config.py - the version check logic must be fixed before merging

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/test/sim/test_simulation_render_config.py 1/5 Logic error: checks isaac_sim_version < 5 but uses isaacsim_5 folder, should check < 6
source/isaaclab/isaaclab/sim/simulation_context.py 5/5 Correctly updates version check from < 5 to < 6 and adds new RT2 rendering settings mappings
source/isaaclab/isaaclab/sim/simulation_cfg.py 5/5 Adds comprehensive RT2 rendering configuration fields with proper documentation
source/isaaclab/isaaclab/app/app_launcher.py 5/5 Renames version check method from is_isaac_sim_version_4_5() to is_isaac_sim_version_5() and updates logic
apps/rendering_modes/balanced.kit 5/5 Replaces RT1 settings with RT2 settings, comments out deprecated RT1 configurations
apps/rendering_modes/performance.kit 5/5 Updates rendering settings from RT1 to RT2, sets appropriate maxBounces for performance mode
apps/rendering_modes/quality.kit 5/5 Migrates to RT2 settings with maxBounces=3 for global illumination support

Sequence Diagram

sequenceDiagram
    participant User
    participant AppLauncher
    participant SimulationContext
    participant RenderCfg
    participant CarbSettings
    participant KitFiles

    User->>AppLauncher: Initialize with rendering args
    AppLauncher->>AppLauncher: is_isaac_sim_version_5()
    alt Isaac Sim version < 6
        AppLauncher->>KitFiles: Load from apps/isaacsim_5/
    else Isaac Sim version >= 6
        AppLauncher->>KitFiles: Load from apps/
    end
    
    User->>RenderCfg: Configure rendering (mode, RT2 settings)
    Note over RenderCfg: New RT2 fields: max_bounces,<br/>split_glass, split_clearcoat, etc.
    
    User->>SimulationContext: Initialize with RenderCfg
    SimulationContext->>SimulationContext: Check Isaac Sim version
    alt version < 6
        SimulationContext->>KitFiles: Load rendering_modes from isaacsim_5/
    else version >= 6
        SimulationContext->>KitFiles: Load rendering_modes from apps/
    end
    
    SimulationContext->>KitFiles: Parse rendering mode kit file
    KitFiles-->>SimulationContext: Return RT2 settings (rtx.rtpt.*)
    
    SimulationContext->>SimulationContext: Map RenderCfg fields to carb settings
    Note over SimulationContext: Maps max_bounces -> /rtx/rtpt/maxBounces<br/>split_glass -> /rtx/rtpt/splitGlass, etc.
    
    SimulationContext->>CarbSettings: Apply rendering settings
    CarbSettings-->>SimulationContext: Settings applied
    
    SimulationContext-->>User: Simulation ready with RT2 rendering
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.

19 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 111 to +112
if isaac_sim_version < 5:
isaaclab_app_exp_path = os.path.join(isaaclab_app_exp_path, "isaacsim_4_5")
isaaclab_app_exp_path = os.path.join(isaaclab_app_exp_path, "isaacsim_5")
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Logic error: condition checks isaac_sim_version < 5 but then uses isaacsim_5 folder. This is inconsistent with simulation_context.py:749 which correctly checks < 6. For Isaac Sim 5.x versions, this will incorrectly use the isaacsim_5 folder when it should use the base apps folder.

Suggested change
if isaac_sim_version < 5:
isaaclab_app_exp_path = os.path.join(isaaclab_app_exp_path, "isaacsim_4_5")
isaaclab_app_exp_path = os.path.join(isaaclab_app_exp_path, "isaacsim_5")
if isaac_sim_version < 6:
isaaclab_app_exp_path = os.path.join(isaaclab_app_exp_path, "isaacsim_5")

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

Labels

isaac-lab Related to Isaac Lab team isaac-sim Related to Isaac Sim team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant