Skip to content

Conversation

@Mayankm96
Copy link
Contributor

@Mayankm96 Mayankm96 commented Jan 3, 2026

Description

This MR removes the dependency on SimulationContext from Isaac Sim, helping to decouple internal logic from the physics and simulation contexts. It also adds several tests to improve coverage and validate simulation context behavior.

The class still depends on Isaac Sim’s SimulationManager for the underlying IsaacEvents callbacks.

Requires merging: #4313

Type of change

  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

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

@github-actions github-actions bot added enhancement New feature or request isaac-lab Related to Isaac Lab team labels Jan 3, 2026
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 3, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 3, 2026

Greptile Summary

This PR successfully removes the inheritance dependency on Isaac Sim's SimulationContext base class, reimplementing the functionality directly. The changes include:

Major Changes:

  • SimulationContext no longer inherits from isaacsim.core.api.simulation_context.SimulationContext, implementing all simulation control directly
  • New XformPrimView class provides optimized batch operations for USD transform manipulation, replacing Isaac Sim's XFormPrim
  • Singleton pattern, timeline operations (play, stop, pause, reset), physics configuration, and fabric interface management all reimplemented
  • Removed builtins.ISAAC_LAUNCHED_FROM_TERMINAL checks as callbacks now always register

Test Coverage:

  • 40+ test cases for XformPrimView covering all operations, index types, and edge cases
  • Comprehensive SimulationContext tests including singleton behavior, callbacks, exception handling, and physics configuration
  • Comparison tests validate compatibility with Isaac Sim's original implementation

Breaking Changes:

  • Method clear_all_callbacks() removed from SimulationContext (handled by clear_instance())
  • extras property in InteractiveScene now returns dict[str, XformPrimView] instead of dict[str, XFormPrim]
  • Stage cache management now handled explicitly with UsdUtils.StageCache

Issues Found:

  • Critical: _physics_device attribute accessed before initialization in device property

Confidence Score: 3/5

  • This PR contains one critical runtime error that must be fixed before merging
  • The PR represents a significant architectural improvement with excellent test coverage. However, there is a critical bug where the device property attempts to access _physics_device before it's initialized, which will cause runtime failures. Once fixed, the implementation is solid with comprehensive tests validating behavior.
  • source/isaaclab/isaaclab/sim/simulation_context.py requires immediate attention to fix the _physics_device initialization issue

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/simulation_context.py Major refactor removes inheritance from Isaac Sim's SimulationContext base class. One critical issue with _physics_device access before initialization. Singleton pattern and timeline operations reimplemented correctly.
source/isaaclab/isaaclab/sim/views/xform_prim_view.py New file implementing optimized transform view for USD prims. Well-documented with comprehensive docstrings. Implementation uses efficient batch operations with Sdf.ChangeBlock and UsdGeom.XformCache.
source/isaaclab/test/sim/test_simulation_context.py Comprehensive test coverage for SimulationContext including singleton pattern, timeline operations, physics configuration, callbacks, and exception handling. Tests validate the new implementation thoroughly.

Sequence Diagram

sequenceDiagram
    participant User
    participant SimContext as SimulationContext
    participant Timeline as Timeline Interface
    participant PhysX as PhysX Interface
    participant Stage as USD Stage
    
    User->>SimContext: __init__(cfg)
    SimContext->>SimContext: Create/Get Singleton Instance
    SimContext->>Stage: Create or Get Stage
    SimContext->>Stage: Add to StageCache
    SimContext->>Stage: Setup Stage Properties (Units, Axis)
    SimContext->>Stage: Create PhysicsScene Prim
    SimContext->>SimContext: _configure_simulation_dt()
    SimContext->>SimContext: _apply_physics_settings()
    SimContext->>SimContext: Initialize _physics_device
    SimContext->>Timeline: Register Stop Callback
    
    User->>SimContext: reset()
    SimContext->>Timeline: stop() if not stopped
    SimContext->>Timeline: play()
    SimContext->>PhysX: initialize_physics()
    SimContext->>SimContext: initialize_kinematic_bodies()
    
    User->>SimContext: step(render=True)
    SimContext->>PhysX: simulate(dt)
    SimContext->>PhysX: fetch_results()
    
    User->>SimContext: clear_instance()
    SimContext->>Timeline: Unsubscribe Callbacks
    SimContext->>PhysX: detach_stage()
    SimContext->>Stage: Remove from StageCache
    SimContext->>SimContext: Clear Singleton Instance
Loading

@Mayankm96 Mayankm96 closed this Jan 3, 2026
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 136-138 (link)

    logic: if already initialized, __init__ returns early without setting self.cfg or other instance attributes. this means accessing these attributes on the returned instance could fail if they weren't set during the first initialization

44 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@Mayankm96 Mayankm96 reopened this Jan 3, 2026
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 376 (link)

    logic: self._physics_device is accessed before being initialized. The _apply_physics_settings() method, which sets self._physics_device (lines 875-897), is called at line 294, but the device property accesses self._physics_device without ensuring it's set. If the property is accessed before _apply_physics_settings() runs, this will cause an AttributeError.

46 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@pascal-roth pascal-roth left a comment

Choose a reason for hiding this comment

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

Look at it high level, really nice. Thanks for addressing this

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

Labels

documentation Improvements or additions to documentation enhancement New feature or request isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants