Add G1 Wuji teleop example#699
Open
supinelay wants to merge 1 commit into
Open
Conversation
Signed-off-by: lei.hu <lei.hu@git.lightwheel.ai>
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a complete G1-Wuji AVP+MANUS teleoperation example for Isaac Teleop. It adds a new Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 6
🤖 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 `@avp_manus.sh`:
- Around line 170-172: The final demo launch command uses an invalid config path
that will not resolve from a normal repo checkout. Update the config argument in
the main teleop command in avp_man.sh to use a repo-relative path or the correct
full IsaacTeleop path, and make sure the g1_wuji_teleop_main.py invocation still
points to the avp_manus.yml config correctly.
In `@examples/g1_wuji_teleop/python/g1_wuji_teleop/devices/avp_manus_stream.py`:
- Around line 53-58: The retargeter state path in
FIXED_G1_WUJI_RETARGET_DEFAULTS is hardcoded to a shared /tmp filename, which
can collide across runs and be abused via pre-created symlinks. Update the
left/right parameter_config_path entries in avp_manus_stream.py to generate a
unique file per process in a private runtime directory instead of using fixed
/tmp/...json names, and route the code that consumes these defaults to use the
generated path via the existing default config structure.
In `@examples/g1_wuji_teleop/python/g1_wuji_teleop/paths.py`:
- Around line 32-37: resolve_repo_relative_path currently checks app_root() but
never actually falls back to repo_root(), so repo-relative paths are resolved
from the wrong base. Update this function to first try the app_root()-based path
and, if it does not exist, resolve the same relative_path against repo_root()
instead; keep the behavior centered on resolve_repo_relative_path, app_root(),
and repo_root() so the fallback is clear and correct.
In `@examples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/debug_viz.py`:
- Around line 350-378: The frame marker updater only handles "left" and "right",
so head markers are skipped and never refreshed. Update
update_frame_axis_markers() to also process the "head" key (or otherwise make
the side iteration configurable) so update_head_marker() can pass {"head":
head_pose} and still drive frame_markers.visualize for handles.head_link_frame.
In `@examples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/runtime.py`:
- Around line 1002-1017: The early return in runtime.py’s hand retargeting path
is too strict: the current left_input/right_input check in the logic around
_last_left_skeleton, _last_right_skeleton, and _waiting_announced blocks updates
for one side when only the other side is missing. Change the fallback flow so
each hand is processed independently using its own skeleton or last-known value,
and only skip the side that truly has no usable input instead of returning both
_last_left_targets and _last_right_targets together. Keep the waiting message in
the same retargeting helper, but gate it on both sides being unavailable rather
than one side missing.
In `@examples/g1_wuji_teleop/python/g1_wuji_teleop/viz/teleop_visualizer.py`:
- Line 24: The teleop visualizer is hardcoding a shared MPLCONFIGDIR under /tmp,
which should be replaced with a uniquely created per-user/per-process cache
directory. Update teleop_visualizer.py where
os.environ.setdefault("MPLCONFIGDIR", ...) is set so it points to a freshly
created private temp directory for each run, using a location derived from the
current process/user rather than a fixed /tmp/matplotlib path.
🪄 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: a2eb2022-8fea-477e-9c9e-c6423d590ccb
📒 Files selected for processing (147)
.gitattributes.gitignore.gitmodulesAGENTS.mdREUSE.tomlavp_manus.shdeps/cloudxr/webxr_client/helpers/react/CloudXRComponent.tsxdeps/cloudxr/webxr_client/src/App.tsxdeps/cloudxr/webxr_client/src/CloudXRUI.tsxexamples/g1_wuji_teleop/README.mdexamples/g1_wuji_teleop/assets/g1_wuji/g1_three_fingers.usdexamples/g1_wuji_teleop/assets/g1_wuji/g1_wuji.usdexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger1_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger2_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger3_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger4_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/finger5_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/palm_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/left/palm_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger1_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger2_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger3_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger4_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link1.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link1_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link2.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link2_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link3.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link3_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link4.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_link4_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_tip_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/finger5_tip_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/palm_link.STLexamples/g1_wuji_teleop/assets/wuji_hand/meshes/right/palm_link_collision.STLexamples/g1_wuji_teleop/assets/wuji_hand/urdf/left.urdfexamples/g1_wuji_teleop/assets/wuji_hand/urdf/right.urdfexamples/g1_wuji_teleop/config/avp_manus.ymlexamples/g1_wuji_teleop/config/local/g1_wuji/hand_left_config.ymlexamples/g1_wuji_teleop/config/local/g1_wuji/hand_right_config.ymlexamples/g1_wuji_teleop/config/official/g1_wuji/manus_wuji_left.yamlexamples/g1_wuji_teleop/config/official/g1_wuji/manus_wuji_right.yamlexamples/g1_wuji_teleop/python/g1_wuji_teleop/__init__.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/cli.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/devices/avp_manus_stream.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/hand_retargeting/__init__.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/hand_retargeting/base.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/hand_retargeting/wuji_official_adapter.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/paths.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/__init__.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/debug_viz.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/runtime.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/scene.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/session.pyexamples/g1_wuji_teleop/python/g1_wuji_teleop/viz/teleop_visualizer.pyexamples/g1_wuji_teleop/scripts/g1_wuji_teleop_main.pyexamples/g1_wuji_teleop/scripts/g1_wuji_teleop_visualizer.pyexamples/teleop/CMakeLists.txtexamples/teleop/python/pyproject.tomlscripts/setup_v2d_src.shsrc/plugins/manus/app/main.cppsrc/plugins/manus/core/inc/manus/manus_hand_tracking_plugin.hppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/viz/python/layers_bindings.cppsrc/viz/python_tests/test_offscreen_session.pythird_party/wuji-retargeting
Comment on lines
+170
to
+172
| python examples/g1_wuji_teleop/scripts/g1_wuji_teleop_main.py \ | ||
| --config /examples/g1_wuji_teleop/config/avp_manus.yml \ | ||
| --device cuda:0 \ |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix invalid config path in the final demo command.
Line 171 uses an absolute path (/examples/...) that won’t resolve from the repo checkout and will break the main launch step. Use a repo-relative path or the full /path/to/IsaacTeleop/... path.
Suggested fix
python examples/g1_wuji_teleop/scripts/g1_wuji_teleop_main.py \
- --config /examples/g1_wuji_teleop/config/avp_manus.yml \
+ --config examples/g1_wuji_teleop/config/avp_manus.yml \
--device cuda:0 \
--viz kit📝 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
| python examples/g1_wuji_teleop/scripts/g1_wuji_teleop_main.py \ | |
| --config /examples/g1_wuji_teleop/config/avp_manus.yml \ | |
| --device cuda:0 \ | |
| python examples/g1_wuji_teleop/scripts/g1_wuji_teleop_main.py \ | |
| --config examples/g1_wuji_teleop/config/avp_manus.yml \ | |
| --device cuda:0 \ |
🤖 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 `@avp_manus.sh` around lines 170 - 172, The final demo launch command uses an
invalid config path that will not resolve from a normal repo checkout. Update
the config argument in the main teleop command in avp_man.sh to use a
repo-relative path or the correct full IsaacTeleop path, and make sure the
g1_wuji_teleop_main.py invocation still points to the avp_manus.yml config
correctly.
Comment on lines
+53
to
+58
| FIXED_G1_WUJI_RETARGET_DEFAULTS = { | ||
| "left": { | ||
| "config": Path("local/g1_wuji/hand_left_config.yml"), | ||
| "urdf": Path("../assets/wuji_hand/urdf/left.urdf"), | ||
| "parameter_config_path": "/tmp/avp_manus_left_g1_wuji_dex_params.json", | ||
| "joint_names": [ |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not use fixed filenames under /tmp for retargeter state.
These paths are shared across users/processes, so concurrent runs can stomp each other and a pre-created symlink can redirect writes outside the temp area. Generate a unique file in a private runtime directory instead of hardcoding /tmp/...json.
Also applies to: 82-85
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 56-56: Do not hardcode temporary file or directory names
Context: "/tmp/avp_manus_left_g1_wuji_dex_params.json"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🤖 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/g1_wuji_teleop/python/g1_wuji_teleop/devices/avp_manus_stream.py`
around lines 53 - 58, The retargeter state path in
FIXED_G1_WUJI_RETARGET_DEFAULTS is hardcoded to a shared /tmp filename, which
can collide across runs and be abused via pre-created symlinks. Update the
left/right parameter_config_path entries in avp_manus_stream.py to generate a
unique file per process in a private runtime directory instead of using fixed
/tmp/...json names, and route the code that consumes these defaults to use the
generated path via the existing default config structure.
Source: Linters/SAST tools
Comment on lines
+32
to
+37
| def resolve_repo_relative_path(relative_path: str | Path) -> Path: | ||
| relative = Path(relative_path) | ||
| app_path = app_root() / relative | ||
| if app_path.exists(): | ||
| return app_path.resolve() | ||
| return (app_root() / relative).resolve() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resolve_repo_relative_path never falls back to repo root.
Lines 35-37 return the same app_root()-based path in both branches, so repo-relative inputs are never resolved from repo_root().
Proposed fix
def resolve_repo_relative_path(relative_path: str | Path) -> Path:
relative = Path(relative_path)
- app_path = app_root() / relative
- if app_path.exists():
- return app_path.resolve()
- return (app_root() / relative).resolve()
+ app_candidate = (app_root() / relative).resolve()
+ if app_candidate.exists():
+ return app_candidate
+ return (repo_root() / relative).resolve()📝 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
| def resolve_repo_relative_path(relative_path: str | Path) -> Path: | |
| relative = Path(relative_path) | |
| app_path = app_root() / relative | |
| if app_path.exists(): | |
| return app_path.resolve() | |
| return (app_root() / relative).resolve() | |
| def resolve_repo_relative_path(relative_path: str | Path) -> Path: | |
| relative = Path(relative_path) | |
| app_candidate = (app_root() / relative).resolve() | |
| if app_candidate.exists(): | |
| return app_candidate | |
| return (repo_root() / relative).resolve() |
🤖 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/g1_wuji_teleop/python/g1_wuji_teleop/paths.py` around lines 32 - 37,
resolve_repo_relative_path currently checks app_root() but never actually falls
back to repo_root(), so repo-relative paths are resolved from the wrong base.
Update this function to first try the app_root()-based path and, if it does not
exist, resolve the same relative_path against repo_root() instead; keep the
behavior centered on resolve_repo_relative_path, app_root(), and repo_root() so
the fallback is clear and correct.
Comment on lines
+350
to
+378
| def update_frame_axis_markers( | ||
| frame_markers: Any | None, | ||
| frame_poses: Mapping[str, FramePose], | ||
| axis_length: float, | ||
| ) -> None: | ||
| if frame_markers is None: | ||
| return | ||
| axis_translations: list[np.ndarray] = [] | ||
| axis_orientations: list[np.ndarray] = [] | ||
| axis_indices: list[int] = [] | ||
| for side in ("left", "right"): | ||
| frame_pose = frame_poses.get(side) | ||
| if frame_pose is None: | ||
| continue | ||
|
|
||
| position, rotation = frame_pose | ||
| for axis_index in range(3): | ||
| axis = rotation[:, axis_index] | ||
| axis_translations.append( | ||
| position + axis.astype(np.float32) * 0.5 * axis_length | ||
| ) | ||
| axis_orientations.append(_axis_to_cylinder_orientation(axis)) | ||
| axis_indices.append(axis_index) | ||
| if axis_translations: | ||
| frame_markers.visualize( | ||
| translations=np.stack(axis_translations), | ||
| orientations=np.stack(axis_orientations), | ||
| marker_indices=np.asarray(axis_indices, dtype=np.int32), | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Head frame markers never update.
update_head_marker() calls this helper with {"head": head_pose}, but this loop only reads "left"/"right", so handles.head_link_frame stays at its bootstrap pose.
Suggested fix
- for side in ("left", "right"):
- frame_pose = frame_poses.get(side)
- if frame_pose is None:
- continue
-
+ for frame_pose in frame_poses.values():
position, rotation = frame_pose
for axis_index in range(3):
axis = rotation[:, axis_index]📝 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
| def update_frame_axis_markers( | |
| frame_markers: Any | None, | |
| frame_poses: Mapping[str, FramePose], | |
| axis_length: float, | |
| ) -> None: | |
| if frame_markers is None: | |
| return | |
| axis_translations: list[np.ndarray] = [] | |
| axis_orientations: list[np.ndarray] = [] | |
| axis_indices: list[int] = [] | |
| for side in ("left", "right"): | |
| frame_pose = frame_poses.get(side) | |
| if frame_pose is None: | |
| continue | |
| position, rotation = frame_pose | |
| for axis_index in range(3): | |
| axis = rotation[:, axis_index] | |
| axis_translations.append( | |
| position + axis.astype(np.float32) * 0.5 * axis_length | |
| ) | |
| axis_orientations.append(_axis_to_cylinder_orientation(axis)) | |
| axis_indices.append(axis_index) | |
| if axis_translations: | |
| frame_markers.visualize( | |
| translations=np.stack(axis_translations), | |
| orientations=np.stack(axis_orientations), | |
| marker_indices=np.asarray(axis_indices, dtype=np.int32), | |
| ) | |
| def update_frame_axis_markers( | |
| frame_markers: Any | None, | |
| frame_poses: Mapping[str, FramePose], | |
| axis_length: float, | |
| ) -> None: | |
| if frame_markers is None: | |
| return | |
| axis_translations: list[np.ndarray] = [] | |
| axis_orientations: list[np.ndarray] = [] | |
| axis_indices: list[int] = [] | |
| for frame_pose in frame_poses.values(): | |
| position, rotation = frame_pose | |
| for axis_index in range(3): | |
| axis = rotation[:, axis_index] | |
| axis_translations.append( | |
| position + axis.astype(np.float32) * 0.5 * axis_length | |
| ) | |
| axis_orientations.append(_axis_to_cylinder_orientation(axis)) | |
| axis_indices.append(axis_index) | |
| if axis_translations: | |
| frame_markers.visualize( | |
| translations=np.stack(axis_translations), | |
| orientations=np.stack(axis_orientations), | |
| marker_indices=np.asarray(axis_indices, dtype=np.int32), | |
| ) |
🤖 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/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/debug_viz.py`
around lines 350 - 378, The frame marker updater only handles "left" and
"right", so head markers are skipped and never refreshed. Update
update_frame_axis_markers() to also process the "head" key (or otherwise make
the side iteration configurable) so update_head_marker() can pass {"head":
head_pose} and still drive frame_markers.visualize for handles.head_link_frame.
Comment on lines
+1002
to
+1017
| left_input = ( | ||
| left_skeleton if left_skeleton is not None else self._last_left_skeleton | ||
| ) | ||
| right_input = ( | ||
| right_skeleton if right_skeleton is not None else self._last_right_skeleton | ||
| ) | ||
| if left_input is None or right_input is None: | ||
| if not self._waiting_announced: | ||
| print( | ||
| f"[{self._config.status_label}] waiting for MANUS hand skeletons; " | ||
| "AVP/controller EE teleop remains independent.", | ||
| flush=True, | ||
| ) | ||
| self._waiting_announced = True | ||
| return self._last_left_targets, self._last_right_targets | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
One missing hand blocks retargeting for the other hand.
This early return waits for both skeletons to exist at least once, so a missing/late right hand prevents left-hand targets from updating at all. That defeats the per-side fallback behavior the adapter already implements.
Suggested fix
- if left_input is None or right_input is None:
+ if left_input is None and right_input is None:
if not self._waiting_announced:
print(
f"[{self._config.status_label}] waiting for MANUS hand skeletons; "
"AVP/controller EE teleop remains independent.",
flush=True,
)
self._waiting_announced = True
return self._last_left_targets, self._last_right_targets
+
+ if left_input is None:
+ left_input = np.zeros((21, 3), dtype=np.float32)
+ if right_input is None:
+ right_input = np.zeros((21, 3), dtype=np.float32)🤖 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/g1_wuji_teleop/python/g1_wuji_teleop/robots/g1_wuji/runtime.py`
around lines 1002 - 1017, The early return in runtime.py’s hand retargeting path
is too strict: the current left_input/right_input check in the logic around
_last_left_skeleton, _last_right_skeleton, and _waiting_announced blocks updates
for one side when only the other side is missing. Change the fallback flow so
each hand is processed independently using its own skeleton or last-known value,
and only skip the side that truly has no usable input instead of returning both
_last_left_targets and _last_right_targets together. Keep the waiting message in
the same retargeting helper, but gate it on both sides being unavailable rather
than one side missing.
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| os.environ.setdefault("MPLCONFIGDIR", "/tmp/matplotlib") |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid a fixed shared MPLCONFIGDIR under /tmp.
Using /tmp/matplotlib makes every run share the same writable cache/config directory, which can fail under multi-user contention and is susceptible to symlink-based clobbering. Point this at a uniquely created per-user/per-process directory instead.
🤖 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/g1_wuji_teleop/python/g1_wuji_teleop/viz/teleop_visualizer.py` at
line 24, The teleop visualizer is hardcoding a shared MPLCONFIGDIR under /tmp,
which should be replaced with a uniquely created per-user/per-process cache
directory. Update teleop_visualizer.py where
os.environ.setdefault("MPLCONFIGDIR", ...) is set so it points to a freshly
created private temp directory for each run, using a location derived from the
current process/user rather than a fixed /tmp/matplotlib path.
Source: Linters/SAST tools
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
1 participant
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.
Description
Add a new G1 Wuji teleoperation example that integrates AVP + Manus input with the Wuji hand retargeting workflow.
Changes include:
examples/g1_wuji_teleopexample package, configs, scripts, and README.third_party/wuji-retargetingsubmodule.Fixes: N/A
Type of change
Testing
Tested locally on Linux.
Commands run: