Skip to content

Add temporary isaac_cap env directory and gear insertion env - #1241

Merged
peterd-NV merged 14 commits into
mainfrom
peterd/isaac_cap_gear_insertion_env
Sep 11, 2026
Merged

Add temporary isaac_cap env directory and gear insertion env#1241
peterd-NV merged 14 commits into
mainfrom
peterd/isaac_cap_gear_insertion_env

Conversation

@peterd-NV

@peterd-NV peterd-NV commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add a temporary directory under isaaclab_arena_environments/isaac_cap to park all cap validation envs.

Adds the cap gear insertion environment and a demo script to validate env behaviour to this directory.

Adds a shared registration and embodiment directory for other CAP envs.

@peterd-NV peterd-NV changed the title Peterd/isaac cap gear insertion env Add temporary isaac_cap env directory and gear insertion env Sep 10, 2026
Comment thread isaaclab_arena_environments/isaac_cap/gear_insertion/validation_demo.py Outdated
@peterd-NV
peterd-NV marked this pull request as ready for review September 10, 2026 18:24
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 2/5

The PR is not safe to merge because standard discovery cannot register the environments, external users cannot load their internal-only assets, and the new recorder can fail or lose data on the first completed reset.

Findings

  1. P1 Package Is Never Discovered
  2. P1 Assets Require Internal Access
  3. P1 First Episode Reset Mishandled

Summary

  • Defines relation-solved two-gear and three-gear assembly scenes.
  • Adds absolute-joint and relative differential-IK control configurations.
  • Evaluates insertion using pose, support, orientation, and velocity constraints over consecutive frames.
  • Registers task, asset, embodiment, and environment components.
  • Adds a scripted multi-environment validation cycle for grasping, placement, success, and reset behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Import[Import isaac_cap package] --> Register[Register assets, embodiments, task, and factories]
  YAML[Easy or medium YAML specification] --> Factory[Gear environment factory]
  Register --> Factory
  Assets[Remote USD assets] --> Builder[Arena environment builder]
  Factory --> Builder
  Builder --> Newton[Newton physics configuration]
  Builder --> Runtime[Manager-based environment]
  Runtime --> Actions[FR3 and Robotiq actions]
  Runtime --> Predicate[Gear seating predicate]
  Predicate --> Reset[Success termination and reset]
  Predicate --> Metrics[Success and insertion-fraction metrics]
Loading

Reviews (1) · Last reviewed commit: "lint"

Comment thread isaaclab_arena_environments/isaac_cap/__init__.py Outdated
Comment thread isaaclab_arena_environments/isaac_cap/__init__.py Outdated
Comment thread isaaclab_arena_environments/isaac_cap/gear_insertion/validation_demo.py Outdated
Comment thread isaaclab_arena_environments/isaac_cap/gear_insertion/gear_easy.yaml
@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Reviewed c7f8b5c — 17 files, ~1760 added lines.

Summary

This adds a new isaac_cap environment package with a full gear-insertion task: four gear assets, an FR3 + Robotiq 2F-85 embodiment with cameras and a holding-IK action, a seating-based termination term, a per-gear insertion-fraction recorder, two graph-spec YAMLs, and a runnable validation demo. The task and termination logic are the strongest part — all_gears_seated precomputes its collision-corner bounds once in __init__, defers its pxr imports correctly, and uses assert for its internal invariants. The demo script also gets env.unwrapped right, which is a common trip-up.

The main thing to resolve before this lands is discovery: isaac_cap is a package, and the environment walker in isaaclab_arena_environments/__init__.py skips packages (if not _ispkg). So the registration code in gear_insertion/registration.py runs only when something imports the module directly — ensure_environments_registered() never does, which means the two vabar_contact_rich_insertion__gear_* names never appear in the EnvironmentRegistry and can't be launched from the CLI. Only validation_demo.py works today. One explicit import fixes it.

