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 |
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. 👍
Update (2c8bbd9)
Documentation and changelog formatting fixes only:
- Added notes in
visualization.rstclarifying thattiled_cam_target_prim_pathandtiled_cam_prim_pathdefaults may need adjustment per environment - Reformatted all changelog fragments to proper RST structure with category headers (
Changed,Fixed,Added)
CI Status: pre-commit ✅ and changelog fragments ✅ are now passing — original Finding 5 resolved.
No functional code changes. No new issues. 👍
Update (8b40ba0)
Import-hygiene cleanup only: moved eager submodule imports (mdp, ui in envs; patterns in ray_caster) into the lazy .pyi stubs. No functional changes. No visualizer impact. No new issues. 👍
Update (800d24e)
Newton contact visualization feature and CI link-checker fixes:
- Contact rendering pipeline:
NewtonVisualizer.step()now logs native Newton contact buffers viaNewtonManager.get_contacts(), with a PhysX fallback that renders Isaac LabContactSensordata as synthesized arrows. Good progressive-enhancement design. - Scene data hook:
SceneDataProvider.get_contact_sensors()added with lazyBaseContactSensorimport — clean and avoids hard dependency. NewtonManager.get_contacts()— minimal public accessor, follows existing API style.- Env-filtering:
_filter_visible_env_tensorcorrectly applies_resolved_visible_env_idsto sensor tensors, consistent with the existing visible-worlds system. - Tests: Two new tests cover native path (monkeypatched
get_contactsreturns object) and fallback path (verifyingshow_contactstoggle and arrow geometry). - Docs: New "Newton Contact Visualization" section in
visualization.rstclearly documents the backend difference. - CI: Added link exclusions for unreachable/flaky URLs in
check-links.yml.
No new issues. Implementation is well-structured with proper defensive error handling throughout. 👍
Update (7895ff9)
Final cleanup and polish:
- Docs: Removed a duplicate paragraph in
record_video.rstabout multi-visualizer recording, and reformatted line wrapping invisualization.rstfor readability (no content change). - Code cleanup: Removed the
NEWTON_HUD_IMPORT_LOG_WARNINGconstant and_log_newton_hud_dependency_issue()helper fromnewton_visualizer.py— this HUD dependency check has been simplified/deferred. Tests updated accordingly (assert_no_newton_hud_dependency_warning→assert_no_newton_imgui_bundle_warning). - Exports: Cleaned up
newton/__init__.pylazy-export stubs to remove the deleted constant.
Purely cosmetic/refactoring changes, no functional impact. PR looks ready to merge. 👍
Update (7abba9e)
CI fixes:
- Updated
typing_extensionslicense identifier toPSF-2.0in.github/workflows/license-exceptions.json - Removed erroneous
Tiled Renderingtoctree entry fromdocs/index.rst(was causing Sphinx warnings)
No functional code changes. No new issues. 👍
Update (e9f31a4)
Documentation formatting and Newton physics tuning:
- Docs: RST syntax fixes — replaced
:py:keyword:with inline literal, changed:ref:contributingto `:doc:`contributingfor correct cross-referencing, and reformatted a long bullet paragraph invisualization.rst. - Physics config: Spot flat-env Newton config tuned — increased
njmax(45→130) andnconmax(30→40), addedNewtonCollisionPipelineCfg(max_triangle_pairs=2_500_000),NewtonShapeCfg(margin=0.01), anduse_mujoco_contacts=False. Reasonable parameter tuning for more complex contact scenarios.
No functional visualizer changes. No new issues. 👍
Update (686ebc6)
Release prep and housekeeping push — version bumps, changelog consolidation, and a handful of focused fixes:
Visualizer fix (regression #5262):
VisualizationMarkers.visualize()now tracks_has_visualizedso the first call with marker count == prototype count correctly defaults to prototype index 0 instead of skipping index assignment. New test covers the regression case.
PhysX scene-data backend simplification:
- Removed the articulation-transform special-casing from
PhysxManager— the rigid body view now includes articulation links directly. Wildcard pattern generation is smarter: exact paths are used when a body name collides with a non-rigid prim, preventing PhysX from resolving wildcards incorrectly. - Removed dead
SceneDataBackend.set_interactive_scene()hook and its call insimulation_context.py.
Newton ls_parallel deprecation:
MJWarpSolverCfg.ls_parallelnow emitsDeprecationWarningand is force-set toFalse. Manager ignores it via theignoredset. All task/test configs cleaned of the field. Docs updated to remove parallel line search references.
Other:
- Pinned
ovphysx==0.4.13in pyproject + CI to avoid breaking API changes from newer releases. - Removed obsolete
scripts/tools/wrap_warp_to_torch.pymigration helper. - CI
license-checkmigrated touvfor faster installs. - Benchmark script wraps first step in
torch.inference_mode(). - Docs:
isaaclab_docs.pyresolves branch fromsmv_current_versionfor versioned docs, camera demo subtitles fixed, toctree entry corrected. - Version bumps: isaaclab 6.3.0, isaaclab_newton 0.15.0, isaaclab_rl 0.6.0, isaaclab_tasks 2.0.2, isaaclab_contrib 0.4.2, isaaclab_experimental 0.1.1.
No new issues. Clean release cut. 👍
Update (79c98a8)
PR feedback iteration — final polish addressing reviewer comments:
- Docs: Fixed RST table formatting in
visualization.rst(camera-mode column width), corrected:py:keyword:→ literal markup,:ref:→:doc:cross-references inmigration.rst. - Visualizer cfg docs: Expanded existing-camera-mode table entry to clarify that
tiled_cam_prim_pathrequires environments with Isaac LabCamerasensors registered inscene.sensors(e.g.Isaac-Cartpole-Cameravs plainIsaac-Cartpole). - Code: No functional changes beyond what was reviewed in 686ebc6.
No new issues. PR is clean. 👍
Update (b470716)
Minor housekeeping:
license-exceptions.json:typing_extensionslicense string updated fromPSF-2.0toPython Software Foundation License(full name form for compliance tooling).simulation_context.py: Docstring clarification onregister_interactive_scene— "scene-data providers can expose scene-owned sensors" (more accurate description of the data flow direction).
No functional changes. No new issues. 👍
Update (243f43a)
Cosmetic cleanup only:
simulation_context.py: Removed hyphen in docstring ("scene-data" → "scene data")test_visualization_markers.py: Removed inline comment referencing regression case #5262 (the test itself remains)pyproject.toml: Added explanatory comments for thepyarrow==22.0.0pin ("Match rerun-sdk's supported Arrow stack and avoid resolver drift")
No functional changes. No new issues. 👍
Update (35e470b)
Livestream safety guards:
app_launcher.py: NewValueErrorraised whenisaacsim.exp.full.streaming.kitexperience is used with livestreaming enabled (modes 1/2) — prevents a known hang/PhysX tensor invalidation issue.sim_launcher.py: Added_get_livestream_mode()(CLI-over-env precedence) and_ensure_livestream_kit_visualizer()— auto-injects the Kit visualizer when livestreaming is active, or raises if--viz nonewas explicitly set. Integrated intolaunch_simulation().- Tests: 3 new tests in
test_kwarg_launch.py(experience rejection absolute/relative + pass-through when disabled) and a newtest_sim_launcher.pywith 5 tests covering kit-injection, append, rejection, env-var, and no-op cases.
Clean implementation, good test coverage, follows existing CLI-over-env patterns. No new issues. 👍
|
|
||
| # 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 |
| "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
matthewtrepte
force-pushed
the
mtrepte/debug
branch
from
686ebc6 to
79c98a8
Compare
matthewtrepte
force-pushed
the
mtrepte/debug
branch
from
35e470b to
243f43a
Compare
2 participants
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