-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fixes noisy velocities around limits #3989
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?
Changes from all commits
3e5c224
01dd15d
f18dbd5
849b256
34b748e
2a6fda9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -792,6 +792,19 @@ def _set_additional_physx_params(self): | |
| physx_prim.CreateAttribute("physxScene:solveArticulationContactLast", Sdf.ValueTypeNames.Bool).Set( | ||
| self.cfg.physx.solve_articulation_contact_last | ||
| ) | ||
| # -- Enable external forces every iteration, helps improve the accuracy of velocity updates. | ||
|
|
||
| if self.cfg.physx.solver_type == 1: | ||
| if not self.cfg.physx.enable_external_forces_every_iteration: | ||
| omni.log.warn( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we move away from omni.log?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I wait for Pascal's PR to be in?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it has landed :) |
||
| "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." | ||
| ) | ||
|
Comment on lines
+799
to
+804
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: The backtick wrapping of |
||
| physx_scene_api.CreateEnableExternalForcesEveryIterationAttr( | ||
| self.cfg.physx.enable_external_forces_every_iteration | ||
| ) | ||
AntoineRichard marked this conversation as resolved.
Show resolved
Hide resolved
AntoineRichard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| # -- Gravity | ||
| # note: Isaac sim only takes the "up-axis" as the gravity direction. But physics allows any direction so we | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok yup that was my thought as well. let's keep it False for now until 3.0.