Skip to content

fix(newton): use correct kernel launch dim in actuator effort copy#5113

Open
Cdfghglz wants to merge 1 commit intoisaac-sim:dev/newtonfrom
Cdfghglz:fix/newton-actuator-kernel-dim
Open

fix(newton): use correct kernel launch dim in actuator effort copy#5113
Cdfghglz wants to merge 1 commit intoisaac-sim:dev/newtonfrom
Cdfghglz:fix/newton-actuator-kernel-dim

Conversation

@Cdfghglz
Copy link
Copy Markdown

Summary

  • Fix kernel launch dimension bug in ImplicitActuator.compute() and IdealPDActuator.compute() (Newton backend)
  • The update_array2D_with_array2D_masked kernel was launched with self.num_joints (group size) instead of self._num_joints (total joints), preventing effort from being written to joints with indices >= group size
  • This caused explicit actuators (DelayedPD, IdealPD, DCMotor) to produce zero torque on most joints when multiple actuator groups are defined

Impact

Any robot using multiple explicit actuator groups on the Newton backend is affected. Robots with a single actuator group (all joints in one group) are not affected, nor are robots using only implicit actuators (Newton handles PD internally).

Example: A quadruped with 4 actuator groups (rockers, hips, front_knees, rear_knees) of 4 joints each across 16 total joints. The kernel iterates joint_index = 0..3 but joint_mask has True entries at positions like [6, 11, 12, 14] — these are never reached, producing zero torque.

Fix

self.num_jointsself._num_joints on 2 lines in source/isaaclab_newton/isaaclab_newton/actuators/actuator_pd.py. This is consistent with the other kernel launches (compute_pd_actuator, _clip_effort) in the same methods, which already use self._num_joints.

Test plan

  • Verified on Artaban v0.3 quadruped (4 groups × 4 joints) — robot was collapsing with zero torque on 9/12 actuated joints, now squats correctly
  • Existing CI tests should pass — the fix only affects robots with multiple explicit actuator groups

🤖 Generated with Claude Code

The `update_array2D_with_array2D_masked` kernel in both
`ImplicitActuator.compute()` and `IdealPDActuator.compute()` was
launched with `dim=(num_envs, self.num_joints)` where `self.num_joints`
is the number of joints in the actuator group, not the total joints in
the articulation. Since `joint_mask` is indexed over all joints, the
kernel never reached joints with indices >= group size.

This caused explicit actuator groups (DelayedPD, IdealPD, DCMotor) to
produce zero torque for most joints when multiple groups are used,
making the robot collapse under gravity.

Fix: use `self._num_joints` (= joint_mask.shape[0] = total joints)
which is consistent with the other kernel launches in the same methods.
@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Mar 26, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR fixes a one-line (per occurrence) kernel launch dimension bug in the Newton backend's ImplicitActuator.compute() and IdealPDActuator.compute() methods. The update_array2D_with_array2D_masked kernel was launched with self.num_joints (the number of joints in this actuator group) instead of self._num_joints (the total number of joints in the articulation), causing the kernel thread indices to never reach any joint positions with indices ≥ the group size — resulting in zero torque on those joints.\n\n- self.num_joints is a property returning len(self._joint_names) (group size, e.g. 4 for one of four actuator groups)\n- self._num_joints is set in ActuatorBase.__init__ as joint_mask.shape[0], where joint_mask spans all n_dof joints in the articulation (e.g. 16)\n- The Warp kernel uses wp.tid() to get (index_1, index_2) and then checks mask_2[index_2]; if the launch dimension is capped at the group size, any joint whose absolute index exceeds that value is never visited\n- The fix is consistent with all three other kernel launches in the same methods (compute_pd_actuator and clip_efforts_with_limits/clip_efforts_dc_motor), which already correctly used self._num_joints\n- DCMotor is implicitly fixed as it delegates compute() to IdealPDActuator.super().compute()\n- The source and destination arrays (_actuator_effort_target, _applied_effort, joint_effort) are all allocated with shape (n_envs, n_dof), so no out-of-bounds risk is introduced

Confidence Score: 5/5

Safe to merge — minimal, targeted fix with no side effects.

The change is exactly 2 characters per line (s/num_joints/_num_joints/), precisely mirrors the pattern already used by every other kernel launch in the same methods, and is backed by the invariant that joint_mask.shape[0] == n_dof which all the shared data arrays are also sized to. The root cause is clear and well-documented, verification was done on actual hardware, and no existing single-group robots are affected.

No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab_newton/isaaclab_newton/actuators/actuator_pd.py Fixes kernel launch dimension bug in ImplicitActuator and IdealPDActuator: replaces self.num_joints (group size) with self._num_joints (total joints) in two update_array2D_with_array2D_masked launches, consistent with all other kernel launches in the same methods.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["actuator.compute()"] --> B["wp.launch compute_pd_actuator\ndim=(num_envs, _num_joints ✓)"]
    B --> C["_clip_effort()\nwp.launch clip_efforts_with_limits\ndim=(num_envs, _num_joints ✓)"]
    C --> D["wp.launch update_array2D_with_array2D_masked\ndim=(num_envs, _num_joints ✓) FIXED"]

    subgraph Before_Fix
        D2["wp.launch update_array2D_with_array2D_masked\ndim=(num_envs, num_joints ✗)\nGroup size only — e.g. 4 of 16 joints"]
    end

    subgraph Kernel_Logic
        E["index_1, index_2 = wp.tid()"]
        E --> F{"mask_1[index_1] AND mask_2[index_2]?"}
        F -- Yes --> G["array_2d[i1,i2] = new_array_2d[i1,i2]"]
        F -- No --> H["skip"]
    end

    D -.-> E

    style D fill:#90ee90
    style D2 fill:#ffcccb
Loading

Reviews (1): Last reviewed commit: "fix(newton): use correct kernel launch d..." | Re-trigger Greptile

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 fixes a critical kernel launch dimension bug where update_array2D_with_array2D_masked was launched with self.num_joints (joints in the actuator group) instead of self._num_joints (total joints in the articulation). This caused the kernel to never iterate over joint indices beyond the group size, producing zero torque for most joints when multiple explicit actuator groups are used. The fix is correct and minimal.

Architecture Impact

No cross-module impact — the change is self-contained within actuator_pd.py. The two affected call sites (ImplicitActuator.compute() and IdealPDActuator.compute()) now match the launch dimension used by every other kernel in the same file (compute_pd_actuator, clip_efforts_with_limits, clip_efforts_dc_motor), all of which already use self._num_joints. DCMotor inherits from IdealPDActuator and calls super().compute(), so it is also fixed.

Implementation Verdict

Minor fixes needed — The fix itself is correct and should ship. However, the same bug exists in actuator_net.py (see findings below).

Test Coverage

🟡 No regression test included. This is a bug fix that caused silent zero-torque output — one of the worst failure modes in a robotics simulator. A unit test that creates multiple actuator groups and verifies joint_effort is non-zero for joints in all groups would catch this if the bug is reintroduced. Given that the newton backend test infrastructure is still minimal (only benchmarks and mocks exist), I understand the practical difficulty, but this should be tracked as follow-up.

CI Status

Only the labeler check ran and passed ✅. No pre-commit/lint/unit test CI checks appear configured for the dev/newton branch yet.

Findings

🔴 Critical: actuator_net.py:98,195 — Same bug exists in ActuatorNetLSTM.compute() and ActuatorNetMLP.compute()

Both network-based actuator classes launch update_array2D_with_array2D_masked with dim=(self._num_envs, self.num_joints) — the exact same bug this PR fixes in actuator_pd.py. Any user with multiple actuator groups using ActuatorNetLSTM or ActuatorNetMLP will hit the same zero-torque problem. These should be fixed in this PR.

🟡 Warning: No regression test — This bug silently produces incorrect physics. Without a test, nothing prevents re-introduction. Even a minimal test asserting joint_effort is written for high-index joints in a multi-group setup would suffice.

wp.launch(
update_array2D_with_array2D_masked,
dim=(self._num_envs, self.num_joints),
dim=(self._num_envs, self._num_joints),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fix is correct. self._num_joints (= joint_mask.shape[0] = total joints in articulation) is the right dimension for update_array2D_with_array2D_masked because the kernel indexes into joint_mask using wp.tid(). With the old self.num_joints (= len(self._joint_names) = group size), joints at indices ≥ group size were never reached.

Consistent with compute_pd_actuator at line 138 and _clip_effort in the base class, which both use self._num_joints.

⚠️ The same bug exists in actuator_net.pyActuatorNetLSTM.compute() (line 98) and ActuatorNetMLP.compute() (line 195) both have dim=(self._num_envs, self.num_joints) on their update_array2D_with_array2D_masked calls. Please fix those too:

actuator_net.py line 98 (ActuatorNetLSTM):

Suggested change
dim=(self._num_envs, self._num_joints),
dim=(self._num_envs, self._num_joints),

actuator_net.py line 195 (ActuatorNetMLP):

Suggested change
dim=(self._num_envs, self._num_joints),
dim=(self._num_envs, self._num_joints),

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

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants