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 44 commits intoisaac-sim:developfrom
Conversation
This commit adds the foundation for Newton Warp rendering support:
Completed NewtonManager implementation:
Implemented full state synchronization:
Major achievements:
Major achievements:
Added comprehensive documentation and tools:
…v; fix tiled_camera init
| @@ -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.
| "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. |
…and Warp renderer
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: bdilinila <148156773+bdilinila@users.noreply.github.com>
…Lab into dev/bdilinila/physx-warp
| 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.
TODO: Changelog, testing
Description
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there