Fix actuator gain snapshot corruption for multi-env floating-base articulations#6407
Conversation
The init-time gain snapshot kernel decoded environment and local joint slots from global model DOF indices using the articulation's joint count as the per-environment stride. Newton's global DOF space also contains free-root DOFs, so for floating-base articulations with more than one environment the decode misattributed later environments' joints and wrote past the snapshot buffer: silently corrupted gain randomization baselines on CUDA, and heap corruption aborts on CPU under newer Warp allocator layouts. Thread the true per-environment DOF stride from the adapter into the kernel; the PhysX call site passes an identical stride and is behaviorally unchanged. The existing suites missed this because the equivalence tests never read the snapshot and the randomization test reads controller parameters directly. The new regression test asserts the snapshot values for every environment row of a floating-base multi-environment articulation and fails deterministically without the fix.
github-actions
Bot
added
bug
Greptile SummaryThis PR fixes an out-of-bounds write in the
Confidence Score: 4/5Safe to merge — the fix is a targeted, well-scoped change to a single kernel parameter and its two call sites, with a dedicated regression test and thorough docstring explaining the invariants. The core logic is correct: env_stride (the whole-model per-env DOF count) is distinct from num_joints (the articulation-local destination-buffer stride), and the fix correctly separates them. The build_per_dof_env_mask_kernel is unaffected because NewtonActuatorAdapter.num_joints already carries the whole-model stride there. The only gap is that the regression test runs on cuda:0 only, leaving the deterministic CPU SIGABRT scenario without automated coverage. No files require special attention beyond the test device coverage note. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Art as Articulation._initialize_actuators
participant Fn as build_newton_actuator_defaults
participant K as scatter_gain_kernel (Warp)
participant Buf as flat_stiffness/flat_damping
Art->>Fn: "num_joints=12, env_stride=adapter.num_joints(18)"
loop per actuator with kp/kd
Fn->>K: "num_joints=12, env_stride=18, indices"
Note over K: env = global_dof // env_stride
Note over K: local_dof = global_dof - env*env_stride
Note over K: dst[env*num_joints + local_dof] = src[i]
K-->>Buf: correct (env, joint) slot
end
Fn-->>Art: stiffness, damping, joint_indices
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Art as Articulation._initialize_actuators
participant Fn as build_newton_actuator_defaults
participant K as scatter_gain_kernel (Warp)
participant Buf as flat_stiffness/flat_damping
Art->>Fn: "num_joints=12, env_stride=adapter.num_joints(18)"
loop per actuator with kp/kd
Fn->>K: "num_joints=12, env_stride=18, indices"
Note over K: env = global_dof // env_stride
Note over K: local_dof = global_dof - env*env_stride
Note over K: dst[env*num_joints + local_dof] = src[i]
K-->>Buf: correct (env, joint) slot
end
Fn-->>Art: stiffness, damping, joint_indices
Reviews (1): Last reviewed commit: "Fix actuator gain snapshot env stride de..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| gravity_enabled=True, | ||
| add_ground_plane=True, | ||
| sim_cfg=sim_cfg, | ||
| ) as sim: |
There was a problem hiding this comment.
cuda:0
The PR description explicitly calls out the CPU failure mode (glibc corrupted size vs. prev_size → deterministic SIGABRT) as a separate, more visible symptom of the same OOB write. The new test is hardcoded to device="cuda:0", so a CPU-only CI runner would never exercise this path. If the test suite has a mechanism to parameterize over devices, adding a "cpu" variant here would directly cover the documented CPU crash scenario.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Description
The init-time actuator gain snapshot (
scatter_gain_kernel, shipped with the actuators integration #5455) decodes environment/local joint slots from global model DOF indices using the articulation's joint count as the per-environment stride. Newton's global DOF space also contains free-root DOFs, so for a floating-base articulation with more than one environment the per-env stride is larger than the joint count (ANYmal: 18 vs 12), later environments' joints decode to the wrong or a nonexistent environment slot, and the kernel writes past the snapshot buffer.Observed failure modes, all from the same OOB write:
randomize_actuator_gainsbaselines (actuator.stiffness/dampingdefaults) are corrupted (env-1 rows partially zero / misplaced) with no error.corrupted size vs. prev_size→ deterministic SIGABRT at articulation init. Older warp allocator layouts absorbed the write silently.Why no existing test caught it: the equivalence suites build multi-env floating-base ANYmal with Newton actuators — they execute the buggy scatter — but only compare joint trajectories; the corrupted snapshot never feeds the sim step. The gain-randomization test reads
controller.kp/kddirectly rather than the snapshot. Verified side-by-side: the DR test passes while the new regression test fails on the identical unfixed build.Fix: thread the true per-environment DOF stride (
adapter.num_joints, which by construction equals the model's per-env DOF count on both backends) into the kernel and decodeenv = global_dof // env_stride. The PhysX call site passes a stride identical to its joint count and is behaviorally unchanged (covered by its existing tests).patch_actuator_param_kernelshares the decode pattern but its only live caller guarantees stride == joint count; documented that invariant in its docstring. All other kernels in the module audited clean.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdTest Plan
TestNewtonActuatorGainSnapshotEnvStride::test_snapshot_matches_config_for_all_envs(floating-base ANYmal, 2 envs, explicit PD gains) — FAILS without the fix (Mismatched elements: 6 / 24, env-1 rows zeroed), PASSES with it./isaaclab.sh -p -m pytest source/isaaclab_newton/test/assets/test_newton_actuators_newton.py→ 72 passedTestRandomizeActuatorGainsViaEventsPhysx,TestIdealPDEquivalence) → 6 passed (signature threading is a no-op on PhysX)./isaaclab.sh -f→ all hooks pass