-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Removes inheritance in SimulationContext #4324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greptile SummaryThis PR successfully removes the inheritance dependency on Isaac Sim's Major Changes:
Test Coverage:
Breaking Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
source/isaaclab/isaaclab/sim/simulation_context.py, line 136-138 (link)logic: if already initialized,
__init__returns early without settingself.cfgor 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
source/isaaclab/isaaclab/sim/simulation_context.py, line 376 (link)logic:
self._physics_deviceis accessed before being initialized. The_apply_physics_settings()method, which setsself._physics_device(lines 875-897), is called at line 294, but thedeviceproperty accessesself._physics_devicewithout ensuring it's set. If the property is accessed before_apply_physics_settings()runs, this will cause anAttributeError.
46 files reviewed, 1 comment
pascal-roth
left a comment
There was a problem hiding this 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
Description
This MR removes the dependency on
SimulationContextfrom 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
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there