Patch newton model reqs

[matthewtrepte]

Description

Patch newton model reqs issue where in some caes env configs were constructed in a way where newton model reqs were not correctly determined, leading to downstream issues.

Also, revert the revert to physx scene data provider, since the fix above no longer requires the revert, which is non ideal since building newton model from usd fallback is slow.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

🤖 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.