Updates PhysX external forces default to True#6523
Conversation
Default enable_external_forces_every_iteration to true for PhysX and warn when users opt out with the deprecated flag. Add regression coverage for the default scene attribute and the deprecation warning, plus a breaking changelog fragment.
Fold the noisy velocity guidance into the deprecation warning and remove the regression test coverage for this flag.
Greptile SummaryThis PR changes the default value of
Confidence Score: 4/5Safe to merge; the core default-change and deprecation path work correctly for the vast majority of users who use the TGS solver. The change is straightforward and the test validates both the new default and the TGS deprecation warning. The only gap is that PGS users who explicitly wrote enable_external_forces_every_iteration=False will never see the deprecation notice, meaning they could be surprised when the field is eventually removed. physx_manager.py — the solver_type guard on the deprecation warning; physx_manager_cfg.py — the incomplete Sphinx directive. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User creates PhysxCfg] --> B{enable_external_forces_every_iteration}
B -->|not set - defaults to True| C[No warning emitted]
B -->|explicitly set to True| C
B -->|explicitly set to False| D{solver_type == 1 TGS?}
D -->|Yes| E[warnings.warn DeprecationWarning]
D -->|No PGS| F[No warning emitted]
C --> G[Attribute written to USD physxScene]
E --> G
F --> G
G --> H[PhysX simulation uses value]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User creates PhysxCfg] --> B{enable_external_forces_every_iteration}
B -->|not set - defaults to True| C[No warning emitted]
B -->|explicitly set to True| C
B -->|explicitly set to False| D{solver_type == 1 TGS?}
D -->|Yes| E[warnings.warn DeprecationWarning]
D -->|No PGS| F[No warning emitted]
C --> G[Attribute written to USD physxScene]
E --> G
F --> G
G --> H[PhysX simulation uses value]
Reviews (1): Last reviewed commit: "Refine PhysX external force warning" | Re-trigger Greptile |
| if cfg.solver_type == 1 and not cfg.enable_external_forces_every_iteration: | ||
| logger.warning("TGS solver with enable_external_forces_every_iteration=False may cause noisy velocities.") | ||
| warnings.warn( | ||
| "PhysxCfg.enable_external_forces_every_iteration is deprecated and will be removed in a future " | ||
| "PhysX release. External forces are applied every iteration by default; remove this override. " | ||
| "Disabling this behavior with the TGS solver may cause noisy velocities.", | ||
| DeprecationWarning, | ||
| stacklevel=2, |
There was a problem hiding this comment.
Deprecation warning skips PGS users
The deprecation guard cfg.solver_type == 1 means users who explicitly set enable_external_forces_every_iteration=False with the PGS solver (solver_type=0) will never receive the deprecation warning. Although the flag is a no-op for PGS today, the deprecation notice says the field itself will be removed from PhysxCfg entirely, so PGS users who explicitly wrote enable_external_forces_every_iteration=False in their configs will still encounter a config error at that future removal point without ever being told to clean it up.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Kelly Guo <kellyg@nvidia.com>
AntoineRichard
left a comment
There was a problem hiding this comment.
Lots of behavioral changes which is a bit concerning, let's hope it doesn't show in training.
|
|
||
| torch.testing.assert_close( | ||
| cube_object.data.root_lin_vel_w.torch, initial_velocity[:, :3], rtol=1e-5, atol=tolerance | ||
| cube_object.data.root_lin_vel_w.torch[:, :2], initial_velocity[:, :2], rtol=1e-5, atol=tolerance |
There was a problem hiding this comment.
That change is odd, why not increase the tolerance?
There was a problem hiding this comment.
I think this was a friction test with object sliding on a surface, so in theory, the check is only necessary on the x and y axes
Description
Default enable_external_forces_every_iteration to true for PhysX and warn when users opt out with the deprecated flag.
Add a deprecation warning and a breaking changelog fragment.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there