examples/mujoco_xr: a MuJoCo scene in XR, with the SO-101 leader gripper - #900
examples/mujoco_xr: a MuJoCo scene in XR, with the SO-101 leader gripper#900jiwenc-nv wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdded a complete MuJoCo XR example with standalone and in-tree CMake builds, pinned MuJoCo validation, Vulkan/CUDA rendering, pybind11 bindings, XR teleoperation, SO-101 leader-gripper assets, CloudXR rig integration, and hardware-independent tests. The application supports stereo rendering, controller-driven ghost updates, fixed-step simulation, projection checks, tracking diagnostics, and explicit resource cleanup. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant XRSession
participant TeleopSession
participant MuJoCo
participant Renderer
XRSession->>TeleopSession: Provide controller poses and display timing
TeleopSession->>MuJoCo: Update mocap bodies and simulation state
MuJoCo->>Renderer: Provide scene model and data
XRSession->>Renderer: Provide view poses and FOVs
Renderer-->>XRSession: Return color and depth buffers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
750157b to
970f036
Compare
e2dc74c to
0bc685c
Compare
4c5175c to
c6dbb42
Compare
f79bb2c to
051bef7
Compare
One OpenXR session shared between VizSession (rendering) and TeleopSession (input), with a MuJoCo scene drawn by Vulkan into images viz owns and handed to ProjectionLayer.submit() by CUDA pointer, never through host memory. Nothing else here does that, and MuJoCo's own renderer is OpenGL and cannot hand Vulkan images to viz -- which is why cpp/ exists at all. The scene is the leader gripper and nothing else, locked to the right controller's grip pose, its trigger driven by the shipped SO101GripperRetargeter as a node in the pipeline graph rather than a library call beside it. Two things worth knowing going in. The renderer computes its own vertex normals: MuJoCo welds an STL's vertices and keeps one averaged normal per welded vertex, which on a CAD part smears every crease -- 2111 of Wrist_Roll's 18474 face corners end up pointing away from their own face -- and lit one-sided that renders as shattered facets rather than as a gripper. And the meshes are fetched, not vendored: scripts/fetch-so-arm.sh pulls them from SO-ARM100 at a pinned commit, checksum-verified, so no build step reaches the network and the app exits at startup naming the script when they are absent. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
051bef7 to
a657e1c
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # or, with an RPATH, silently loads a second copy (not clean). | ||
| import mujoco as _mujoco | ||
|
|
||
| from . import _mujoco_xr |
|
|
||
| is_none = True | ||
|
|
||
| def __getitem__(self, index): # pragma: no cover -- reaching this IS the bug |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/mujoco_xr/cpp/mujoco_xr_bindings.cpp`:
- Around line 135-140: Update the lambda around model_address and
build_mesh_buffers to reject a null model before dereferencing it. Match the
existing null-model behavior used by SceneRenderer, returning the appropriate
failure result for this binding while preserving the current meshid validation
for valid models.
In `@examples/mujoco_xr/cpp/render_target.cpp`:
- Around line 171-176: Update the successful import path following
cudaImportExternalMemory in render-target initialization to remove the
::close(memory_fd_) call and only set memory_fd_ to -1. Preserve the existing
close in destroy() so failed imports still clean up the descriptor.
In `@examples/mujoco_xr/cpp/scene_renderer.cpp`:
- Around line 638-644: Update render() to validate the incoming Fov before
constructing the projection, routing the per-frame projection calculation
through the exported projection_from_fov entry point instead of calling the
unchecked proj_from_fov directly. Preserve the existing projection storage and
view-projection flow, and reject degenerate zero-width or zero-height FOV values
before any inf/NaN matrix reaches ubo.viewproj.
In `@examples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/app.py`:
- Around line 410-416: Update _resolve_ghost to read both body_mocapid values
and return None when either mocap id is negative, in addition to the existing
body-name validation. Only construct _GhostChannels after both IDs are confirmed
valid, so _update_ghost never receives -1 indices.
🪄 Autofix
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: 430f58c6-1eca-44a9-ae4b-ac7b394bab3f
📒 Files selected for processing (33)
.gitignoreCMakeLists.txtdocs/source/getting_started/build_from_source/index.rstexamples/mujoco_xr/CMakeLists.txtexamples/mujoco_xr/DESIGN.mdexamples/mujoco_xr/README.mdexamples/mujoco_xr/cpp/CMakeLists.txtexamples/mujoco_xr/cpp/compile_shader.cmakeexamples/mujoco_xr/cpp/frames.hppexamples/mujoco_xr/cpp/mesh_buffers.cppexamples/mujoco_xr/cpp/mesh_buffers.hppexamples/mujoco_xr/cpp/mujoco_xr_bindings.cppexamples/mujoco_xr/cpp/render_target.cppexamples/mujoco_xr/cpp/render_target.hppexamples/mujoco_xr/cpp/scene_renderer.cppexamples/mujoco_xr/cpp/scene_renderer.hppexamples/mujoco_xr/cpp/shaders/scene.fragexamples/mujoco_xr/cpp/shaders/scene.vertexamples/mujoco_xr/pyproject.tomlexamples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/__init__.pyexamples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/__main__.pyexamples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/app.pyexamples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/assets/leader/leader_gripper.xmlexamples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/assets/scene.xmlexamples/mujoco_xr/scripts/fetch-so-arm.shexamples/mujoco_xr/tests/CMakeLists.txtexamples/mujoco_xr/tests/conftest.pyexamples/mujoco_xr/tests/pyproject.tomlexamples/mujoco_xr/tests/test_app_helpers.pyexamples/mujoco_xr/tests/test_frames.pyexamples/mujoco_xr/tests/test_ghost.pyexamples/mujoco_xr/tests/test_projection.pyrigs/mujoco_xr.yaml
| [](uintptr_t model_address, int meshid) | ||
| { | ||
| const mjModel* model = reinterpret_cast<const mjModel*>(model_address); | ||
| mujoco_xr::MeshBuffers mb; | ||
| mujoco_xr::build_mesh_buffers(model, &mb); | ||
| if (meshid < 0 || meshid >= static_cast<int>(mb.meshes.size())) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add the null check SceneRenderer already performs.
Line 137 casts model_address and line 139 dereferences it inside build_mesh_buffers. A caller that passes 0 crashes the interpreter with no traceback. SceneRenderer rejects a null model at scene_renderer.cpp lines 193-196; make this entry point agree.
🛡️ Proposed guard
const mjModel* model = reinterpret_cast<const mjModel*>(model_address);
+ if (model == nullptr)
+ {
+ throw std::invalid_argument("mujoco_xr: model address is null");
+ }
mujoco_xr::MeshBuffers mb;📝 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.
| [](uintptr_t model_address, int meshid) | |
| { | |
| const mjModel* model = reinterpret_cast<const mjModel*>(model_address); | |
| mujoco_xr::MeshBuffers mb; | |
| mujoco_xr::build_mesh_buffers(model, &mb); | |
| if (meshid < 0 || meshid >= static_cast<int>(mb.meshes.size())) | |
| [](uintptr_t model_address, int meshid) | |
| { | |
| const mjModel* model = reinterpret_cast<const mjModel*>(model_address); | |
| if (model == nullptr) | |
| { | |
| throw std::invalid_argument("mujoco_xr: model address is null"); | |
| } | |
| mujoco_xr::MeshBuffers mb; | |
| mujoco_xr::build_mesh_buffers(model, &mb); | |
| if (meshid < 0 || meshid >= static_cast<int>(mb.meshes.size())) |
🤖 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/mujoco_xr/cpp/mujoco_xr_bindings.cpp` around lines 135 - 140, Update
the lambda around model_address and build_mesh_buffers to reject a null model
before dereferencing it. Match the existing null-model behavior used by
SceneRenderer, returning the appropriate failure result for this binding while
preserving the current meshid validation for valid models.
| check_cuda(cudaImportExternalMemory(&ext_mem, &ext_desc), "cudaImportExternalMemory"); | ||
| cuda_external_memory_ = ext_mem; | ||
|
|
||
| // CUDA dup'd the fd on import; close ours so we do not leak one per buffer. | ||
| ::close(memory_fd_); | ||
| memory_fd_ = -1; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'cudaImportExternalMemory|cudaExternalMemoryHandleTypeOpaqueFd|::close\(memory_fd_\)' \
examples/mujoco_xr/cpp/render_target.cppRepository: NVIDIA/IsaacTeleop
Length of output: 1759
🌐 Web query:
CUDA Runtime API cudaImportExternalMemory cudaExternalMemoryHandleTypeOpaqueFd file descriptor ownership transferred closed documentation
💡 Result:
When using cudaImportExternalMemory with a handle type of cudaExternalMemoryHandleTypeOpaqueFd, ownership of the file descriptor is transferred to the CUDA driver upon a successful import operation [1][2][3]. According to the official CUDA documentation, once the handle is successfully imported, you must not perform any operations on the file descriptor, as doing so results in undefined behavior [1][2][4]. Because the driver assumes ownership, the application should not manually close the file descriptor after a successful import, as this would interfere with the driver's management of the memory object [1][2]. In contrast, if the import operation fails, ownership of the file descriptor remains with the application, and it is the application's responsibility to manage or close it as necessary. For non-FD handle types (such as Windows NT handles), ownership is generally not transferred to CUDA, and the application must continue to manage and release those handles using appropriate system calls [5][2][3].
Citations:
- 1: https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__EXTRES__INTEROP.html
- 2: https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EXTRES__INTEROP.html
- 3: https://docs.nvidia.com/cuda/archive/13.2.2/cuda-runtime-api/group__CUDART__EXTRES__INTEROP.html
- 4: https://docs.nvidia.com/cuda/archive/13.1.2/cuda-runtime-api/group__CUDART__EXTRES__INTEROP.html
- 5: https://docs.rs/cudarc/latest/src/cudarc/driver/safe/external_memory.rs.html
Do not close memory_fd_ after a successful CUDA import.
cudaImportExternalMemory() transfers ownership of an opaque file descriptor to CUDA. Set memory_fd_ = -1 without closing it after success. Keep the close in destroy() for import-failure cleanup.
🤖 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/mujoco_xr/cpp/render_target.cpp` around lines 171 - 176, Update the
successful import path following cudaImportExternalMemory in render-target
initialization to remove the ::close(memory_fd_) call and only set memory_fd_ to
-1. Preserve the existing close in destroy() so failed imports still clean up
the descriptor.
| float proj[16]; | ||
| float view[16]; | ||
| float pv[16]; | ||
| proj_from_fov(fov, config_.near_z, config_.far_z, proj); | ||
| std::memcpy(projections_[v].data(), proj, sizeof(proj)); | ||
| view_from_pose(pose, pose + 3, view); | ||
| mat4_mul(pv, proj, view); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
render() bypasses the fov validation and can build an inf/NaN projection.
Line 641 calls the internal proj_from_fov, which divides by (tr - tl) and (td - tu) with no guard. The exported projection_from_fov at lines 161-172 is the only place that rejects a degenerate fov, and the per-frame path does not use it. If the runtime hands over a default-constructed viz::Fov of four zeros, both divisors are zero, so out[0] and out[5] become inf and out[8] / out[9] become NaN. The NaN then reaches ubo.viewproj at line 647 and every vertex.
The _assert_projection call in app.py runs only after renderer.render() returns, so the frame is already rendered and submitted before anything reports the fault. Validate at the boundary where the values arrive.
🐛 Route the render path through the validating entry point
const float* pose = poses_xyz_qwxyz.data() + v * 7;
const float* fov = fovs_lrud.data() + v * 4;
float proj[16];
float view[16];
float pv[16];
- proj_from_fov(fov, config_.near_z, config_.far_z, proj);
- std::memcpy(projections_[v].data(), proj, sizeof(proj));
+ // Same validation as the exported helper: a degenerate fov must be a
+ // hard error here, not an inf/NaN matrix handed to the GPU.
+ projections_[v] = projection_from_fov({ fov[0], fov[1], fov[2], fov[3] }, config_.near_z, config_.far_z);
+ std::memcpy(proj, projections_[v].data(), sizeof(proj));
view_from_pose(pose, pose + 3, view);📝 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.
| float proj[16]; | |
| float view[16]; | |
| float pv[16]; | |
| proj_from_fov(fov, config_.near_z, config_.far_z, proj); | |
| std::memcpy(projections_[v].data(), proj, sizeof(proj)); | |
| view_from_pose(pose, pose + 3, view); | |
| mat4_mul(pv, proj, view); | |
| float proj[16]; | |
| float view[16]; | |
| float pv[16]; | |
| // Same validation as the exported helper: a degenerate fov must be a | |
| // hard error here, not an inf/NaN matrix handed to the GPU. | |
| projections_[v] = projection_from_fov({ fov[0], fov[1], fov[2], fov[3] }, config_.near_z, config_.far_z); | |
| std::memcpy(proj, projections_[v].data(), sizeof(proj)); | |
| view_from_pose(pose, pose + 3, view); | |
| mat4_mul(pv, proj, view); |
🤖 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/mujoco_xr/cpp/scene_renderer.cpp` around lines 638 - 644, Update
render() to validate the incoming Fov before constructing the projection,
routing the per-frame projection calculation through the exported
projection_from_fov entry point instead of calling the unchecked proj_from_fov
directly. Preserve the existing projection storage and view-projection flow, and
reject degenerate zero-width or zero-height FOV values before any inf/NaN matrix
reaches ubo.viewproj.
| def _resolve_ghost(model) -> _GhostChannels | None: | ||
| """Both ghost mocap rows, or None if this scene declares no ghost.""" | ||
| body = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_BODY) | ||
| jaw = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_JAW_BODY) | ||
| if body < 0 or jaw < 0: | ||
| return None | ||
| return _GhostChannels(int(model.body_mocapid[body]), int(model.body_mocapid[jaw])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard body_mocapid against -1.
_resolve_ghost validates only that the two body names exist. It does not validate that both bodies are mocap bodies. For a non-mocap body, model.body_mocapid[body] is -1. _update_ghost then writes data.mocap_pos[-1] and data.mocap_quat[-1], which NumPy resolves to the last mocap row. A --scene-xml that names a non-mocap body leader_ghost or leader_ghost_jaw therefore corrupts an unrelated mocap body silently, with no error.
Return None when either mocap id is negative, so the scene reports "no ghost" instead.
🐛 Proposed fix
body = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_BODY)
jaw = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_JAW_BODY)
if body < 0 or jaw < 0:
return None
- return _GhostChannels(int(model.body_mocapid[body]), int(model.body_mocapid[jaw]))
+ body_mocap = int(model.body_mocapid[body])
+ jaw_mocap = int(model.body_mocapid[jaw])
+ if body_mocap < 0 or jaw_mocap < 0:
+ # Declared, but not `mocap="true"`: a -1 row index would write into
+ # another body's mocap row instead of failing.
+ return None
+ return _GhostChannels(body_mocap, jaw_mocap)📝 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.
| def _resolve_ghost(model) -> _GhostChannels | None: | |
| """Both ghost mocap rows, or None if this scene declares no ghost.""" | |
| body = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_BODY) | |
| jaw = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_JAW_BODY) | |
| if body < 0 or jaw < 0: | |
| return None | |
| return _GhostChannels(int(model.body_mocapid[body]), int(model.body_mocapid[jaw])) | |
| def _resolve_ghost(model) -> _GhostChannels | None: | |
| """Both ghost mocap rows, or None if this scene declares no ghost.""" | |
| body = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_BODY) | |
| jaw = mujoco.mj_name2id(model, mujoco.mjtObj.mjOBJ_BODY, GHOST_JAW_BODY) | |
| if body < 0 or jaw < 0: | |
| return None | |
| body_mocap = int(model.body_mocapid[body]) | |
| jaw_mocap = int(model.body_mocapid[jaw]) | |
| if body_mocap < 0 or jaw_mocap < 0: | |
| # Declared, but not `mocap="true"`: a -1 row index would write into | |
| # another body's mocap row instead of failing. | |
| return None | |
| return _GhostChannels(body_mocap, jaw_mocap) |
🤖 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/mujoco_xr/python/isaacteleop_examples/mujoco_xr/app.py` around lines
410 - 416, Update _resolve_ghost to read both body_mocapid values and return
None when either mocap id is negative, in addition to the existing body-name
validation. Only construct _GhostChannels after both IDs are confirmed valid, so
_update_ghost never receives -1 indices.
1 participant
Description
gripper-ghost.mp4
One OpenXR session shared between
VizSession(rendering) andTeleopSession(input), with a MuJoCo scene drawn by Vulkan into images viz owns and handed toProjectionLayer.submit()by CUDA pointer, never through host memory. MuJoCo's own renderer is OpenGL and cannot hand Vulkan images to viz — which is whycpp/exists at all.The scene is the SO-101 leader gripper and nothing else, locked to the right controller's grip pose, its trigger driven by the shipped
SO101GripperRetargeteras a node in the pipeline graph rather than a library call beside it. Meshes are fetched, not vendored:scripts/fetch-so-arm.shpulls them fromTheRobotStudio/SO-ARM100at a pinned commit, checksum-verified, and no build step reaches the network.Two calls to confirm rather than inherit. (1) The renderer computes its own vertex normals. MuJoCo welds an STL's vertices and keeps one averaged normal per welded vertex, which smears every crease on a CAD part — 2111 of
Wrist_Roll's 18474 face corners point away from their own face — and lit one-sided that renders as shattered facets rather than as a gripper. (2) The extension is compiled twice, in place forctestand by scikit-build-core for the wheel; collapsing to wheel-only is blocked on version identity, not availability, since what PyPI serves forisaacteleopis a release rather than the viz in this checkout.Type of change
Testing
Ubuntu 22.04 / aarch64 (Tegra). Full
cmake --preset py3.12 -DBUILD_VIZ=ONbuild green;ctest -L mujoco_xrgreen — 4 files, 45 tests, all executed rather than skipped (worth confirming withpytest -v, since an all-skipped run also exits 0).The normals bug above was found by running this example's own Vulkan path offscreen through
viz.DisplayMode.kOffscreenand diffing it against MuJoCo's renderer on the same model and viewpoint: the silhouettes agreed to 1 % while the shading did not. That is a scratch script rather than a test, because it needs a GPU.mesh_triangles()exposes what the renderer will draw, so the invariant it violated is checked on a CPU instead.Not verified, and not verifiable without hardware: everything a headset touches —
submit(), OpenXR session sharing, controllers on a shared session, whether the runtime accepts the depth layer, and whether the gripper lands in the operator's hand. Nomujoco_xrtest can run in CI yet because nothing in.github/workflows/installsmujoco— that is #880.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCO