General Visualizer Fixes#5935
Conversation
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: General Visualizer Fixes
Thanks for cleaning up the visualizer stack! The infinite-plane expansion, improved error messages, and imgui dependency checks are all solid improvements. A few items worth discussing:
Summary of Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | 🟡 Medium | visualizer_cfg.py |
Default tiled_cam_view changed from False → True — potential breaking behavior change |
| 2 | 🟡 Medium | newton_adapter.py |
expand_infinite_plane_scale replaces both axes even when only one is non-positive |
| 3 | 🟢 Low | rerun_visualizer.py / viser_visualizer.py |
log_geo override is duplicated verbatim between both viewers |
| 4 | 🟢 Low | pyproject.toml |
pyarrow==22.0.0 exact pin may conflict with user environments |
| 5 | 🟢 Low | CI | changelog fragments and pre-commit checks are currently failing |
CI Status: changelog fragments ❌ and pre-commit ❌ are failing — likely minor formatting or fragment issues to address before merge.
Update (126f197)
New commits address the main concerns:
- ✅ Finding 1 Fixed:
tiled_cam_viewdefault reverted toFalse— no breaking behavior change. - ✅ Finding 2 Fixed:
expand_infinite_plane_scalenow preserves valid dimensions independently (only replaces non-positive axes). - ✅ Finding 3 Fixed: Shared
log_geo_with_expanded_plane_scalehelper extracted tonewton_adapter.py— DRY. - ✅ HUD warning Fixed: Module-level
NEWTON_HUD_IMPORT_LOG_WARNINGconstant now shared between visualizer and integration tests.
New additions in this push (scene-data backend hook for articulation transforms in physx_manager.py, video recorder multi-visualizer warning, Viser plane-grid caching, docs clarifications) look clean. No new issues found in the incremental changes. 👍
Update (feb7046)
Large batch of changes unrelated to the visualizer fixes — primarily task/env architecture refactoring. Visualizer-specific changes in this push are limited to:
isaaclab_visualizersversion bump (0.1.0 → 0.1.1) and changelog consolidation from fragments- Test import path update (
CartpoleCameraPresetsEnvCfg→CartpoleCameraEnvCfg) following the cartpole task refactor
No new visualizer-specific issues. The broader changes (Newton SDF collision API, cartpole camera consolidation to channel-first output, isaaclab_experimental lazy-export refactoring, TacSL PhysX migration, cloner nested-template fix, RSL-RL CNNModel for image-only policies) are well-structured and follow established patterns. LGTM on the visualizer portions. 👍
|
|
||
| # Tiled camera settings | ||
| tiled_cam_view: bool = False | ||
| tiled_cam_view: bool = True |
There was a problem hiding this comment.
🟡 Behavior change: Changing the default from False to True means all visualizer users will now get tiled camera views by default, even if their environment has no Camera sensors. Combined with the improved error message in camera_view.py, this could surface RuntimeError for existing configurations that previously worked (because tiled cam was off).
Was this intentional? If so, consider mentioning it in the changelog fragment as a behavior change rather than just a bug fix, so downstream users are aware.
| tiled_cam_view: bool = True | |
| tiled_cam_view: bool = True |
| length = scale[1] if len(scale) > 1 else 0.0 | ||
| if width > 0.0 and length > 0.0: | ||
| return scale | ||
| tail = scale[2:] if len(scale) > 2 else () |
There was a problem hiding this comment.
🟡 Logic concern: When only one axis is non-positive (e.g. (-1.0, 25.0)), the function replaces both with plane_size, discarding the valid dimension. Newton may encode "infinite in X, finite in Y" planes — in that case the intended 25 m extent would be silently overwritten to 1000 m.
Consider replacing each axis independently:
def expand_infinite_plane_scale(
geo_scale: tuple[float, ...], plane_size: float = VISUALIZER_INFINITE_PLANE_SIZE
) -> tuple[float, ...]:
scale = tuple(float(value) for value in geo_scale)
width = scale[0] if len(scale) > 0 else 0.0
length = scale[1] if len(scale) > 1 else 0.0
if width > 0.0 and length > 0.0:
return scale
new_width = width if width > 0.0 else plane_size
new_length = length if length > 0.0 else plane_size
tail = scale[2:] if len(scale) > 2 else ()
return (new_width, new_length, *tail)If Newton's convention guarantees both are always non-positive together, a brief comment explaining that invariant would be helpful.
| @@ -134,6 +139,21 @@ def _render_ui(self): | |||
| if imgui.button("Pause Rendering" if not self._paused_rendering else "Resume Rendering"): | |||
There was a problem hiding this comment.
🟢 DRY: This log_geo override is byte-for-byte identical to the one in ViserVisualizerViewer. Consider extracting a shared mixin or a standalone helper function (e.g. in newton_adapter.py) to avoid maintaining the same logic in two places:
# In newton_adapter.py:
def patched_log_geo(super_log_geo, name, geo_type, geo_scale, geo_thickness, geo_is_solid, geo_src, hidden):
if geo_type == newton.GeoType.PLANE:
geo_scale = expand_infinite_plane_scale(geo_scale)
return super_log_geo(name, geo_type, geo_scale, geo_thickness, geo_is_solid, geo_src, hidden)Not blocking, but would reduce maintenance burden as more visualizer backends are added.
| "imgui-bundle>=1.92.5", | ||
| "typing-extensions>=4.15.0", | ||
| ] | ||
| rerun = [ |
There was a problem hiding this comment.
🟢 Nit: pyarrow==22.0.0 is a very tight pin. If this is inherited from the old setup.py for compatibility with rerun-sdk, consider documenting why this exact version is required (e.g. a compatibility note), or relaxing to pyarrow>=22.0.0,<23 if rerun supports a range. Exact pins can cause resolution conflicts in larger environments.
| "Install isaaclab-visualizers[newton] or fix its transitive dependencies " | ||
| "(for example typing-extensions>=4.15.0). ImportError: %s", | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
🟢 Minor: The warning message string here ("[NewtonVisualizer] Newton HUD disabled: failed to import imgui_bundle.") differs from the sentinel string checked in the integration test (_NEWTON_HUD_IMPORT_LOG_WARNING = "Newton Visualizer HUD disabled: failed to import imgui_bundle. This can be caused by conflicting libraries."). The test's sentinel won't match this actual warning.
Double-check that _NEWTON_HUD_IMPORT_LOG_WARNING in visualizer_integration_utils.py exactly matches the logged message, or use a substring that's guaranteed to appear in both.
Greptile SummaryThis PR fixes several visualizer issues: infinite ground planes now render at a proper large finite size in Rerun and Viser by overriding
Confidence Score: 3/5Two changes need attention before merging: the _NEWTON_HUD_IMPORT_LOG_WARNING constant is mismatched with the actual warning text making the new regression detector a no-op, and tiled_cam_view was silently flipped to True as the default changing behavior for all existing visualizer users. The ground-plane and HUD-probe changes are well-structured and tested. However, the integration-test helper added to catch imgui_bundle failures will never fire because the substring it checks does not appear anywhere in the codebase. Additionally, the silent flip of tiled_cam_view changes the out-of-the-box behavior of every VisualizerCfg subclass without a changelog note. source/isaaclab_visualizers/test/visualizer_integration_utils.py (broken warning constant) and source/isaaclab/isaaclab/visualizers/visualizer_cfg.py (default-value flip). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NewtonVisualizer.__init__] --> B[_log_newton_hud_dependency_issue]
B --> C{imgui_bundle importable?}
C -- Yes --> D[HUD enabled, create NewtonViewerGL]
C -- No --> E[logger.warning: Newton HUD disabled]
E --> D
F[NewtonViewerRerun.log_geo] --> G{geo_type == PLANE?}
G -- Yes --> H[expand_infinite_plane_scale]
H --> I[super.log_geo with large extents]
G -- No --> I
J[NewtonViewerViser.log_geo] --> K{geo_type == PLANE?}
K -- Yes --> L[expand_infinite_plane_scale]
L --> M[super.log_geo with large extents]
K -- No --> M
Reviews (1): Last reviewed commit: "init fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ASSERT_VISUALIZER_WARNINGS = False | ||
|
|
||
| _NEWTON_IMGUI_BUNDLE_PRINT_WARNING = "Warning: imgui_bundle not found" | ||
| _NEWTON_HUD_IMPORT_LOG_WARNING = "Newton Visualizer HUD disabled: failed to import imgui_bundle. This can be caused by conflicting libraries." |
There was a problem hiding this comment.
_NEWTON_HUD_IMPORT_LOG_WARNING is checked as a substring of every log record's message, but it will never match the warning actually emitted by _log_newton_hud_dependency_issue(). The function logs "[NewtonVisualizer] Newton HUD disabled: failed to import imgui_bundle. Install isaaclab-visualizers[newton]…", while the constant here is "Newton Visualizer HUD disabled: failed to import imgui_bundle. This can be caused by conflicting libraries." — both the prefix and suffix differ. Because the substring never matches, logged_warnings will always be empty and assert_no_newton_hud_dependency_warning will always pass even when the real warning fires, making the entire new regression detector inoperable.
| _NEWTON_HUD_IMPORT_LOG_WARNING = "Newton Visualizer HUD disabled: failed to import imgui_bundle. This can be caused by conflicting libraries." | |
| _NEWTON_HUD_IMPORT_LOG_WARNING = "Newton HUD disabled: failed to import imgui_bundle." |
| tiled_cam_view: bool = True | ||
| """Enable a non-interactive tiled camera image view.""" |
There was a problem hiding this comment.
tiled_cam_view was changed from False to True, enabling the tiled camera image view by default for every VisualizerCfg subclass when no explicit value is set. Any existing user that creates a visualizer without passing tiled_cam_view=False will now silently get a tiled camera view and the associated camera sensor creation overhead. The PR description and changelog entry don't mention this change.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
matthewtrepte
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
ossamaAhmed
as code owners
github-actions
Bot
added
the
documentation
1 participant
Description
Fixes
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