🤖 Isaac Lab Review Bot

Summary

This PR fixes an issue where Newton model requirements were not correctly determined when environment configs were constructed in certain orders (e.g., cameras added in _setup_scene after scene cloning). The fix adds early registration of renderer requirements in Camera.__init__ and refreshes the visualizer clone function before cloning environments. It also reverts a previous fallback mechanism that built Newton models from USD traversal, which was slow and is no longer needed with the fix in place.

Architecture Impact

Cross-module impact is significant:

1. Camera.__init__ now calls SimulationContext.update_scene_data_requirements() — affects any code path that creates cameras before scene cloning
2. InteractiveScene.clone_environments() now re-evaluates requirements and updates the visualizer clone function — affects all environment cloning
3. PhysxSceneDataProvider loses its USD fallback mechanism — any code path that previously relied on this fallback will now fail if the prebuilt artifact is missing

The changes create a dependency where Camera sensors MUST be instantiated before clone_environments() is called for requirements to propagate correctly.

Implementation Verdict

Minor fixes needed — The core fix is sound, but there's a subtle ordering issue and missing error handling that could cause confusing failures.

Test Coverage

• Tests were updated to match the new behavior (removed fallback tests, removed xfail marks)
• Missing: No test verifies the new early-registration path in Camera._register_renderer_scene_data_requirements
• Missing: No test verifies _refresh_visualizer_clone_fn_from_requirements actually fixes the described issue
• The removed xfail marks on physx-warp-* tests suggest those now pass, which is good coverage for the fix

CI Status

No CI checks available yet.

Findings

🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:158-177 — Silent failure when SimulationContext doesn't exist
The _register_renderer_scene_data_requirements method silently returns if SimulationContext.instance() is None. If a Camera is created before SimulationContext is initialized (which can happen in certain config-only workflows), the requirements won't be registered and the Newton model build will fail later with an obscure "missing artifact" error. Consider logging a debug message when this happens:

sim = SimulationContext.instance()
if sim is None:
    logger.debug("SimulationContext not available; deferring renderer requirements registration")
    return

🟡 Warning: source/isaaclab/isaaclab/scene/interactive_scene.py:257-275 — Duplicate requirements resolution logic
_refresh_visualizer_clone_fn_from_requirements duplicates logic from the __init__ method (lines 165-181). Both resolve requirements from sensor renderer types and update the simulation context. This creates maintenance burden and risk of divergence. Consider extracting to a shared helper method.

🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:232 — Unnecessary refresh when no sensors exist
_refresh_visualizer_clone_fn_from_requirements() is called unconditionally in clone_environments(), but if _sensor_renderer_types() returns an empty list (no sensors with renderer configs), the method does unnecessary work including requirements resolution and clone function resolution. Add an early return:

def _refresh_visualizer_clone_fn_from_requirements(self) -> None:
    renderer_types = self._sensor_renderer_types()
    if not renderer_types:
        return
    # ... rest of method

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:168-198 — Redundant artifact validation
The method checks not artifact on line 170, then separately checks model is None or state is None on line 180. The first check makes the second check redundant for the not artifact case. The error messages are also nearly identical. Consider consolidating:

if not artifact or artifact.model is None or artifact.state is None:
    self._last_newton_model_build_source = "missing"
    logger.error("[PhysxSceneDataProvider] Missing or incomplete visualizer prebuilt artifact...")
    self._clear_newton_model_state()
    return

🔵 Improvement: source/isaaclab/test/sim/test_physx_scene_data_provider_visualizer_contract.py — Missing test for new Camera registration path
The test file was updated to remove fallback tests but doesn't add coverage for the new Camera._register_renderer_scene_data_requirements method. Consider adding a test that verifies creating a Camera with a newton_warp renderer type updates the SimulationContext requirements:

