Fixes noisy velocities around limits #3989
Conversation
AntoineRichard
requested review from
kellyguo11,
matthewtrepte and
ossamaAhmed
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds a new PhysX configuration flag enable_external_forces_every_iteration to help mitigate noisy velocities around joint limits.
Key changes:
- Added
enable_external_forces_every_iterationboolean flag toPhysxCfg(defaults to False for backward compatibility) - Implemented the flag by calling
CreateEnableExternalForcesEveryIterationAttron the PhysX scene API - Added a helpful warning message when the flag is disabled, suggesting users enable it if experiencing noisy velocities
- Updated CHANGELOG.rst with version 0.48.1 entry
Issues found:
- The CHANGELOG contains incorrect attribute reference paths that will cause broken documentation links
Confidence Score: 4/5
- This PR is safe to merge after fixing the documentation paths
- The implementation follows existing patterns correctly and adds a useful feature with appropriate documentation. The only issue is incorrect attribute paths in the CHANGELOG that will cause broken documentation links. The code changes are minimal, well-documented, and maintain backward compatibility.
- source/isaaclab/docs/CHANGELOG.rst needs the attribute reference paths corrected
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/docs/CHANGELOG.rst | 3/5 | Added changelog entry for v0.48.1 with incorrect attribute reference paths that need correction |
| source/isaaclab/isaaclab/sim/simulation_cfg.py | 5/5 | Added enable_external_forces_every_iteration flag with clear documentation following existing patterns |
| source/isaaclab/isaaclab/sim/simulation_context.py | 5/5 | Implemented PhysX attribute setting with helpful warning message for users experiencing noisy velocities |
Sequence Diagram
sequenceDiagram
participant User
participant SimulationCfg
participant PhysxCfg
participant SimulationContext
participant PhysxSceneAPI
participant PhysicsEngine
User->>SimulationCfg: Create config with PhysxCfg
User->>PhysxCfg: Set enable_external_forces_every_iteration=True/False
User->>SimulationContext: Initialize with config
SimulationContext->>SimulationContext: _set_additional_physx_params()
alt Flag is False
SimulationContext->>SimulationContext: Log warning about noisy velocities
Note over SimulationContext: Warns user to enable flag if<br/>experiencing noisy velocities
end
SimulationContext->>PhysxSceneAPI: CreateEnableExternalForcesEveryIterationAttr(flag_value)
PhysxSceneAPI->>PhysicsEngine: Configure external forces behavior
Note over PhysicsEngine: If enabled: applies external forces<br/>every iteration for more<br/>accurate velocity updates
3 files reviewed, 2 comments
source/isaaclab/docs/CHANGELOG.rst
Outdated
| * Added :attr:`~isaaclab.sim.SimulationCfg.enable_external_forces_every_iteration` to enable external forces every iteration. | ||
| This can help improve the accuracy of velocity updates. Consider enabling this flag if the velocities generated by the simulation are noisy. |
There was a problem hiding this comment.
syntax: incorrect attribute reference path - should be PhysxCfg not SimulationCfg
| * Added :attr:`~isaaclab.sim.SimulationCfg.enable_external_forces_every_iteration` to enable external forces every iteration. | |
| This can help improve the accuracy of velocity updates. Consider enabling this flag if the velocities generated by the simulation are noisy. | |
| * Added :attr:`~isaaclab.sim.PhysxCfg.enable_external_forces_every_iteration` to enable external forces every iteration. | |
| This can help improve the accuracy of velocity updates. Consider enabling this flag if the velocities generated by the simulation are noisy. |
source/isaaclab/docs/CHANGELOG.rst
Outdated
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Added warning when :attr:`~isaaclab.sim.SimulationCfg.enable_external_forces_every_iteration` is set to False. |
There was a problem hiding this comment.
syntax: incorrect attribute reference path - should be PhysxCfg not SimulationCfg
| * Added warning when :attr:`~isaaclab.sim.SimulationCfg.enable_external_forces_every_iteration` is set to False. | |
| * Added warning when :attr:`~isaaclab.sim.PhysxCfg.enable_external_forces_every_iteration` is set to False. |
There was a problem hiding this comment.
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.
Key Changes
- Configuration: Added
enable_external_forces_every_iterationboolean flag toPhysxCfgclass (defaults to False) - Documentation: Comprehensive docstring explains the flag only applies to TGS solver (type 1) and is ignored for PGS solver (type 0)
- Implementation: Sets the PhysX attribute only when TGS solver is used, includes helpful warning message when flag is disabled
- Changelog: Documents the new feature for version 0.48.1
Notes
- The implementation correctly restricts attribute setting to TGS solver only, since the flag is explicitly documented as being ignored for PGS
- Default warning message will appear for all TGS users (the default configuration) to raise awareness about this velocity accuracy feature
- Integration follows existing patterns in the codebase (similar to
enable_stabilizationwarning)
Confidence Score: 4/5
- PR is safe to merge with one implementation detail to verify regarding conditional attribute setting
- The implementation is straightforward and follows existing patterns. The flag correctly defaults to False for backward compatibility. One comment was raised about whether the PhysX attribute should be set for all solver types or only TGS, but based on documentation stating it's "ignored for PGS" and the commit message clarifying it's "only true for TGS solver", the conditional setting is likely correct. Recommend author confirm PhysX API behavior to ensure the attribute doesn't need to exist (set to False) for PGS solver.
- The
simulation_context.pyfile should be reviewed to confirm the conditional attribute setting (lines 805-807) is correct for the PhysX API
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/simulation_cfg.py | 5/5 | added enable_external_forces_every_iteration flag with comprehensive documentation for TGS solver |
| source/isaaclab/isaaclab/sim/simulation_context.py | 4/5 | implemented TGS-specific attribute setting with warning for users, attribute only set when solver_type is TGS |
Sequence Diagram
sequenceDiagram
participant User
participant SimulationContext
participant PhysxCfg
participant PhysxSceneAPI
User->>SimulationContext: Initialize simulation with config
SimulationContext->>PhysxCfg: Read enable_external_forces_every_iteration (default: False)
SimulationContext->>SimulationContext: Check solver_type
alt solver_type == 1 (TGS)
alt enable_external_forces_every_iteration == False
SimulationContext->>User: Log warning about noisy velocities
end
SimulationContext->>PhysxSceneAPI: CreateEnableExternalForcesEveryIterationAttr(value)
PhysxSceneAPI-->>SimulationContext: Attribute set
else solver_type == 0 (PGS)
SimulationContext->>SimulationContext: Skip attribute setting (ignored for PGS)
end
SimulationContext-->>User: Simulation initialized
2 files reviewed, 1 comment
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds enable_external_forces_every_iteration flag to PhysxCfg to help mitigate noisy velocities when using the TGS solver.
Key Changes:
- Added new configuration parameter with comprehensive documentation explaining TGS solver-specific behavior
- Implemented PhysX scene API integration in
simulation_context.py:805that properly sets the attribute - Added warning message when flag is disabled with TGS solver to guide users experiencing noisy velocities
- Updated changelog with new version 0.48.1
Implementation Details:
The flag is correctly applied only when solver_type == 1 (TGS solver), as documented. The warning message provides helpful guidance about increasing velocity iterations alongside enabling this flag.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk - adds opt-in feature flag with proper documentation and warnings
- Clean implementation that follows existing codebase patterns, comprehensive documentation, proper solver-type conditional logic, helpful user warnings, and no breaking changes. The only issues are documentation reference paths already flagged in previous comments.
- No files require special attention beyond addressing the documentation reference path fixes already noted in previous comments
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/simulation_cfg.py | 5/5 | Added new PhysX configuration flag enable_external_forces_every_iteration with comprehensive documentation explaining TGS solver usage |
| source/isaaclab/docs/CHANGELOG.rst | 4/5 | Added changelog entries for new feature, but has incorrect attribute reference paths (already flagged in previous comments) |
Sequence Diagram
sequenceDiagram
participant User
participant SimulationCfg
participant SimulationContext
participant PhysxSceneAPI
participant PhysXEngine
User->>SimulationCfg: Configure enable_external_forces_every_iteration
User->>SimulationContext: Initialize with SimulationCfg
SimulationContext->>SimulationContext: _set_additional_physx_params()
alt solver_type == 1 (TGS)
alt enable_external_forces_every_iteration == False
SimulationContext->>User: Log warning about noisy velocities
end
SimulationContext->>PhysxSceneAPI: CreateEnableExternalForcesEveryIterationAttr(flag_value)
PhysxSceneAPI->>PhysXEngine: Set external forces iteration mode
PhysXEngine-->>PhysxSceneAPI: Configured
else solver_type == 0 (PGS)
Note over SimulationContext,PhysxSceneAPI: Flag ignored for PGS solver
end
PhysxSceneAPI-->>SimulationContext: Configuration complete
SimulationContext-->>User: Simulation ready with improved velocity accuracy
2 files reviewed, no comments
| """ | ||
|
|
||
| enable_external_forces_every_iteration: bool = False | ||
| """Enable/disable external forces every position iteration in the TGS solver. Default is False. |
There was a problem hiding this comment.
I think @Mayankm96's preference was to keep this to True so that by default we always get the most accurate data. If users need better perf, they can set it to False.
I don't feel too strongly either way. My concern was mostly that setting it to True by default might break existing policies if they use joint velocities, but data accuracy is also important.
There was a problem hiding this comment.
That was my concern too. It may change the default behavior of the sim, and that may be considered a breaking change? I'd say for this version we keep it this way, and for 3.0 we can make it a default.
There was a problem hiding this comment.
ok yup that was my thought as well. let's keep it False for now until 3.0.
|
|
||
| if self.cfg.physx.solver_type == 1: | ||
| if not self.cfg.physx.enable_external_forces_every_iteration: | ||
| omni.log.warn( |
There was a problem hiding this comment.
should we move away from omni.log?
There was a problem hiding this comment.
Should I wait for Pascal's PR to be in?
kellyguo11
changed the title
|
Copying over some comments from Mayank from an earlier PR:
|
Greptile OverviewGreptile SummaryThis PR adds a new PhysX configuration flag Key changes:
Observations:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
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)
|
| omni.log.warn( | ||
| "The `enable_external_forces_every_iteration` parameter in the PhysxCfg is set to False. If you are" | ||
| " experiencing noisy velocities, consider enabling this flag. You may need to slightly increase the" | ||
| " number of velocity iterations (setting it to 1 or 2 rather than 0), together with this flag, to" | ||
| " improve the accuracy of velocity updates." | ||
| ) |
There was a problem hiding this comment.
style: The backtick wrapping of enable_external_forces_every_iteration in the warning message is inconsistent with Python string formatting and will display the backticks literally to users
|
I would merge as is and make the breaking change with 3.0 @Mayankm96 would you be ok with this? |
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. |
kellyguo11
left a comment
kellyguo11
left a comment
There was a problem hiding this comment.
should be good to go once omni.log is updated
2 participants
Description
Enables a new PhysX flag to help mitigate noisy velocities.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there