Adds fixed tendon support to articulation#5164
Adds fixed tendon support to articulation#5164nv-rgresia wants to merge 10 commits intoisaac-sim:developfrom
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds fixed tendon support to the Newton articulation by implementing MuJoCo actuator control APIs. However, the implementation is incomplete and contains several critical bugs that will cause runtime failures. The environment code calls methods with arguments that don't exist in the function signatures.
Design Assessment
Problem being solved: Enable control of fixed tendons in articulations via MuJoCo's actuator CTRL buffer.
Design evaluation:
- ✅ Placing tendon control in the Articulation class is correct — articulations own their actuators/tendons
- ✅ Using
_tendon_name_to_ctrl_idxmapping is a reasonable approach for translating tendon names to ctrl indices ⚠️ The ctrl buffer is 1D(actuator_count,)instead of 2D(num_instances, actuator_count), breaking per-environment control⚠️ _process_tendons()unconditionally requires MuJoCo actuator attributes, breaking non-tendon articulations
Alternative considered: Could have added ctrl support as a separate mixin or manager, but integrating into Articulation directly is more consistent with how joint control already works.
Verdict: Acceptable short-term design, but the buffer shapes need to follow the (num_instances, num_X) convention for proper per-environment support.
Architecture Impact
num_fixed_tendonsandfixed_tendon_namesnow return real data from_root_view.tendon_count/tendon_names_process_tendons()now unconditionally requires MuJoCo attributes — this will break any Newton articulation that doesn't have MuJoCo actuators- New
write_actuator_ctrl_to_sim()method added to public API - New
set_fixed_tendon_ctrl()method added but is non-functional (empty body)
Implementation Verdict
Needs rework — Multiple critical bugs that will cause immediate runtime failures.
Test Coverage
🟡 Warning: No unit tests included. This is a new feature that adds:
- New public methods (
write_actuator_ctrl_to_sim,set_fixed_tendon_ctrl) - New data buffers (
_actuator_ctrl,_tendon_stiffness,_tendon_damping) - Modified initialization logic (
_process_tendons)
At minimum, tests should verify:
- Articulations with tendons can be created and ctrl buffers are accessible
write_actuator_ctrl_to_simcorrectly updates the simulation- Articulations WITHOUT tendons still work (regression test for the
_process_tendonschange)
CI Status
- ❌
pre-commit— failing (likely formatting issues) - ❌
environments_training— failing - Multiple other checks pending/skipped
Findings
🔴 Critical: articulation.py:3143-3153 — set_fixed_tendon_ctrl is an empty stub
The method has a statement before the docstring, making the docstring a no-op string. More importantly, the method body does nothing after resolving env_ids — it never actually sets the ctrl buffer.
🔴 Critical: articulation.py:3155 — write_actuator_ctrl_to_sim signature mismatch
The method takes no arguments (def write_actuator_ctrl_to_sim(self) -> None), but the environment code calls it with ctrl=[-1.0, -1.0]. This will raise TypeError: got unexpected keyword argument 'ctrl'.
🔴 Critical: articulation.py:3497 — _process_tendons breaks non-tendon articulations
The method unconditionally raises Exception("Mujoco has no actuator trntype") when MuJoCo actuator attributes are missing. This breaks any Newton articulation that doesn't use MuJoCo actuators. Should check if self.num_fixed_tendons > 0 before requiring these attributes.
🔴 Critical: articulation_data.py:1371 — actuator_count may not be initialized
actuator_count is only set inside a conditional block at line 1337, but accessed unconditionally at line 1371. This will raise AttributeError when MuJoCo actuator attributes are missing.
🟡 Warning: articulation_data.py:1372-1373 — Ctrl buffers have wrong shape
Buffers are shaped (actuator_count,) (1D) instead of (num_instances, actuator_count) (2D). This means ctrl values are shared across all environments instead of being per-environment. All other articulation buffers use the 2D pattern.
🟡 Warning: capsules.py:29 — Hardcoded local path
The USD path /home/rgresia/Repositories/tendon-assets/... only works on the author's machine. Should use ISAAC_NUCLEUS_DIR or a relative path.
🔵 Improvement: capsules.py:7-16 — Wrong docstring
Docstring describes Shadow Robot hand but this file is for capsules. Also, ImplicitActuatorCfg is imported but unused.
🔵 Improvement: capsules_flex_env.py:82 — Wrong type annotation syntax
tuple(torch.Tensor, torch.Tensor) should be tuple[torch.Tensor, torch.Tensor]. Using parentheses calls the tuple constructor instead of creating a type annotation.
🔵 Improvement: articulation.py:3249 — Missing space
self._ALL_ACTUATOR_INDICES =wp.array(...) is missing a space before =.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Outdated
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Outdated
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py
Show resolved
Hide resolved
source/isaaclab_tasks/isaaclab_tasks/direct/flex_capsules/capsules_flex_env.py
Show resolved
Hide resolved
source/isaaclab_tasks/isaaclab_tasks/direct/flex_capsules/capsules_flex_env.py
Show resolved
Hide resolved
source/isaaclab_tasks/isaaclab_tasks/direct/flex_capsules/capsules_flex_env.py
Show resolved
Hide resolved
Co-authored-by: isaaclab-review-bot[bot] <270793704+isaaclab-review-bot[bot]@users.noreply.github.com> Signed-off-by: nv-rgresia <rgresia@nvidia.com>
…nto fixed-tendons-impl
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up
Reviewed incremental changes since 90d3c5f. Several previous issues were addressed (signature mismatch fixed via write_actuator_ctrl_to_sim_index), but 3 new critical bugs were introduced in this push.
Previously addressed: ✅ write_actuator_ctrl_to_sim signature mismatch (replaced with write_actuator_ctrl_to_sim_index), env call-sites updated.
New findings below.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Show resolved
Hide resolved
| self._tendon_stiffness = wp.clone(self._sim_bind_tendon_stiffness) | ||
| self._tendon_damping = wp.clone(self._sim_bind_tendon_damping) | ||
| else: | ||
| self._tendon_stiffness = None |
There was a problem hiding this comment.
🟡 Warning: Changed from zero-sized array to None — breaks downstream code
Previously these were wp.zeros((num_instances, 0), ...) which is a valid empty array. Setting them to None means any code path that accesses .shape, .assign(), or passes them to Warp kernels will crash with AttributeError. Zero-sized arrays were the safer sentinel value.
| self._tendon_stiffness = None | |
| self._tendon_stiffness = wp.zeros((self._num_instances, 0), dtype=wp.float32, device=self.device) | |
| self._tendon_damping = wp.zeros((self._num_instances, 0), dtype=wp.float32, device=self.device) |
Description
Updates articulation to support the Mujoco actuator control api in order to allow control over fixed tendons. I tried to stick to existing patterns where possible, but articulation/data may need to be overhauled to support mjc actuators + tendons properly since they are no longer tied to a joint; configuration updates to follow. A prototype testing environment is included, but needs to be upgraded to a proper tutorial enviroment before merge.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there