[hujc7]

Review Map

• A stacked PR's page shows the cumulative diff of its dependency chain; the "Exact changes" link pins that PR's own commit range.
• SHA links can go stale after a branch update; the copy on [Warp] Part 1/8: Resolve id and mask selections through shared helpers #8821 is refreshed first.

PR	Status	Depends on	Exact changes
#8821 Part 1/8: shared id and mask helpers, mask writers without host sync		—	—
#8824 Part 2/8: PhysX tendon mask setters through the masks		#8821	link
#6646 Part 3/8: mask-correct partial resets		#8821	link
#8783 Part 4/8: shared Warp RNG state		#6646	link
#6607 Part 5/8: per-term capture, stable stepping parity		#8783	link
#8228 Part 6/8: Warp commands, Rough tasks, live term changes		#6607	link
📌 #6582 Part 7/8: Shadow Warp twin, Allegro twin fixes		#6607	link
#7012 Part 8/8: --video on the Warp frontend		—	—

Summary

Isaac-Reorient-Cube-Shadow-Direct runs under --frontend warp and trains like the stable task (last-50-iteration success 0.946 vs 0.941, 3 seeds each), and the Allegro Warp twin's orientation error (~π for identical orientations on main) and swapped object-velocity observation are fixed.
Scope is narrower than the original PR: Shadow only; the handover twin and the camera change are dropped.

Description

Update (2026-10-26): merged #6607 and, through it, #8821 and main dae8b799146; this PR's own diff is unchanged.

Update (2026-10-22): merged #6607 after #8783 (the shared Warp RNG state) was split out of #6646; this PR's own diff is unchanged.

Update (2026-10-20): merged #6607 and, through it, main 2dbb6146d75 (the default branch, renamed from develop; "main" below means main without this PR, measured at aa9bafef8a0). #8474 made the stable reorient goal_marker_position optional (None follows the hand); the Warp twin now does the same instead of failing to construct. After the merge, test_reorient_warp_env.py (4) and the capture tests pass, Allegro steps 300 and Shadow 1000 times under --frontend warp, and the environment browser is regenerated.

Update (2026-09-30) — rescoped: rebuilt on #6607 against develop (synced to f2f11b3f608). The handover Warp twin is parked (there is no Warp multi-agent environment), the camera commit is dropped (superseded by #7133), and the older dexterous commits already landed through #6413. No -Warp task IDs are registered.

1. Changes

1. Allegro twin fixes (main):
   • rotation_distance read (y, z, w) of an xyzw quaternion, so identical orientations gave ~π and the rotation reward, success flag and goal resampling differed from stable.
   • The full observation swapped the object's linear and angular velocity (columns 39–44 differed from stable by up to 45.1).
2. Shared reorient base: success computed once in _get_dones, resets through the public mask writers, constants from the config, Metrics/success_rate; asymmetric_obs raises NotImplementedError instead of silently omitting the critic observations.
3. Shadow twin: ShadowHandDirectWarpEnv(ReorientDirectWarpEnv) commands the four finger tendons from the last four action columns, like the stable task; it is registered through the Warp frontend, with docs and the environment browser updated.

2. Evidence (MEASURED)

• Same-state parity against the stable task, 64 envs × 120 steps (910a0ba9b90): Allegro observations within 2.4e-7, rewards 3.6e-5, 9/9 goal resamples match; Shadow observations 2.4e-7, rewards 7.8e-5; dones, successes and success_rate identical. Shadow joint trajectories were within 2.4e-5 rad before the main sync.
• Over 40 captured steps with resets (9 on Shadow, 27 on Allegro), neither twin activates a wrench composer, so the captured reset stage never depends on the composer's host-side reset flag.
• Training, rsl_rl, 8192 envs, Newton MJWarp, L40, configured iterations (Shadow 3000, Allegro 10000); last-50-iteration means:

Task	Arm	Success per seed	Mean success	Mean reward
Shadow	Warp	0.943 / 0.945 / 0.950	0.946	6538
Shadow	stable	0.942 / 0.937 / 0.945	0.941	6455
Allegro	Warp	0.864 / 0.747 / 0.854 / 0.857 / 0.874	0.839	1556
Allegro	stable	0.882 / 0.862 / 0.833 / 0.877 / 0.871	0.865	1808

• Allegro: one Warp seed (0.747) falls below the stable range; Mann–Whitney two-sided exact p = 0.31 (success) and 0.55 (reward), so no difference is detectable at 5 seeds. Shadow: 3 seeds per arm, overlapping ranges.
• The runs used earlier tips of this branch (0df4fd8981c; Allegro seeds 1–3 e2a36b3752c), before [Warp] Part 5/8: Capture Warp stages per term without warm-up and step like the stable environments #6607's log-copy fix, so the Warp arm's per-iteration success metric is the iteration's last value rather than its mean. Mean reward comes from rsl_rl's episode-return buffer and is unaffected. Parity was re-measured after each rebase.
• test_reorient_warp_env.py fails on main (observation diff 5.79; rotation-mutation reward diff 0.60); at this tip it and the frontend, config-conversion and scene tests pass.
• Docs build: sphinx-build -W --keep-going (the CI flags) on all five parts merged onto develop f2f11b3f608 succeeded with 0 warnings.

3. Out of scope / follow-ups

• Handover Warp twin: needs a Warp multi-agent environment.
• The Diagnostics/episode_min_orientation_error_* metrics of the stable task are not ported.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Release backport

• Backport this pull request to the active release branch after it merges into main

Screenshots

Not applicable.

Checklist

Docker and GPU tests run on demand. Push the commits you want tested, then
comment run-ci on the pull request.

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with uv run isaaclab --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have validated the changed behavior with focused coverage and applied the test-audit criteria to any test changes
• I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump package versions — CI handles that)
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[AntoineRichard]

[AI Review] Requesting changes. The current Part 9 head is not merge-ready: the new Gym registrations and environment modules contain stale imports, and the camera PhysX-default override is ineffective under the current PresetCfg resolution rules. The kernel tests are useful and pass on CPU/CUDA in CI, but they do not instantiate the registered environments; the current isaaclab_tasks [3/3] CI shard is red on the three Shadow Warp config loads.

Before re-review, please also tighten the source comments to durable functional constraints, remove hot-loop duplication/conversions called out inline, use concrete Warp array annotations on public launchers, and add an isaaclab_tasks changelog fragment for the camera-default change.

[AntoineRichard]

[AI Review] Blocking: shadow_hand_env_cfg.py no longer exists. All three Shadow Warp registrations in this file use this stale module name, and the current isaaclab_tasks [3/3] CI shard fails exactly these three config loads. Point them to shadow_hand_direct_env_cfg, then add a registry/config-load test covering every new ID so a successful kernel-only suite cannot mask broken entry points.

[AntoineRichard]

[AI Review] Blocking: reorient_task_base was removed; these constants now live in reorient_common. The TYPE_CHECKING import below also still names the removed shadow_hand_env_cfg instead of shadow_hand_direct_env_cfg. Once the registration strings are fixed, importing this environment will fail here, so please update both stale references and add an environment-entry import smoke test.

[AntoineRichard]

[AI Review] Blocking: handover_task_base.py does not exist in this head, and GOAL_POSITION_OFFSET has no current definition. This makes the new handover entry point fail on import. Please put the goal definition at a genuine shared boundary (configuration or a small common module used by both torch and Warp implementations) rather than restoring a single-consumer “task base,” and cover the registered entry point with a smoke test.

[AntoineRichard]

[AI Review][Pointed follow-up] The stable implementation currently hard-codes the goal at handover_env.py:73-77. That is the concrete duplication to remove: define one environment-frame GOAL_POSITION in handover_env_cfg.py or a handover_common.py, then make both HandoverEnv and HandoverWarpEnv consume it. Do not recreate the deleted handover_task_base.py solely to satisfy this import.

[AntoineRichard]

[AI Review] Blocking: assigning default on these PresetCfg instances does not change preset resolution. _preset_fields() explicitly prefers the class attribute, so the inherited class-level Newton defaults still win and the camera remains Newton-default. Define camera-specific preset types or declarative preset(...) fields whose class-level default is PhysX, and add a collect_presets(ShadowHandCameraEnvCfg()) assertion for physics, robot, and object defaults. The preceding seven-line rationale should also become one functional sentence, e.g. “Isaac RTX tiled cameras require the PhysX Fabric cloning path”; benchmark CLI history belongs in tests/docs, not this method.

[AntoineRichard]

[AI Review][Pointed follow-up] The exact precedence rule is in _preset_fields(): getattr(type(preset_obj), field_name) wins over the instance field. The existing regression test_collect_fields_prefers_class_attr_over_instance codifies that behavior. Add the new camera-specific assertion beside the shadow_hand_camera_presets fixture, checking that sim.physics.default, robot_cfg.default, and object_cfg.default resolve to their PhysX alternatives.

[AntoineRichard]

[AI Review] The comment immediately above says reward reuses the success flags and orientation distances computed in _get_dones, but this call passes object/goal quaternions and reorient_reward_kernel recomputes _rotation_distance and the tolerance comparison. This is both misleading documentation and duplicate GPU hot-loop work. Pass success_flags and orientation_error_buf into the reward path, matching the torch implementation, and remove the redundant quaternion evaluation.

[AntoineRichard]

[AI Review][Pointed follow-up] The stable torch path shows the intended API: ReorientDirectEnv._get_rewards() passes _success_flags and _orientation_error_buf into reorient_reward. By contrast, the Warp kernel recomputes both at reorient_kernels.py:283-288. Concretely, replace the object/target quaternion plus tolerance inputs with goal_reached: wp.array(dtype=wp.bool) and rotation_distance: wp.array(dtype=wp.float32), then use those values for the reset, bonus, and inverse-error reward.

[AntoineRichard]

[AI Review] The initialization comment claims the hot loop launches kernels without conversions, but wp.from_torch(self.actions[agent]) is created here and again in _hand_proprio_obs for both agents every step. Use persistent per-agent action tensors, copy incoming actions into them in _pre_physics_step, and cache their Warp views once, as the reorientation variant already does.

[AntoineRichard]

[AI Review] The left-hand reset block duplicates the right-hand block above almost statement-for-statement. Please extract a _reset_hand(hand, prev_targets, curr_targets, env_ids) helper (or iterate a per-agent state map). This is exactly the kind of copied lifecycle code that drifts during follow-up fixes and is avoidable within this PR.

[AntoineRichard]

[AI Review] This public launcher uses generic wp.array annotations for every array argument, contrary to the repository requirement to annotate Warp arrays with concrete dimensionality and dtype. Please specify the actual contracts here (wp.bool, wp.float32, wp.vec3f, wp.quatf, and 2D float actions) and on the return type. compute_cube_keypoints() has the same issue.

[AntoineRichard]

[AI Review][Pointed follow-up] The concrete annotation map is: reset_buf/goal_resets → wp.array(dtype=wp.bool), successes/consecutive_successes → wp.array(dtype=wp.float32), positions/origins → wp.array(dtype=wp.vec3f), quaternions → wp.array(dtype=wp.quatf), actions → wp.array2d(dtype=wp.float32), and the return → wp.array(dtype=wp.float32). For compute_cube_keypoints, use wp.array2d(dtype=wp.float32) for poses and wp.array2d(dtype=wp.vec3f) for keypoints. This makes the public launcher contract match the typed kernel signatures instead of discarding their information.

[AntoineRichard]

[AI Review] Please reduce this module docstring to the module’s functional purpose. The long “math root”/step-order narrative is maintenance-heavy, and it claims manager terms consume these kernels even though the current tree only imports them from the experimental environments/tests. Keep execution contracts beside the relevant launcher; remove historical validation and aspirational dependency narration.

[AntoineRichard]

[AI Review] This “math root” and “reverse dependency never exists” prose is architectural narration rather than a functional kernel contract. A concise module description plus precise function-level frame/unit comments is enough. In particular, retain useful invariants such as world-frame versus environment-frame positions, but remove commentary about how the codebase is intended to be organized.

[AntoineRichard]

[AI Review][Comment cleanup] Follow-up pass covering every comment/docstring block that violates the requested standard. Each inline note identifies the exact narration, restatement, stale claim, or hard-coded dimensional prose to remove or rewrite. Comments that encode coordinate frames, zero-copy lifetime, unavailable-sensor behavior, sticky state, or synchronization constraints are intentionally left untouched.

[AntoineRichard]

[AI Review][Comment cleanup] Replace this seven-line narrative with one durable functional constraint: # Isaac RTX tiled cameras require the PhysX Fabric cloning path. The failure history, num_envs scenario, CLI spelling, and benchmark path belong in the regression test or user documentation, not in __post_init__.

[AntoineRichard]

[AI Review][Comment cleanup] Make this module docstring self-contained. “See the reorient variant for the conventions” forces readers to inspect an unrelated module and does not describe a functional contract. A single sentence such as Warp Direct environment for the Shadow handover task. is sufficient.

[AntoineRichard]

[AI Review][Comment cleanup] Remove the labels on lines 49, 63, 70, 78, 83, and 91; each merely repeats the immediately following variable lookup/allocation. Also drop the historical mirrors HandoverCommand.__init__ clause on line 87. Once the goal definition is moved to a shared config/common boundary, its name should carry that relationship; if a comment remains, document only the coordinate frame or invariant.

[AntoineRichard]

[AI Review][Comment cleanup] This comment is factually incorrect: action views are still created with wp.from_torch(...) in _apply_hand_action and _hand_proprio_obs every step. After introducing persistent per-agent action buffers, rewrite this narrowly as # Cache Warp views of persistent Torch buffers. Until then, remove it rather than claiming a property the implementation does not have.

[AntoineRichard]

[AI Review][Comment cleanup] Remove # add hand..., # add ground plane, and # add lights; they only narrate the next constructor/call. Keep the EventManager constraint on line 145, but tighten it to the functional reason: # Register assets with the scene for EventManager randomization.

[AntoineRichard]

[AI Review][Comment cleanup] Keep the launch shape, but remove the speculative warp-uniformity explanation. The branch behavior is an implementation/performance claim without a benchmark or enforced invariant and makes this already-long observation docstring harder to scan.

[AntoineRichard]

[AI Review][Comment cleanup] Keep the launch shape and output layout, but remove the warp-uniformity narrative. It is an implementation/performance claim rather than the kernel’s functional contract and duplicates the same prose in full_obs_kernel.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this line; handover_reward_kernel, goal_distance, and the immediately following exponential expression already state exactly what it says.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this comment. The test inputs (q1, q1.clone()), assertion, and test name make the zero-distance case self-explanatory.

[AntoineRichard]

[AI Review][Comment cleanup] Remove this generic label. The _ref variable names and subsequent assertions already distinguish the Torch reference computation from the Warp result.

[AntoineRichard]

[AI Review][Pointed follow-up] Three additional places where the review should name the exact missing coverage or ownership boundary, rather than leaving the fixer to infer it.

[AntoineRichard]

[AI Review][Pointed follow-up] Part 9 also changes source/isaaclab_tasks/.../shadow_hand_camera_env_cfg.py, but this is the only fragment added by the Part 9 commit range. The inherited source/isaaclab_tasks/changelog.d/task-cleanup-dex-part03.minor.rst belongs to Part 3 and does not mention the camera default; after the stack is rebased, Part 9 will touch isaaclab_tasks with no fragment. Either move commit 386272f2 to its owning stable-task PR or add a separate source/isaaclab_tasks/changelog.d/<slug>.rst Fixed entry for the camera backend default.

[AntoineRichard]

[AI Review][Pointed follow-up] Keep these kernel tests Kit-free, but add a separate registration smoke test because these 44 cases never load an env_cfg_entry_point or import either environment class. The current CI failure proves the gap: parameterize the four new Gym IDs, resolve each config entry point, and import its environment entry point. That test would fail on all five stale references currently present (shadow_hand_env_cfg ×3, reorient_task_base, and handover_task_base) while preserving this file’s focused kernel contract.

[AntoineRichard]

[AI Review][Pointed follow-up] This reaches into EpisodeErrorRecorder._has_sample, a private field owned by isaaclab_tasks.core.utils, and makes the experimental kernel depend on that class’s internal storage layout. EpisodeErrorRecorder.update() is the public boundary and also owns shape/finite-value semantics. Expose an explicit Warp-compatible recorder API/buffer contract, or keep Warp-owned minimum/sample buffers in this environment and pass summarized values to the recorder at reset; do not bind a kernel output directly to another class’s private attribute.