🤖 Isaac Lab Review Bot

Summary

This PR adds optional first-frame screenshot capture and scene metadata logging to the non-RL benchmark script. The implementation is well-isolated behind the --preview_dir flag, uses best-effort error handling, and correctly gates execution to rank 0 only in distributed scenarios. However, there is one clear bug and several minor issues.

Architecture Impact

Self-contained. The new capture_scene_preview() utility is only called from benchmark_non_rl.py when the optional flag is set. No existing code paths are affected when the flag is not used. The function is designed to be reusable by other benchmark scripts as noted in the PR description.

Implementation Verdict

Minor fixes needed — one bug must be fixed before merge.

Test Coverage

No tests are provided. This is acceptable for a best-effort diagnostic utility behind an optional flag in a benchmark script, but the manual test plan in the PR description should be executed before merge.

CI Status

No CI checks available yet — cannot assess.

Findings

🔴 Critical: benchmark_non_rl.py:145 — os.path.abs does not exist, should be os.path.abspath

log_root_path = os.path.abs(f"benchmark/{args_cli.task}")

This will raise AttributeError: module 'os.path' has no attribute 'abs' when --video is used. While this is in a pre-existing code path (not introduced by this PR), the diff shows the line was present before and the PR touches surrounding logic. This is a latent bug that should be fixed opportunistically — though strictly speaking it's not introduced by this PR.

🟡 Warning: utils.py:109 — numpy import is unused in the screenshot path

import numpy as np

The np import is not used — env.render() returns an array and Image.fromarray() accepts it directly. This is dead code that adds a small overhead. Remove the import.

🟡 Warning: utils.py:112-113 — Missing type validation for render output

rgb = env.render()
if rgb is not None and rgb.size > 0:

The check rgb.size > 0 assumes rgb has a .size attribute (numpy array). If env.render() returns something unexpected (e.g., a list or dict in edge cases with custom environments), this could raise AttributeError. Consider:

if rgb is not None and hasattr(rgb, 'size') and rgb.size > 0:

Or catch AttributeError in the existing exception handler.

🟡 Warning: utils.py:93-150 — Function docstring claims env should be "unwrapped" but env wrapper is passed

Args:
    env: The unwrapped IsaacLab gym environment, created with render_mode="rgb_array".

In benchmark_non_rl.py:158, the wrapped env is passed (potentially wrapped with RecordVideo). The function works correctly because env.render() propagates through wrappers, but the docstring is misleading. Update to: "The IsaacLab gym environment (may be wrapped)."

🔵 Improvement: utils.py:130-131 — Consider logging renderer setting path for debugging

When /rtx/shadows/enabled is None, it's unclear whether shadows are disabled or the setting doesn't exist. Consider:

shadows = settings.get("/rtx/shadows/enabled")
meta["shadows_enabled"] = shadows if shadows is not None else "unknown"

🔵 Improvement: benchmark_non_rl.py:27 — Argument help text should mention distributed behavior

parser.add_argument("--preview_dir", type=str, default=None, help="If set, capture a first-frame screenshot and scene metadata into this directory.")

The preview is only captured on rank 0. Consider updating help text: "...this directory (rank 0 only in distributed mode)."

🔵 Improvement: utils.py:137-138 — stage.Traverse() iteration could be expensive on large scenes

For complex USD stages, traversing all prims to count lights adds overhead. This is likely acceptable for a one-time diagnostic capture, but consider adding a warning in the docstring that this may be slow on large scenes, or adding an early-exit after finding N lights.