add isaaclab bimanual arm example#548
Open
yizhouzhao-nvidia wants to merge 1 commit into
Open
Conversation
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a complete bimanual robot teleoperation example for Isaac Lab. It provides environment configurations for two robot arms (UR10 and Flexiv Rizon), an OpenXR retargeting system that maps controller poses to robot end-effectors, differential inverse kinematics controllers with multiple solver methods, and teleoperation scripts for XR-based control and dataset recording. The implementation includes a shared base environment configuration that both robots inherit from, per-robot specializations (joint setup, actuator configs), and executable scripts for testing and interactive control. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/isaaclab_bimanual_robot/scripts/record_se3_agent_bimanual_xr.py`:
- Around line 192-233: The START/STOP callbacks toggle teleoperation_active (via
start_teleoperation and stop_teleoperation) but the main control path never
checks it before computing/applying actions, so STOP doesn't disable control;
update the section that computes/applies actions (the main loop block where
actions are generated and sent to the env/robot) to first check
teleoperation_active and skip or no-op the action computation/application when
it's False (e.g., return/continue or set zeroed actions), referencing
teleoperation_active, start_teleoperation, stop_teleoperation and the
action-application logic so teleoperation truly pauses when deactivated.
- Around line 144-150: The current code computes output_dir =
os.path.dirname(args_cli.dataset_file) which can be an empty string for
filenames like "name.hdf5", causing os.makedirs("") to fail; update the logic
around output_dir/output_file_name to handle that case by checking that
output_dir is non-empty (or defaulting it to "." ) before calling
os.path.exists/os.makedirs, e.g. only attempt to create the directory when
output_dir is truthy and not already present, and continue using
output_file_name as before.
- Around line 63-65: The current logic only enables XR when
args_cli.teleop_device.lower() contains "handtracking", which misses other
OpenXR device keys like "motion_controllers"; change the condition that sets
app_launcher_args["xr"] to check for membership in a set/list of supported
OpenXR device keys (e.g. {"handtracking", "motion_controllers", ...}) using
args_cli.teleop_device.lower() and set app_launcher_args["xr"]=True when it
matches; update the check around the args_cli.teleop_device /
app_launcher_args["xr"] lines so all configured OpenXR devices enable XR
initialization.
In `@examples/isaaclab_bimanual_robot/scripts/teleop_se3_agent_bimanual_xr.py`:
- Around line 160-200: The callbacks start_teleoperation/stop_teleoperation
correctly flip the teleoperation_active flag but the main control loop still
computes and applies actions unconditionally; update the loop that
computes/applies actions (where teleoperation actions are generated and env.step
is called) to check teleoperation_active from
teleoperation_callbacks/teleoperation_active and only compute and send
teleoperation commands when teleoperation_active is True, otherwise skip
applying actions (or send explicit no-op/zero commands) so the START/STOP
callbacks actually enable/disable teleoperation; ensure any places that compute
actions (the block referenced near teleoperation_callbacks and the similar block
around the other occurrence) are both guarded by this check.
- Around line 147-158: reset_recording_instance() only sets
should_reset_recording_instance = True but the main runtime loop never reads
that flag, so the callback is ineffective; update the main control loop (the
section that currently performs the button-slice reset logic) to check
should_reset_recording_instance each iteration and call the same reset routine
used by the button handlers (or invoke the existing reset code path directly)
when the flag is true, then clear the flag; apply the same change for the other
reset_recording_instance occurrence so both callback paths trigger the actual
environment reset logic.
- Around line 58-60: The XR enable condition currently only checks
args_cli.teleop_device for "handtracking" so app_launcher_args["xr"] can remain
False for OpenXR keys; update the conditional around args_cli.teleop_device (the
block that sets app_launcher_args["xr"]) to also check for the OpenXR key by
using args_cli.teleop_device.lower() and enabling XR when it contains either
"handtracking" or "motion_controllers" so tracking data is populated for both
device keys.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/dummy_retargeter.py`:
- Around line 44-47: The retarget method currently does nothing and returns
None, breaking callers that expect a torch.Tensor; update
dummy_retargeter.Retargeter.retarget (the retarget method) to fail fast by
raising a NotImplementedError (with a short message like "dummy retargeter not
implemented") instead of using pass so callers get a clear, immediate error
rather than a None value; ensure the method signature remains the same and that
any tests expecting an explicit tensor are adjusted to handle the
NotImplementedError if necessary.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/retargeter.py`:
- Around line 64-87: compute_action() currently assumes initialization tensors
(e.g., ctrl_left_pos_init, ctrl_left_quat_init, ee_left_pos_init,
ee_left_quat_init, ctrl_right_pos_init, ctrl_right_quat_init, ee_right_pos_init,
ee_right_quat_init, robot_left_quat_w, robot_right_quat_w) exist and will crash
if reset() hasn't run; add a guard at the start of compute_action() that
verifies these attributes (or a single readiness flag) are present and valid,
and if not either call self.reset() to populate them or raise a clear
RuntimeError instructing to call reset() first. Ensure the guard is used before
calling self._delta_action(...) for left and right so the subsequent calls
cannot access uninitialized tensors.
- Around line 10-13: BimanualOpenXRRetargeter currently breaks the
RetargeterBase/RetargeterCfg factory contract: change its constructor and API to
match the cfg-based pattern (or remove the cfg type). Specifically, update
BimanualOpenXRRetargeter.__init__ to accept a single cfg object (like
DummyRetargeter(cfg)) or update BimanualOpenXRRetargeterCfg so it correctly
constructs the class with (env, device); implement retarget(self, data: dict) on
BimanualOpenXRRetargeter to delegate to the existing reset/compute_action logic
(or make reset/compute_action internal and call them from retarget) so the
standard retarget(data: dict) signature is honored; ensure any factory usage
that expects RetargeterCfg can instantiate BimanualOpenXRRetargeter without
signature errors and update call sites to call retarget(data) instead of the
custom methods.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/flexiv_rizon_robots.py`:
- Around line 26-31: ASSET_URL currently points to a mutable branch ref
("refs/heads/main"); update ASSET_URL to reference an immutable commit SHA
(replace the branch ref in the URL with the specific commit hash) and update
FLEXIV_RIZON_4S_CFG / sim_utils.UsdFileCfg usage to verify integrity (e.g., add
or populate a checksum or expected_hash field, or implement a checksum
verification step after download) so the USD is pinned and validated; ensure the
new URL points to the specific commit and the checksum matches that commit.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.py`:
- Around line 165-167: Guard access to self.cfg.joint_weights before calling
.to(): instead of unconditionally doing "joint_weights =
self.cfg.joint_weights.to(self._device)", first check "if self.cfg.joint_weights
is not None" and only then call ".to(self._device)" and downstream
".float().pow(-0.5)"; otherwise set joint_weights = None so later "if
joint_weights is not None" remains valid. This change should be applied in
differential_ik.py where joint_weights and scale are computed.
- Around line 222-229: The pseudoinverse computation hardcodes a 6D task by
slicing Vh with :6, which breaks position-only Jacobians; derive the actual task
dimension from the SVD result (e.g. task_dim = S.shape[1]) and replace the
hardcoded slice with one using task_dim so the transpose of Vh is truncated to
the number of singular values (use task_dim when selecting columns from
torch.transpose(Vh, ...) so jacobian_pinv = torch.transpose(Vh, ..., ...)[:, :,
:task_dim] @ torch.diag_embed(S_inv) @ torch.transpose(U, ..., ...)); keep the
existing min_singular_value logic and S_inv shaping unchanged.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/task_space_actions.py`:
- Around line 190-191: The IO_descriptor population unconditionally dereferences
self.cfg.body_offset.__dict__ (and similarly at the other occurrence around
lines 522-523), causing AttributeError when body_offset is None; change the
assignments that set self._IO_descriptor.extras["body_offset"] to first check if
self.cfg.body_offset is truthy (or hasattr(self.cfg, "body_offset") and
self.cfg.body_offset is not None) and only use self.cfg.body_offset.__dict__ in
that case, otherwise set extras["body_offset"] to None or an empty dict; do the
same defensively for controller if needed (e.g., when setting
extras["controller_cfg"]) so both IO_descriptor builders safely handle missing
optional config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 119db1ee-9a1c-40d8-83f6-0cce852d98cd
⛔ Files ignored due to path filters (3)
examples/isaaclab_bimanual_robot/images/bimanual_flexiv.pngis excluded by!**/*.pngexamples/isaaclab_bimanual_robot/images/bimanual_ur.pngis excluded by!**/*.pngexamples/isaaclab_bimanual_robot/images/start_xr.pngis excluded by!**/*.png
📒 Files selected for processing (22)
examples/isaaclab_bimanual_robot/Readme.mdexamples/isaaclab_bimanual_robot/datasets/.gitignoreexamples/isaaclab_bimanual_robot/pyproject.tomlexamples/isaaclab_bimanual_robot/scripts/list_envs.pyexamples/isaaclab_bimanual_robot/scripts/record_se3_agent_bimanual_xr.pyexamples/isaaclab_bimanual_robot/scripts/teleop_se3_agent_bimanual_xr.pyexamples/isaaclab_bimanual_robot/scripts/zero_agent.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/__init__.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/__init__.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/base_env_cfg.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/dummy_retargeter.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/retargeter.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/__init__.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/flexiv_rizon_bimanual_env.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/flexiv_rizon_robots.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/__init__.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik_cfg.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/task_space_actions.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/ur10/__init__.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/ur10/universal_robots.pyexamples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/ur10/ur10_bimanual_env.py
Comment on lines
+63
to
+65
| if "handtracking" in args_cli.teleop_device.lower(): | ||
| app_launcher_args["xr"] = True | ||
|
|
Contributor
There was a problem hiding this comment.
Enable XR for configured OpenXR device keys, not just "handtracking" substrings.
Line 63 only turns on XR when --teleop_device contains "handtracking". With this PR’s env config using motion_controllers, XR can stay disabled and controller tracking never initializes.
Suggested fix
-if "handtracking" in args_cli.teleop_device.lower():
+if args_cli.teleop_device.lower() in {"handtracking", "motion_controllers"}:
app_launcher_args["xr"] = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "handtracking" in args_cli.teleop_device.lower(): | |
| app_launcher_args["xr"] = True | |
| if args_cli.teleop_device.lower() in {"handtracking", "motion_controllers"}: | |
| app_launcher_args["xr"] = True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/isaaclab_bimanual_robot/scripts/record_se3_agent_bimanual_xr.py`
around lines 63 - 65, The current logic only enables XR when
args_cli.teleop_device.lower() contains "handtracking", which misses other
OpenXR device keys like "motion_controllers"; change the condition that sets
app_launcher_args["xr"] to check for membership in a set/list of supported
OpenXR device keys (e.g. {"handtracking", "motion_controllers", ...}) using
args_cli.teleop_device.lower() and set app_launcher_args["xr"]=True when it
matches; update the check around the args_cli.teleop_device /
app_launcher_args["xr"] lines so all configured OpenXR devices enable XR
initialization.
Comment on lines
+144
to
+150
| output_dir = os.path.dirname(args_cli.dataset_file) | ||
| output_file_name = os.path.splitext(os.path.basename(args_cli.dataset_file))[0] | ||
|
|
||
| # create directory if it does not exist | ||
| if not os.path.exists(output_dir): | ||
| os.makedirs(output_dir) | ||
| print(f"Created output directory: {output_dir}") |
Contributor
There was a problem hiding this comment.
Handle dataset paths without a directory component.
If --dataset_file is passed as just name.hdf5, output_dir becomes empty string and os.makedirs("") fails.
Suggested fix
output_dir = os.path.dirname(args_cli.dataset_file)
output_file_name = os.path.splitext(os.path.basename(args_cli.dataset_file))[0]
+ if output_dir == "":
+ output_dir = "."
# create directory if it does not exist
if not os.path.exists(output_dir):
os.makedirs(output_dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| output_dir = os.path.dirname(args_cli.dataset_file) | |
| output_file_name = os.path.splitext(os.path.basename(args_cli.dataset_file))[0] | |
| # create directory if it does not exist | |
| if not os.path.exists(output_dir): | |
| os.makedirs(output_dir) | |
| print(f"Created output directory: {output_dir}") | |
| output_dir = os.path.dirname(args_cli.dataset_file) | |
| output_file_name = os.path.splitext(os.path.basename(args_cli.dataset_file))[0] | |
| if output_dir == "": | |
| output_dir = "." | |
| # create directory if it does not exist | |
| if not os.path.exists(output_dir): | |
| os.makedirs(output_dir) | |
| print(f"Created output directory: {output_dir}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/isaaclab_bimanual_robot/scripts/record_se3_agent_bimanual_xr.py`
around lines 144 - 150, The current code computes output_dir =
os.path.dirname(args_cli.dataset_file) which can be an empty string for
filenames like "name.hdf5", causing os.makedirs("") to fail; update the logic
around output_dir/output_file_name to handle that case by checking that
output_dir is non-empty (or defaulting it to "." ) before calling
os.path.exists/os.makedirs, e.g. only attempt to create the directory when
output_dir is truthy and not already present, and continue using
output_file_name as before.
Comment on lines
+192
to
+233
| def start_teleoperation() -> None: | ||
| """ | ||
| Activate teleoperation control of the robot. | ||
|
|
||
| Enables the application of teleoperation commands to the environment. | ||
|
|
||
| Returns: | ||
| None | ||
| """ | ||
| nonlocal teleoperation_active | ||
| teleoperation_active = True | ||
| print("Teleoperation activated") | ||
|
|
||
| def stop_teleoperation() -> None: | ||
| """ | ||
| Deactivate teleoperation control of the robot. | ||
|
|
||
| Disables the application of teleoperation commands to the environment. | ||
|
|
||
| Returns: | ||
| None | ||
| """ | ||
| nonlocal teleoperation_active | ||
| teleoperation_active = False | ||
| print("Teleoperation deactivated") | ||
|
|
||
| # Create device config if not already in env_cfg | ||
| teleoperation_callbacks: dict[str, Callable[[], None]] = { | ||
| "R": reset_recording_instance, | ||
| "START": start_teleoperation, | ||
| "STOP": stop_teleoperation, | ||
| "RESET": reset_recording_instance, | ||
| } | ||
|
|
||
| # For hand tracking devices, add additional callbacks | ||
| if args_cli.xr: | ||
| # Default to inactive for hand tracking | ||
| teleoperation_active = False | ||
| else: | ||
| # Always active for other devices | ||
| teleoperation_active = True | ||
|
|
Contributor
There was a problem hiding this comment.
START/STOP callbacks are currently ineffective.
teleoperation_active is toggled in callbacks (Line 192, Line 205) but never checked before computing/applying actions (Line 353 onward), so stop does not stop control.
Suggested fix
if not start_button_pressed:
env.sim.render()
continue
+
+ if not teleoperation_active:
+ env.sim.render()
+ continueAlso applies to: 353-365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/isaaclab_bimanual_robot/scripts/record_se3_agent_bimanual_xr.py`
around lines 192 - 233, The START/STOP callbacks toggle teleoperation_active
(via start_teleoperation and stop_teleoperation) but the main control path never
checks it before computing/applying actions, so STOP doesn't disable control;
update the section that computes/applies actions (the main loop block where
actions are generated and sent to the env/robot) to first check
teleoperation_active and skip or no-op the action computation/application when
it's False (e.g., return/continue or set zeroed actions), referencing
teleoperation_active, start_teleoperation, stop_teleoperation and the
action-application logic so teleoperation truly pauses when deactivated.
Comment on lines
+58
to
+60
| if "handtracking" in args_cli.teleop_device.lower(): | ||
| app_launcher_args["xr"] = True | ||
|
|
Contributor
There was a problem hiding this comment.
XR activation condition misses motion_controllers.
Line 58 only enables XR when the device name contains "handtracking". For this PR’s OpenXR device key (motion_controllers), XR may stay off and tracking data stays empty.
Suggested fix
-if "handtracking" in args_cli.teleop_device.lower():
+if args_cli.teleop_device.lower() in {"handtracking", "motion_controllers"}:
app_launcher_args["xr"] = True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/isaaclab_bimanual_robot/scripts/teleop_se3_agent_bimanual_xr.py`
around lines 58 - 60, The XR enable condition currently only checks
args_cli.teleop_device for "handtracking" so app_launcher_args["xr"] can remain
False for OpenXR keys; update the conditional around args_cli.teleop_device (the
block that sets app_launcher_args["xr"]) to also check for the OpenXR key by
using args_cli.teleop_device.lower() and enabling XR when it contains either
"handtracking" or "motion_controllers" so tracking data is populated for both
device keys.
Comment on lines
+147
to
+158
| def reset_recording_instance() -> None: | ||
| """ | ||
| Reset the environment to its initial state. | ||
|
|
||
| Sets a flag to reset the environment on the next simulation step. | ||
|
|
||
| Returns: | ||
| None | ||
| """ | ||
| nonlocal should_reset_recording_instance | ||
| should_reset_recording_instance = True | ||
| print("Reset triggered - Environment will reset on next step") |
Contributor
There was a problem hiding this comment.
Reset callback path is disconnected from runtime reset logic.
reset_recording_instance() sets should_reset_recording_instance = True, but the loop never checks that flag. Only button slice checks trigger reset, so callback-based reset is broken.
Suggested fix
- # press x or y to reset
- if np.sum(left_controller_data[1][4:6]) > 0:
+ # press x or y to reset (or callback-triggered reset)
+ if should_reset_recording_instance or np.sum(left_controller_data[1][4:6]) > 0:
# reset environment
env.reset()
teleop_interface.reset()
@@
start_button_pressed = False
retargeter._initialized = False
+ should_reset_recording_instance = FalseAlso applies to: 324-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/isaaclab_bimanual_robot/scripts/teleop_se3_agent_bimanual_xr.py`
around lines 147 - 158, reset_recording_instance() only sets
should_reset_recording_instance = True but the main runtime loop never reads
that flag, so the callback is ineffective; update the main control loop (the
section that currently performs the button-slice reset logic) to check
should_reset_recording_instance each iteration and call the same reset routine
used by the button handlers (or invoke the existing reset code path directly)
when the flag is true, then clear the flag; apply the same change for the other
reset_recording_instance occurrence so both callback paths trigger the actual
environment reset logic.
Comment on lines
+64
to
+87
| has_gripper = gripper_left is not None and gripper_right is not None | ||
|
|
||
| left = self._delta_action( | ||
| ctrl_left_pos_w, | ||
| ctrl_left_quat_w, | ||
| self.ctrl_left_pos_init, | ||
| self.ctrl_left_quat_init, | ||
| self.ee_left_pos_init, | ||
| self.ee_left_quat_init, | ||
| self.robot_left_quat_w, | ||
| scale, | ||
| reverse_yz, | ||
| ) | ||
| right = self._delta_action( | ||
| ctrl_right_pos_w, | ||
| ctrl_right_quat_w, | ||
| self.ctrl_right_pos_init, | ||
| self.ctrl_right_quat_init, | ||
| self.ee_right_pos_init, | ||
| self.ee_right_quat_init, | ||
| self.robot_right_quat_w, | ||
| scale, | ||
| reverse_yz, | ||
| ) |
Contributor
There was a problem hiding this comment.
compute_action() can crash if called before reset().
The method reads cached init tensors unconditionally; if reset() has not run, this path raises attribute errors.
Proposed guard
def compute_action(
self,
ctrl_left_pos_w: torch.Tensor, # (3,) current left controller pos in world
@@
) -> torch.Tensor:
"""Returns action tensor of shape (1, 14) = [left_7D, right_7D]."""
+ if not self._initialized:
+ raise RuntimeError("BimanualOpenXRRetargeter.reset() must be called before compute_action().")
has_gripper = gripper_left is not None and gripper_right is not None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/common/retargeter.py`
around lines 64 - 87, compute_action() currently assumes initialization tensors
(e.g., ctrl_left_pos_init, ctrl_left_quat_init, ee_left_pos_init,
ee_left_quat_init, ctrl_right_pos_init, ctrl_right_quat_init, ee_right_pos_init,
ee_right_quat_init, robot_left_quat_w, robot_right_quat_w) exist and will crash
if reset() hasn't run; add a guard at the start of compute_action() that
verifies these attributes (or a single readiness flag) are present and valid,
and if not either call self.reset() to populate them or raise a clear
RuntimeError instructing to call reset() first. Ensure the guard is used before
calling self._delta_action(...) for left and right so the subsequent calls
cannot access uninitialized tensors.
Comment on lines
+26
to
+31
| ASSET_URL = "https://github.com/flexivrobotics/isaac_sim_ws/raw/refs/heads/main/exts/isaacsim.robot.manipulators.examples/data/flexiv/Rizon4s_with_Grav.usd" | ||
| # ASSET_URL = "/home/linfan/Downloads/Rizon4s_with_Grav_without_gripper.usd" | ||
|
|
||
| FLEXIV_RIZON_4S_CFG = ArticulationCfg( | ||
| spawn=sim_utils.UsdFileCfg( | ||
| usd_path=ASSET_URL, |
Contributor
There was a problem hiding this comment.
Asset URL is mutable (refs/heads/main) and introduces supply-chain risk
Loading a USD from a moving branch head makes runs non-reproducible and allows silent upstream changes. Pin to an immutable commit URL (and ideally verify checksum).
Proposed fix
-ASSET_URL = "https://github.com/flexivrobotics/isaac_sim_ws/raw/refs/heads/main/exts/isaacsim.robot.manipulators.examples/data/flexiv/Rizon4s_with_Grav.usd"
+ASSET_URL = "https://raw.githubusercontent.com/flexivrobotics/isaac_sim_ws/<pinned-commit-sha>/exts/isaacsim.robot.manipulators.examples/data/flexiv/Rizon4s_with_Grav.usd"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/flexiv_rizon_robots.py`
around lines 26 - 31, ASSET_URL currently points to a mutable branch ref
("refs/heads/main"); update ASSET_URL to reference an immutable commit SHA
(replace the branch ref in the URL with the specific commit hash) and update
FLEXIV_RIZON_4S_CFG / sim_utils.UsdFileCfg usage to verify integrity (e.g., add
or populate a checksum or expected_hash field, or implement a checksum
verification step after download) so the USD is pinned and validated; ensure the
new URL points to the specific commit and the checksum matches that commit.
Comment on lines
+165
to
+167
| joint_weights = self.cfg.joint_weights.to(self._device) | ||
| if joint_weights is not None: | ||
| scale = joint_weights.float().pow(-0.5) # (num_joints,) |
Contributor
There was a problem hiding this comment.
Guard joint_weights before calling .to()
joint_weights is optional in config. Calling .to() unconditionally will raise when it is None.
Proposed fix
- joint_weights = self.cfg.joint_weights.to(self._device)
- if joint_weights is not None:
+ joint_weights = self.cfg.joint_weights
+ if joint_weights is not None:
+ joint_weights = joint_weights.to(self._device)
scale = joint_weights.float().pow(-0.5) # (num_joints,)
jacobian = jacobian * scale[None, None, :]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| joint_weights = self.cfg.joint_weights.to(self._device) | |
| if joint_weights is not None: | |
| scale = joint_weights.float().pow(-0.5) # (num_joints,) | |
| joint_weights = self.cfg.joint_weights | |
| if joint_weights is not None: | |
| joint_weights = joint_weights.to(self._device) | |
| scale = joint_weights.float().pow(-0.5) # (num_joints,) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.py`
around lines 165 - 167, Guard access to self.cfg.joint_weights before calling
.to(): instead of unconditionally doing "joint_weights =
self.cfg.joint_weights.to(self._device)", first check "if self.cfg.joint_weights
is not None" and only then call ".to(self._device)" and downstream
".float().pow(-0.5)"; otherwise set joint_weights = None so later "if
joint_weights is not None" remains valid. This change should be applied in
differential_ik.py where joint_weights and scale are computed.
Comment on lines
+222
to
+229
| U, S, Vh = torch.linalg.svd(jacobian) | ||
| S_inv = 1.0 / S | ||
| S_inv = torch.where(min_singular_value < S, S_inv, torch.zeros_like(S_inv)) | ||
| jacobian_pinv = ( | ||
| torch.transpose(Vh, dim0=1, dim1=2)[:, :, :6] | ||
| @ torch.diag_embed(S_inv) | ||
| @ torch.transpose(U, dim0=1, dim1=2) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "differential_ik.py" -path "*/flexiv_rizon/ik/*"Repository: NVIDIA/IsaacTeleop
Length of output: 168
🏁 Script executed:
cat -n examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.py | sed -n '200,250p'Repository: NVIDIA/IsaacTeleop
Length of output: 3064
🏁 Script executed:
cat -n examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.py | head -50Repository: NVIDIA/IsaacTeleop
Length of output: 2523
SVD pseudoinverse hardcodes 6D task and breaks position-mode IK
Line 226 hardcodes :6 slice, but the Jacobian can be (N, 3, num_joints) for position-only commands or (N, 6, num_joints) for full 6D pose. The slice should use the actual task dimension derived from the SVD result.
Replace the hardcoded slice with:
U, S, Vh = torch.linalg.svd(jacobian)
S_inv = 1.0 / S
S_inv = torch.where(min_singular_value < S, S_inv, torch.zeros_like(S_inv))
+ task_dim = S.shape[-1]
jacobian_pinv = (
- torch.transpose(Vh, dim0=1, dim1=2)[:, :, :6]
+ torch.transpose(Vh, dim0=1, dim1=2)[:, :, :task_dim]
@ torch.diag_embed(S_inv)
@ torch.transpose(U, dim0=1, dim1=2)
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/differential_ik.py`
around lines 222 - 229, The pseudoinverse computation hardcodes a 6D task by
slicing Vh with :6, which breaks position-only Jacobians; derive the actual task
dimension from the SVD result (e.g. task_dim = S.shape[1]) and replace the
hardcoded slice with one using task_dim so the transpose of Vh is truncated to
the number of singular values (use task_dim when selecting columns from
torch.transpose(Vh, ...) so jacobian_pinv = torch.transpose(Vh, ..., ...)[:, :,
:task_dim] @ torch.diag_embed(S_inv) @ torch.transpose(U, ..., ...)); keep the
existing min_singular_value logic and S_inv shaping unchanged.
Comment on lines
+190
to
+191
| self._IO_descriptor.extras["controller_cfg"] = self.cfg.controller.__dict__ | ||
| self._IO_descriptor.extras["body_offset"] = self.cfg.body_offset.__dict__ |
Contributor
There was a problem hiding this comment.
IO_descriptor crashes when body_offset is not configured
body_offset is optional, but both descriptor builders unconditionally dereference __dict__. This can raise AttributeError.
Proposed fix
- self._IO_descriptor.extras["body_offset"] = self.cfg.body_offset.__dict__
+ self._IO_descriptor.extras["body_offset"] = (
+ self.cfg.body_offset.__dict__ if self.cfg.body_offset is not None else None
+ )- self._IO_descriptor.extras["body_offset"] = self.cfg.body_offset.__dict__
+ self._IO_descriptor.extras["body_offset"] = (
+ self.cfg.body_offset.__dict__ if self.cfg.body_offset is not None else None
+ )Also applies to: 522-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/isaaclab_bimanual_robot/source/bimanual/tasks/manager_based/flexiv_rizon/ik/task_space_actions.py`
around lines 190 - 191, The IO_descriptor population unconditionally
dereferences self.cfg.body_offset.__dict__ (and similarly at the other
occurrence around lines 522-523), causing AttributeError when body_offset is
None; change the assignments that set self._IO_descriptor.extras["body_offset"]
to first check if self.cfg.body_offset is truthy (or hasattr(self.cfg,
"body_offset") and self.cfg.body_offset is not None) and only use
self.cfg.body_offset.__dict__ in that case, otherwise set extras["body_offset"]
to None or an empty dict; do the same defensively for controller if needed
(e.g., when setting extras["controller_cfg"]) so both IO_descriptor builders
safely handle missing optional config.
jiwenc-nv
reviewed
May 26, 2026
| ] | ||
|
|
||
| typeCheckingMode = "basic" | ||
| pythonVersion = "3.11" |
Collaborator
There was a problem hiding this comment.
@yizhouzhao-nvidia IIRC, this this still using Isaac Lab 2.3? So this example is actually using the legacy CloudXR path in Isaac Lab rather than the new Isaac Teleop path.
For the simple XR controller path this is probably find and we can actually land this as-is and then make a follow up PR to upgrade this to Isaac Lab 3.0. That PR can be used as a follow up example as well.
jiwenc-nv
approved these changes
May 28, 2026
jiwenc-nv
approved these changes
May 28, 2026
| @@ -0,0 +1,145 @@ | |||
| <!-- | |||
Collaborator
There was a problem hiding this comment.
nit: please rename to README.md so that it's consistent with the rest of the code base. thanks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a directory under the examples folder: ./examples/isaaclab_bimanual_robot
Contains minimal usage examples for teleoperating bimanual robot arms in IsaacLab.
Assets are imported directly from web public urls and don't include any other information
Summary by CodeRabbit
New Features
Documentation