Implements passive fixed tendons with mjwarp#5522
Conversation
nv-rgresia
requested review from
ClemensSchwarke,
Mayankm96,
Toni-SM,
hhansen-bdai,
jtigue-bdai,
kellyguo11,
masoudmoghani,
matthewtrepte,
ooctipus,
ossamaAhmed,
pascal-roth,
peterd-NV and
xyao-nv
as code owners
github-actions
Bot
added
documentation
| # newton requires implicitactuators be specified in usd and there's a bug with physx tendons | ||
| usd_path=f"{ISAAC_NUCLEUS_DIR}/Robots/ShadowRobot/ShadowHand/shadow_hand_instanceable_newton.usd", | ||
| #usd_path=f"{ISAAC_NUCLEUS_DIR}/Robots/ShadowRobot/ShadowHand/shadow_hand_instanceable_newton.usd", | ||
| usd_path=f"/home/rgresia/Repositories/mujoco_menagerie/shadow_hand/right_hand.usd/right_shadow_hand.usda", |
| num_envs=8192, env_spacing=0.75, replicate_physics=True, clone_in_fabric=False | ||
| ) | ||
| default: InteractiveSceneCfg = physx | ||
| default: InteractiveSceneCfg = newton_mjwarp |
There was a problem hiding this comment.
non-breaking, but might prefer to not change default?
|
@nv-rgresia many of the tasks tests are failing with this error Could you check if it's related to the change? |
|
@kellyguo11 i'm looking into it, I found some errors related to the mock arti view and the lack of usd asset, but didn't see what your referring to. Considering I didn't touch physx-related code or the asset, i'm not sure how my changes are related. |
|
https://github.com/isaac-sim/IsaacLab/actions/runs/25945216312/job/76271830895?pr=5522 - this job for example seems to be failing on a lot of physx related errors |
| # newton requires implicitactuators be specified in usd and there's a bug with physx tendons | ||
| usd_path=f"{ISAAC_NUCLEUS_DIR}/Robots/ShadowRobot/ShadowHand/shadow_hand_instanceable_newton.usd", | ||
| # newton/mujoco have separate usd schema | ||
| usd_path=f"{ISAAC_NUCLEUS_DIR}/Robots/ShadowRobot/ShadowHand/shadow_hand_newton.usd/right_shadow_hand.usda", |
There was a problem hiding this comment.
this seems to be the source of all the remaining test failure. which file is the correct USD asset? and has it been uploaded to S3?
🤖 Isaac Lab Review Bot — Update (a2ded62)Minor path cleanup in this commit — changed Shadow Hand Newton USD path from No substantive code changes. Previously identified findings remain documented in the original review. ✅ Approved by @ooctipus — ready for merge pending CI green. |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Update (d32c593): Merge from develop branch reviewed. The 8 new commits since 29068bf are primarily infrastructure updates merged from develop:
- PPISP Camera Wrapper (#5499): Adds post-render ISP pipeline across renderers
- Tiled Camera Views (#5696): Adds tiled camera views to Kit/Newton visualizers
- CI Updates (#5628, #5549): Isaac Sim image updates and workaround removal
- Newton Manager Fix (#5710): Scene data provider backend detection fix
- Multi-GPU FrameView (#5514): Enables FabricFrameView on non-primary GPUs
- USD Core Update (#5495): Updates to usd-core 25.11.0
- Test Seed Fix (#5709): Deterministic test ordering fix
No changes to the tendon implementation files — the merge only brought develop changes into this feature branch.
Update (bda9004): Another merge from develop reviewed (d32c593..bda9004). This commit contains more infrastructure updates merged from develop:
- Additions to
isaaclab_ppisppackage (PPISP post-render pipeline) - Visualizer tiled camera views and Newton visualizer improvements
- Test infrastructure updates (env test utils, string test seed fix)
- OVRTX/Newton renderer HDR output support
- Various changelog and version bumps
No changes to the tendon implementation files — the core tendon feature (mjwarp passive fixed tendons in articulation, schemas, Newton manager) remains unchanged. The merge only brought develop infrastructure into this feature branch.
Previous Update (29068bf): Configuration changes reviewed - defaults switched from newton_mjwarp to physx backend, with position/rotation adjustments in shadow_hand_env_cfg.py. No new issues found.
Remaining Items (from original review)
-
Minor typo (schemas.py:880):
"exiss"→"exists" -
Docstring/implementation mismatch (articulation.py
write_fixed_tendon_properties_to_sim_index):- Consider adding partial tendon write support or updating the docstring
-
Mock incomplete (mock_articulation_view.py):
- Missing
tendon_namesproperty for test mocks
- Missing
-
Test coverage: No unit tests for new tendon APIs
Previous reviews preserved above for context.
Update (5ac3500): Merge from develop reviewed (bda9004..5ac3500). This commit brings 10 new commits from develop:
- Docs: Windows installation instructions for LEAPP (#5726), environment presets and experimental docs reorganization (#5734)
- CI: Auto version bump changelogs (3 instances), ARM64 docker build fix + JUnit artifacts (#5757), rendering correctness tests restoration (#5764)
- Fixes: OVRTX renderer default log file path (#5749), non-deterministic test_noise failures (#5732), duplicate shadow hand vision preset test removal (#5767)
No changes to the tendon implementation files — the core tendon feature (mjwarp passive fixed tendons) remains unchanged. The merge only brought develop infrastructure into this feature branch.
Remaining Items (unchanged)
- Minor typo (schemas.py:880):
"exiss"→"exists" - Docstring/implementation mismatch (articulation.py
write_fixed_tendon_properties_to_sim_index) - Mock incomplete (mock_articulation_view.py): Missing
tendon_namesproperty - Test coverage: No unit tests for new tendon APIs
Update (ceb4270): Merge from develop reviewed (5ac3500..ceb4270). This commit brings infrastructure updates from develop:
Key Changes:
- OVRTX Renderer Cleanup: Removed legacy OVRTX 0.2.x code paths from
OVRTXRenderer- eliminates_legacykernels, removes version checks (_IS_OVRTX_0_3_0_OR_NEWER), simplifiesopen_usd_from_stringusage, and cleans up scene partition workarounds. This is a good modernization. - LEAPP Export Fixes:
- Fixed typo in
export.pyargparser description ("Train" → "Export") - Renamed
train_task_name→checkpoint_task_namefor clarity - Added null checkpoint check with informative error message
- Added height_data annotation support in
anymal_c_env.py
- Fixed typo in
- Test Updates: Removed legacy kernel test classes (
TestExtractAllDepthTilesKernelLegacy,TestRandomColorsFromIdsKernelLegacy) - Golden Images: Updated shadow_hand test golden images (binary changes)
- Changelog: Added changelog entry for OVRTX legacy code removal
No changes to the tendon implementation files — the core tendon feature (mjwarp passive fixed tendons) remains unchanged. The merge only brought develop infrastructure into this feature branch.
Remaining Items (unchanged)
- Minor typo (schemas.py:880):
"exiss"→"exists" - Docstring/implementation mismatch (articulation.py
write_fixed_tendon_properties_to_sim_index) - Mock incomplete (mock_articulation_view.py): Missing
tendon_namesproperty - Test coverage: No unit tests for new tendon APIs
Update (c9ca6f2): Merge from develop reviewed (ceb4270..c9ca6f2). Infrastructure updates only:
- Docs: Tab-set formatting for Windows/Linux in LEAPP tutorials, ThreeDWorld URL fix, Windows conda env note
- pyproject.toml: Added
isaaclab-ppispas workspace dependency - Demo Scripts: Added PPISP camera QA demos (
ppisp_camera.py,ppisp_camera_ovrtx.py) - Tutorial Fix: Fixed
add_new_robot.py- tensor device issue for Jetbot velocity targets - isaaclab_ppisp: Bug fix in
auto_camera_ppisp_cfgdiscovery (RenderProducts without PPISP no longer mask later ones) - Tests: PPISP test docstring updates, added test case for the discovery fix
No changes to the tendon implementation files — the core tendon feature remains unchanged.
Remaining Items (unchanged)
- Minor typo (schemas.py:880):
"exiss"→"exists" - Docstring/implementation mismatch (articulation.py
write_fixed_tendon_properties_to_sim_index) - Mock incomplete (mock_articulation_view.py): Missing
tendon_namesproperty - Test coverage: No unit tests for new tendon APIs
5 participants
Description
I have implemented the ability to use and modify passive tendons properties. Despite the shortcomings, this is on feature parity with the existing shadow hand physx environment. While mjc supports spatial tendons, this PR assumes that only fixed tendons are present in the scene, if any. Only 1:1 properties are currently supported (stiffness and damping; there is no equivalent for limit stiffness and rest length/position limits don't have the same data type as in physx (vec2f instead of float), so they can't be ported without restructuring the buffers/apis.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --format