Kamino task presets

[aserifi]

Description

This is part of the Kamino integration into IsaacLab work.
Stacked on #6026 and #6027

After the latest changes in newton-physics/newton#2822, we can expose the Kamino solver as an opt-in physics preset on a set of existing tasks. Default backends are unchanged.

• Add a newton_kamino preset to the velocity (cassie, g1, go2, h1, spot, a1, anymal_b, anymal_c, go1), reach, cabinet, shadow-hand, and humanoid task configs.
• Minor reach table init-pose tweak so the task behaves under the Kamino preset.

[isaaclab-review-bot]

Code Review: Kamino Task Presets

This PR adds Kamino solver integration as opt-in physics presets across velocity, reach, cabinet, shadow-hand, and humanoid tasks. The core changes include a new ClosedLoopView for closed kinematic chains, torque routing via route_torque_to, and significant rework of the Kamino reset logic.

Summary

The architecture is well-conceived — making Kamino an opt-in preset that doesn't change default behavior is the right approach. The ClosedLoopView duck-typing pattern and the _get_root_view_articulation_ids defensive accessor are good defensive designs. However, there are several issues worth addressing around robustness, potential performance regressions, and missing safety checks.

Findings

---

1. 🔴 Performance regression: unconditional write_data_to_sim + sim.forward() + scene.update() on every reset step (all backends)

File: source/isaaclab/isaaclab/envs/manager_based_rl_env.py

The comment says "for others this is a no-op", but self.scene.write_data_to_sim(), self.sim.forward(), and self.scene.update(dt=self.physics_dt) are not no-ops for PhysX or MuJoCo-Warp. These calls flush buffers, run forward kinematics, and update all scene entities. This effectively doubles the per-step scene update cost when resets occur (which is frequent in RL training).

Consider gating this behind a check for maximal-coordinate solvers:

if getattr(self.sim, 'is_maximal_coordinate_solver', False):
    self.scene.write_data_to_sim()
    self.sim.forward()
    self.scene.update(dt=self.physics_dt)

Or add a backend-specific hook that is a true no-op for non-Kamino solvers.

---

2. 🟡 _forward_kamino uses .numpy() inside a hot path — GPU→CPU sync stall

File: source/isaaclab_newton/isaaclab_newton/physics/kamino_manager.py (_forward_kamino method)

Multiple .numpy() calls appear inside _forward_kamino:

• _model.joint_type.numpy()[0]
• _model.joint_child.numpy()[: max(_joints_per_world, 0)]

These force GPU→CPU synchronization on every reset. Since the model topology is static, consider caching _first_joint_is_free, _per_world_children, and related derived values at solver construction time (in _build_solver) rather than recomputing them per call.

---

3. 🟡 Integer division assumption in ClosedLoopView can silently produce incorrect strides

File: source/isaaclab_newton/isaaclab_newton/assets/articulation/closed_loop_view.py (constructor)

The view computes per-world counts via integer division:

self._bodies_per_world = model.body_count // nw
self._joints_per_world = model.joint_count // nw

If model.body_count is not evenly divisible by world_count (e.g., due to shared kinematic anchors or asymmetric world construction), this silently produces incorrect strides, leading to memory access violations or corrupted data. Add a divisibility assertion:

assert model.body_count % nw == 0, f"body_count {model.body_count} not divisible by world_count {nw}"

---

4. 🟡 world_reset_mask dtype changed from wp.int32 to wp.bool — verify Kamino solver API compatibility

File: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py

The _world_reset_mask dtype was changed from wp.int32 to wp.bool. The Kamino solver.reset(world_mask=...) API must accept boolean masks — if it expects int32 (as it did before this PR), this will cause a silent type mismatch or runtime error. Please confirm the upstream SolverKamino.reset() signature accepts wp.bool arrays, or add an explicit cast before passing to the solver.

---

5. 🟡 _joint_act_target_sim buffer is never zeroed between steps

File: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py (_apply_actuator_model)

The new _joint_act_target_sim buffer is allocated with wp.zeros_like(...) at creation, but unlike _joint_effort_target_sim, it's never explicitly zeroed at the start of each write_data_to_sim call. If an actuator switches route_torque_to between steps, or if only some actuators route to joint_act, stale values from a previous step will persist for unwritten indices. Consider zeroing both target buffers at the top of _apply_actuator_model.

---

6. 🟡 _prim_has_closed_kinematic_loops silently swallows errors during USD traversal

File: source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py

The broad except Exception: continue block:

try:
    joint = UsdPhysics.Joint(descendant)
    targets = joint.GetBody1Rel().GetTargets()
except Exception:
    continue

This swallows any exception — including RuntimeError from invalid stage state, AttributeError from API changes, or memory errors. If the USD stage is in an inconsistent state, this function will silently return False (no loops detected), causing the code to attempt tree-structured articulation on a closed-loop robot, likely producing a cryptic downstream crash. Consider narrowing the catch or at minimum logging a warning:

except (ValueError, Tf.ErrorException) as exc:
    logger.debug("Skipping joint %s during loop detection: %s", descendant.GetPath(), exc)
    continue

---

7. 🟡 NewtonKaminoManager.forward() runs full eval_fk on all worlds when no reset is pending

File: source/isaaclab_newton/isaaclab_newton/physics/kamino_manager.py (forward method)

When _kamino_needs_reset is False, the method falls through to:

eval_fk(cls._model, cls._state_0.joint_q, cls._state_0.joint_qd, cls._state_0, None)

Passing None as the mask means FK is recomputed for all articulations/worlds unconditionally. Since forward() is called from the new post-reset path in manager_based_rl_env.py (and potentially from other call sites), this could be expensive for large world counts. Consider either: (a) passing an empty mask to make it a no-op when not needed, or (b) adding an early return when no FK invalidation has occurred.

---

8. 🟡 Inconsistent max_contacts_per_world across task presets — missing rationale

Files: Various task config files

Some presets specify max_contacts_per_world=64 (a1, cassie, g1, h1, cabinet, reach=32) while others leave it as None (anymal_b, anymal_c, go1, go2, spot, humanoid, shadow_hand). The PR description doesn't explain the selection criteria. For contact-rich tasks like shadow_hand (24 fingers + object) and humanoid (many body links), the absence of a cap could cause GPU OOM at scale, which is exactly what this parameter was designed to prevent.

---

Minor Observations

• The reach table init-pose change (pos=(0.5, 0.15, -0.5) → pos=(0.5, 0, -0.5)) modifies the newton_mjwarp preset, not just the new kamino preset. Confirm this is intentional and doesn't break existing MuJoCo-Warp reach training.
• _prim_has_closed_kinematic_loops only detects cycles via multi-parent children (Body1Rel). Exotic topologies where the cycle manifests differently may be missed — worth a brief comment noting this limitation.
• The changelog files are well-written and properly categorized. 👍

Verdict