def test_camera_registers_renderer_requirements():
    """Camera creation updates SimulationContext scene data requirements."""
    # Test that creating a Camera with renderer_cfg.renderer_type="newton_warp"
    # causes sim.update_scene_data_requirements to be called with newton requirements

🔵 Improvement: source/isaaclab_tasks/test/test_shadow_hand_vision_presets.py:329-330 — Removed xfail without explanation in test
The xfail marks for physx-warp-rgb and physx-warp-depth were removed, implying the tests now pass. This is correct behavior, but the reason ("PhysX tendon schemas unsupported by Newton ModelBuilder") was the original failure mode. A comment explaining why this now works would help future maintainers understand that the fix bypasses Newton ModelBuilder's USD traversal by using prebuilt artifacts instead.

🤖 Isaac Lab Review Bot

Summary

This PR fixes a bug where Newton model requirements were not correctly determined in some environment configurations (particularly Direct envs that add cameras in _setup_scene after scene construction). The fix refactors requirement resolution into a reusable _refresh_visualizer_clone_fn_from_requirements() method that aggregates discovered requirements with those already registered on the simulation context. Additionally, the PR reverts a previous fallback path in PhysxSceneDataProvider that built Newton models from USD traversal at runtime, which was slow and is no longer needed.

Architecture Impact

Moderate cross-module impact:

• InteractiveScene: Now calls _refresh_visualizer_clone_fn_from_requirements() in both __init__ and clone_environments(), ensuring requirements registered after scene init (e.g., by sensors/cameras) are captured.
• Camera: New _register_renderer_scene_data_requirements() method proactively registers renderer-driven requirements at camera construction time, before clone-time prebuild.
• PhysxSceneDataProvider: Removes the USD-traversal fallback (_build_newton_artifact_from_usd_fallback), making the provider strictly dependent on prebuilt artifacts from the cloner.
• The change to visualizer_cfg.py (default cam_source from "cfg" to "prim_path") affects all visualizers using the default config.

Implementation Verdict

Minor fixes needed — The core fix is sound and well-tested, but there are a few issues to address.

Test Coverage

Good test coverage for the new paths:

• test_interactive_scene.py: New test test_refresh_visualizer_clone_fn_uses_registered_requirements verifies the refactored method respects registered requirements.
• test_camera.py: New test test_camera_registers_renderer_scene_data_requirements verifies early registration.
• test_physx_scene_data_provider_visualizer_contract.py: Tests updated to reflect removal of USD fallback.

Missing: No integration test verifying the end-to-end scenario (Direct env with camera added in _setup_scene → Newton model correctly built). This was the original bug.

CI Status

Most CI jobs are pending or skipped. pre-commit and Build Wheel passed, which covers linting and packaging.

Findings

🔴 Critical: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:165-199 — Removal of fallback creates silent failure mode for Direct envs

The PR removes _build_newton_artifact_from_usd_fallback() entirely, but the camera's early registration (the fix) only works when SimulationContext.instance() is available. For Direct envs that construct cameras before SimulationContext exists, _register_renderer_scene_data_requirements() silently returns (line 166-168 in camera.py), and the removed fallback was the safety net. This could break Direct envs that don't follow the expected initialization order.

# camera.py:166-168
sim = SimulationContext.instance()
if sim is None:
    logger.debug("SimulationContext not available; deferring renderer requirements registration.")
    return  # No retry mechanism exists

🟡 Warning: source/isaaclab/isaaclab/visualizers/visualizer_cfg.py:43 — Unrelated default change without migration guidance

The default cam_source changed from "cfg" to "prim_path". This is a behavior change affecting all visualizers using the default config. Users who relied on eye/lookat positioning will now get camera-prim following instead. This should be documented in release notes or reverted if unintentional.

🟡 Warning: source/isaaclab/isaaclab/scene/interactive_scene.py:234-261 — Double resolution in clone_environments when called after init

clone_environments() now calls _refresh_visualizer_clone_fn_from_requirements() unconditionally, but __init__ already calls it. When has_scene_cfg_entities is True, clone_environments is called from __init__, resulting in two consecutive calls. The second call is redundant but not harmful — just wasteful.

