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?
| if "PhysxTendonAxisRootAPI" not in schema_name: | ||
| continue | ||
| # set into PhysX API by attribute prefix schema_name: (e.g. PhysxTendonAxisRootAPI:default:stiffness) | ||
| if prim_type != "MjcTendon": |
There was a problem hiding this comment.
I think there are other mujoco schema also got leaked into core schema. and also seems like we have not prepared a home for mujoco schema under isaaclab_newton properly. This check is no ideal but probably makes sense for this PR, and would require follow up work to split it.
| if self.num_fixed_tendons > 0 or self.num_spatial_tendons > 0: | ||
| raise NotImplementedError("Fixed and spatial tendons are not supported yet.") | ||
| if self._root_view.tendon_count > 0: | ||
| tendon_types = wp.to_torch( |
There was a problem hiding this comment.
Seems wasteful to convert to torch just to check if there is a non-zero element. Any way we can do this in a warp way
There was a problem hiding this comment.
I'm open to suggestions, but the only alternative I can think is writing a kernel since warp array don't have a lot of the same utility
| """ | ||
| raise NotImplementedError() | ||
| wp.launch( | ||
| shared_kernels.write_2d_data_to_buffer_with_indices, |
There was a problem hiding this comment.
These two kernels can be combined into one. This might be out of the scope of this PR, if so can we leave a TODO so someone know to refactor it later
| env_ids: Sequence[int] | torch.Tensor | wp.array | None = None, | ||
| self, | ||
| *, | ||
| limit: float | torch.Tensor | wp.array, |
There was a problem hiding this comment.
What's going on with these argument types? It seems below that they're meant to be warp arrays since we're going to use a warp kernels, maybe we should make the typing strict again. Also if its proxy array we can just keep the type as ProxyArray
| (num_instances, num_fixed_tendons) if full_data. | ||
| fixed_tendon_ids: The tendon indices to set the damping for. Defaults to None (all fixed tendons). | ||
| env_ids: Environment indices. If None, then all indices are used. | ||
| full_data: Whether to expect full data. Defaults to False. |
There was a problem hiding this comment.
This argument does not seem to change the logic at all, just used to check the shape. It seems either way we're going to use env_ids, can we just drop the full data and full data assert below?
There was a problem hiding this comment.
it has been removed
| damping: float | torch.Tensor | wp.array, | ||
| fixed_tendon_mask: wp.array | None = None, | ||
| env_mask: wp.array | None = None, | ||
| self, |
There was a problem hiding this comment.
whats happening with the indentation
|
make sure run the linter, everything else looks ok to me, thanks! |
| self.assert_shape_and_dtype( | ||
| stiffness, (env_ids.shape[0], fixed_tendon_ids.shape[0]), wp.float32, "stiffness" | ||
| ) | ||
| # Warp kernels can ingest torch tensors directly, so we don't need to convert to warp arrays here. |
There was a problem hiding this comment.
can you check its probably not torch tensor but ProxyArray, we don't wnat to directly give torch into warp kernel, as far as I am aware.
There was a problem hiding this comment.
the randomized damping/stiffness buffers are being generated in torch and passed to the kernel. we would have to port _randomize_prop_by_op to a warp kernel to avoid the conversion all together. The data it copies from is warp array too.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR implements passive fixed tendon support for Newton/MuJoCo-based articulations (mjwarp), enabling tendon stiffness and damping randomization. The implementation follows the established Isaac Lab patterns for property setters with index/mask variants and correctly handles the subset of tendon properties that map between PhysX and MuJoCo. The core implementation appears sound, though there are a few edge cases and documentation items to address.
Design Assessment
Architecture ✅
- Clean separation between PhysX and Newton code paths using
self.newtonflag in event handlers - Proper use of Warp kernels (
shared_kernels.write_2d_data_to_buffer_with_indices) matching existing patterns - Correct staging buffer pattern: internal buffers →
write_fixed_tendon_properties_to_sim_index()→ simulation - Appropriate
NotImplementedErrorexceptions for unsupported properties (limit_stiffness, rest_length, offset, position_limits)
Data Flow ✅
- Simulation bindings correctly created from
mujoco.tendon_stiffnessandmujoco.tendon_dampingattributes - ProxyArray wrappers properly initialized in
_pin_proxy_arrays() - Staging buffers cloned from sim bindings allowing batch writes
API Surface ✅
- Index and mask methods implemented consistently
full_dataparameter added appropriately for handling both partial and full data paths
Findings
🔴 Critical
None identified.
🟡 Medium
-
Missing
full_dataparameter forwarding (articulation.py:2699, 2795)set_fixed_tendon_stiffness_index()andset_fixed_tendon_damping_index()acceptfull_dataparameter- The mask methods call these with
full_data=True, but the index methods signature showsfull_datais not used in the implementation - Consider actually implementing the
full_databranching logic or document that it is currently ignored
-
write_fixed_tendon_properties_to_sim_indexmissing parameter usage (articulation.py:3049-3077)- Method signature includes
fixed_tendon_idsparameter but implementation always usesself._ALL_FIXED_TENDON_INDICES - This ignores partial tendon updates, writing all tendons regardless of the parameter
dim=(env_ids.shape[0], self._ALL_FIXED_TENDON_INDICES.shape[0]), # Always uses ALL
- Method signature includes
-
Assumption comment needs verification (articulation_data.py:1410)
- Comment says "assumes all tendons are fixed and only one arti in scene"
- This assumption may not hold in complex scenes - should either validate or relax the constraint
🟢 Low
-
Typo in schema check comment (schemas.py:876)
- "exiss" → "exists"
-
Shadow Hand config hardcoded position change (shadow_hand_env_cfg.py:161)
- Position changed from
(0.0, 0.0, 0.5)to(-0.33, -0.36, 0.3)- is this intentional or debug artifact?
- Position changed from
Test Coverage
The mock articulation view now supports tendons (num_tendons parameter, tendon_count property), which enables unit testing. However, actual test cases are still needed:
- No unit tests for
set_fixed_tendon_stiffness_index/mask - No unit tests for
set_fixed_tendon_damping_index/mask - No unit tests for
write_fixed_tendon_properties_to_sim_index - No integration tests for the Newton tendon randomization path
Recommendation: Add tests covering:
- Tendon property getters return correct shapes and initial values
- Stiffness/damping setters modify buffers correctly
write_fixed_tendon_properties_to_sim_indexpropagates to simulation- Randomization event with Newton backend
- Edge case: articulation with zero tendons
CI Status
⚠️ Check changelog fragments: Failed - changelog entry required- 🔄 Most CI checks still pending (recently pushed commits)
- ✅ Pre-commit, license check, base builds passing
Verdict
Minor fixes needed 🟡
The implementation is architecturally sound and follows established patterns. Key items before merge:
- Add changelog fragment (blocking)
- Address the
write_fixed_tendon_properties_to_sim_indexnot usingfixed_tendon_idsparameter - Consider adding basic test coverage for the new tendon APIs
- Clean up commented code and typos
Update (2744b3e): ✅ Issue #5 (commented-out code) has been addressed - the commented num_fixed_tendons check has been removed. Other findings remain open.
Update (e054aa2): ✅ Changelog fragments have been added (blocking CI issue resolved). The incremental diff also includes unrelated Jacobian/mass-matrix task-space controller support in articulation_data.py — these appear to be additional features merged into this PR branch. Original tendon-related findings (items #1–4, #6–7) remain open; item #5 (commented code) was previously addressed.
Update (c7dd84d): Release housekeeping only — version bumps (5.2.1, 0.9.1, 0.7.1) and changelog consolidation. No tendon-related code changes. Previous findings (items #1–4, #6–7) remain open.
Update (99b1a61): Merge commit bringing in unrelated changes from develop branch:
- AppLauncher CUDA fix: Deferred
torch.cuda.set_device()call to afterSimulationAppstarts, fixing startup crashes when OpenBLAS at-fork handlers were unsafe - LEAPP export refactor: Major restructure of
export.pyfor better modularity (functions separated:create_arg_parser(),parse_export_args(),export_rsl_rl_agent(),main_cli()) - Test improvements: Export tests now batch multiple tasks per Kit process, reducing startup overhead; removed unnecessary
AppLauncherfromtest_noise.pyandtest_wrench_composer.py - CI/build fixes: Import paths corrected to use
isaaclab.utils.configclassdirectly; lazy loading updates toisaaclab.utils - Various changelog skip files for unrelated packages
No changes to the core tendon implementation. Previous findings (items #1–4, #5–6) remain open.
Update (614dbb6): Merge commit bringing in develop branch changes. Key updates include:
- Unified RL train/play entrypoints (#5623): Added
./isaaclab.sh trainand./isaaclab.sh playcommands dispatching to library-specific implementations (rsl_rl, rl_games, skrl, sb3, rlinf). Deprecated old per-library scripts with warnings. - URDF/MJCF importer refactor (#5394): Simplified converters to delegate to Isaac Sim's importer pipeline; removed duplicate IsaacLab implementations of
_apply_fix_base,_apply_link_density,_apply_joint_drives, etc. - Newton replication label fix (#5433): Fixed tendon labels (e.g.
mujoco:tendon_label) keeping source proto path after replication; now properly rewrites per-env paths - Teleop replay agent (#5507): Added non-interactive entry point for CI replay of teleop sessions
- skrl drone-ARL config fix (#5613): Changed
input: STATEStoinput: OBSERVATIONSfixing AttributeError
No changes to the core passive tendon implementation. Previous findings (items #1–4, #5–6) remain open.
Update (5b2f873): Continuation of merge from develop. Incremental diff shows only the merge-in of unrelated PRs (same items as 614dbb6 update above). The tendon randomization event fix in source/isaaclab/isaaclab/envs/mdp/events.py is notable — it removes the self.newton instance variable set during __init__ and instead queries env.sim.physics_manager.__name__.lower() at call-time, making the backend check more robust for multi-backend scenarios.
No new findings. Previous findings (items #1–4, #5–6) remain open.
Update (4b50d5f): ✅ Test infrastructure improvement — the mock articulation view (MockArticulationView) now supports tendon mocking:
- Added
num_tendons: int = 0constructor parameter - Added
tendon_countproperty returning the configured value
This addresses the test infrastructure gap I noted earlier. However, actual test cases exercising the tendon APIs have not been added yet. The mock is ready; tests using it are still missing. Previous code findings (items #1–4, #5) remain open.
Update (997200d): Merge commit bringing in unrelated changes from develop branch. Key updates include:
- Camera warp migration: Camera data outputs now use
ProxyArraywrappers backed by Warp arrays; renderers (NewtonWarpRenderer,OVRTXRenderer,IsaacRtxRenderer) updated to acceptProxyArrayinset_outputsandupdate_camera - Newton v1.2.0 bump: All
newton[sim]pins updated fromv1.2.0rc2to stablev1.2.0 - Preset CLI improvements: Added typed preset selectors (
physics=NAME,renderer=NAME) and bucketed help-time variant enumeration - OVRTX renderer fixes: Fixed multi-GPU device mismatch; fixed cloned environments disappearing; renamed
use_cloningtouse_ovrtx_cloning - Cubric ABI verification: Added runtime version check for
omni::cubric::IAdapterinterface - Frame stacking: Added
CartpoleCameraPresetsEnvwithCircularBuffer-backed frame stacking - Test improvements: Added determinism training tests, preset CLI tests, stacked image tests
No changes to the core passive tendon implementation. Previous findings (items #1–4, #5) remain open.
Update (ad103b4): Release housekeeping commit — version bumps (isaaclab 5.3.0, isaaclab_newton 0.10.0, isaaclab_ov 0.2.0, isaaclab_physx 0.8.0, isaaclab_ovphysx 0.2.0, isaaclab_tasks 1.7.0, isaaclab_teleop 0.4.0) and changelog consolidation from fragment files into main CHANGELOG.rst.
Key changes in this commit:
- OVPhysX Articulation implementation: Major addition of
ArticulationandArticulationDataclasses for the OVPhysX backend, with full API surface (index/mask writers/setters, actuator pipeline, tendon support, wrench composers). This is new backend work, not changes to the existing Newton/PhysX tendon code. - OVPhysX tendon tensor types: Moved tendon tensor types out of
_CPU_ONLY_TYPESsince OVPhysX wheel exposes tendons on the simulation device (GPU-resident). - OVPhysX step update: Added
update_articulations_kinematic()call in physics step. - Allegro hand task preset: Added
ovphysxpreset toObjectCfgandPhysicsCfgsoIsaac-Repose-Cube-Allegro-Direct-v0can run against OVPhysX backend. - Test infrastructure: Added
test_articulation_helpers.pyfor OVPhysX-specific scaffolding tests (USD tendon-scope resolution, mock binding-set shape contracts).
No changes to the original passive fixed tendon implementation for Newton/MuJoCo. Previous findings (items #1–4, #5) remain open.
Update (40135b6): Merge commit bringing in code-quality improvements from develop:
- Import path migration: 200+ files updated from
from isaaclab.utils import configclasstofrom isaaclab.utils.configclass import configclass— this fixes lazy-import issues and improves module loading performance - CI workflow updates: New
test-rendering-correctnessandtest-rendering-correctness-kitlessjobs added to build.yaml;ovrtxpip package removed from the generalisaaclab_taskstest shards - Python version pin:
pyproject.tomlnow requires>=3.12,<3.13(was>=3.12) - OVRTX renderer optimization: Avoided disk I/O by exporting USD to string in memory (
export_stage_to_string,open_usd_from_string) instead of always writing intermediate files;temp_usd_dirnow defaults toNone - Documentation fixes: Updated
configclassimport paths in RST files (hydra.rst, cloning.rst, record_video.rst, contributing.rst, etc.) - Changelog skip files: Added for lazy-import fix across isaaclab, isaaclab_contrib, isaaclab_experimental, isaaclab_mimic, isaaclab_newton, isaaclab_ov, isaaclab_ovphysx, isaaclab_physx, isaaclab_rl, isaaclab_tasks
No changes to the core passive tendon implementation. Previous findings (items #1–4, #5) remain open.
| logger.info(f"Simulation parameters for joints in {self.cfg.prim_path}:\n" + joint_table.get_string()) | ||
|
|
||
| # read out all fixed tendon parameters from simulation | ||
| if self.num_fixed_tendons > 0: |
There was a problem hiding this comment.
removal instead of commenting?
nv-rgresia
force-pushed
the
passive-tendons
branch
from
970ca12 to
aa87f5f
Compare
|
|
||
| manager_name = env.sim.physics_manager.__name__.lower() | ||
| if "newton" in manager_name: | ||
| self.newton = True |
There was a problem hiding this comment.
In other places in this file _backend is used, let's use the same convention
| self.asset.set_fixed_tendon_limit_stiffness( | ||
| limit_stiffness[env_ids[:, None], tendon_ids], tendon_ids, env_ids | ||
| ) | ||
| if not self.newton: |
There was a problem hiding this comment.
this would become _backend == "physx" if we use the above comment
|
@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?
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