Skip to content

Agentic env gen supports replicator kitchen#942

Draft
qianl-nv wants to merge 4 commits into
qianl/fix/review_gui_camera_viewfrom
qianl/dev/gui_replicator_kitchen
Draft

Agentic env gen supports replicator kitchen#942
qianl-nv wants to merge 4 commits into
qianl/fix/review_gui_camera_viewfrom
qianl/dev/gui_replicator_kitchen

Conversation

@qianl-nv

Copy link
Copy Markdown
Collaborator

Summary

Add Replicator kitchen review previews

Detailed description

  • Register Replicator kitchen backgrounds and add cached 360° panorama rendering to the review GUI.
  • Show background USD and full prim paths above the editor for easier scene inspection.
  • Preserve full paths during pass-2 prim inference and support relation-placed viewer targets.

qianl-nv added 4 commits July 22, 2026 18:29
Discover Replicator/seed_* USDA floorplans and register them as
replicator_kitchen_<seed> background assets with a framing viewer cfg.

Signed-off-by: Qian Lin <qianl@nvidia.com>
- Add a "Background 360° panorama" toggle that renders the background asset as a
  raw fisheyeSpherical panorama, cached in a separate panorama cache directory.
- Thread the background_panorama flag through the SimApp client/server and the
  thumbnail capture path, and flag the resulting card as a panorama.

Signed-off-by: Qian Lin <qianl@nvidia.com>
Show the background USD path and prim tree above the YAML editor, resolving
from the background block even when the full spec fails validation.

Signed-off-by: Qian Lin <qianl@nvidia.com>
List default-prim-relative paths verbatim in BACKGROUND PRIM TREE so the LLM
returns values like Kitchen/Floor instead of leaf-only Floor on Replicator kitchens.

Signed-off-by: Qian Lin <qianl@nvidia.com>
_REPLICATOR_ROOT = Path(__file__).resolve().parents[2] / "Replicator"


def _discover_replicator_kitchen_usda_paths() -> dict[str, Path]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Background set depends on untracked local files

_discover_replicator_kitchen_usda_paths() globs <repo>/Replicator/seed_*/*.usda at import time, but Replicator/ is neither committed nor in .gitignore — so which replicator_kitchen_<seed> backgrounds get registered varies per machine. A spec that references one resolves on the box that generated it and silently fails elsewhere, while every other background here points to Nucleus or downloads on demand. For an eval library that's a reproducibility gap. Where are these USDAs meant to come from — could the seed→path mapping be explicit (and the chosen paths logged) rather than discovered from a local dir at import time?

"""Return a single world-frame pose for viewer targeting."""
initial_pose = lookat_object.get_initial_pose()
if initial_pose is None:
object_cfg = getattr(lookat_object, "object_cfg", None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 getattr/hasattr chain over known attributes

object_cfg, init_state, and pos are all declared on ObjectBase/the Isaac Lab cfg, so if lookat_object is always an ObjectBase this defensive getattr/hasattr chain can be plain attribute access. It also reads the pose from a second place (object_cfg.init_state.pos) when get_initial_pose() is None — is that case real? If so, could it be exposed through get_initial_pose() so there's a single source of truth? This fallback branch is also currently untested (the new test only covers the PosePerEnv path).

st.session_state["edited_text"] = new_text

validation = validate_yaml_text(st.session_state["edited_text"])
from isaaclab_arena_examples.agentic_environment_generation.review_gui.spec_visualization.visualization_widgets import (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Imports inside the function

These two review_gui imports don't need the sim running — is this deferring to break an import cycle? If not, they'd read better at module top.

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

This PR adds Replicator-kitchen support to the agentic-env-gen review GUI: it registers Replicator/seed_* floorplans as backgrounds, adds a cached 360° fisheye panorama render, shows the background USD path + full prim tree above the editor, and preserves full prim paths in pass-2 inference. The GUI/thumbnail/prim-tree plumbing is clean and reasonably tested. Two things are worth a look before merge — one design/reproducibility question in core, one style nit.

Design, Boundaries & Scope

The one concern that reaches beyond the GUI: background_library.py now discovers backgrounds by globbing an untracked Replicator/ directory at the repo root at import time. That dir isn't committed or gitignored, so the registered replicator_kitchen_<seed> set is machine-dependent and unlogged — a spec referencing one is silently non-portable across machines. Every other background here resolves from Nucleus or downloads on demand. Worth deciding where these assets should live and whether the seed→path mapping should be explicit rather than filesystem-derived (inline comment).

Findings

🟡 Warning: background_library.py:205 — registered backgrounds depend on untracked local Replicator/ files → per-machine, non-reproducible asset set.
🔵 Improvement: cameras.py:143getattr/hasattr chain over attributes already declared on ObjectBase; pulls pose from a second source; fallback branch untested.
🔵 Improvement: editor_panel.py:161 — imports inside the render function with no sim/cycle reason apparent — hoist to top unless breaking a cycle.

Test Coverage

Good additions: full-path prim-tree formatting, the PosePerEnv viewer path, and background-preview resolution from an invalid-but-parseable spec. Gap: the new _resolve_lookat_pose object_cfg.init_state.pos fallback branch has no test. The panorama capture is sim-side GUI code and reasonably left to manual verification.

Verdict

Minor fixes needed

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds Replicator kitchen support to the agentic environment generation review GUI: dynamically discovering and registering seed_N_* USD assets, rendering cached 360° fisheyeSpherical panoramas for background previews, and showing the background USD path and full prim tree above the YAML editor.

  • Prim-path inference fix: _prim_tree_catalog now emits full default-prim-relative paths on every line instead of leaf-only suffixes, and the LLM prompt instructs verbatim copy — eliminating the class of errors where corpus was used instead of cab_1_main_group/corpus.
  • Panorama rendering pipeline: a new background_panorama toggle triggers a fisheyeSpherical camera capture on the background stage, cached separately under agentic_env_gen_panorama_thumbnails/; the camera is restored and the prim removed in a finally block before any subsequent object-reference captures on the same stage.
  • PosePerEnv viewer support: _resolve_lookat_pose extracts env-0 from a PosePerEnv initial pose, fixing viewer targeting for relation-placed objects.

Confidence Score: 4/5

Safe to merge; the core panorama pipeline and camera restore logic are well-structured, and the prim-path inference fix is correct and tested.

The changes introduce a non-trivial rendering pipeline (fisheye camera, stage mutation, cache management) alongside dynamic asset registration. The panorama capture finally block cleanly restores viewport state before subsequent ref captures. The main concern is that _discover_replicator_kitchen_usda_paths silently overwrites seed entries when a directory contains multiple USDA files, which could register the wrong background USD with no diagnostic output.

isaaclab_arena/assets/background_library.py (silent overwrite in USDA discovery) and isaaclab_arena_examples/.../simapp/thumbnail_capture.py (double pump before panorama capture).

Important Files Changed

Filename Overview
isaaclab_arena/agentic_environment_generation/prim_path_inference.py Fixes prompt guidance and _prim_tree_catalog to show full prim paths instead of relative leaf suffixes, preventing the LLM from constructing shortened paths.
isaaclab_arena/assets/background_library.py Adds ReplicatorKitchenBackground class and dynamic discovery/registration of Replicator seed USDs; silent overwrite when a seed dir contains multiple USDA files.
isaaclab_arena/utils/cameras.py Extracts _resolve_lookat_pose helper that handles PosePerEnv (returns env-0 pose) in addition to PoseRange; logic is correct and tested.
isaaclab_arena_examples/agentic_environment_generation/review_gui/editor_panel.py Adds background USD path / prim-tree display above the editor via a placeholder slot; deferred imports inside function body rather than at module level.
isaaclab_arena_examples/agentic_environment_generation/review_gui/simapp/thumbnail_capture.py Adds fisheyeSpherical panorama capture path; camera restore in finally is solid, but has a double pre-capture pump that may be unintentional.
isaaclab_arena_examples/agentic_environment_generation/review_gui/visualization_service.py Introduces BackgroundPreview dataclass and refactors background-preview resolution; caching by YAML text is correct. build_asset_cards_with_thumbnails return type simplified to list[AssetCard].

Comments Outside Diff (1)

  1. isaaclab_arena/assets/background_library.py, line 87-92 (link)

    P2 Silent overwrite when seed directory contains multiple USDA files

    _discover_replicator_kitchen_usda_paths iterates all .usda files under each seed_N_* directory, but the dict key is keyed only on the seed number. If a single seed directory contains two .usda files (e.g., scene.usda and scene_v2.usda), the first assignment is silently overwritten by the last file in sorted order — no warning is printed and the wrong USD may be registered.

    Consider a guard that detects and logs when a seed directory yields more than one USDA file, so the overwrite is detectable.

Reviews (1): Last reviewed commit: "Show full prim paths in pass-2 backgroun..." | Re-trigger Greptile

Comment on lines +161 to +166
from isaaclab_arena_examples.agentic_environment_generation.review_gui.spec_visualization.visualization_widgets import (
render_background_prim_tree,
)
from isaaclab_arena_examples.agentic_environment_generation.review_gui.visualization_service import (
resolve_background_preview,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Deferred imports inside function body

render_background_prim_tree and resolve_background_preview are imported inside render_editor_panel on every Streamlit render pass. There is no circular dependency between editor_panel, visualization_widgets, and visualization_service — none of those modules import from editor_panel. Moving these to module-level imports at the top of the file would make the dependency graph easier to read and consistent with the other imports in this module.

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!

Comment on lines +291 to +295
viewport.camera_path = PANORAMA_CAMERA_PRIM_PATH
pump_app(app, count=PRE_CAPTURE_UPDATES)

try:
png_bytes = capture_viewport_png(app, cache_path, pre_capture_updates=PRE_CAPTURE_UPDATES)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Double pre-capture pump for panorama

_capture_background_panorama calls pump_app(app, count=PRE_CAPTURE_UPDATES) explicitly before the try block, and then passes pre_capture_updates=PRE_CAPTURE_UPDATES to capture_viewport_png, which calls pump_app again internally (kit_viewport.py line 135). The fisheye camera therefore settles for 2 × PRE_CAPTURE_UPDATES = 10 frames before the capture fires.

If the double pump is intentional (fisheye projection needs more settle time), a clarifying comment here would help; otherwise the outer pump_app call is redundant since capture_viewport_png already handles pre-capture pumping.

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.

1 participant