# Line 189-195 in __init__:
self._refresh_visualizer_clone_fn_from_requirements(requested_viz_types)

if has_scene_cfg_entities:
    self.clone_environments(...)  # Calls _refresh_visualizer_clone_fn_from_requirements() again

🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:157-178 — ValueError silently caught and ignored

When requirement_for_renderer_type(renderer_type) raises ValueError for an unknown renderer type, the exception is caught and the method returns silently. This hides configuration errors from users.

try:
    renderer_req = requirement_for_renderer_type(renderer_type)
except ValueError:
    return  # Should this log a warning for unknown renderer types?

🔵 Improvement: source/isaaclab/test/scene/test_interactive_scene.py:135-186 — Test setup duplicates mock configuration

Both test_clone_environments_non_cfg_invokes_visualizer_clone_fn and test_refresh_visualizer_clone_fn_uses_registered_requirements set up similar mock scenes. Consider extracting a shared fixture or helper to reduce duplication and improve maintainability.

🔵 Improvement: source/isaaclab/isaaclab/scene/interactive_scene.py:237-261 — Consider caching resolved clone_fn to avoid recomputation

_refresh_visualizer_clone_fn_from_requirements() recomputes requirements and resolves the clone function on every call. If requirements haven't changed, this work is redundant. Consider short-circuiting when requirements == current_req and visualizer_clone_fn is already set.

🤖 Isaac Lab Review Bot

Summary

This PR claims to "Patch newton model reqs" but actually contains 203 files changed with 10,362 additions. The PR bundles multiple unrelated changes: (1) CI/workflow refactoring to centralize image configuration, (2) new experimental Warp-first MDP terms with extensive parity tests, (3) documentation rewrites for the mimic tutorial, (4) scene data requirements propagation fixes, and (5) replay_demos.py improvements. The core "newton model reqs" fix appears to be in interactive_scene.py where requirements are now refreshed before cloning, but this is buried among many other changes.

Architecture Impact

High impact across multiple subsystems:

1. CI Infrastructure: All workflows now depend on a new config.yaml for image tags, creating a single point of failure/configuration
2. Scene Data Requirements: Changes to InteractiveScene affect how cameras and visualizers register their requirements at clone time
3. Experimental MDP Pipeline: New Warp-first observation/reward/termination/event/action terms with capture-safety considerations
4. PhysxSceneDataProvider: Removed USD fallback path when prebuilt artifact is missing—this is a behavioral change

Implementation Verdict

Needs rework — This PR should be split into multiple focused PRs. The newton model requirements fix is reasonable but obscured by unrelated changes.

Test Coverage

The experimental Warp MDP parity tests are thorough with capture-mutate-replay patterns. However:

• No regression test for the core newton model requirements fix
• No test for the Camera requirements registration path
• The removed USD fallback in PhysxSceneDataProvider lacks test coverage for the new "missing" error state behavior

CI Status

Most jobs are pending. Pre-commit passed, base image builds succeeded.

Findings

🔴 Critical: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:~line 84 — Removed USD fallback may break existing workflows

The diff removes the _build_newton_artifact_from_usd_fallback call when no prebuilt artifact exists. The test file test_physx_scene_data_provider_visualizer_contract.py confirms this: the old test_load_prebuilt_artifact_missing_falls_back_to_usd_build test was replaced with test_load_prebuilt_artifact_missing_sets_error_state. This changes behavior from "graceful fallback" to "fail with missing state". Any code path that previously relied on the USD fallback will now silently fail to build the newton model.

🔴 Critical: source/isaaclab/isaaclab/sensors/camera/camera.py:132-154 — Potential initialization order issue

The new _register_renderer_scene_data_requirements() is called in __init__ before super().__init__(cfg). If the SimulationContext isn't instantiated yet (which depends on scene initialization order), this silently skips registration with only a debug log. The fix in interactive_scene.py calls _refresh_visualizer_clone_fn_from_requirements() to catch this, but there's a race condition window where cameras could be constructed after scene init but before cloning.

🟡 Warning: source/isaaclab/isaaclab/scene/interactive_scene.py:237-261 — Redundant requirements computation

_refresh_visualizer_clone_fn_from_requirements is now called twice during normal scene setup: once in __init__ (line 193) and again in clone_environments (line 209). While the aggregation is idempotent, this adds overhead:

def clone_environments(self, copy_from_source: bool = False):
    self._refresh_visualizer_clone_fn_from_requirements()  # Called here

Consider adding a flag to skip if already computed.

🟡 Warning: source/isaaclab_experimental/isaaclab_experimental/envs/mdp/events.py:30-31 — WarpCapturable decorator on non-capturable function

@WarpCapturable(False, reason="set_coms_mask calls SimulationManager.add_model_change")
def randomize_rigid_body_com(...):

The WarpCapturable(False) decorator marks this as not capture-safe, but the decorator implementation isn't shown. If this is just documentation, it should be a docstring note. If it's runtime enforcement, verify it raises during graph capture.

🟡 Warning: scripts/tools/replay_demos.py:280-281 — Mutable default argument pattern

episode_indices_to_replay = list(args_cli.select_episodes)

While list() creates a copy, this is passed to replay_episodes_loop where it's mutated via pop(0):

next_episode_index = episode_indices_to_replay.pop(0)  # line 161

This works but is fragile. Consider using an iterator or index counter instead of mutating the list.

🔵 Improvement: .github/workflows/config.yaml — Missing version pinning validation

The new centralized config doesn't validate that the isaacsim_image_tag exists before jobs start. A typo in this file would cause all CI jobs to fail at docker pull time rather than failing fast. Consider adding a validation step in the config job.

🔵 Improvement: source/isaaclab_experimental/test/envs/mdp/parity_helpers.py:1-613 — Test helper module location

This 600+ line helper module is placed in the test directory but uses relative imports (from parity_helpers import ...). This works due to pytest's path manipulation but would break if tests are run differently. Consider making this a proper package or using absolute imports.

🔵 Improvement: PR scope and description mismatch

The PR description says "Patch newton model reqs issue" but includes massive unrelated changes: CI refactoring, documentation rewrites, new experimental features. This makes review difficult and risks regression. Split into focused PRs.

🤖 Isaac Lab Review Bot

Summary

This PR has a misleading title ("Patch newton model reqs") but contains massive scope creep — 203 files, 10K+ additions spanning CI infrastructure, documentation rewrites, experimental warp MDP implementations, and scene data requirements fixes. The core fix appears to be ensuring Newton model requirements are correctly determined when environment configs construct sensors dynamically, but this is buried under unrelated changes that should be separate PRs.

Architecture Impact

High cross-module impact:

1. CI Infrastructure — Complete overhaul of workflow configs, adding a central config.yaml and removing postmerge-ci.yml in favor of publish-images.yaml
2. Scene Data Requirements — Changes to InteractiveScene and Camera to eagerly register renderer requirements before cloning
3. Experimental Warp MDP — Large additions to isaaclab_experimental with new parity tests, events, observations, rewards, terminations
4. PhysX Scene Data Provider — Removes USD fallback path for Newton model building (potential breaking change)

Implementation Verdict

Needs rework — This PR conflates at least 4 independent changes that should be separate:

1. Newton model requirements fix (the stated goal)
2. CI workflow refactoring
3. Mimic documentation rewrite
4. Experimental warp MDP additions

Test Coverage

The experimental warp MDP additions have extensive parity tests (~2500 lines), which is excellent. However:

• The core Newton model requirements fix lacks a direct regression test
• The PhysX scene data provider change removing USD fallback has test changes but no new coverage for the error path

CI Status