Beyond that, most of the feedback is about leaning on machinery Arena already has instead of re-deriving it locally: @register_asset / @register_environment instead of the hand-rolled _registered flag; ArenaCameraCfg.set_use_tiled_camera instead of a new wrapper with a TypeError guard; Object/Background subclasses instead of stapling name/tags/object_type onto plain functions; assert instead of a 27-line if/raise validation block in GearInsertionTask. The two copies of _normalize_initial_pose are the clearest symptom — they exist because the graph spec hands raw YAML dicts to constructors, which is a boundary problem worth fixing once centrally rather than per asset.

Design, Boundaries & Scope

The layering is respected — the task doesn't reach into the embodiment, the embodiment doesn't know about the task, and sim-specific imports stay inside the sim-dependent modules. The PR is self-contained: it adds a new directory and touches nothing existing, which keeps blast radius near zero but is also why the discovery gap slipped through.

One scope note: registration.py looks hand-rolled because @register_environment asserts direct ArenaEnvironmentFactory[Cfg] inheritance, and GearInsertionEasyNewtonEnvironment subclasses the medium environment instead. If the empty GearInsertionEasyNewtonEnvironmentCfg goes away and both factories register against one cfg type, the standard decorators should work and ~70 lines of bespoke registration can go with it.

The vabar prefix in the public environment names and the "temporary" framing of isaac_cap are worth a decision now rather than later — once a name is in the registry it tends to stay.

Findings

Sev Finding
🔴 isaac_cap is a package, so ensure_environments_registered() never imports it — neither environment reaches the registry
🟡 metrics.py dumps per-gear diagnostics at WARNING on every reset, and builds the payload unconditionally
🟡 Four near-identical gear factories with name/tags/object_type stapled on in a module-level loop
🟡 _normalize_initial_pose implemented twice with different validation — belongs at the graph-spec boundary
🟡 GearInsertionTask.__init__ uses 27 lines of if/raise + type-policing where the convention is assert
🔵 hasattr(success_term, "success_per_gear") guards a contract the task itself establishes
🔵 set_use_tiled_cameras() re-implements ArenaCameraCfg.set_use_tiled_camera and adds an isinstance check
🔵 reward_config = None / mimic_env = None already set by EmbodimentBase
🔵 Deferred isaaclab.utils.math import in cameras.py with nothing to defer
🔵 Duplicate copyright header, empty cfg subclass, and module/env naming in gear_medium_environment.py
🔵 self.torch = torch and arm_action._scale private access in validation_demo.py
🔵 gear_easy.yaml is gear_medium.yaml minus one gear — 85 lines that will drift

Test Coverage

There are no tests in this PR. ~1760 lines land with validation_demo.py as the only exercise path, and that script needs a GUI-capable sim run, so nothing here is covered by Phase 1.

A few things look cheaply testable without the simulator:

  • compute_gear_insertion_fraction — pure tensor math, including the empty and all-seated cases.
  • _normalize_initial_pose — this is exactly the kind of dict/tuple/Pose coercion that a table-driven test pins down in a few lines (and would have surfaced that there are two of them).
  • GearInsertionTask construction — mismatched offset counts, duplicate gear names, a bad threshold. Currently these raise, so they're easy to assert on.
  • The YAML specs — a Phase 1 test that loads each ArenaEnvGraphSpec and checks it validates catches typos and renamed assets without launching anything.

all_gears_seated and the camera geometry reasonably need sim, but the seating predicate itself (given corner positions and a target) could be extracted and tested directly.

Verdict

Minor fixes needed — the design is sound and the task logic is careful, but the discovery gap means the environments aren't reachable through the normal path, and the reimplementation of existing Arena helpers is worth collapsing before this becomes the template for the next isaac_cap environment.


Automated review — not a substitute for human review. Push a new commit to trigger a re-review.

@qianl-nv qianl-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this will be a good starting point.
Have some comments about making embodiment/registry shared within the isaac_cap folder if possible

Comment thread isaaclab_arena_environments/isaac_cap/gear_insertion/registration.py Outdated
@peterd-NV
peterd-NV merged commit 71374c9 into main Sep 11, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants