Jiwenc nv/robot viz - #995
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
| "LocomotionRootCmdRetargeter", | ||
| "LocomotionRootCmdRetargeterConfig", | ||
| # Manipulator retargeters | ||
| "ControllerPoseSource", |
| "LocomotionRootCmdRetargeterConfig", | ||
| # Manipulator retargeters | ||
| "ControllerPoseSource", | ||
| "EngageAlignmentGate", |
| # Manipulator retargeters | ||
| "ControllerPoseSource", | ||
| "EngageAlignmentGate", | ||
| "EngageGateConfig", |
| "ControllerPoseSource", | ||
| "EngageAlignmentGate", | ||
| "EngageGateConfig", | ||
| "GateVerdict", |
| # handles, so destroying the session first would pull them out from | ||
| # under it. `destroy()` is what releases this. | ||
| self._destroy.wait() | ||
| except BaseException as error: # noqa: BLE001 -- reported to the owner |
7e638fd to
f5de61f
Compare
| "JointMap", | ||
| "RobotTwin", | ||
| "RobotTwinPublisher", | ||
| "SceneTwin", |
The MuJoCo-backed digital twin was buried in examples/mujoco_xr, compiled as part of the example and reachable only by installing that example. It is a Televiz capability, not an example, so the backend moves to src/viz/robot_twin and the Python surface to isaacteleop.viz.robot, both shipping in the wheel. What is left of the example is the app itself, renamed examples/robot_viz. The twin no longer depends on a `mujoco` wheel. cmake/Mujoco.cmake fetches MuJoCo and builds it under a private name, reached by dlopen/dlsym so the extension carries no undefined mj* for a foreign libmujoco to answer; the example's version-pin cross-check goes with it. Also here: the engage gate and controller pose source under isaacteleop.retargeters, and the twin runner in teleop_session_manager. Tests are stripped from this branch and land in a follow-up PR. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
MuJoCo was fetched from src/viz/CMakeLists.txt, the one dependency not declared where every other one is. Its FetchContent_Declare now sits beside glfw and glm under the same BUILD_VIZ gate, and the isolation contract it exists for moves next to it as deps/third_party/Mujoco.cmake. Declaration order relative to every other dependency is unchanged. ISAACTELEOP_MUJOCO_VERSION becomes a cache variable because src/viz/robot_twin_tests is now a sibling of where it is set. Two mechanisms did not survive being asked to justify themselves. `-Wl,--exclude-libs,ALL` is redundant against the version script, a whitelist and so strictly stronger: on robot_twin_py the version script alone leaves 1 export and --exclude-libs alone leaves 11. The BUILD_SHARED_LIBS guard cannot fire -- upstream's add_library is unconditionally SHARED off Emscripten -- and the failure it converted was already loud. include_guard(GLOBAL) goes with the single include site. The OUTPUT_NAME comment had the interposition backwards. The wheel's extension modules carry DT_NEEDED on libmujoco.so.3.x, so it is ours answering theirs: loaded first under an unrenamed SONAME, our copy is the only libmujoco the process maps and the user's `import mujoco` silently runs on it. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
124b2f0 to
bce10b7
Compare
The follow-up half of #995, which carries the twin with every test stripped out. All 20 files land here: src/viz/robot_twin_tests, examples/robot_viz/tests, the four src/viz/python_tests/test_robot_*.py, test_engage_gate.py and test_robot_twin_session.py, plus the CMake that registers them and the example README's Tests section. test_symbol_isolation.py runs against the shipped _robot_twin rather than a test-only extension carrying a second private MuJoCo. Review asked why the tests reached MuJoCo directly, and they should not have: that probe's two unique assertions exercised its own reimplemented longjmp handler rather than mj_guard.cpp, and its ELF assertions were the weaker copy of ones the shipped module already gets -- --version-script has nothing to hide in a module linking no static archive, and the shipped one links cudart_static. Two replacements do cover shipped code: that install_mujoco_handlers() wrote the twin's own mju_user_error and not the wheel's, and that an unguarded mju_error reaches mj_guard.cpp's abort rather than MuJoCo's exit(1). The example's tests are keyed on TARGET robot_twin_py, which says the actual dependency instead of restating where the twin gets built. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
| # rather than a knob -- gl_context.cpp makes its OpenGL context through EGL's device | ||
| # platform, which is how it renders with no display, and Windows has no equivalent yet. | ||
| if(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
| # MuJoCo itself. Included here rather than at the root so the decision and the |
There was a problem hiding this comment.
why don't we add it even deeper? such as inside robot_twin.
Also, why would robot_twin_tests needs mujoco direclty? I think it should just test the interfaces of robot_twin, not its mujoco internals.
| !/examples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/assets/leader/leader_gripper.xml | ||
| /examples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/assets/follower/* | ||
| !/examples/mujoco_xr/python/isaacteleop_examples/mujoco_xr/assets/follower/follower_arm.xml | ||
| /examples/robot_viz/python/isaacteleop_examples/robot_viz/assets/leader/* |
There was a problem hiding this comment.
where does those xml come from? mujoco? if so, can we just download them?
| endif() | ||
| # Pure Python, but its tests import the robot twin, which ships with Televiz on | ||
| # Linux (src/viz/CMakeLists.txt says why not on Windows). | ||
| if(BUILD_VIZ AND CMAKE_SYSTEM_NAME STREQUAL "Linux") |
There was a problem hiding this comment.
this check against CMAKE_SYSTEM_NAME STREQUAL "Linux" is YAGNI. Fix or prove me wrong.
| endif() | ||
|
|
||
| # ============================================================================== | ||
| # MuJoCo (robot twin physics + renderer) |
There was a problem hiding this comment.
do we really use any physics from MuJoCo at all? The use case of MuJoCo here in this PR should be visualization only. Please double check whether we are doing physics at all. That's YAGNI, if so, please remove.
| mujoco | ||
| GIT_REPOSITORY https://github.com/google-deepmind/mujoco.git | ||
| GIT_TAG ${ISAACTELEOP_MUJOCO_VERSION} | ||
| # The ref is a tag, so it is fetchable without history. Measured here: 140 MB of |
There was a problem hiding this comment.
too verbose. no need to comment GIT_SHALLOW remove.
| GIT_SHALLOW TRUE | ||
| ) | ||
|
|
||
| # Plain variables, not the `CACHE ... FORCE` the entries above use: MuJoCo sets |
There was a problem hiding this comment.
check: should those go into mujoco.cmake? or it is required to configure before FetchContent_MakeAvailable(mujoco)
| ``pip install isaacteleop`` already ships the compiled ``isaacteleop.viz`` module. See | ||
| `Other Build options`_ for the full option table. | ||
|
|
||
| On **Linux**, ``BUILD_VIZ=ON`` also builds the robot twin's scene backend |
There was a problem hiding this comment.
YAGNI. Please remove.
| * - **Televiz visualization** | ||
| - ``BUILD_VIZ`` | ||
| - Auto: ``ON`` when Vulkan and the CUDA Toolkit are detected, else ``OFF``. Force with ``-DBUILD_VIZ=ON`` / ``-DBUILD_VIZ=OFF``. (Most users don't need this — ``pip install isaacteleop`` already ships the compiled ``isaacteleop.viz`` module.) | ||
| - Auto: ``ON`` when Vulkan and the CUDA Toolkit are detected, else ``OFF``. Force with ``-DBUILD_VIZ=ON`` / ``-DBUILD_VIZ=OFF``. On Linux it also builds the robot twin's scene backend, which needs ``libegl-dev`` and fetches MuJoCo on the first configure. (Most users don't need this — ``pip install isaacteleop`` already ships the compiled ``isaacteleop.viz`` module.) |
There was a problem hiding this comment.
YAGNI. Please remove.
Description
The MuJoCo-backed digital twin was buried in
examples/mujoco_xr— compiled as part of that example, reachable only by installing it, and gated on the developer having a matchingmujocowheel. It is a Televiz capability, not an example. The backend moves tosrc/viz/robot_twinand its Python surface toisaacteleop.viz.robot, both shipping in the wheel; the app that is left behind is renamedexamples/robot_viz.deps/third_partynow fetches MuJoCo and builds it under a private name, reached throughdlopen/dlsymso the extension carries no undefinedmj*for a foreignlibmujocoto answer.Mujoco.cmakebeside it states that contract. Users maypip install mujocoat any version, or none.Two calls for a reviewer: the wheel now redistributes MuJoCo binaries, and every
BUILD_VIZbuild compiles it with no opt-out (~40 s, needslibegl-dev).No tests here — they are #999, one commit on top of this branch. This alone drops six test files that exist on
main, so the two should land together.Type of change
isaacteleop_examples.mujoco_xris gone; the docs that referenced it are updated.Testing
cteston Linux aarch64 (Jetson AGX Orin), Release, CUDA, Python 3.12: 316 passed, 0 failed. #999 on the same machine is 330 / 0.readelfon the built artifacts confirms the isolation: one export, nolibmujocoor EGLNEEDED, private SONAME,SYMBOLICset.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCO