Expand Visualizer Tests and Patch Visualizer Bugs#5103
Expand Visualizer Tests and Patch Visualizer Bugs#5103matthewtrepte wants to merge 12 commits intoisaac-sim:developfrom
Conversation
Greptile SummaryThis PR fixes two visualizer bugs and expands unit-test coverage: the Newton Visualizer's "Pause Rendering" button was unresponsive because the event loop was completely skipped, and the Kit Visualizer's Play/Pause was only affecting rendering instead of simulation because it relied on a Key changes:
Acknowledged open issue: The PR description notes a known regression where body transforms remain stale after unpausing the Kit Visualizer; this is deferred to a follow-up. Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[update_visualizers called with dt] --> B[update_scene_data_provider]
B --> C{For each visualizer}
C --> D{is_closed OR not is_running?}
D -- Yes --> E[Add to removal list]
E --> C
D -- No --> F{is_rendering_paused?}
F -- Yes --> G["viz.step(0.0)\n(keep UI responsive)"]
G --> C
F -- No --> H{is_training_paused\nAND is_running?}
H -- Yes --> I["viz.step(0.0)\n(spin-wait loop)"]
I --> H
H -- No --> J["viz.step(dt)\n(normal advance)"]
J --> C
C -- Done --> K[Close & remove\nstale visualizers]
Reviews (1): Last reviewed commit: "move branch" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "torch>=2.10", | ||
| "onnx>=1.18.0", # 1.16.2 throws access violation on Windows | ||
| "prettytable==3.3.0", | ||
| "munch", |
There was a problem hiding this comment.
munch dependency has no version constraint
All other dependencies in this file are pinned or have minimum version bounds (e.g. torch>=2.10, prettytable==3.3.0). An unpinned munch can silently pull in a breaking release. Consider adding a lower bound that has been verified to work:
| "munch", | |
| "munch>=4.0.0", |
| try: | ||
| import omni.kit.app | ||
|
|
||
| from isaaclab.app.settings_manager import get_settings_manager | ||
|
|
||
| app = omni.kit.app.get_app() | ||
| if app is not None and app.is_running(): | ||
| # Keep app pumping for viewport/UI updates only. | ||
| # Simulation stepping is owned by SimulationContext. | ||
| settings = get_settings_manager() | ||
| settings.set_bool("/app/player/playSimulations", False) | ||
| # Pump Kit/UI events only; SimulationContext owns physics stepping. | ||
| app.update() | ||
| settings.set_bool("/app/player/playSimulations", True) | ||
| except (ImportError, AttributeError) as exc: | ||
| logger.debug("[KitVisualizer] App update skipped: %s", exc) |
There was a problem hiding this comment.
playSimulations guard
The removed code blocked Kit from advancing physics during app.update() by temporarily setting /app/player/playSimulations to False. The guard was clearly broken — step() toggled the flag back to True immediately, so the old is_training_paused() could never observe False — but removing it entirely means Kit is now free to tick physics during app.update() when the timeline is in "playing" state, which could cause double-stepping alongside SimulationContext.
In practice Isaac Sim's app.update() should not independently step physics (physics ownership is expected to live in SimulationContext), and the new is_training_paused() correctly checks the timeline transport state rather than the now-gone setting. However, if a regression appears where physics advances twice per simulation step, this is the first place to investigate.
It would be worth adding a short comment here explaining why the guard was removed (it was effectively a no-op), so future readers don't re-introduce it.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #5103
Summary
This PR addresses two visualizer bugs and expands test coverage with non-black-frame integration tests. The changes span 5 files:
simulation_context.py— Addsviz.step(0.0)when rendering is paused, keeping the visualizer event loop responsive so UI controls (e.g. "Resume Rendering") remain interactive.setup.py— Addsmunchas a dependency for the Newton visualizer.test_simulation_context_visualizers.py— Adds a dedicated unit test for the rendering-paused fix and updates an existing test assertion.kit_visualizer.py— Rewritesstep()to usecarb.settingsdirectly (replacingget_settings_manager), and conditionally skipsapp.update()whenplaySimulationsis already falsy. Rewritesis_training_paused()to useomni.timelineinstead of the settings flag.test_visualizer_smoke_logs.py— Adds ~174 lines of integration test infrastructure and two new parametrized tests (test_kit_visualizer_non_black_viewport_frame,test_newton_visualizer_non_black_viewer_frame) that load a Cartpole camera scene and assert non-black rendered output.
Design Assessment
Rendering-paused fix (simulation_context.py): ✅ Sound approach. Calling viz.step(0.0) with zero dt while rendering is paused keeps event loops responsive without advancing simulation time. The fix targets the root cause — previously the continue statement completely skipped the visualizer, leaving its UI unresponsive. The zero-dt convention is already established for the training-pause loop on the adjacent line.
Kit visualizer step() rewrite: ✅ Good improvement. Replacing get_settings_manager() with direct carb.settings access removes an unnecessary abstraction layer. The conditional should_restore logic avoids the previous bug where app.update() was unconditionally called even when simulations weren't playing, which caused the Play/Pause button to erroneously unpause the physics.
Kit visualizer is_training_paused() rewrite: ✅ Correct. Using omni.timeline transport state is semantically accurate — the old implementation checked the /app/player/playSimulations settings flag which could be transiently toggled by step() itself, creating false positives for training pause detection. Timeline transport state is the authoritative source.
Non-black-frame tests:
Architecture Impact
- Low risk. The
simulation_context.pychange is a one-line addition inside an existing code path. - The
kit_visualizer.pychanges replace one internal API (get_settings_manager) with another (carb.settings), reducing coupling. The timeline-based pause detection is more robust. - Adding
munchtosetup.pyis a new runtime dependency — this is appropriate if Newton visualizer requires it, but the version should ideally be pinned (see Finding #1).
Implementation Verdict
Approve with findings. The bug fixes are correct and well-targeted. The test infrastructure is solid. However, the new integration tests have CI reliability issues that should be addressed before merge.
Test Coverage
| Area | Coverage |
|---|---|
Rendering-paused viz.step(0.0) |
✅ Dedicated unit test + updated existing test assertion |
Kit step() conditional update |
carb.settings |
Kit is_training_paused() timeline |
|
| Non-black frame detection helpers | ✅ Exercised by integration tests |
| Non-black viewport tests | 🔴 3/3 new integration test parametrizations failing in CI |
CI Status
| Job | Status | Notes |
|---|---|---|
isaaclab_visualizers |
🔴 FAIL | 3 new tests fail: test_kit_visualizer_non_black_viewport_frame[newton] (CUDA OOM during render product allocation), test_newton_visualizer_non_black_viewer_frame[physx] and [newton] (CUDA illegal memory access during SimulationContext init — GPU state corrupted by prior test) |
isaaclab (core) [1-3/3] |
🔴 FAIL | Failures in test_non_headless_launch, test_first_frame_textured_rendering, test_ray_caster_camera — unrelated to this PR |
isaaclab_physx |
🔴 FAIL | Likely pre-existing, unrelated |
isaaclab_tasks [3/3] |
🔴 FAIL | Likely pre-existing, unrelated |
environments_training |
🔴 FAIL | Likely pre-existing, unrelated |
pre-commit |
✅ PASS | |
license-check |
✅ PASS |
Findings
Finding 1 — munch dependency is unpinned
File: source/isaaclab/setup.py, line 25
Severity: Low
Issue: munch is added without a version pin. Every other dependency in this section has an explicit version or minimum version constraint. An unpinned munch could cause unexpected breakage if a future major version introduces breaking API changes.
Suggestion: Pin to a minimum version, e.g. "munch>=4.0.0" or "munch==4.0.0".
Finding 2 — Kit step() skips app.update() when playSimulations is already falsy
File: source/isaaclab_visualizers/isaaclab_visualizers/kit/kit_visualizer.py, lines 108-119
Severity: Medium
Issue: The new logic only calls app.update() inside if should_restore, meaning when playSimulations is already False, app.update() is never called at all. The old code unconditionally called app.update() (after disabling playSimulations). This behavioral change means the Kit viewport will not pump UI events when simulation is already paused, which could prevent viewport refresh in paused-then-rendering scenarios.
# Current:
should_restore = bool(previous_play_simulations)
if should_restore:
settings.set_bool(play_simulations_path, False)
app.update()
if should_restore:
settings.set_bool(play_simulations_path, True)
# Consider instead:
should_restore = bool(previous_play_simulations)
if should_restore:
settings.set_bool(play_simulations_path, False)
app.update() # Always pump UI events
if should_restore:
settings.set_bool(play_simulations_path, True)Alternatively, if the intent is specifically to skip app.update() when paused, document this behavior with a comment explaining why.
Finding 3 — New integration tests are not resilient to GPU state corruption from prior tests
File: source/isaaclab_visualizers/test/test_visualizer_smoke_logs.py
Severity: Medium
Issue: The CI failures show that test_newton_visualizer_non_black_viewer_frame[physx] and [newton] fail with torch.AcceleratorError: CUDA error: an illegal memory access during SimulationContext.__init__ — this is GPU state corruption carried over from the preceding test_kit_visualizer_non_black_viewport_frame[newton] failure (CUDA OOM in render product allocation). Since all tests run in a single process (required by Kit app lifetime), a CUDA error in one test poisons all subsequent GPU operations.
Suggestion: Consider adding pytest.mark.order to run the non-black frame tests last, or adding a CUDA device reset/sync check at the start of each test. Alternatively, mark these as @pytest.mark.xfail(reason="Requires dedicated GPU resources") until the CI environment is stabilized, so they don't block the PR pipeline.
Finding 4 — PR description mentions a known TODO that is not addressed
Severity: Info
Issue: The PR body states: "TODO: fix Unpause behavior, currently body transforms remain stale after Unpausing... recent regression". This indicates a known remaining bug in the Kit visualizer's pause/unpause lifecycle. This is fine as a known limitation, but consider filing a tracking issue so it doesn't get lost.
Review by 🤖 Isaac Lab Review Bot — 4a42c18b
matthewtrepte
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai and
ooctipus
as code owners
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up
New commits (4a42c18→053adbd4): renamed camera_position/camera_target → eye/lookat across the codebase, simplified Kit step(), and merged develop.
One new issue found in the rename.
|
|
||
| eye: tuple[float, float, float] = (7.5, 7.5, 7.5) | ||
| eye: tuple[float, float, float] = (7.5, 7.5, 117.5) | ||
| """Initial camera position (in m). Default is (7.5, 7.5, 7.5).""" |
There was a problem hiding this comment.
ViewerCfg.eye default appears to have a typo: (7.5, 7.5, 117.5). The old value was (7.5, 17.5, 17.5) — so both the y and z components changed. A z of 117.5 places the camera ~118m above the scene, which seems unintentional. The docstring also says "Default is (7.5, 7.5, 7.5)" which doesn't match either value.
Did you mean (7.5, 7.5, 7.5) (matching the docstring) or (7.5, 17.5, 17.5) (preserving the old default)?
1 participant
Description
Tests
Newton Visualizer
Kit Visualizer
RTX Renderer
Else
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there