Skip to content

Conversation

@kellyguo11
Copy link
Contributor

@kellyguo11 kellyguo11 commented Nov 7, 2025

Description

Introducing new PhysxCfg flag disable_sleeping to disable sleeping on a global level. This flag will now override per-object sleeping settings if set to True. If sleeping is desired for any object in the scene, ensure this flag is set to False. We are setting this to True by default to keep a consistent behavior between CPU and GPU simulation, since sleeping is not supported by PhysX on the GPU pipeline. This has also previously caused confusion for some users where the contact forces reported on CPU and GPU simulation were different due to sleeping.

Type of change

  • New feature (non-breaking change which adds functionality)
  • 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 Nov 7, 2025
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.

Greptile Overview

Greptile Summary

This PR adds two new PhysX configuration parameters to improve simulation control and consistency:

  • disable_sleeping (default: True): Disables sleeping globally to ensure consistent behavior between CPU and GPU simulation pipelines, as GPU doesn't support sleeping in PhysX
  • enable_external_forces_every_iteration (default: False): Applies external forces in lockstep with TGS solver iterations to reduce noisy joint velocity data at minor performance cost (~4%)

Key Changes:

  • Configuration flags added to PhysxCfg with comprehensive documentation
  • Implementation in simulation_context.py with GPU pipeline validation
  • Comprehensive test coverage for both flags including error cases
  • CHANGELOG and version bump (0.47.11 → 0.49.0) reflecting breaking change
  • Runtime validation prevents incompatible disable_sleeping=False + GPU pipeline configuration

Breaking Change:
The default disable_sleeping=True changes existing behavior. Users who need sleeping on CPU must explicitly set this to False.

Confidence Score: 4/5

  • This PR is safe to merge with minor style improvement recommended
  • The implementation is solid with comprehensive tests and documentation. Score of 4 (not 5) due to: (1) validation happens after attribute is set, though exception prevents scene creation, and (2) this is a breaking change that alters default behavior. The feature itself is well-designed and addresses real user needs (CPU/GPU consistency and joint velocity noise).
  • Pay attention to simulation_context.py - consider reordering validation before attribute setting

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/simulation_cfg.py 5/5 Added disable_sleeping (default True) and enable_external_forces_every_iteration (default False) flags with clear documentation
source/isaaclab/isaaclab/sim/simulation_context.py 4/5 Implements the two new PhysX settings with GPU pipeline validation; attribute is set before validation check
source/isaaclab/test/sim/test_simulation_context.py 5/5 Added comprehensive tests covering default behavior, CPU/GPU configurations, and error handling for both new PhysX flags

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationCfg
    participant SimulationContext
    participant ParentContext
    participant CarbSettings
    participant PhysXScene

    User->>SimulationCfg: Create config with device="cuda:0"
    Note over SimulationCfg: disable_sleeping=True (default)<br/>enable_external_forces_every_iteration=False (default)
    
    User->>SimulationContext: __init__(cfg)
    SimulationContext->>ParentContext: super().__init__(device=cfg.device)
    ParentContext->>CarbSettings: Set /physics/suppressReadback=True
    Note over CarbSettings: GPU pipeline enabled
    
    SimulationContext->>SimulationContext: _apply_physics_settings()
    Note over SimulationContext: Configure carb physics settings
    
    SimulationContext->>SimulationContext: _initialize_stage()
    SimulationContext->>SimulationContext: _set_additional_physx_params()
    
    SimulationContext->>PhysXScene: CreateAttribute("physxSceneAPI:disableSleeping")
    SimulationContext->>PhysXScene: Set(disable_sleeping=True)
    
    alt disable_sleeping is False
        SimulationContext->>CarbSettings: get_as_bool("/physics/suppressReadback")
        CarbSettings-->>SimulationContext: suppressReadback=True
        SimulationContext-->>User: RuntimeError: GPU pipeline incompatible
    else disable_sleeping is True
        Note over SimulationContext: Validation passed
    end
    
    SimulationContext->>PhysXScene: CreateEnableExternalForcesEveryIterationAttr()
    SimulationContext->>PhysXScene: Set(enable_external_forces_every_iteration)
    
    SimulationContext-->>User: Simulation ready
Loading

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

kellyguo11 and others added 3 commits November 7, 2025 15:27
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Kelly Guo <[email protected]>
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.

Greptile Overview

Greptile Summary

This PR introduces two new PhysX configuration flags for better simulation control:

Key Changes:

  • disable_sleeping: New flag (default: True) that globally disables sleeping for all physics objects, overriding per-object settings. This ensures consistent behavior between CPU and GPU simulation pipelines, as sleeping is not supported on GPU. The implementation includes validation that prevents the invalid configuration of disable_sleeping=False with GPU pipeline enabled.
  • enable_external_forces_every_iteration: New TGS solver flag (default: False) that applies external forces in lockstep with every solver iteration. When enabled, reduces noisy joint velocity data at ~4% performance cost.

Implementation Quality:

  • Proper error handling with clear, actionable error messages
  • Validation logic correctly prevents PhysX scene creation failures
  • Comprehensive test coverage including edge cases (CPU-only, GPU error conditions)
  • Good documentation in config class explaining behavior and trade-offs
  • Breaking change is well-documented (default disable_sleeping=True changes behavior for users who relied on sleeping)

Architecture:
The changes are localized to the simulation context setup, adding configuration in PhysxCfg and applying settings during physics scene initialization. The validation occurs before setting USD attributes, preventing invalid states.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is well-designed with proper validation, comprehensive testing, and clear documentation. The validation logic correctly prevents invalid GPU+sleeping configurations before they cause runtime failures. Tests cover all important scenarios including defaults, CPU/GPU differences, and error conditions.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/simulation_context.py 5/5 Added two new PhysX flags: disable_sleeping (with GPU validation) and enable_external_forces_every_iteration. Implementation is correct with proper error handling.
source/isaaclab/test/sim/test_simulation_context.py 5/5 Added comprehensive tests for both new flags covering default behavior, CPU/GPU configurations, and error conditions. Tests are thorough and well-structured.

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationCfg
    participant SimulationContext
    participant PhysxScene
    participant CarbSettings
    
    User->>SimulationCfg: Create config with device & physx flags
    User->>SimulationContext: Initialize SimulationContext(cfg)
    
    SimulationContext->>SimulationContext: _set_simulation_dt()
    SimulationContext->>SimulationContext: Setup PhysX scene
    
    alt disable_sleeping = False
        SimulationContext->>CarbSettings: get_as_bool("/physics/suppressReadback")
        CarbSettings-->>SimulationContext: GPU pipeline status
        alt GPU pipeline enabled
            SimulationContext-->>User: RuntimeError (invalid config)
        else CPU pipeline
            SimulationContext->>PhysxScene: Set physxSceneAPI:disableSleeping = False
        end
    else disable_sleeping = True
        SimulationContext->>PhysxScene: Set physxSceneAPI:disableSleeping = True
    end
    
    SimulationContext->>PhysxScene: CreateEnableExternalForcesEveryIterationAttr()
    PhysxScene->>PhysxScene: Apply enable_external_forces_every_iteration flag
    
    SimulationContext-->>User: Simulation ready
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

message and fail scene creation as this is not supported.
"""

enable_external_forces_every_iteration: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this flag only work when TGS is used? Also if this flag is enabled, then within the asset classes, I wonder if we still need to set forces at simulation steps. Previously those were getting cleared which wasn't a desired effect.

By iteration: do you mean solver iteration or sim.step? The document is clear about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Mayankm96
Copy link
Contributor

enable_external_forces_every_iteration is setting for the TGS solver the applies external forces in lockstep with every solver step. This flag was previously not set, which we are keeping as the default behavior with setting it to False. However, this can introduce noisy joint velocities as some users have noticed. To avoid these artifacts, the flag can be set to True at the cost of minor performance hits (we measured around 4% for a scene with 2048 humanoids)

If the values are more correct, I think we should keep this flag as True by default. Most usecases where sim-to-real is concerned, bad joint velocities affects the deployment. In my experience, many users remove joint velocities from observations because of the jitteriness in the deployed behavior.

We should inform the users that if they care about performance, then they can disable these flags to get "approximated" values, but the default should try to remain a "correct" value.

@kellyguo11 kellyguo11 changed the title Adds disable sleeping and apply forces in lockstep with solver steps Adds disable sleeping global flag for physx Nov 10, 2025
@kellyguo11
Copy link
Contributor Author

Keeping external force flag independent in #3989

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 10, 2025

Greptile Overview

Greptile Summary

Adds a global disable_sleeping flag to PhysxCfg that overrides per-object sleeping settings, defaulting to True for CPU/GPU consistency since GPU pipeline doesn't support sleeping.

Key Changes:

  • New PhysxCfg.disable_sleeping parameter (default: True) in simulation_cfg.py:183-193
  • Applied via physxSceneAPI:disableSleeping USD attribute in simulation_context.py:808-810
  • Validation prevents unsupported GPU+sleeping combination in simulation_context.py:798-806
  • Comprehensive test coverage for default, CPU, and GPU validation scenarios
  • Breaking change: users wanting sleeping on any object must now set this flag to False

Implementation Quality:
All previously identified issues from earlier review rounds have been resolved - the attribute is properly set, validation logic is in place, and tests validate the expected behavior.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is clean and well-tested with proper validation, documentation, and tests covering all scenarios including the GPU incompatibility edge case
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/simulation_context.py 5/5 Adds global sleep disabling with GPU validation - implementation looks correct with proper attribute setting and error handling for unsupported GPU+sleep=False combination
source/isaaclab/isaaclab/sim/simulation_cfg.py 5/5 Adds disable_sleeping config parameter with clear documentation and warning about GPU incompatibility
source/isaaclab/test/sim/test_simulation_context.py 5/5 Adds comprehensive tests for sleep disabling: default True, CPU with False, and GPU+False error validation

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationCfg
    participant SimulationContext
    participant PhysxPrim
    participant CarbSettings
    
    User->>SimulationCfg: Create config with disable_sleeping=True (default)
    User->>SimulationContext: Initialize SimulationContext(cfg)
    SimulationContext->>SimulationContext: _configure_simulation_flags()
    
    alt disable_sleeping is False
        SimulationContext->>CarbSettings: get_as_bool("/physics/suppressReadback")
        CarbSettings-->>SimulationContext: Returns GPU pipeline status
        alt GPU pipeline enabled (suppressReadback=True)
            SimulationContext->>User: RuntimeError: GPU + sleeping not supported
        end
    end
    
    SimulationContext->>PhysxPrim: CreateAttribute("physxSceneAPI:disableSleeping")
    SimulationContext->>PhysxPrim: Set(cfg.physx.disable_sleeping)
    PhysxPrim-->>SimulationContext: Attribute applied globally
    
    Note over PhysxPrim: Global sleeping disabled,<br/>overrides per-object settings
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.

5 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@kellyguo11 kellyguo11 moved this to In progress in Isaac Lab Nov 10, 2025
@kellyguo11 kellyguo11 moved this from In progress to In review in Isaac Lab Nov 10, 2025
kellyguo11 and others added 2 commits November 10, 2025 16:00
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Kelly Guo <[email protected]>
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.

5 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

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.

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

Labels

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

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants