Improve Partial Visuallization#5347
Conversation
Signed-off-by: matthewtrepte <mtrepte@nvidia.com>
Signed-off-by: matthewtrepte <mtrepte@nvidia.com>
matthewtrepte
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai,
kellyguo11 and
ossamaAhmed
as code owners
github-actions
Bot
added
documentation
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
![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
Summary
This PR refactors the partial visualization feature, renaming --visualizer_max_worlds to --max_visible_envs, adding visible_env_indices and randomly_sample_visible_envs to VisualizerCfg, and aligning Newton visualizers to use set_visible_worlds() API instead of building filtered models. The implementation is fundamentally sound but has a few issues that need addressing.
Architecture Impact
The change removes the filtered Newton model building path from PhysxSceneDataProvider. Downstream code now relies exclusively on the prebuilt Newton artifact from scene setup + set_visible_worlds() on the viewer side. The provider's update() and get_newton_state() methods no longer take env_ids, which is correct given that filtering moved to the viewer layer. Cross-module impact is well-handled — the tests and all three visualizer backends (Newton, Rerun, Viser) plus Kit are updated consistently.
Implementation Verdict
Minor fixes needed — The approach is correct and clean. The issues below are minor but should be addressed before merge.
Test Coverage
✅ New tests added: test_newton_adapter.py covers resolve_visible_env_indices and apply_viewer_visible_worlds well.
✅ Existing tests updated: Tests were updated to reflect the new API signatures (update() without env_ids, get_newton_state() without args).
randomly_sample_visible_envs=True with max_visible_envs set) in _compute_visualized_env_ids should have a deterministic test. The current test (test_compute_visualized_env_ids_random_cap_only_sorted_once) only verifies sorting and uniqueness, but not determinism or the distribution. Consider adding a seed mechanism for reproducibility in tests, or at minimum verify it doesn't crash with edge cases like num_envs=0.
CI Status
Branch Status
❌ This branch has merge conflicts with develop. Rebase needed before merge. I can help — just reply and I'll push an update.
Findings
🔵 Improvement: source/isaaclab/isaaclab/visualizers/visualizer_cfg.py:58-60 — Docstring typo and clarity
The docstring for randomly_sample_visible_envs has a typo ("Note" should start sentence properly) and the interaction between fields could be clearer.
🔵 Improvement: source/isaaclab/isaaclab/visualizers/base_visualizer.py:158-169 — Random sampling without seed is non-deterministic across runs
The random sampling path uses random.sample() without a seed, which means different runs will show different env subsets. This may be intentional for variety, but makes debugging harder. Consider accepting an optional seed in VisualizerCfg for reproducible partial visualization during development/testing.
🔵 Improvement: docs/source/features/visualization.rst:166 — Trailing whitespace
Minor: there's trailing whitespace at the end of line 166 ("does not improve performance much. "). Pre-commit may catch this.
🔵 Improvement: source/isaaclab_visualizers/isaaclab_visualizers/newton_adapter.py:1-63 — Module docstring could mention Newton PR #2267
The module-level docstring could reference the Newton PR that introduced set_visible_worlds() for future maintainers who need to trace the dependency.
| * Note ``visible_env_indices`` overrides this field. | ||
| """ | ||
|
|
||
| def get_visualizer_type(self) -> str | None: |
There was a problem hiding this comment.
The docstring has a minor issue: "Note" should be properly capitalized/formatted, and it would help to clarify that when visible_env_indices is set, randomly_sample_visible_envs is ignored entirely (not just "overrides").
| def get_visualizer_type(self) -> str | None: | |
| randomly_sample_visible_envs: bool = True | |
| """When ``max_visible_envs`` is set and ``visible_env_indices`` is None, randomly sample which envs to show. | |
| When ``visible_env_indices`` is explicitly provided, this field is ignored. | |
| """ |
| # k == 0: sample(range(n), 0) is []; contiguous resolver used the same convention. | ||
| return sorted(random.sample(range(num_envs), k)) | ||
| return None | ||
|
|
There was a problem hiding this comment.
Using random.sample() without a seed means different visualization subsets across runs. This is probably fine for production, but consider adding an optional visible_env_seed: int | None = None to VisualizerCfg for reproducible debugging sessions. When set, call random.seed(visible_env_seed) before sampling (and restore state after).
If intentionally non-deterministic, a brief comment explaining this choice would help future maintainers.
| """Shared helpers for viewer env selection (Newton viewers and Kit partial USD visibility).""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
Consider adding a brief note in the module docstring referencing the Newton PR that introduced set_visible_worlds() (newton#2267 or similar). This helps future maintainers trace the API dependency when debugging viewer visibility issues.
|
can you resolve the conflicts so that we can run CI? |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot (Re-review)
Summary
This PR refactors partial visualization from per-visualizer max_worlds to a unified max_visible_envs + visible_env_indices + randomly_sample_visible_envs API in VisualizerCfg. The implementation is fundamentally correct — Newton-based visualizers now use set_visible_worlds() instead of building filtered models, and the scene data providers return full state while letting the viewer handle visibility. The recent commits fix lint issues and update docs.
Architecture Impact
The refactoring removes the get_newton_model_for_env_ids and get_newton_state(env_ids) patterns from scene data providers. This is a clean simplification — filtering now happens at the viewer layer via set_visible_worlds() API from Newton. Cross-module changes are consistent: all three Newton-based visualizers (Newton, Rerun, Viser) plus KitVisualizer are updated to use the new newton_adapter.py helpers.
Implementation Verdict
Ship it — The issues from the previous review were minor documentation/style suggestions. The core implementation is solid, well-tested, and the approach is correct. The remaining items below are optional polish.
Test Coverage
✅ Excellent coverage: New test_newton_adapter.py covers resolve_visible_env_indices and apply_viewer_visible_worlds with edge cases (truncation, cap-only, explicit lists, zero envs).
✅ Existing tests updated correctly to reflect the new API signatures (update() without env_ids, get_newton_state() without args).
✅ Integration tests updated with randomly_sample_visible_envs=False for deterministic behavior.
CI Status
Installation Tests: Docker buildkit resolver error (open /var/lib/docker/buildkit/executor/resolv.conf.tmp: no such file or directory)Build Latest Docs: Gymnasium circular import during autodoc (pre-existing issue in CI environment)- Other failed tests have the same infrastructure root cause (waiting on base docker image)
The pre-commit and isaaclab_visualizers tests pass ✅.
Findings
The previous review's suggestions remain applicable as optional polish — they're not blocking:
🔵 Optional: source/isaaclab/isaaclab/visualizers/visualizer_cfg.py:62 — Minor docstring formatting
The docstring * Note ``visible_env_indices`` overrides this field. has extra whitespace before the field name. Minor polish.
🔵 Optional: base_visualizer.py:168 — Non-deterministic random sampling
random.sample() is used without a seed, so different runs show different env subsets. This is likely intentional for variety, but a visible_env_seed: int | None = None option in VisualizerCfg could help debugging. Not blocking — mentioning for consideration in a future PR.
Previous Review Status
✅ Previous issues were minor polish suggestions that don't block merge. The core implementation was and remains correct.
kellyguo11
left a comment
kellyguo11
left a comment
There was a problem hiding this comment.
@ncournia-nv heads up some incoming changes in scene data provider
|
looks like there are some visualizer related test failures. one example: |
# Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html 💡 Please try to keep PRs small and focused. Large PRs are harder to review and merge. --> Improve Partial Visualization feature where a subset of envs are shown in the launched visualizers to speed up performance, making use of new updates to Newton. Newton visualizers align on using the set_visible_worlds API to select envs to show. Kit visualizer just sets non selected envs invisible. Updated docs and added tests. <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - New feature (non-breaking change which adds functionality) - Documentation update ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [ ] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Signed-off-by: matthewtrepte <mtrepte@nvidia.com>
2 participants
Description
Improve Partial Visualization feature where a subset of envs are shown in the launched visualizers to speed up performance, making use of new updates to Newton.
Newton visualizers align on using the set_visible_worlds API to select envs to show.
Kit visualizer just sets non selected envs invisible.
Updated docs and added tests.
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