WIP: DR Legs

[aserifi]

DR Legs closed-loop hold-pose and walk task

Opening early for visibility. Parts of this PR are moving into digestible sub-PRs #6026 #6027 #6028, leaving the pure DR Legs task here.

• Adds the Disney DR Legs closed-loop biped (Kamino solver) and a Isaac-DrLegs-HoldPose-v0 hold-pose task as well as Isaac-DrLegs-Walk-v0 walking task.
• New ClosedLoopView: a duck-typed ArticulationView for robots with cyclic joint graphs / no ArticulationRootAPI (DR Legs can't form a tree). Open question is this adapter the right direction, or should closed-loop support be handled differently? Happy to discuss.
• Fixes Kamino environment resets: per-env reset to the init pose, scoped FK invalidation, no cross-env leakage or post-reset velocity spike. These work but are a bit of a hack and could be cleaner. I think there are many open threads about this.
• DR Legs USD is loaded from a local path for now; @AntoineRichard already sent a request to the Nucleus team.
• Follow-ups: Walk task relies on Newton PR 3095 for contact force agg.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This WIP PR adds support for the Disney DR Legs closed-loop biped robot (36 joints: 12 actuated, 24 passive linkage) to Isaac Lab using the Kamino solver. Key additions:

1. ClosedLoopView — a duck-typed replacement for ArticulationView that provides strided views into Newton's global arrays for robots with cyclic kinematic loops (no ArticulationRootAPI).
2. Kamino environment reset fixes — per-env scoped FK invalidation and dual-state sync to prevent cross-env leakage and post-reset velocity spikes.
3. route_torque_to actuator routing — new ClassVar on ActuatorBase to route torques to Control.joint_act (Kamino implicit constraint) instead of Control.joint_f.
4. Isaac-DrLegs-HoldPose-v0 task — a manager-based RL hold-pose task with full reward/obs/termination/event config.
5. newton_kamino presets — added across ~10 existing locomotion/manipulation env configs.

Findings

🔴 Critical / Correctness

1. manager_based_rl_env.py — unconditional write_data_to_sim() + sim.forward() + scene.update() after reset (line ~255)
   This affects all environments, not just Kamino/DR-Legs. The comment says "for others this is a no-op", but write_data_to_sim() will flush any dirty buffers for PhysX/MJWarp envs too, and sim.forward() is never free — it re-evaluates the sim state. This should be guarded by a check (e.g. if self.cfg.sim.physics.solver_cfg.solver_type == "kamino" or a flag on the physics manager) to avoid performance regression on the ~20+ existing PhysX/MJWarp tasks. At minimum, this needs benchmarking.

2. ClosedLoopView._get_strided_view — raw pointer arithmetic without bounds checking
   The method computes base_ptr = int(attrib.ptr) + val_offset * int(value_stride) and constructs a wp.array from it. If val_offset >= items_per_world (misconfigured frequency lookup), this silently reads out-of-bounds GPU memory. An assertion assert val_offset < items_per_world would catch misconfigurations early.

3. ClosedLoopView assumes uniform distribution of bodies/joints/shapes across worlds (model.body_count // nw etc.)
   This assumption breaks if any world has a different number of bodies (e.g. debug prims added to world 0). The code should validate model.body_count % nw == 0 at construction time and raise if not.

4. _forward_kamino — .numpy() calls on GPU tensors in the hot path (lines ~50-70 of kamino_manager.py)
   _model.joint_type.numpy()[0] and _model.joint_child.numpy() do GPU→CPU synchronization on every reset. These values are model constants — they should be computed once at solver construction time and cached.

🟡 Design Concerns

5. _prim_has_closed_kinematic_loops called inside _initialize_impl for EVERY articulation
   This iterates the full USD prim subtree on every env. For large scenes with many articulations, this is potentially expensive. Consider caching the result per prim path or moving the check to builder time.

6. ClosedLoopView.articulation_ids returns a freshly-allocated wp.zeros(...) every access
   This is a @property that allocates on every call. If any code calls it in a loop, it creates garbage. Should be cached as self._articulation_ids.

7. _joint_act_target_sim buffer is never zeroed between steps
   _joint_effort_target_sim appears to be zeroed/overwritten each step by _apply_actuator_model, but if an actuator switches route_torque_to dynamically (or if not all joint indices are covered), the _joint_act_target_sim buffer may accumulate stale values. Ensure it's zeroed at the start of each write_data_to_sim.

8. ActionRate2L2.reset uses env_ids as a tensor slice but also accepts None
   When env_ids is None, slice(None) correctly zeros everything. But the __call__ method updates _prev_action/_prev_prev_action unconditionally — after a partial reset, the history for non-reset envs is correct, but the just-reset envs get the current action stored into their history. This means the first reward computation after reset computes jerk against zero (correct), but the second step computes jerk relative to the action taken during the reset step (also correct). This is fine — just noting it's a design choice.

🟢 Minor / Nits

9. DR_LEGS_JOINT_ORDER hardcodes 36 joint names — if the USD changes, this silently diverges. A runtime validation (asserting discovered joint names match the list) would be defensive.

10. hold_pose_env_cfg.py imports mdp.JointPositionActionCfg via the wildcard re-export — this works but makes it non-obvious where JointPositionActionCfg originates.

11. Several reward terms (base_lin_vel_l2, base_ang_vel_l2) shadow stock Isaac Lab MDP terms with identical names — since the module does from isaaclab.envs.mdp import * then from .rewards import *, the local definitions override the stock ones silently. This is intentional (per the module docstring) but should be documented with a comment at the override site.

12. gait_phase observation function is defined but never used in ObservationsCfg — dead code for the hold-pose task (presumably for the upcoming walk task).

Verdict

Significant concerns

The ClosedLoopView adapter and Kamino reset logic are architecturally sound for a WIP, but the unconditional post-reset write_data_to_sim()/sim.forward() in manager_based_rl_env.py (item 1) has regression risk for all existing envs and needs to be gated. The .numpy() calls in the reset hot path (item 4) will also cause performance issues at scale. These should be addressed before merging.

---

Update (fa8efb0): Reviewed incremental diff (300 files changed since 0c28922). This is a large commit that touches the PR's DR Legs work and sweeps across the broader codebase (lazy pxr imports, test renames, install CI refactor, version bumps to 6.3.0, etc.).

Changes relevant to this PR

1. manager_based_rl_env.py — unconditional post-reset write_data_to_sim()/sim.forward()/scene.update() is now committed (Finding #1 from original review).

   • The comment says "for others this is a no-op", which is inaccurate — write_data_to_sim() + sim.forward() are never free (they flush Fabric/PhysX state).
   • ⚠️ Status: Still unguarded. Should be conditioned on solver type or at least benchmarked on PhysX tasks before merging out of WIP.

2. EventTermCfg.resample_interval_on_reset added — new bool field (default True) lets interval event terms preserve their counter across resets. Clean design, well-documented, tested in both the stable and experimental event managers. ✅ Good.

3. dr_legs.py asset config added — the 36-joint split (12 actuated / 24 passive) is cleanly documented with canonical joint ordering and explicit zero-PD for passive linkages. Uses local data/ directory for USD asset (with TODO to switch to Nucleus). ✅

4. Lazy pxr imports across isaaclab.sim — massive refactor moving top-level from pxr import ... into function-level imports behind TYPE_CHECKING. This is unrelated to DR Legs but significantly changes the isaaclab.sim module. Well-structured: runtime imports in function bodies, type stubs preserved.

5. NewtonSDFCollisionPropertiesCfg added to lazy-forwarding sets in isaaclab.sim.__init__ and isaaclab.sim.schemas.__init__. Test coverage added in test_schemas_shim.py. ✅

6. isaaclab_experimental lazy exports — envs, envs.mdp, managers packages converted to lazy export with .pyi stubs. This avoids eager backend imports at config-load time. ✅ Good for Warp task configs.

7. Gravity kernel fix in isaaclab_experimental observations/rewards: now reads per-env gravity_w[i] and normalizes, instead of always reading gravity_w[0]. Fixes per-env gravity randomization on Newton. ✅

Previous findings status

#	Finding	Status
1	🔴 Unconditional post-reset write_data_to_sim()/sim.forward()	Still present, now committed — needs gating
2	🔴 .numpy() GPU→CPU sync in Kamino reset	Not addressed in this diff
3	🔴 ClosedLoopView uniform distribution assumption	Not addressed in this diff
4	🟡 articulation_ids allocates on every access	Not addressed
5	🟡 _joint_act_target_sim never zeroed	Not addressed

New observations

8. from_files.py — RigidBodyMaterialCfg replaced with PhysxRigidBodyMaterialCfg (direct import from isaaclab_physx). This tightens the coupling to PhysX for compliant contact materials in spawn_from_usd_with_compliant_contact_material. Intentional since compliant contacts are PhysX-only, but breaks the shim pattern used elsewhere.

9. Version bump to 6.3.0 (from 6.1.0) with pin-pink and daqp pinned to exact versions (==3.1.0, ==0.8.5) to avoid breaking changes. Reasonable defensive pinning.

10. modify_fixed_tendon_properties now handles MjcTendon prims — clean branching on prim_type != "MjcTendon". No issues.

Verdict

The PR continues to mature. The DR Legs asset config is clean and well-documented. The biggest outstanding concern remains Finding #1 (unconditional post-reset physics step for all backends). The other critical findings (#2, #3) from the Kamino/ClosedLoopView code remain unaddressed but may be in a future commit.

---

Update (51ed92e): Incremental diff contains only directory renames (moving dr_legs from manager_based/locomotion/ to contrib/) and a corresponding import path fix in hold_pose_env_cfg.py. No substantive code changes. Previous findings remain unaddressed (as expected for a reorganization commit).

---

Update (16734e4): Single-file incremental change — adds a tendon_count property to ClosedLoopView (returns hardcoded 0). This satisfies the ArticulationView duck-type interface so that ArticulationData can allocate empty tendon bindings without crashing. Clean, well-documented docstring explaining why closed-loop assets don't expose MuJoCo-style fixed tendons. ✅ No issues.

Previous critical findings (#1–#3) remain unaddressed.

---

Update (f6ed606): Single-file incremental change — renames state_out= keyword argument to state= in three cls._solver.reset() calls in kamino_manager.py. This aligns with an upstream Kamino solver API rename. No functional change. ✅ No issues.

Previous critical findings (#1–#3) remain unaddressed.

Update (2f9d208): Incremental diff adds the velocity-tracking walk task, Kamino contact force aggregation, and a wp.bool type cleanup for _world_reset_mask.

Changes reviewed:

• kamino_manager.py — Adds ContactAggregation init in _build_solver (deferred import, cleaned in clear()). ✅
• newton_manager.py — _world_reset_mask dtype changed from wp.int32 → wp.bool consistently across all kernels and allocations. ✅ Clean type-correctness refactor.
• contact_sensor.py / contact_sensor_kernels.py — New Kamino-specific path reads per-body net forces via gather_body_net_forces_kernel. Lazy body-index init, proper env_mask/timestamp guards. ✅
• walk_env_cfg.py — New walk env config extending hold-pose with velocity commands, gait-phase obs, contact sensor, and reward suite. Well-structured. ✅
• mdp/rewards.py — feet_flat added (penalizes foot roll/pitch), feet_collision removed (incompatible with Kamino aggregation-only contacts). ✅
• __init__.py — Registers Isaac-DrLegs-Walk-v0. ✅

Previous findings status:

• Finding #12 (gait_phase dead code): ✅ Resolved — now used in WalkObservationsCfg.
• Findings #1–#3 (unconditional post-reset sim step, .numpy() in hot path, uniform body assumption): Still unaddressed (expected for WIP).

No new issues introduced in this increment.

---

Update (4e636bd): Incremental diff is dominated by base-branch changes (locomotion task restructuring into core/locomotion/{ant,humanoid}, reach/ant/humanoid Gym ID renames dropping -v0, preset CLI fold_preset_tokens removal, typed selector validation, visualizer fixes, version bumps). These are unrelated to DR Legs.

DR Legs-specific changes:

• rsl_rl_ppo_cfg.py renamed HumanoidPPORunnerCfg → DrLegsHoldPosePPORunnerCfg/DrLegsWalkPPORunnerCfg with proper hyperparameters (512×3 network, obs_groups for asymmetric critic, entropy 0.001, LR 1e-3 adaptive). Clean. ✅
• Walk task DrLegsWalkPPORunnerCfg inherits hold-pose and overrides only experiment name/max_iterations via __post_init__. ✅
• No changes to ClosedLoopView, kamino_manager.py, or manager_based_rl_env.py in this increment.

Previous findings status — unchanged:

• Findings #1–#3 (unconditional post-reset sim step, .numpy() in hot path, uniform body assumption): Still unaddressed (expected for WIP).
• No new issues introduced in the DR Legs changes.

---

Update (be83521): Incremental diff removes the Kamino-specific ContactAggregation path from contact_sensor.py and kamino_manager.py, replacing it with a single unified copy_from_newton_kernel path for all solver backends. The comment explains that Newton's SensorContact now accumulates per-contact forces from Contacts.force (which both MuJoCo-Warp and Kamino solvers populate in their update_contacts conversions). This is a clean simplification — one code path instead of two, with the aggregation responsibility pushed upstream into Newton. ✅ No issues.

Previous findings status — unchanged:

• Findings #1–#3 (unconditional post-reset sim step, .numpy() in hot path, uniform body assumption): Still unaddressed (expected for WIP).