Skip to content

Adds fixed tendon support to articulation#5164

Draft
nv-rgresia wants to merge 10 commits intoisaac-sim:developfrom
nv-rgresia:fixed-tendons-impl
Draft

Adds fixed tendon support to articulation#5164
nv-rgresia wants to merge 10 commits intoisaac-sim:developfrom
nv-rgresia:fixed-tendons-impl

Conversation

@nv-rgresia
Copy link
Copy Markdown
Contributor

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

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@nv-rgresia nv-rgresia self-assigned this Apr 3, 2026
@nv-rgresia nv-rgresia added enhancement New feature or request help wanted Extra attention is needed labels Apr 3, 2026
@github-actions github-actions bot added bug Something isn't working asset New asset feature or request labels Apr 3, 2026
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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_idx mapping 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_tendons and fixed_tendon_names now 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:

  1. Articulations with tendons can be created and ctrl buffers are accessible
  2. write_actuator_ctrl_to_sim correctly updates the simulation
  3. Articulations WITHOUT tendons still work (regression test for the _process_tendons change)

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 =.

nv-rgresia and others added 5 commits April 5, 2026 17:02
Co-authored-by: isaaclab-review-bot[bot] <270793704+isaaclab-review-bot[bot]@users.noreply.github.com>
Signed-off-by: nv-rgresia <rgresia@nvidia.com>
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request bug Something isn't working enhancement New feature or request help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant