Centralize shared physics preset ownership - #7301
Conversation
Greptile SummaryThe PR centralizes stack, lift, and locomotion physics presets while retaining robot-specific Newton configurations.
Confidence Score: 4/5The shared stack contact-ordering change should be scoped back to SO-101 or explicitly validated for every affected stack robot before merging. Enabling solve_articulation_contact_last in StackEnvCfg changes the effective PhysX solve order for existing non-SO-101 stack tasks that previously used the default false value. Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/contrib/stack/stack_env_cfg.py Important Files Changed
Reviews (1): Last reviewed commit: "Centralize shared physics presets" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| gpu_total_aggregate_pairs_capacity=2**21, | ||
| friction_correlation_distance=0.00625, | ||
| # Let object contacts stall position-driven grippers before they tunnel through a grasp. | ||
| solve_articulation_contact_last=True, |
There was a problem hiding this comment.
When a non-SO-101 stack task uses the default Isaac Sim PhysX preset, StackEnvCfg.__post_init__ now enables solve_articulation_contact_last, changing the articulation/contact solve order from its previous default and therefore changing grasp and contact dynamics for existing Franka, Galbot, and UR10 tasks.
Knowledge Base Used: isaaclab_tasks: Task Registration and Organization
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!
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The locomotion preset reuse preserves the robot-specific Newton overrides, but the stack and lift centralization broadens two formerly robot-specific PhysX settings to entire task families. These scope changes conflict with the stated behavior-preserving cleanup and need either narrower ownership or explicit treatment as user-visible changes.
- Design and architecture: Reusing
RoughPhysicsCfgfor the nine flat locomotion configurations follows the existing preset layering while retaining robot-specific Newton settings. However, movingsolve_articulation_contact_last=Truefrom the SO-101 preset into the shared stackPhysicsCfgchanges contact ordering for every stack environment, not just SO-101. - API: The resolved backend preset behavior changes for existing consumers: all stack environments now receive the SO-101 contact-ordering setting, and all lift/reorient environments now receive the Kuka-derived OV PhysX capacity configuration. If these family-wide changes are intentional, they should be documented with appropriate release metadata rather than represented as a no-user-facing-change
.skipentry. - Implementation: The velocity inheritance changes preserve the prior shared PhysX values and local Newton solver configurations. In contrast, wiring the OV preset into the shared lift
PhysxAutoCfgbroadens its allocation settings beyond Kuka Allegro, while adding the contact-ordering flag to shared stack physics broadens an SO-101-specific dynamics workaround. Keep those settings scoped to their original composition configs or explicitly acknowledge and validate the wider behavior and resource effects.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| gpu_total_aggregate_pairs_capacity=2**21, | ||
| friction_correlation_distance=0.00625, | ||
| # Let object contacts stall position-driven grippers before they tunnel through a grasp. | ||
| solve_articulation_contact_last=True, |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — SO-101 contact ordering becomes stack-family default
solve_articulation_contact_last=True previously applied only when SO101CubeStackEnvCfg.__post_init__ installed SO101StackPhysicsCfg. On the family PhysicsCfg it changes PhysX contact-solve ordering for every environment deriving from StackEnvCfg, altering grasp dynamics and demonstration replay for robots that never opted in. Keep the flag in an SO-101-specific preset, or accept the family-wide change and record it as user-visible instead of a .skip fragment.
| gpu_max_rigid_patch_count=4 * 5 * 2**15, | ||
| gpu_found_lost_pairs_capacity=2**26, | ||
| ) | ||
| ovphysx = OvPhysxCfg( |
There was a problem hiding this comment.
🔵 Suggestion · Api — Lift OV preset widened beyond Kuka Allegro
The shared lift PhysicsCfg previously built PhysxAutoCfg with no ovphysx preset, so non-Kuka lift/reorient tasks used OV defaults. Wiring these capacities (including gpu_found_lost_pairs_capacity=2**26) into physx changes OV backend allocation for the whole lift family. Either scope them to the Kuka composition config or document the intentional family-wide resource change rather than shipping a .skip fragment.
| @@ -64,6 +70,7 @@ class RoughPhysicsCfg(PresetCfg): | |||
| debug_mode=False, | |||
There was a problem hiding this comment.
Some comments for newton_mjwarp defaults I noticed:
njmax is really high.
num_substeps=1 should maybe be 2?
friction type pyramidal might be alot better than cone.
Also we can probably set defaults that don't need to be overrides, it makes sense that each robot has different value but I would expect the main difference is only njmax
There was a problem hiding this comment.
Updated in 55815de. All Newton MJWarp velocity environments now use two physics substeps. RoughPhysicsCfg owns num_substeps=2 once, and every core/contrib robot-specific .num_substeps assignment has been removed. Spot has an independent velocity physics composition, so its MJWarp value is also set to 2. The shared config already uses cone="pyramidal"; robot-specific constraint capacities remain separate for now.
|
|
||
| # physics | ||
| newton_mjwarp = self.sim.physics.newton_mjwarp | ||
| newton_mjwarp.solver_cfg.njmax = 95 |
There was a problem hiding this comment.
I would have expected njmax 192, and nconmax 48. nconmax 10 is very low.
There was a problem hiding this comment.
95/10 are the existing Isaac Lab G1-flat values; this refactor copies them onto the inherited shared preset rather than introducing new tuning. I also checked current MJLab main: its G1-flat config uses njmax=300 and nconmax=None, while G1 rough sets nconmax=70. Since 192/48 is neither the prior Isaac Lab behavior nor the current MJLab value, I am preserving parity here. I agree nconmax=10 is worth a separate capacity/stability benchmark and tuning PR.
…7312) # Description Backports #7301 to `release/3.0.0` by cherry-picking the canonical merged commit `f86b4e1ad66e637e7770a727e181ffe150a2077b`. This centralizes backend physics presets at the task-family composition roots: - Kuka Allegro reuses the shared lift physics configuration. - SO-101 reuses the shared stack physics configuration. - Flat velocity environments mutate robot-specific MJWarp values directly instead of defining preset subclasses. - Newton MJWarp velocity environments use two shared physics substeps, with no robot-specific substep overrides. - The corresponding Anymal-D Newton visualizer goldens and changelog fragments are included. The cherry-pick applied without conflicts, and all 21 affected files are byte-for-byte identical to the merged #7301 versions. ## Validation - `uv run --extra test --frozen python -m pytest -q source/isaaclab_tasks/test/core/test_velocity_newton_cfg.py source/isaaclab_tasks/test/core/test_lift_kuka_ovphysx_presets.py source/isaaclab_tasks/test/core/test_lift_env_cfg.py` — 8 passed. - Direct construction audit — all 10 affected Newton velocity configs use `num_substeps=2`; SO-101 and Kuka inherit the intended shared physics values. - Structural audit — no `PhysicsCfg(RoughPhysicsCfg)`, `KukaAllegroPhysicsCfg`, or `SO101StackPhysicsCfg` remains in task code; robot-specific velocity configs do not assign `.num_substeps`. - `uv run --frozen isaaclab -f` — all hooks passed, including changelog and Git LFS checks.
## Summary - make resolve_task_config and parse_env_cfg the task configuration composition boundary, with explicit programmatic overrides - remove late preset fallback handling from environments, simulation, launch scanning, camera validation, benchmarks, and RL summaries - require runtime consumers to receive concrete physics, renderer, and camera configurations - route scripts, tools, integrations, and relevant tests through registered task composition This is a separate follow-up to #7301; it does not mix the earlier preset ownership cleanup into this PR. ## Validation - 129 Hydra and Shadow Hand camera tests passed - 31 benchmark capture and RL entrypoint tests passed - 23 experimental frontend tests passed, 1 skipped - 5 custom-coupling tests passed - representative composition audit confirmed six task trees contain no remaining PresetCfg nodes - Python compile checks passed for all changed runtime and script paths - ruff and ruff-format passed The full format command passes every hook except the changelog comparison, which uses the stale local origin/develop ref and flags six pre-existing upstream fragments that this branch does not modify. This PR includes all required package fragments.
## Summary - make resolve_task_config and parse_env_cfg the task configuration composition boundary, with explicit programmatic overrides - remove late preset fallback handling from environments, simulation, launch scanning, camera validation, benchmarks, and RL summaries - require runtime consumers to receive concrete physics, renderer, and camera configurations - route scripts, tools, integrations, and relevant tests through registered task composition This is a separate follow-up to isaac-sim#7301; it does not mix the earlier preset ownership cleanup into this PR. ## Validation - 129 Hydra and Shadow Hand camera tests passed - 31 benchmark capture and RL entrypoint tests passed - 23 experimental frontend tests passed, 1 skipped - 5 custom-coupling tests passed - representative composition audit confirmed six task trees contain no remaining PresetCfg nodes - Python compile checks passed for all changed runtime and script paths - ruff and ruff-format passed The full format command passes every hook except the changelog comparison, which uses the stale local origin/develop ref and flags six pre-existing upstream fragments that this branch does not modify. This PR includes all required package fragments. (cherry picked from commit 393fc37)
…#7303 (#7313) # Description Backports six merged PRs to `release/3.0.0` as separate provenance-preserving cherry-picks: | Source PR | Source commit | Backport commit | Scope | | --- | --- | --- | --- | | #7272 | `0081477fea2` | `c9fc1997476` | Fix scene-wide gravity distributions for PhysX and OvPhysX. | | #7121 | `9f65e3d4fd2` | `cce8acd84e1` | Fix backend-factory fallback before simulator initialization. | | #7183 | `3fcc9c6b824` | `99d58d3d3c4` | Streamline the getting-started documentation. | | #7306 | `6aad90be0ae` | `9a786cc028a` | Optimize Newton test runtime. | | #7305 | `1c9ba908cf1` | `a2dc8ce948e` | Disable Warp autodiff in the test suite. | | #7303 | `393fc37d2b0` | `249a5cb97d8` | Remove post-Hydra preset resolution. | #7272 forwards the validated `uniform`, `log_uniform`, or `gaussian` distribution through both scene-wide backend paths. Newton behavior is unchanged. #7121 makes factory resolution fall back to Newton when no `SimulationContext` exists while retaining the visualizer contract of reporting no active backend before context creation. #7183 is documentation-only. #7306 and #7305 change test infrastructure only; they do not change runtime behavior. #7303 makes `resolve_task_config` and `parse_env_cfg` the task-composition boundary. Runtime consumers now require concrete physics, renderer, and camera configurations and no longer attempt late preset fallback after Hydra composition. The branch was synchronized with the current `release/3.0.0` tip containing #7301 before #7303 was applied. All six source cherry-picks applied without conflicts. A file-by-file existence, mode, and blob audit confirmed that every #7303 path matches its merged source commit exactly. ## Validation ### #7272 - Regression verification against the unpatched `release/3.0.0` tip — both PhysX and OvPhysX cases failed as expected. - `uv run --extra test --frozen python -m pytest -q source/isaaclab/test/envs/test_gravity_randomization.py source/isaaclab/test/envs/test_mdp_event_selectors.py` — 7 passed. - `uv run --extra test --extra ovphysx --frozen python -m pytest -q source/isaaclab_ov/test/physics/test_ovphysx_gravity.py` — 1 passed. ### #7121 - Regression verification before the #7121 cherry-pick reproduced the original `NoneType.physics_manager` failure. - `uv run --extra test --frozen python -m pytest -q source/isaaclab/test/utils/test_backend_utils.py source/isaaclab/test/visualizers/test_visualizer.py` — 18 passed. ### #7183 - `uv run --frozen --extra test python -m pytest --noconftest -q tools/test/test_environ_docs.py` — 26 passed. ### #7306 - All 9 affected paths match the merged source PR exactly. - The full simulator-backed Newton suite is left to backport CI because this local worktree does not include the full Isaac Sim runtime. ### #7305 - Loaded the root `conftest.py` successfully with Warp unavailable. - Loaded it with Warp installed and confirmed `wp.config.enable_backward is False`. ### #7303 - Hydra and Shadow Hand camera suites — 129 passed. - Benchmark capture and RL entrypoint suites — 31 passed. - Experimental frontend and custom-coupling suites — 74 passed, 1 skipped. - Ruff and Ruff-format passed for every changed Python path. - Source/backport audit — 54 paths checked, 0 mismatches. - The Isaac Sim-backed integration case is left to CI because this local worktree is kitless. ### Repository gates - `git diff --check upstream/release/3.0.0...HEAD` — passed. --------- Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: Antoine RICHARD <antoiner@nvidia.com> Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: Mustafa H <34825877+StafaH@users.noreply.github.com>
3 participants
Description
Centralize backend physics presets at task-family composition roots so robot-specific files own only genuinely robot-specific scalar configuration. Standardize Newton MJWarp velocity environments on two physics substeps at the shared family boundary.
PhysicsCfgand removeKukaAllegroPhysicsCfg.PhysicsCfgand removeSO101StackPhysicsCfg.RoughPhysicsCfgown the shared PhysX, MJWarp, and Kamino alternatives once.num_substeps=2and remove every robot-specific substep assignment.PhysicsCfg(RoughPhysicsCfg)wrappers. Their environment configs now mutate only robot-specific MJWarp values on the inheritedRoughPhysicsCfginstance.isaaclab_tasksmajor changelog fragment for the shared two-substep behavior change.No additional dependencies are required.
Type of change
Validation
PhysicsCfg(SomeOtherPhysicsCfg)inheritance underisaaclab_tasks..num_substeps.uv run --frozen isaaclab -f: Ruff, formatting, and applicable source checks passed; the repository-wide changelog gate still reports pre-existing missing or modified fragments in untouched packages.Screenshots
Not applicable.
Checklist
isaaclab_tasksmajor changelog fragment is included.