The PR is architecturally sound and the opt-in preset pattern is the correct integration strategy. The main concern is the potential performance regression for all backends from the unconditional post-reset refresh (Finding #1). The GPU→CPU sync in the hot path (Finding #2), the missing buffer zeroing (Finding #5), and the silent error swallowing in loop detection (Finding #6) should also be addressed before merge. The remaining items are lower priority but worth considering.

[isaaclab-review-bot]

🔴 Performance concern: These three calls (write_data_to_sim, sim.forward, scene.update) are not no-ops for PhysX/MuJoCo-Warp backends — they flush buffers, run FK, and update all scene entities. This can double the per-step scene-update cost when resets occur (which is common in RL). Consider gating behind a solver-type check or adding a backend hook that's truly a no-op for non-maximal-coordinate solvers.

[isaaclab-review-bot]

🟡 GPU→CPU sync in hot path: _model.joint_type.numpy()[0] and _model.joint_child.numpy() force device synchronization on every reset call. Since model topology is static after construction, consider caching these derived values (_first_joint_is_free, _per_world_children, etc.) at solver construction time in _build_solver().

[isaaclab-review-bot]

🟡 Silent corruption risk: If model.body_count is not evenly divisible by world_count, the integer division silently produces incorrect per-world counts leading to wrong strides. Consider adding divisibility assertions here.

[isaaclab-review-bot]

🟡 Stale buffer values: _joint_act_target_sim is never zeroed between steps. If only some actuators route to joint_act, indices belonging to other actuators retain stale values from prior steps. Consider zeroing this buffer (and _joint_effort_target_sim) at the top of _apply_actuator_model.

[greptile-apps]

Greptile Summary

This PR exposes the Kamino PADMM solver as an opt-in newton_kamino physics preset across 13 existing tasks (velocity locomotion, reach, cabinet, shadow-hand, humanoid), and introduces the core infrastructure needed to make those tasks work: a ClosedLoopView adapter for cyclic-joint robots, a route_torque_to field on ActuatorBase for backward-Euler joint treatment, and a stale-FK fix in the RL env step loop.

• Adds ClosedLoopView as a duck-typed replacement for ArticulationView when cyclic joint graphs prevent tree articulation, with strided views into global Newton model arrays and automatic selection via _prim_has_closed_kinematic_loops.
• Fixes stale Kamino body poses after in-step resets by calling write_data_to_sim + sim.forward + scene.update in the reset block of ManagerBasedRLEnv.step; changes _world_reset_mask dtype from int32 to bool and adds a _kamino_needs_reset flag so solver.reset() only fires when an actual reset occurred.

Confidence Score: 3/5

The task preset additions are additive and safe; the deeper infrastructure changes in the RL env step loop touch a path that runs every training step for every backend, warranting a second look before merging.

The scene.update(dt=self.physics_dt) call added inside the reset block of ManagerBasedRLEnv.step executes unconditionally for PhysX, MJWarp, and Kamino backends alike. The changelog describes it as a no-op for non-Kamino backends, but sensors that maintain time-history or apply dt-dependent filtering will run a second update pass on all environments — including ones that did not reset — on every step that contains any resets. This is the hot path in large-scale RL training and affects the core env loop shared by all tasks.

source/isaaclab/isaaclab/envs/manager_based_rl_env.py — the extra scene.update call; source/isaaclab_newton/isaaclab_newton/assets/articulation/closed_loop_view.py — the articulation_ids allocation pattern

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/envs/manager_based_rl_env.py	Adds write_data_to_sim / sim.forward / scene.update after in-step resets to fix stale FK for Kamino; the extra scene.update runs unconditionally for all backends and may double-integrate sensor state for non-reset envs.
source/isaaclab_newton/isaaclab_newton/assets/articulation/closed_loop_view.py	New ClosedLoopView adapter that duck-types ArticulationView for closed-kinematic-chain robots; articulation_ids property allocates a new GPU array on every call (should be cached).
source/isaaclab_newton/isaaclab_newton/physics/kamino_manager.py	Significantly reworks _forward_kamino with use_fk_solver flag and proper state_1 sync; adds forward() to consume the _kamino_needs_reset flag from sim.forward() calls; changes step() to only run solver.reset() when needed.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	Changes world_reset_mask dtype from int32 to bool, adds _kamino_needs_reset flag, and adds new kernel paths for closed-loop assets without articulation_ids.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py	Adds joint_act_target buffer for Kamino backward-Euler path, replaces direct _root_view.articulation_ids accesses with _get_root_view_articulation_ids() helper, and adds ClosedLoopView instantiation path for cyclic-joint robots.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py	Adds joint_act_target ProxyArray binding in both first-time creation and rebind paths; correctly mirrored in both branches.
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py	Adds _prim_has_closed_kinematic_loops helper using UsdPhysics.Joint body child counting; registers SolverKamino custom attributes in builder loop.

Sequence Diagram

sequenceDiagram
    participant Env as ManagerBasedRLEnv
    participant Scene as Scene
    participant Sim as sim (forward/step)
    participant KM as NewtonKaminoManager

    Note over Env: RL training step (with resets)
    Env->>Sim: sim.step()
    Sim->>KM: step() — physics advance
    KM-->>Sim: "(no reset, _kamino_needs_reset=False)"
    Env->>Env: compute rewards / terminations
    Env->>Env: _reset_idx(reset_env_ids)
    Note over KM: invalidate_fk() sets _kamino_needs_reset=True, ORs _world_reset_mask
    Env->>Scene: write_data_to_sim()
    Env->>Sim: sim.forward()
    Sim->>KM: forward()
    KM->>KM: _forward_kamino(world_mask)
    Note over KM: solver.reset(), sync state_1, eval_fk
    KM->>KM: clear _kamino_needs_reset and masks
    Env->>Scene: scene.update(dt)
    Note over Scene: runs for ALL backends, not just Kamino
    Env->>Env: compute observations

Loading

Comments Outside Diff (1)

1. source/isaaclab_newton/isaaclab_newton/assets/articulation/closed_loop_view.py, line 461-470 (link)

   GPU allocation on every property access

   articulation_ids returns a freshly allocated wp.zeros((world_count, 0), ...) array on every call. Articulation._get_root_view_articulation_ids() calls this property, and that helper is invoked at six reset call sites. On training runs where thousands of environments reset every few steps, these repeated allocations from the GPU memory pool add up. Caching the empty array as an instance attribute (created once in __init__) and returning it from the property would eliminate the repeated allocations.

   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!

_{Reviews (1): Last reviewed commit: "Add newton_kamino presets to existing ta..." | Re-trigger Greptile}

[greptile-apps]

Extra scene.update() runs for all backends, not just Kamino

scene.update(dt=self.physics_dt) is unconditionally called inside the reset block for every backend. The comment says it's "a no-op for other backends", but this isn't accurate: sensors and scene entities that use the dt argument (e.g. contact sensors that maintain contact history or apply velocity-based filtering) will advance their internal time-integrators a second time within the same physics step for non-reset environments. The PhysX path's existing scene.update (from sim.step()) has already run; calling it again here would update non-reset envs' sensor state twice per step whenever any env resets — exactly the steps with the highest training throughput pressure.

Wrapping the extra calls in a Kamino-specific guard would avoid this for other backends.