Fixes noisy velocities around limits
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-commitchecks 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.tomlfile - [x] I have added my name to the
CONTRIBUTORS.mdor my name already exists there
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.
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_iterationboolean flag toPhysxCfg(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
PhysxCfgnotSimulationCfg) 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)
I would merge as is and make the breaking change with 3.0 @Mayankm96 would you be ok with this?
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.
should be good to go once omni.log is updated
Updated