Fix --video producing black frames without --viz kit (issue #5052)

[pascal-roth]

Summary

• Root cause: SimulationContext.render() was not calling omni.kit.app.get_app().update() when no active visualizer pumped the Kit app loop. Replicator render products (used by gym.wrappers.RecordVideo via render_mode="rgb_array") were never refreshed, producing black video frames.
• Fix: Call app.update() in render() when has_kit() is True and no visualizer with pumps_app_update() == True is active. KitVisualizer already calls app.update() in its own step(), so we skip the call to avoid double-rendering when --viz kit is used.
• Tests: Two regression tests added to test_simulation_context.py — one verifying app.update() IS called without a visualizer, one verifying it is NOT called when a visualizer already pumps the Kit loop.

Test plan

• Regression test test_render_pumps_app_update_without_visualizer — fails without fix, passes with fix
• Regression test test_render_skips_app_update_when_visualizer_pumps_it — passes with fix (no double-rendering)
• Pre-commit checks pass (./isaaclab.sh -f)
• Manual: ./isaaclab.sh -p scripts/reinforcement_learning/rsl_rl/train.py --task=Isaac-Velocity-Rough-Anymal-C-v0 --video should produce correct (non-black) video

Closes #5052

🤖 Generated with Claude Code

[nblauch]

Hi @pascal-roth . Thanks for this! I tested it out. It does fix the blank videos, but it seems the camera position is not updated properly according to the ViewerCfg. For the example listed in my issue (#5052 ), the video outputs are from a different perspective for --viz none (headless) vs. --viz kit. I also found this for other custom scripts using a DirectRLEnv setup.

[pascal-roth]

Thanks for testing and reporting this, @nblauch!

The camera perspective mismatch you're seeing (headless --viz none vs --viz kit) is a separate issue from the black frames this PR addresses. In short: this PR only ensures app.update() is called so replicator render products are refreshed — it doesn't set the recording camera's position from ViewerCfg.

Good news: that exact problem is already being fixed in PR #5011 (Add perspective video recording via Newton GL viewer for Kitless backends; Fix for Kitfull backends camera position). That PR explicitly reads viewer.eye / viewer.lookat from the task config and passes them to the video recorder as camera_position / camera_target, so the recording matches the configured viewer perspective regardless of the visualizer backend.

To avoid duplicating the fix here and keeping both diffs focused, I'd suggest we track resolution in #5011. Once that lands, the ViewerCfg perspective will be honoured in all render modes.

Let me know if the scope there differs from what you experienced!

[kellyguo11]

@matthewtrepte please also review this along with the other related issues around headless visualization

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary: The fix correctly identifies the root cause — replicator render products need app.update() to refresh their buffers. However, the current implementation has a critical bug: it calls app.update() without first disabling /app/player/playSimulations, which risks double-stepping physics when the timeline is active. KitVisualizer.step() carefully brackets its app.update() call with playSimulations=False/True — this new code path must do the same. Tests and changelog are solid otherwise.

Architecture: Low-risk surface area (1 method touched), but the app.update() call is in the hot path for every render() invocation and has correctness + performance implications that need addressing.

[isaaclab-review-bot]

🔴 Critical: Missing playSimulations=False guard — potential double physics step

Calling app.update() while the timeline is playing and /app/player/playSimulations is True (the default) will cause Kit to pump its internal physics loop, resulting in a double physics step per frame. This is exactly what KitVisualizer.step() guards against:

# KitVisualizer.step() does this correctly:
settings.set_bool('/app/player/playSimulations', False)
app.update()
settings.set_bool('/app/player/playSimulations', True)

The fix here should mirror that pattern:

if has_kit() and not any(v.pumps_app_update() for v in self._visualizers):
    try:
        import omni.kit.app
        app = omni.kit.app.get_app()
        if app is not None and app.is_running():
            self.set_setting('/app/player/playSimulations', False)
            app.update()
            self.set_setting('/app/player/playSimulations', True)
    except (ImportError, AttributeError):
        pass

Without this, every env.step() → sim.step() → render() cycle runs physics twice, causing 2× simulation speed, energy non-conservation, and incorrect RL rewards.

[kellyguo11]

yes we should have the playSimulations guard. could this also be moved to the kit visualizer script so that we don't have kit-specific logic in the simulation context?

[isaaclab-review-bot]

⚠️ Performance concern: unconditional app.update() on every render

app.update() is expensive — it pumps the entire Kit event loop (USD hydra, viewport compositing, extension ticks, etc.). This now runs on every render() call, even when:

1. No video recording is active (RecordVideo wrapper not used)
2. No replicator render products exist
3. The user is just doing headless RL training with --video flag absent

Consider gating this more tightly. At minimum, checking self._has_offscreen_render or self.get_setting('/isaaclab/render/rtx_sensors') would limit the call to cases where render products actually exist. The has_kit() check alone is too broad — Kit is always present in Isaac Sim mode, even for pure headless training.

Alternatively, a dedicated "needs_app_pump" flag set when RecordVideo or camera sensors are initialized would be more precise and avoid per-frame setting lookups.

[isaaclab-review-bot]

🟡 Nit: redundant import + get_app() when has_kit() already confirmed Kit is running

has_kit() (in version.py) already calls sys.modules.get('omni.kit.app') → mod.get_app() and confirms the result is not None. Re-importing and re-calling get_app() here is redundant. You could simplify:

import sys
mod = sys.modules['omni.kit.app']  # guaranteed present by has_kit()
app = mod.get_app()                 # guaranteed non-None by has_kit()
# app.is_running() is still worth checking
if app.is_running():
    app.update()

Not blocking, but it would make the code cleaner and avoid the misleading except ImportError.

[isaaclab-review-bot]

🟡 Test gap: mock patches omni.kit.app.get_app but has_kit() is not mocked

The has_kit() function checks sys.modules.get('omni.kit.app') and calls the real get_app() — it doesn't use the patched version from unittest.mock.patch('omni.kit.app.get_app', ...). In CI where Kit IS running, has_kit() returns True from the real module, so this test passes. But in a kitless test environment, has_kit() would return False and the patched get_app would never be reached, making the assertion vacuously pass.

Consider also patching has_kit to make the test robust in all environments:

with patch('isaaclab.sim.simulation_context.has_kit', return_value=True), \
     patch('omni.kit.app.get_app', return_value=mock_app):
    sim.render()

Since these tests are gated by @pytest.mark.isaacsim_ci, this is not a blocker — but it's worth noting for maintainability.

[isaaclab-review-bot]

🟡 Test improvement: verify update_visualizers interaction with mock visualizer

This test injects a mock visualizer into _visualizers but doesn't fully account for what update_visualizers() does with it. update_visualizers() calls viz.is_closed, viz.is_running(), viz.is_rendering_paused(), viz.is_training_paused(), and viz.step(dt) — all of which are mocked. However, update_scene_data_provider() is also called, which accesses _scene_data_provider. If a future refactor changes update_visualizers' behavior, this test could break in non-obvious ways.

Consider also asserting that mock_viz.step.assert_called() to verify the visualizer's step() method (which is where KitVisualizer does its own app.update()) was actually invoked, confirming the intended code path.

[isaaclab-review-bot]

🟢 Good: Cleaning up sim._visualizers = [] after the test to avoid leaking mock state into the teardown's clear_instance(). Defensive and correct.

[isaaclab-review-bot]

🟢 Changelog format looks correct — follows the existing pattern (version, date, section header, bullet). Minor note: the date 2026-03-18 is in the past relative to the PR creation — just verify this is intentional per your release process.

[pascal-roth]

@michaellin6 confirmed that this is still an issue on latest develop

@AntoineRichard for viz