Add Option for Warp Renderer backend with PhysX simulation engine#4612
Add Option for Warp Renderer backend with PhysX simulation engine#4612bdilinila wants to merge 74 commits intoisaac-sim:developfrom
Conversation
| @@ -67,6 +67,52 @@ class RslRlPpoActorCriticRecurrentCfg(RslRlPpoActorCriticCfg): | |||
| """The number of RNN layers.""" | |||
|
|
|||
|
|
|||
| @configclass | |||
| @@ -0,0 +1,47 @@ | |||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
Change here and other files in config/kuka_allegro likely came from: https://github.com/isaac-sim/IsaacLab/pull/4421/changes#diff-d0e6df098d4f172bbe27cf3e9dd93c0e4d91eb6231e64f57380922ec7d51461aR1
| @@ -116,6 +116,28 @@ def __post_init__(self): | |||
| policy: PolicyCfg = PolicyCfg() | |||
|
|
|||
|
|
|||
| @configclass | |||
There was a problem hiding this comment.
Changes here and this folder come from https://github.com/ooctipus/IsaacLab/tree/feature/dexsuite_vision/source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/reach
| @@ -26,7 +26,7 @@ | |||
| from isaaclab.utils import replace_strings_with_slices | |||
There was a problem hiding this comment.
This file and source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/reach/reach_env_cfg.py both come from https://github.com/ooctipus/IsaacLab/tree/feature/dexsuite_vision/source/isaaclab_tasks
317b60d to
42dbb9b
Compare
Greptile SummaryThis PR adds an optional Newton Warp renderer backend for tiled cameras while keeping PhysX as the simulation engine. It introduces a new Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Train as train.py
participant RLEnv as ManagerBasedRLEnv
participant SimCtx as SimulationContext
participant Scene as InteractiveScene
participant TiledCam as TiledCamera
participant NM as NewtonManager
participant NWR as NewtonWarpRenderer
participant Warp as SensorTiledCamera (Warp)
Train->>RLEnv: step(action)
RLEnv->>SimCtx: sim.step(render=False)
Note over SimCtx: PhysX physics step
RLEnv->>Scene: scene.update(dt)
RLEnv->>RLEnv: observation_manager.compute()
RLEnv->>TiledCam: _update_buffers_impl()
alt renderer_type == "newton_warp"
TiledCam->>NM: update_state_from_physx_tensors_gpu()
NM->>NM: GPU kernel: copy PhysX poses → Newton body_q
TiledCam->>NWR: render(pos_w, quat_w, intrinsics)
NWR->>NWR: _prepare_camera_transforms()
NWR->>Warp: SensorTiledCamera.render()
Warp-->>NWR: color_image, depth_image
NWR->>NWR: _copy_outputs_to_buffers()
NWR-->>TiledCam: output buffers (rgba, depth)
else RTX (default)
TiledCam->>TiledCam: annotator.get_data()
TiledCam->>TiledCam: reshape_tiled_image kernel
end
TiledCam-->>RLEnv: observation tensors
Last reviewed commit: 42dbb9b |
| disable_env_checker=True, | ||
| kwargs={ | ||
| "env_cfg_entry_point": f"{__name__}.dexsuite_kuka_allegro_env_cfg:DexsuiteKukaAllegroLiftEnvCfg", | ||
| "rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_ppo_cfg.yaml", | ||
| "rsl_rl_cfg_entry_point": f"{agents.__name__}.rsl_rl_ppo_cfg:DexsuiteKukaAllegroPPORunnerCfg", |
There was a problem hiding this comment.
Missing classes will cause ImportError at registration time
The gym registrations reference four classes from dexsuite_kuka_allegro_vision_env_cfg that do not exist:
DexsuiteKukaAllegroLiftSingleCameraEnvCfg_PLAY— class definition is commented out in vision_env_cfg (line 248)DexsuiteKukaAllegroReorientSingleCameraEnvCfg— never defined anywhereDexsuiteKukaAllegroReorientSingleCameraEnvCfg_PLAY— never defined anywhere
Attempting to create any of these gym environments (e.g., Isaac-Dexsuite-Kuka-Allegro-Lift-Single-Camera-Play-v0) will fail with an AttributeError/ImportError at runtime.
| self._save_dir = "/tmp/newton_renders" | ||
| if os.path.exists(self._save_dir): | ||
| shutil.rmtree(self._save_dir) | ||
| os.makedirs(self._save_dir, exist_ok=True) |
There was a problem hiding this comment.
Hardcoded /tmp path and unconditional rmtree on construction
This constructor unconditionally deletes /tmp/newton_renders via shutil.rmtree every time a NewtonWarpRenderer is instantiated. If multiple renderers are created (e.g. multi-camera setups), the second instance will wipe the first's saved images. This is also fragile in multi-process scenarios.
Consider making the save directory configurable (e.g. via cfg) and/or appending a unique suffix (PID, timestamp) to avoid collisions.
| # -- step interval events | ||
| if "interval" in self.event_manager.available_modes: | ||
| self.event_manager.apply(mode="interval", dt=self.step_dt) | ||
| # -- compute observations | ||
| # -- compute observations (includes camera/tiled camera rendering) | ||
| # note: done after reset to get the correct observations for reset envs | ||
| self.obs_buf = self.observation_manager.compute(update_history=True) | ||
| if self.common_step_counter <= 3 or self.common_step_counter % 50 == 0: | ||
| msg = ( | ||
| f"[PERF][manager_based_rl_env] Computing observations (camera/tiled render) " | ||
| f"step #{self.common_step_counter}..." |
There was a problem hiding this comment.
Debug print statements in hot loop for all users
The conditional print(msg, flush=True) executes for all users on steps 1-3 and every 50th step, regardless of whether Newton Warp is being used. This adds unwanted console noise for all training runs. Consider using logger.debug() or gating behind a verbosity/debug flag.
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!
|
|
||
| @classmethod | ||
| def _body_path_to_newton_idx_lookup(cls, body_path: str, root_path: str, body_name: str) -> int: | ||
| """Resolve Newton body index: try exact path, then match body_key by path components.""" | ||
| idx = cls._body_path_to_newton_idx.get(body_path, -1) | ||
| if idx >= 0: | ||
| return idx | ||
| # Newton's body_key may use different path format; match by root + body_name as last component | ||
| suffix = "/" + body_name |
There was a problem hiding this comment.
Debug logger.warning calls left in production code
The _build_physx_to_newton_mapping method uses logger.warning for "DEBUG" messages (e.g., "[NewtonManager] DEBUG sample Newton body_key (first 15)"). These are development debugging aids that will appear in production logs for all users. Use logger.debug instead of logger.warning for these, or remove them.
| @classmethod | |
| def _body_path_to_newton_idx_lookup(cls, body_path: str, root_path: str, body_name: str) -> int: | |
| """Resolve Newton body index: try exact path, then match body_key by path components.""" | |
| idx = cls._body_path_to_newton_idx.get(body_path, -1) | |
| if idx >= 0: | |
| return idx | |
| # Newton's body_key may use different path format; match by root + body_name as last component | |
| suffix = "/" + body_name | |
| if not _debug_done: | |
| newton_keys = list(cls._body_path_to_newton_idx.keys()) | |
| logger.debug("[NewtonManager] sample Newton body_key (first 15): %s", newton_keys[:15]) | |
| if len(newton_keys) > 20: | |
| logger.debug("[NewtonManager] Newton body_key (last 5): %s", newton_keys[-5:]) |
| if not hasattr(NewtonManager, "_is_initialized") or not NewtonManager._is_initialized: | ||
| device_str = str(self.device).replace("cuda:", "cuda:") | ||
| NewtonManager.initialize(num_envs=self._num_envs, device=device_str) |
There was a problem hiding this comment.
No-op string replace for device conversion
str(self.device).replace("cuda:", "cuda:") replaces "cuda:" with the identical string "cuda:", making this a no-op. If this was intended to strip or transform the device string in some way, the logic is incorrect. Otherwise, it should just be str(self.device).
| if not hasattr(NewtonManager, "_is_initialized") or not NewtonManager._is_initialized: | |
| device_str = str(self.device).replace("cuda:", "cuda:") | |
| NewtonManager.initialize(num_envs=self._num_envs, device=device_str) | |
| device_str = str(self.device) |
| try: | ||
| _repo_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", "..")) | ||
| _save_script = os.path.join(_repo_root, "scripts", "save_run_artifacts.sh") | ||
| _dest = os.path.join(_repo_root, "run_artifacts", datetime.now().strftime("%Y-%m-%d_%H-%M-%S")) | ||
| os.makedirs(_dest, exist_ok=True) | ||
| if _timing_lines is not None: | ||
| _timing_file = os.path.join(_dest, "timing_summary.txt") | ||
| with open(_timing_file, "w") as f: | ||
| f.write("Training time: " + str(round(time.time() - start_time, 2)) + " seconds\n") | ||
| f.write("[Timing summary]\n") | ||
| f.write("\n".join(_timing_lines) + "\n") | ||
| if os.path.isfile(_save_script): | ||
| import subprocess | ||
|
|
There was a problem hiding this comment.
References non-existent save_run_artifacts.sh script
The code references scripts/save_run_artifacts.sh but this file doesn't exist in the repository. When the script isn't found, the os.path.isfile check prevents a crash, but the timing summary file and os.makedirs side-effect still occur, and the "Run artifacts saved" message is never printed, which may confuse users. Either add the script to the PR or remove this dead code path.
source/isaaclab_rl/setup.py
Outdated
| "gym", | ||
| ], # rl-games still needs gym :( | ||
| "rsl-rl": ["rsl-rl-lib==3.1.2", "onnxscript>=0.5"], # linux aarch 64 requires manual onnxscript installation | ||
| "rsl-rl": ["rsl-rl-lib==3.3.0", "onnxscript>=0.5"], # linux aarch 64 requires manual onnxscript installation |
There was a problem hiding this comment.
Major version bump of rsl-rl-lib (3.1.2 → 3.3.0)
This bumps rsl-rl-lib from 3.1.2 to 3.3.0. This is a significant version jump that could introduce breaking API changes (e.g., the new ActorCriticCNN class used by RslRlActorCriticCNNCfg). Make sure this is intentional and that all existing RSL-RL training configurations remain compatible with v3.3.0. Is the bump to rsl-rl-lib==3.3.0 intentional and have existing training configs been validated against the new version?
source/isaaclab/setup.py
Outdated
| "junitparser", | ||
| "coverage==7.6.1", | ||
| "flatdict==4.0.1", | ||
| "flatdict>=3.4.0", |
There was a problem hiding this comment.
Loosened dependency constraint for flatdict
Changing flatdict==4.0.1 to flatdict>=3.4.0 allows any version from 3.4.0 upward, including potential future versions with breaking changes. Unless there's a specific need for an older version, a safer pattern would be flatdict>=3.4.0,<5 to at least pin the major version.
Additional Comments (1)
The
This instrumentation should be removed or gated behind a debug flag / logging level before merge. |
| self._data_types = [] | ||
|
|
||
| # output buffer format: dict mapping data type -> list of buffers (one per camera) | ||
| # TODO: Document the standard format of the output data buffers. Need discussion. |
There was a problem hiding this comment.
TODOs in this file originate from https://github.com/ooctipus/IsaacLab/blob/newton/dexsuite_warp_rendering/source/isaaclab/isaaclab/renderer/renderer.py#L26
We can disregard unless directed otherwise.
This commit adds the foundation for Newton Warp rendering support:
Completed NewtonManager implementation:
Implemented full state synchronization:
Major achievements:
78e849a to
88004ce
Compare
docs/source/features/hydra.rst
Outdated
| In the above example, we could also disable the ``joint_pos_rel`` observation by setting it to None with | ||
| ``env.observations.policy.joint_pos_rel=null``. | ||
|
|
||
| TiledCamera renderer type |
There was a problem hiding this comment.
I wouldn't put that in documentation yet. We need to do some refactoring in current "architecture". At this moment Camera "owns" the renderer. This is not a long term solution. Camera shouldn't own the render instance.
| from typing import TYPE_CHECKING, Any, Union | ||
|
|
||
| from .base_renderer import BaseRenderer | ||
| from .isaac_rtx_renderer_cfg import IsaacRtxRendererCfg |
There was a problem hiding this comment.
This seems incorrect, because this violates the dependency graph:
Renderers <- Renderer Implementation
What you are doing here, is introducing circular dependency because IsaacRTXRendererCfg is an implementation of a renderer.
| from .renderer_cfg import RendererCfg | ||
|
|
||
|
|
||
| def renderer_cfg_from_type(renderer_type: str | None) -> RendererCfg: |
There was a problem hiding this comment.
I think this is actually a workaround for something that is missing in Renderer factory. Have a look in renderer.py factory in the same directory and that's the place where you should be adding this feature.
Also keep this impl agnostic.
| name = name_or_cfg | ||
| if name in ("newton_warp", "warp_renderer"): | ||
| from isaaclab_newton.renderers import NewtonWarpRenderer | ||
| return NewtonWarpRenderer |
There was a problem hiding this comment.
Those are implementation details! Use factory for this!
| from isaaclab.sensors import SensorBase | ||
|
|
||
|
|
||
| class Renderer: |
There was a problem hiding this comment.
Why do we need yet another renderer class?
… newton_warp tweaks
… and dict updates
TODO: Changelog, testing
Description
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there