-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix --video producing black frames without --viz kit (issue #5052) #5056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -604,6 +604,21 @@ def render(self, mode: int | None = None) -> None: | |
| for callback in self._render_callbacks.values(): | ||
| callback(None) # Pass None as event data | ||
|
|
||
| # When running with Isaac Sim (Kit) and no active visualizer already pumps the Kit | ||
| # app loop, call app.update() so the viewport and replicator render products | ||
| # (used e.g. by gym.wrappers.RecordVideo with render_mode="rgb_array") are refreshed. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider gating this more tightly. At minimum, checking Alternatively, a dedicated "needs_app_pump" flag set when |
||
| # KitVisualizer.pumps_app_update() returns True and calls app.update() in its own | ||
| # step(), so we skip this call to avoid double-rendering in that case. | ||
| if has_kit() and not any(v.pumps_app_update() for v in self._visualizers): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit: redundant
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 |
||
| try: | ||
| import omni.kit.app | ||
|
|
||
| app = omni.kit.app.get_app() | ||
| if app is not None and app.is_running(): | ||
| app.update() | ||
| except (ImportError, AttributeError): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Critical: Missing Calling # 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):
passWithout this, every
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| pass | ||
|
|
||
| def update_visualizers(self, dt: float) -> None: | ||
| """Update visualizers without triggering renderer/GUI.""" | ||
| if not self._visualizers: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -290,6 +290,67 @@ def test_render(): | |
| assert sim.is_playing() | ||
|
|
||
|
|
||
| @pytest.mark.isaacsim_ci | ||
| def test_render_pumps_app_update_without_visualizer(): | ||
| """Regression test for issue #5052: render() must call app.update() when no visualizer pumps the Kit loop. | ||
|
|
||
| Without this call, replicator render products (used by gym.wrappers.RecordVideo for | ||
| rgb_array rendering) are never updated, producing black video frames. | ||
| """ | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| cfg = SimulationCfg(dt=0.01) | ||
| sim = SimulationContext(cfg) | ||
| sim.reset() | ||
|
|
||
| mock_app = MagicMock() | ||
| mock_app.is_running.return_value = True | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Test gap: mock patches The Consider also patching 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 |
||
| with patch("omni.kit.app.get_app", return_value=mock_app): | ||
| sim.render() | ||
|
|
||
| # app.update() must be called when no visualizer pumps the Kit app loop | ||
| mock_app.update.assert_called_once() | ||
|
|
||
|
|
||
| @pytest.mark.isaacsim_ci | ||
| def test_render_skips_app_update_when_visualizer_pumps_it(): | ||
| """Regression test: render() must NOT call app.update() when a visualizer already does. | ||
|
|
||
| A visualizer that returns ``pumps_app_update() == True`` (e.g. KitVisualizer) calls | ||
| ``app.update()`` in its own ``step()``, so ``SimulationContext.render()`` must not | ||
| call it again to avoid double-rendering. | ||
| """ | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from isaaclab.visualizers.base_visualizer import BaseVisualizer | ||
|
|
||
| cfg = SimulationCfg(dt=0.01) | ||
| sim = SimulationContext(cfg) | ||
| sim.reset() | ||
|
|
||
| # Inject a mock visualizer that pumps the app update | ||
| mock_viz = MagicMock(spec=BaseVisualizer) | ||
| mock_viz.pumps_app_update.return_value = True | ||
| mock_viz.is_closed = False | ||
| mock_viz.is_running.return_value = True | ||
| mock_viz.is_rendering_paused.return_value = False | ||
| mock_viz.is_training_paused.return_value = False | ||
| mock_viz.get_rendering_dt.return_value = None | ||
| sim._visualizers = [mock_viz] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Test improvement: verify This test injects a mock visualizer into Consider also asserting that |
||
|
|
||
| mock_app = MagicMock() | ||
| mock_app.is_running.return_value = True | ||
|
|
||
| with patch("omni.kit.app.get_app", return_value=mock_app): | ||
| sim.render() | ||
|
|
||
| # app.update() must NOT be called since the visualizer already pumps it | ||
| mock_app.update.assert_not_called() | ||
|
|
||
| sim._visualizers = [] | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 Good: Cleaning up |
||
|
|
||
| """ | ||
| Stage Operations Tests | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Changelog format looks correct — follows the existing pattern (version, date, section header, bullet). Minor note: the date
2026-03-18is in the past relative to the PR creation — just verify this is intentional per your release process.