Add automatic PhysX backend selection - #6668
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Matthew Taylor <mataylor@nvidia.com>
…cLab into mataylor/rtx-preset
Resolve the physx selector at launch time so Isaac Sim PhysX is used when Kit is required and OvPhysX is used for kitless runs. Expose isaacsim_physx as the explicit concrete Isaac Sim PhysX selector for direct task presets and document the updated behavior.
github-actions
Bot
added
documentation
|
Closing this in favor of a fresh PR from a clean branch based on current develop. |
Greptile SummaryThis PR introduces automatic PhysX-family backend selection:
Confidence Score: 4/5Safe to merge for most users; the automatic backend selection is correct under all tested Kit/kitless signal combinations and the three findings are non-blocking. The two-phase placeholder resolution in scan() is logically sound and well-covered by new tests. The main concerns are an undocumented visited-set asymmetry that could silently drop an update if a PhysxAutoCfg instance is shared across tree nodes, a missing isinstance guard in _is_auto_rtx_renderer inconsistent with _is_ovrtx_renderer, and the absence of log output when automatic selection chooses OvPhysX making the backend switch invisible to users who relied on physics=physx for headless Isaac Sim PhysX runs. source/isaaclab/isaaclab/app/sim_launcher.py — the visit() function's asymmetric visited-guard handling and missing isinstance in _is_auto_rtx_renderer are in the hot path of every simulation launch. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Config has PhysxAutoCfg / _AutoRtxRendererCfg] --> B["scan(cfg, launcher_args)"]
B --> C[Walk config tree once]
C --> D{Auto-PhysX found?}
D -- Yes --> E["_has_kit_runtime_intent()\nkit_camera OR kit_physics OR\nkit_visualizer OR livestream"]
E -- True --> F["resolve to PhysxCfg Isaac Sim PhysX"]
E -- False --> G["resolve to OvPhysxCfg kitless"]
F --> H[_refresh_physics_scan_flags needs_kit = True]
G --> I[_refresh_physics_scan_flags needs_kit = has_kit_camera]
D -- No --> J{Auto-RTX found?}
H --> J
I --> J
J -- Yes --> K["_uses_isaac_sim_runtime()\nneeds_kit OR kit_visualizer OR livestream"]
K -- True --> L[resolve to IsaacRtxRendererCfg]
K -- False --> M[resolve to OVRTXRendererCfg]
J -- No --> N[Return Scan]
L --> N
M --> N
N --> O{needs_kit?}
O -- Yes --> P[AppLauncher / Kit launch]
O -- No --> Q[Kitless launch]
P --> R[SimulationContext fallback auto_physx check]
Q --> R
Reviews (1): Last reviewed commit: "Add automatic PhysX backend selection" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _is_auto_rtx_renderer(node) -> bool: | ||
| """True when the node is an automatic RTX renderer placeholder.""" | ||
| return getattr(node, "renderer_type", None) == "auto_rtx" |
There was a problem hiding this comment.
isinstance guard in _is_auto_rtx_renderer
_is_auto_rtx_renderer matches any object whose renderer_type attribute equals "auto_rtx", whereas the parallel _is_ovrtx_renderer requires isinstance(node, RendererCfg) before checking the attribute. Any config class that coincidentally defines renderer_type = "auto_rtx" would be silently recorded in auto_rtx_locations and subsequently passed to setattr(parent, key, renderer_factory()), overwriting the attribute with a renderer instance. Adding the same isinstance guard keeps the two predicates consistent.
| @@ -222,10 +271,16 @@ def visit(node, parent, attr): | |||
| if physics_str: | |||
| node = make_physics_cfg(physics_str) | |||
| if parent is not None: | |||
| setattr(parent, attr, node) | |||
| setattr(parent, key, node) | |||
| else: | |||
| effective_cfg = node | |||
| physics_cfgs.append(node) | |||
| if _is_auto_physx_physics(node): | |||
| has_auto_physx = True | |||
| auto_physx_locations.append((node, parent, key, len(physics_cfgs) == 1)) | |||
| return | |||
| else: | |||
| concrete_physics_cfgs.append(node) | |||
| elif _is_ovrtx_renderer(node): | |||
| has_ovrtx = True | |||
There was a problem hiding this comment.
_is_auto_rtx_renderer is checked before the visited guard, so every parent holding an auto-RTX renderer is recorded even if the node was already visited (allowing setattr to update all owners of a shared instance). _is_auto_physx_physics is evaluated after the guard: if the same PhysxAutoCfg instance appears under two attributes, only the first parent is recorded and updated — the other retains the stale placeholder. A comment documenting the invariant (auto-physx instances are unique per tree location; auto-rtx instances may be shared) would prevent future regressions.
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!
| # Resolve recorded auto PhysX placeholders first. Automatic RTX resolution | ||
| # then sees the concrete physics backend selected for this runtime. | ||
| if has_auto_physx: | ||
| use_isaac_sim = _has_kit_runtime_intent(config_scan, launcher_args) | ||
| for node, parent, key, is_first_physics in auto_physx_locations: | ||
| physics_cfg = resolve_physx_auto_cfg(node, use_isaac_sim) | ||
| concrete_physics_cfgs.append(physics_cfg) | ||
| if parent is None: | ||
| effective_cfg = physics_cfg | ||
| config_scan.effective_cfg = physics_cfg | ||
| else: | ||
| setattr(parent, key, physics_cfg) | ||
| if is_first_physics: | ||
| config_scan.resolved_physics_cfg = physics_cfg | ||
|
|
||
| _refresh_physics_scan_flags(config_scan, concrete_physics_cfgs, has_physics) |
There was a problem hiding this comment.
The auto-PhysX resolution block in scan() and resolve_physx_auto_cfg are both silent. Users who previously ran headless jobs with physics=physx (which used to force Isaac Sim PhysX) will now silently receive OvPhysxCfg. Emitting a logger.info line stating the selected backend would make the substitution visible in logs and aid debugging when results differ from prior runs.
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!
2 participants
Description
Stacked on #6246.
Adds automatic PhysX-family selection for
physics=physx: Isaac Sim PhysX is selected when the run needs Kit (Kit renderer/viewer or livestream), and OvPhysX is selected for kitless runs. Addsphysics=isaacsim_physxas the explicit selector for forcing Isaac Sim PhysX, updates direct task presets, docs, tests, and changelog fragments.Fixes # N/A
Type of change
Screenshots
N/A
Validation
git diff --check./isaaclab.sh -p -c "from pathlib import Path; files = ['source/isaaclab/isaaclab/app/sim_launcher.py', 'source/isaaclab/isaaclab/physics/physics_manager_cfg.py', 'source/isaaclab/isaaclab/sim/simulation_context.py', 'source/isaaclab_tasks/isaaclab_tasks/core/cabinet/cabinet_direct_env_cfg.py', 'source/isaaclab_tasks/isaaclab_tasks/core/cartpole/cartpole_direct_env_cfg.py', 'source/isaaclab_tasks/isaaclab_tasks/core/locomotion/ant/ant_direct_env_cfg.py', 'source/isaaclab_tasks/isaaclab_tasks/core/locomotion/humanoid/humanoid_direct_env_cfg.py', 'source/isaaclab_tasks/test/core/test_preset_kit_decision.py', 'source/isaaclab_tasks/test/core/test_runtime_compatibility.py']; [compile(Path(f).read_text(), f, 'exec') for f in files]; print('syntax ok')"./isaaclab.sh -fModuleNotFoundError: No module named 'isaaclab_ov'in this environment.Checklist
./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there