[Task Clean-up] Dexterous Part 4/9: Fix MARL-to-single-agent training and enable handover Direct RSL-RL

[hujc7]

Review Map

• Exact changes: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution. This PR ([Task Clean-up] Dexterous Part 4/9: Fix MARL-to-single-agent training and enable handover Direct RSL-RL #6414) is now rebased directly onto develop, so its own Files changed tab is its contribution.
• Links pin specific SHAs and can go stale after a branch update — the table on [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324 is refreshed first.

PR	Status	Depends on	Exact changes
#6411 Part 1/9: Newton cloner/cubric/visualizer fixes		—	merged
#6412 Part 2/9: OVPhysX articulation + manager runtime		—	merged
#6413 Part 3/9: Reorient Direct, torch		—	merged
📌 #6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct (this PR)		— (on develop)	Files changed tab
#6418 Part 5/9: Reorient manager counterparts		#6413	changes
#6421 Part 6/9: Handover + camera manager counterparts		#6414, #6418	changes
#6410 Part 7/9: Environment overview docs		#6421	changes
#6415 Part 8/9: Benchmark success-rate utilities		#6421	changes
#6582 Part 9/9: Warp variants → experimental (draft; merges last)		—	changes
#6324 [DO-NOT-MERGE] Lumped validation reference		ALL	—

Summary

Converts the two-hand Shadow Hand handover task from multi-agent (MARL) to single-agent (enabling RSL-RL training), fixes the underlying MARL→single-agent observation bridge, and consolidates the shared Shadow Hand definitions into the asset.

• MARL → single-agent bridge (general, broader than this task). Single-agent RL libraries train DirectMARLEnv tasks via multi_agent_to_single_agent; the bridge dropped the latest observations from the public buffer. Fixed generally — RSL-RL observations read from the env-owned obs_buf, stored by reset like step on all env bases (DirectRLEnv, DirectMARLEnv, the experimental warp base). Every MARL task + single-agent runner benefits; handover is the first consumer.
• Handover Direct → single-agent + RSL-RL. Single-agent conversion, RSL-RL runner config, shared identity in handover_common, success-rate reward metrics, and a fix for its Newton construction failure (see below).
• Shadow Hand Newton robot → the asset. Moved the Newton (MJWarp) robot cfg into isaaclab_assets as SHADOW_HAND_NEWTON_CFG (beside SHADOW_HAND_CFG); reorient and handover consume it as siblings (neither imports the other). Reorient uses the default gains; handover overrides only the finger stiffness/damping to 20/2 for its catch.
• Shadow camera cleanup. Renamed the camera Direct modules to shadow_hand_direct_camera_env[_cfg]; moved cube-keypoint math to the shared, unit-tested reorient/mdp/observations.py (replacing the per-env compute_keypoints shim); and added OVPhysX physics presets to the handover and camera Direct environments. The camera env config is otherwise unchanged from develop.

Behavior changes

Reorient-on-Newton is unchanged. The Newton hand definition moves verbatim into SHADOW_HAND_NEWTON_CFG — same joint expressions, effort limits, gains, friction and armature — so its config diff vs develop is purely the move, and the Direct env config has no diff at all.

Handover changes in three ways:

• Its default physics backend moves from PhysX to Newton (MJWarp), matching the reorientation tasks. Pass physics=physx for the previous backend.
• Its distal_passive actuator group is removed. It targeted robot0_(FF|MF|RF|LF)J0 — joints that do not exist on the Newton asset (its fingers are numbered J1–J4/J5), so constructing the environment raised No joints found for actuator group. The fingers group already drives those joints, and the comment justifying the group (USD-baked stiffness=286/damping=57) does not apply to this asset, which authors no drive stiffness at all.
• Two orientation fixes: the goal orientation was initialized to a 180-degree rotation instead of identity, and the Newton root rotation replaced the asset's baked rotation instead of composing with it, leaving both palms rotated 90 degrees.

Two pre-existing discrepancies were found while verifying against the asset and are left for a follow-up, since each is a behavior change needing its own validation: the config sets armature=2e-3 where the asset authors 0.0002, and the per-finger J1/J2 pair is coupled by a fixed tendon that the MJWarp solver currently skips — the actuator gains are what hold that pair together today, which is why the J4 knuckle-abduction joints cannot simply be added to the driven set.

Notes

• Rebased onto latest develop; clean 3-commit history: Add single-agent flattening for MARL Direct environments / Convert Shadow Hand handover to single-agent Direct with RSL-RL / Clean up Shadow reorient/camera tasks and consolidate the Newton hand.

Validation

• Kit-free suite green: handover, keypoint math, the MARL adapter, and new checks pinning the RSL-RL wrapper observation contract (verified to fail against the previous implementation).
• Camera Direct reaches training takeoff under rsl_rl (frame-verified videos); its env config now matches develop's established vision env.
• Reorient-on-Newton vs develop (rsl_rl, 2048 envs, 1500 iterations, 4 runs per config): this branch spans 524–664 reward, develop spans 654–822 — overlapping distributions whose per-config spreads (140–168) exceed the gap between them. The move is additionally verified at the config level: the Newton robot configuration is character-for-character identical to develop's.
• Handover-on-Newton trains end-to-end on this branch (reward 1314), which the develop config cannot do at all — it fails during construction. Full evidence: [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324.

[greptile-apps]

Greptile Summary

This PR enables RSL-RL PPO training on the Shadow Hand handover Direct task by fixing the MARL-to-single-agent adapter to expose live observations via _get_observations, adding a HandoverPPORunnerCfg, registering it on Isaac-Shadow-Handover-Direct, and fixing the Newton distal-joint actuator override from J0 to J1 to match the renamed asset joints.

• MARL adapter fix: adds _convert_observations/_get_observations helpers plus an episode_length_buf property with setter and __getattr__ delegation so RSL-RL sees current rather than stale step-return observations; covered by three new unit tests.
• Handover env additions: success-rate metrics, EpisodeErrorRecorder for per-episode goal-distance diagnostics, and sample_joint_positions_within_limits for cleaner reset sampling are wired in; these depend on isaaclab_tasks.core.utils from PR [Task Clean-up] Dexterous Part 3/9: Add success-rate metrics to the reorientation Direct tasks #6413 (declared dependency).
• Missing module: handover_env.py also imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, a module that is absent from the repository and not named as coming from any dependency PR — this will cause an ImportError at task load time.

Confidence Score: 3/5

Not safe to merge as-is: handover_env.py will raise ModuleNotFoundError on import due to a missing mdp/rewards.py module that is neither present in the repo nor named as a dependency.

The handover_env.py change adds an import from isaaclab_tasks.core.handover.mdp.rewards (evaluate_handover_success, handover_reward). That package path does not exist anywhere in the repository, and neither this PR nor its declared dependency (#6413) explicitly accounts for it. Every code path that constructs or imports HandoverEnv will fail immediately with an unresolved import. The MARL adapter changes and the RSL-RL config are clean, and the Newton joint-name fix is correct, but the broken import makes the core task class unusable until the missing module is supplied or the import is corrected.

source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env.py — the import from isaaclab_tasks.core.handover.mdp.rewards references a module that does not exist.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/envs/utils/marl.py	Adds __getattr__ delegation, episode_length_buf property+setter, and _get_observations/_convert_observations helpers to the MARL-to-single-agent adapter so RSL-RL gets fresh observations instead of stale step/reset buffers. Logic is correct; minor inefficiency when state_as_observation=True calls the inner env's _get_observations unnecessarily.
source/isaaclab/test/envs/test_marl_utils.py	New unit tests covering concatenation, state-as-observation mode, and episode-length forwarding for the MARL adapter. Correctly uses torch.testing.assert_close and validates both getter and setter paths.
source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env.py	Imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, a module that does not exist in the repository; will raise ModuleNotFoundError at import time. Also imports from isaaclab_tasks.core.utils (declared dependency from #6413).
source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env_cfg.py	Renames distal-joint regex from J0 to J1 in the Newton actuator override to match the renamed Shadow Hand Newton asset joints; docstring updated to match. One inline comment still says "J0" (stale).
source/isaaclab_tasks/isaaclab_tasks/core/handover/agents/rsl_rl_ppo_cfg.py	New RSL-RL PPO runner config (HandoverPPORunnerCfg) with a 4-layer MLP actor/critic and standard PPO hyperparameters; consistent with other dexterous task configs in the repo.
source/isaaclab_tasks/isaaclab_tasks/core/handover/init.py	Adds rsl_rl_cfg_entry_point to the Isaac-Shadow-Handover-Direct Gym registration, pointing at the new HandoverPPORunnerCfg.
source/isaaclab_tasks/test/core/test_handover_rsl_rl_cfg.py	Tests that the Gym entry point resolves to the correct config class and that combined observation/action dimensions (314/40) match expectations for the two-hand setup.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant RSL as RSL-RL Runner
    participant Adapter as multi_agent_to_single_agent (Env)
    participant MARL as HandoverEnv (DirectMARLEnv)

    RSL->>Adapter: reset(seed, options)
    Adapter->>MARL: reset(seed, options)
    MARL-->>Adapter: obs (per-agent dict), extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}, extras"

    RSL->>Adapter: _get_observations()
    Note over Adapter: NEW: exposes live obs for RSL-RL direct access
    Adapter->>MARL: _get_observations()
    MARL-->>Adapter: obs (per-agent dict)
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}"

    RSL->>Adapter: step(action)
    Adapter->>Adapter: split action by agent
    Adapter->>MARL: "step({right_hand: a0, left_hand: a1})"
    MARL-->>Adapter: obs, rewards, terminated, time_outs, extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: obs, sum(rewards), AND(terminated), AND(time_outs), extras

    RSL->>Adapter: "episode_length_buf = value"
    Note over Adapter: Property setter forwards write to MARL env
    Adapter->>MARL: "episode_length_buf = value"

Loading

%%{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 RSL as RSL-RL Runner
    participant Adapter as multi_agent_to_single_agent (Env)
    participant MARL as HandoverEnv (DirectMARLEnv)

    RSL->>Adapter: reset(seed, options)
    Adapter->>MARL: reset(seed, options)
    MARL-->>Adapter: obs (per-agent dict), extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}, extras"

    RSL->>Adapter: _get_observations()
    Note over Adapter: NEW: exposes live obs for RSL-RL direct access
    Adapter->>MARL: _get_observations()
    MARL-->>Adapter: obs (per-agent dict)
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}"

    RSL->>Adapter: step(action)
    Adapter->>Adapter: split action by agent
    Adapter->>MARL: "step({right_hand: a0, left_hand: a1})"
    MARL-->>Adapter: obs, rewards, terminated, time_outs, extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: obs, sum(rewards), AND(terminated), AND(time_outs), extras

    RSL->>Adapter: "episode_length_buf = value"
    Note over Adapter: Property setter forwards write to MARL env
    Adapter->>MARL: "episode_length_buf = value"

Loading

Comments Outside Diff (1)

1. source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env_cfg.py, line 130 (link)

   The inline comment above _SHADOW_HAND_NEWTON_CFG still references "J0" after the joint rename to "J1" was applied to both the actuator config and the function docstring, leaving this brief description stale.

   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: "Enable RSL-RL training for the handover ..." | Re-trigger Greptile}

[greptile-apps]

Missing module: isaaclab_tasks.core.handover.mdp.rewards

handover_env.py imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, but no such module exists anywhere in the repository — there is no mdp/ subdirectory under core/handover/. The PR description names PR #6413 as the only declared code dependency (for isaaclab_tasks.core.utils), but that PR's description only mentions "core.utils helpers" and does not describe adding a handover-specific mdp/rewards.py. Without this file the task class will raise ModuleNotFoundError on import, blocking all training runs.

[greptile-apps]

Unnecessary _get_observations() call in state-as-observation mode

When _state_as_observation=True, _get_observations still calls self.env._get_observations() (which may trigger sensor data collection) and then immediately discards the result inside _convert_observations, which pivots to self.env.state(). The call is harmless but wasteful — consider short-circuiting before the inner call, e.g. returning {"policy": self.env.state()} directly when in state-as-observation mode.

[Toni-SM]

Fixes the multi-agent-to-single-agent adapter to expose current observations through the Direct RL interface...

Such implementation should not be accepted as it is trying to fix a problem a RL library has (require a private method, that is not part of the standard API Gymnasium / Petting-Zoo Parallel API to initialize its logic) from Isaac Lab side.

There is nothing to fix here. The fix must be implemented in the RL library to do not depend on the private _get_observations method that is not part of the standard API, but instead to rely on public properties and method of the standard environment API to initialize the library.

@kellyguo11 for viz

[Toni-SM]

See #6414 (comment)

[hujc7]

Updated.

[AntoineRichard]

My concern is that this PR is doing some real (important and needed) work on MARL and RSL-RL while hiding that under a RSL-RL only refactor.

Also same concerns around moving entirely to warp.

[StafaH]

rgbd?

[hujc7]

No longer applies — the rgb_depth preset was dropped and the camera config matches develop again.

[StafaH]

As discussed offline, we can move benchmark to contrib. (if this is handled in a future PR please ignore)

[hujc7]

Taking the follow-up option — the ID is still registered unchanged; moving it needs a deprecated alias since the quickstart docs reference it.

[StafaH]

It might be cleaner to do something like:

def reset(...):
    obs, extras = self.env.reset(seed, options)
    self._obs_buf = self._convert_observations(obs)
    return self._obs_buf, extras

def step(...):
    ...
    self._obs_buf = self._convert_observations(obs)
    return self._obs_buf, ...

@property
def obs_buf(self):
    return self._obs_buf

So obs buffer stays true to it's propery, instead of being a conversion mechanism

[hujc7]

Done in f142a22, as sketched — stored in reset()/step(), obs_buf is now a plain property. Also removes the repeated conversion greptile flagged.

[StafaH]

AI Review: The PR changes RslRlVecEnvWrapper.get_observations() from actively computing observations to unconditionally reading env.obs_buf, but there is no wrapper-level regression test. Please add tests showing that get_observations() exactly matches the most recent reset and step outputs, including a noisy/non-idempotent observation case. We should also verify every supported environment family exposes obs_buf.

[hujc7]

There wasn't one. 41b7ed5 adds kit-free test_rsl_rl_wrapper_observations.py: the wrapper returns the env buffer, tracks updates, never calls the private method, and DirectRLEnv publishes it in reset and step. Verified to fail against the previous implementation.

[StafaH]

This might need to be something like: self.env.episode_length_buf.copy_(value)? Not entirely sure though.

[hujc7]

Changed in 6ec4220. Rebinding left anything holding a reference to the wrapped buffer stale; the in-place write keeps them in sync. The test now covers values and buffer identity.

[StafaH]

Cleaner to write:

from isaaclab_assets.robots.shadow_hand import (
    SHADOW_ACTUATED_JOINT_NAMES as ACTUATED_JOINT_NAMES,
    SHADOW_FINGERTIP_BODY_NAMES as FINGERTIP_BODY_NAMES,
)

[hujc7]

ruff splits it back under our isort config (combine-as-imports isn't enabled), so the current shape is what the formatter produces.

[StafaH]

Here and elsewhere, what's going on with this self.device stuff? Are the buffers on the wrong device? Everything should already be on the same device. I would remove all this .to(device) stuff, maybe add a test instead? to(device) could be expensive to always be calling in hot paths.

[hujc7]

Removed. Both were in __init__ rather than a hot path, but no-ops on correctly-placed data.

[StafaH]

here and elsewhere let's make the default for base envs be render_interval = decimation? Seems like default behaviour should be to render at every physics step?

[hujc7]

Restored in 4144bf8. develop had render_interval=decimation and this PR had hard-coded it to 2 — same value today, but the coupling was lost. It was the only such site.

[StafaH]

newton_mjwarp_cfg, since kamino is unknown right now

[hujc7]

Renamed, both sites.

[StafaH]

Comment not really necessary

[hujc7]

Removed.

[AntoineRichard]

Still some comment issues, but overall it looks fine

[AntoineRichard]

I think this is coming from som rl libraries messing with the data from the simulator inplace

[AntoineRichard]

Why not the update data interval to 4?

[hujc7]

Raised to 4 in 4fec9dd.

Every other task pairs update_data_interval with num_substeps 1:1 — all eight sites are 2/2 — and this PR raised handover's substeps to 4 without updating the interval, so 4 restores that invariant.

Validated at 1500 iterations on both 2048 and 8192 envs: reward 1341 and 1483, no NaN. The 8192 run is the scale that motivated the substep bump.

[AntoineRichard]

I think this comment could be trimmed

[hujc7]

Cut to two lines in 4fec9dd. Dropped the probe details (~0.7% of 8192 envs, zero-action probe, 300 steps), kept the reason for 4 substeps.

[AntoineRichard]

I think this comment could be trimmed

[hujc7]

Cut from 6 lines to 3 in 4fec9dd, dropping the migration note (matching the previously used wp.quatd math bit-for-bit). The reason stays: Newton's importer bakes the root orientation, so the task rotation composes rather than replaces it — otherwise both palms end up rotated 90 degrees.

[AntoineRichard]

comment can be trimmed

[hujc7]

Removed entirely in 4fec9dd — it repeated the docstring 20 lines above.

[AntoineRichard]

This here are traces that could be trimmed / removed.

[hujc7]

Trimmed in 4fec9dd — dropped the tuning trace (mean reward at iter 200 / 2048 envs ~27 -> ~777) and kept only why the gains are raised.

Needed the change to unblock. Will sync offline to followup