Several checks still pending (Installation Tests, license-check, Build Latest Docs). Pre-commit and wheel build passed.

Findings

🔴 Critical: source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py:L25-84 — Breaking change removes USD fallback without deprecation
The diff removes _build_newton_artifact_from_usd_fallback and changes the missing artifact path from attempting USD fallback to immediately setting error state. This is a breaking change for any downstream code relying on the fallback behavior. The test file confirms this:

# Before: test_load_prebuilt_artifact_missing_falls_back_to_usd_build
# After: test_load_prebuilt_artifact_missing_sets_error_state

This needs a deprecation warning or migration path, not silent removal.

🔴 Critical: source/isaaclab/isaaclab/scene/interactive_scene.py:233-258 — _refresh_visualizer_clone_fn_from_requirements is called twice with different visualizer_types

# Line 192 (in __init__):
self._refresh_visualizer_clone_fn_from_requirements(requested_viz_types)

# Line 212 (in clone_environments):
self._refresh_visualizer_clone_fn_from_requirements()  # No visualizer_types passed!

The second call passes empty visualizer_types=(), which will only aggregate renderer requirements from _sensor_renderer_types(). This asymmetry seems unintentional and could cause requirements to be incomplete when clone_environments is called separately from __init__.

🟡 Warning: source/isaaclab/isaaclab/sensors/camera/camera.py:157-178 — _register_renderer_scene_data_requirements called before parent init

def __init__(self, cfg: CameraCfg):
    self._check_supported_data_types(cfg)
    super().__init__(cfg)  # Parent init
    self._register_renderer_scene_data_requirements()  # Called after super().__init__

This is correct, but the method accesses self.cfg which is set in parent. If parent init fails, this won't be called. However, looking at the implementation:

def _register_renderer_scene_data_requirements(self) -> None:
    renderer_type = getattr(getattr(self.cfg, "renderer_cfg", None), "renderer_type", None)

This accesses self.cfg from the local assignment, not from parent. This works but is fragile.

🟡 Warning: source/isaaclab_experimental/isaaclab_experimental/envs/mdp/events.py:71-75 — randomize_rigid_body_com marked as non-capturable but may still be called during capture

@WarpCapturable(False, reason="set_coms_mask calls SimulationManager.add_model_change")
def randomize_rigid_body_com(...)

The WarpCapturable decorator documents intent but doesn't enforce it at runtime. If this function is called during CUDA graph capture, it will silently produce incorrect behavior rather than raising an error.

🟡 Warning: scripts/tools/replay_demos.py:178-195 — reset_sim_buffer_each_episode validation happens too late

if args_cli.reset_sim_buffer_each_episode and num_envs != 1:
    raise ValueError(...)

This check happens after episode_indices_to_replay is populated but the docstring implies this is for replay correctness. The check should be at argument parsing time to fail fast.

🔵 Improvement: source/isaaclab_experimental/test/envs/mdp/parity_helpers.py — Hardcoded device string "cuda:0"

DEVICE = "cuda:0"

This should use torch.cuda.current_device() or be parameterized for multi-GPU systems. Currently tests will fail on systems where cuda:0 isn't available or isn't the intended test device.

🔵 Improvement: .github/workflows/config.yaml — Using internal nvidian registry in public config

isaacsim_image_name: nvcr.io/nvidian/isaac-sim
isaaclab_image_name: nvcr.io/nvidian/isaac-lab

The nvidian namespace is NVIDIA-internal. This config is checked into a public repo and will fail for external contributors.

🔵 Improvement: source/isaaclab/isaaclab/envs/manager_based_rl_mimic_env.py:127,144 — @optional_method added but methods still raise NotImplementedError

@optional_method
def get_subtask_start_signals(...):
    raise NotImplementedError

The @optional_method decorator typically indicates a method that can be left unimplemented, but the body still raises. This is inconsistent — either remove the decorator or change the body to return None or an empty dict.