Adds fixed tendon support to articulation#5164
Adds fixed tendon support to articulation#5164nv-rgresia wants to merge 3 commits intoisaac-sim:developfrom
Conversation
nv-rgresia
added
enhancement
github-actions
bot
added
bug
![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 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 =.
| Args: | ||
| ctrl: Value(s) to update. If array, shape is | ||
| """ | ||
|
|
There was a problem hiding this comment.
🔴 Critical: Empty method body
This method does nothing after resolving env_ids. The docstring is placed after the first statement, so it becomes a no-op string expression. The actual ctrl-setting logic is missing.
| def set_fixed_tendon_ctrl( | |
| self, | |
| *, | |
| ctrl: float | torch.Tensor | wp.array | None = None, | |
| tendon_ids: Sequence[int] | torch.Tensor | wp.array | None = None, | |
| env_ids: Sequence[int] | wp.array | None = None, | |
| ) -> None: | |
| """Set the Mujoco CTRL buffer for fixed tendon actuators. | |
| .. note:: | |
| This method only sets the ctrl buffer. Call write_actuator_ctrl_to_sim to update the sim buffer. | |
| Args: | |
| ctrl: Value(s) to update. Shape should be (len(env_ids), len(tendon_ids)) or broadcastable. | |
| tendon_ids: Tendon indices to update. If None, all tendons are updated. | |
| env_ids: Environment indices. If None, all environments are updated. | |
| """ | |
| env_ids = self._resolve_env_ids(env_ids) | |
| # TODO: Implement actual ctrl buffer update logic | |
| raise NotImplementedError("set_fixed_tendon_ctrl is not yet implemented") |
|
|
||
|
|
||
| def write_actuator_ctrl_to_sim(self) -> None: | ||
| self.data._sim_bind_actuator_ctrl.assign(self.data._actuator_ctrl) |
There was a problem hiding this comment.
🔴 Critical: Signature doesn't match usage
This method takes no arguments, but the environment code calls:
write_actuator_ctrl_to_sim(ctrl=[-1.0, -1.0])(line 56)write_actuator_ctrl_to_sim(ctrl=wp.to_torch(...)[env_ids])(line 98)
Both will raise TypeError: got unexpected keyword argument 'ctrl'.
Either add a ctrl parameter here, or remove the ctrl= argument from the env calls and have the caller set self.data._actuator_ctrl first.
| model = SimulationManager.get_model() | ||
| mujoco_attrs = getattr(model, "mujoco", None) | ||
| if mujoco_attrs is None or not hasattr(mujoco_attrs, "actuator_trntype"): | ||
| raise Exception("Mujoco has no actuator trntype") |
There was a problem hiding this comment.
🔴 Critical: Breaks non-tendon articulations
This unconditionally raises when MuJoCo actuator attributes are missing, but _process_tendons() is called for ALL articulations. Any Newton articulation without MuJoCo actuators will crash here.
| raise Exception("Mujoco has no actuator trntype") | |
| if mujoco_attrs is None or not hasattr(mujoco_attrs, "actuator_trntype"): | |
| # No MuJoCo actuators — nothing to process for tendons | |
| return |
| self._ALL_ENV_MASK = wp.ones((self.num_instances,), dtype=wp.bool, device=self.device) | ||
| self._ALL_JOINT_INDICES = wp.array(np.arange(self.num_joints, dtype=np.int32), device=self.device) | ||
| self._ALL_JOINT_MASK = wp.ones((self.num_joints,), dtype=wp.bool, device=self.device) | ||
| self._ALL_ACTUATOR_INDICES =wp.array(np.arange(self.data.actuator_count, dtype=np.int32), device=self.device) |
There was a problem hiding this comment.
🔵 Style: Missing space before =
| self._ALL_ACTUATOR_INDICES =wp.array(np.arange(self.data.actuator_count, dtype=np.int32), device=self.device) | |
| self._ALL_ACTUATOR_INDICES = wp.array(np.arange(self.data.actuator_count, dtype=np.int32), device=self.device) |
| ) | ||
|
|
||
| # -- mujoco ctrl api | ||
| if self.actuator_count > 0: |
There was a problem hiding this comment.
🔴 Critical: actuator_count may not be defined
self.actuator_count is only set at line 1337 inside if hasattr(mujoco_attrs, "actuator_trntype"). If that condition is false, this line raises AttributeError.
| if self.actuator_count > 0: | |
| # -- mujoco ctrl api | |
| if hasattr(self, 'actuator_count') and self.actuator_count > 0: |
Or better: initialize self.actuator_count = 0 before the conditional block in _create_simulation_bindings.
|
|
||
| CAPSULES_CFG = ArticulationCfg( | ||
| spawn=sim_utils.UsdFileCfg( | ||
| usd_path=f"/home/rgresia/Repositories/tendon-assets/tendon-capsules.usda/capsules.usda", |
There was a problem hiding this comment.
🔴 Critical: Hardcoded local path
This path only exists on your machine. Use a Nucleus path or a path relative to the asset directory.
| usd_path=f"/home/rgresia/Repositories/tendon-assets/tendon-capsules.usda/capsules.usda", | |
| usd_path=f"{ISAAC_NUCLEUS_DIR}/Robots/Capsules/capsules.usda", |
Or if the asset isn't on Nucleus yet, document that this is a placeholder and add a TODO.
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| """Configuration for the dexterous hand from Shadow Robot. | ||
|
|
There was a problem hiding this comment.
🔵 Improvement: Wrong docstring
This docstring describes Shadow Robot hand, but this file defines capsules configuration. Also, ImplicitActuatorCfg is imported but unused.
| """Configuration for a tendon-actuated capsule robot. | |
| The following configurations are available: | |
| * :obj:`CAPSULES_CFG`: Capsule robot with fixed tendon actuators. | |
| """ |
| self.joint_vel = wp.to_torch(self.robot.data.joint_vel) | ||
|
|
||
| time_out = self.episode_length_buf >= self.max_episode_length - 1 | ||
| out_of_bounds = torch.any(torch.abs(self.joint_pos) > math.pi , dim=1) |
There was a problem hiding this comment.
🔵 Style: Wrong type annotation syntax
tuple(...) calls the constructor. Use tuple[...] for type annotations.
| out_of_bounds = torch.any(torch.abs(self.joint_pos) > math.pi , dim=1) | |
| def _get_dones(self) -> tuple[torch.Tensor, torch.Tensor]: |
| obs = torch.cat( | ||
| ( | ||
| self.joint_pos[:, 0].unsqueeze(dim=1), | ||
| self.joint_pos[:, 1].unsqueeze(dim=1), |
There was a problem hiding this comment.
🔴 Critical: Method signature mismatch
write_actuator_ctrl_to_sim() takes no arguments, but you're passing ctrl=. This will raise TypeError.
Either:
- Update
write_actuator_ctrl_to_simto accept actrlparameter, or - Set
self.robot.data._actuator_ctrldirectly before calling the method
|
|
||
| self.robot.write_actuator_ctrl_to_sim(ctrl=wp.to_torch(self.robot.data._default_actuator_ctrl)[env_ids]) | ||
|
|
||
| # self.robot.write_root_pose_to_sim_index(default_root_state[:, :7], env_ids) |
There was a problem hiding this comment.
🔴 Critical: Multiple issues
- Same signature mismatch as line 56 —
ctrl=is not a valid argument _default_actuator_ctrlis shaped(actuator_count,), so[env_ids]indexes into actuators, not environments. This is semantically wrong.- Accessing private
_default_actuator_ctrldirectly — should use a public API
1 participant
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