IsaacLab icon indicating copy to clipboard operation
IsaacLab copied to clipboard

Fixes noisy velocities around limits

Open AntoineRichard opened this issue 1 month ago • 4 comments

Description

Enables a new PhysX flag to help mitigate noisy velocities.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

Checklist

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

AntoineRichard avatar Nov 10 '25 14:11 AntoineRichard

Copying over some comments from Mayank from an earlier PR:

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.

kellyguo11 avatar Nov 10 '25 23:11 kellyguo11

Greptile Overview

Greptile Summary

This PR adds a new PhysX configuration flag enable_external_forces_every_iteration to help mitigate noisy velocities when using the TGS solver. The implementation includes proper documentation, a helpful warning message when the flag is disabled with TGS, and appropriate changelog entries.

Key changes:

  • Added enable_external_forces_every_iteration boolean flag to PhysxCfg (defaults to False)
  • Conditionally sets the PhysX scene attribute when solver_type == 1 (TGS solver)
  • Logs a warning when TGS is enabled but the flag is disabled, guiding users to enable it if experiencing noisy velocities
  • Updated CHANGELOG.rst for version 0.48.1

Observations:

  • The PR checklist indicates tests were added, but no test files were modified in this changeset. This may refer to manual testing or integration testing done outside this PR.
  • The previous review comments correctly identified issues with attribute references in CHANGELOG.rst (should be PhysxCfg not SimulationCfg) and suggested making the attribute creation unconditional for all solver types.

Confidence Score: 4/5

  • This PR is safe to merge once the previous syntax issues in CHANGELOG.rst are addressed
  • The implementation is sound with clear documentation and appropriate warnings. The logic correctly checks solver type before enabling the feature. Score reduced by 1 point due to: (1) syntax issues in CHANGELOG.rst identified in previous comments that need correction, and (2) minor style inconsistencies in warning message formatting
  • source/isaaclab/docs/CHANGELOG.rst requires corrections to attribute references (previous comments), source/isaaclab/isaaclab/sim/simulation_context.py has minor style improvements suggested

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/simulation_cfg.py 5/5 Added enable_external_forces_every_iteration config flag with clear documentation about TGS solver usage
source/isaaclab/isaaclab/sim/simulation_context.py 4/5 Conditionally sets PhysX attribute based on solver type with helpful warning for users experiencing noisy velocities
source/isaaclab/docs/CHANGELOG.rst 5/5 Added changelog entry for version 0.48.1 documenting the new feature and warning

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationCfg
    participant SimulationContext
    participant PhysXSceneAPI
    participant PhysXSolver

    User->>SimulationCfg: Configure enable_external_forces_every_iteration
    User->>SimulationContext: Initialize with config
    SimulationContext->>SimulationContext: Check solver_type == 1 (TGS)
    alt TGS solver enabled
        alt enable_external_forces_every_iteration is False
            SimulationContext->>SimulationContext: Log warning about noisy velocities
        end
        SimulationContext->>PhysXSceneAPI: CreateEnableExternalForcesEveryIterationAttr(flag)
        PhysXSceneAPI->>PhysXSolver: Set external forces iteration behavior
        PhysXSolver-->>PhysXSceneAPI: Attribute configured
    else PGS solver (solver_type == 0)
        SimulationContext->>SimulationContext: Skip (flag ignored for PGS)
    end
    PhysXSolver-->>User: Improved velocity accuracy (if enabled)

greptile-apps[bot] avatar Nov 11 '25 01:11 greptile-apps[bot]

I would merge as is and make the breaking change with 3.0 @Mayankm96 would you be ok with this?

AntoineRichard avatar Nov 11 '25 16:11 AntoineRichard

Copying over some comments from Mayank from an earlier PR:

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.

I checked with Gordon, and this flag does NOT allow forces to be conserved between solver steps. What this does is that rather than applying external forces in one go on the first solver position iteration, it applies it (divided by the number of solver position iterations) for each solver position iteration.

AntoineRichard avatar Nov 11 '25 16:11 AntoineRichard

should be good to go once omni.log is updated

Updated

AntoineRichard avatar Dec 15 '25 11:12 AntoineRichard