Limit number of worlds shown in Newton visualizers #1315
Conversation
📝 WalkthroughWalkthroughIntroduces a per-world rendering cap via an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-08-25T21:41:45.795ZApplied to files:
📚 Learning: 2025-12-06T19:10:24.360ZApplied to files:
📚 Learning: 2025-12-12T08:45:43.428ZApplied to files:
🧬 Code graph analysis (1)newton/examples/selection/example_selection_cartpole.py (3)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/viewer/viewer.py (1)
1162-1193: Critical: Fix incorrect arguments in inertia box batch.add() call.Line 1192 passes only 6 arguments to
batch.add(), but the method signature (line 757) expects 7. This causesbody_world[body]to be used asshape_indexinstead ofworld, leavingworldto default to -1.Comparing with correct usage:
- Line 990 (shapes):
batch.add(parent, xform, scale, color, material, s, shape_world[s])- Line 1110 (SDF isomesh):
batch.add(parent, xform, scale, color, material, s, shape_world[s])- Line 1192 (inertia boxes):
batch.add(parent, xform, scale, color, material, body_world[body])❌🐛 Proposed fix
# add render instance - batch.add(parent, xform, scale, color, material, body_world[body]) + batch.add(parent, xform, scale, color, material, body, body_world[body])This ensures the body index is passed as
shape_indexand the world index asworld, matching the pattern used for shapes and SDF isomeshes.
🧹 Nitpick comments (1)
newton/_src/viewer/viewer.py (1)
121-134: Add input validation for max_worlds parameter.The parameter lacks validation for invalid values. Consider adding a check to ensure max_worlds is positive when provided:
♻️ Proposed validation
def set_model(self, model, max_worlds: int | None = None): """ Set the model to be visualized. Args: model: The Newton model to visualize. max_worlds: Maximum number of worlds to render (None = all). Useful for performance when training with many environments. """ if self.model is not None: raise RuntimeError("Viewer set_model() can be called only once.") + + if max_worlds is not None and max_worlds <= 0: + raise ValueError(f"max_worlds must be positive, got {max_worlds}") self.model = model self.max_worlds = max_worlds
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/viewer/viewer.pynewton/_src/viewer/viewer_file.pynewton/_src/viewer/viewer_gl.pynewton/examples/__init__.pynewton/examples/selection/example_selection_cartpole.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/examples/__init__.pynewton/examples/selection/example_selection_cartpole.py
📚 Learning: 2025-12-06T19:10:24.360Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_box_on_plane.py:322-371
Timestamp: 2025-12-06T19:10:24.360Z
Learning: In Newton Kamino examples under newton/_src/solvers/kamino/examples/sim, CLI arguments parsed via argparse (including flags like --test, --device, --headless, etc.) are passed as a complete args object to newton.examples.run(example, args). The run function internally consumes these flags to control execution behavior, so flags may appear "unused" in the example script itself even though they are actively used by the framework.
Applied to files:
newton/examples/__init__.pynewton/examples/selection/example_selection_cartpole.py
📚 Learning: 2025-12-12T08:45:43.428Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:43.428Z
Learning: In Newtown (Newton) example code, specifically files under newton/examples, computing contacts once per frame and reusing them across all substeps is an intentional design choice, not a bug. Reviewers should verify that contacts are computed before the substep loop and reused for every substep within the same frame. This pattern reduces redundant work and preserves frame-consistency; do not flag as a regression unless the behavior is changed for correctness or performance reasons.
Applied to files:
newton/examples/__init__.pynewton/examples/selection/example_selection_cartpole.py
📚 Learning: 2025-12-12T08:46:21.381Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:189-191
Timestamp: 2025-12-12T08:46:21.381Z
Learning: In Newton, SDF data structures are shared across replicated world instances (e.g., when using ModelBuilder.replicate()). Therefore, the memory cost of SDF generation scales with sdf_max_resolution but does not scale with the number of worlds. When reviewing or modifying newton/examples/example_sdf.py (and similar example scripts), verify that SDF memory usage is controlled by sdf_max_resolution and not by the count of replicated worlds, and document or justify any changes to sdf_max_resolution to manage memory and performance.
Applied to files:
newton/examples/__init__.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/viewer/viewer.py
🧬 Code graph analysis (3)
newton/_src/viewer/viewer_gl.py (2)
newton/_src/viewer/viewer.py (1)
set_model(121-142)newton/_src/viewer/viewer_file.py (1)
set_model(59-65)
newton/_src/viewer/viewer_file.py (2)
newton/_src/viewer/viewer.py (1)
set_model(121-142)newton/_src/viewer/viewer_gl.py (1)
set_model(188-202)
newton/_src/viewer/viewer.py (3)
newton/_src/viewer/viewer_file.py (1)
set_model(59-65)newton/_src/viewer/viewer_gl.py (1)
set_model(188-202)newton/_src/sim/builder.py (1)
body_count(976-980)
🪛 Ruff (0.14.10)
newton/_src/viewer/viewer.py
131-131: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (9)
newton/examples/selection/example_selection_cartpole.py (2)
58-58: LGTM - Clean parameter threading.The max_worlds parameter is correctly added to the Example initialization and properly forwarded to the viewer. The None default maintains backward compatibility.
Also applies to: 113-113
238-238: No action needed. Bothargs.num_worldsandargs.max_worldsare properly defined:--num-worldsis added by this example file (lines 242-247), and--max-worldsis provided by the centralized parser innewton/examples/__init__.py. The code correctly uses both arguments.Likely an incorrect or invalid review comment.
newton/examples/__init__.py (1)
340-345: LGTM - CLI argument properly defined.The
--max-worldsargument is correctly added to the centralized parser with appropriate type, default, and help text. This satisfies the dependency from example files.newton/_src/viewer/viewer_gl.py (1)
188-196: LGTM - Correct parameter propagation.The max_worlds parameter is properly added to the signature, documented, and forwarded to the base class. The implementation correctly maintains the override pattern.
newton/_src/viewer/viewer_file.py (1)
59-61: LGTM - Consistent with other viewer subclasses.The parameter forwarding matches the pattern used in ViewerGL. Implementation is correct.
newton/_src/viewer/viewer.py (4)
144-158: LGTM - Well-designed helper methods.The filtering logic correctly handles:
- Global entities (world_idx == -1) are always rendered
- Proper bounds checking with min() to avoid over-counting
- Clear separation of concerns between should-render check and count calculation
902-904: LGTM - Efficient filtering placement.The filtering is applied early in the loop before expensive geometry operations, which is the correct optimization.
1044-1046: LGTM - Consistent filtering across visualization types.The filtering is correctly applied to SDF isomesh instances, maintaining consistency with the regular shape filtering.
212-212: LGTM - Correct usage of helper method.The
_get_render_world_count()helper is appropriately used to ensure world offsets are only computed for worlds that will actually be rendered, maintaining consistency with the filtering logic.Also applies to: 280-280
|
@mzamoramora-nvidia can you please review? |
There was a problem hiding this comment.
@jvonmuralt Thanks for the contribution. It's nice that it works even for the usd viewer.
It would be good to add a note about this argument in the documentation (docs/guide/visualization.rst), so that people can know about this feature from the beginning.
And/or maybe update the execution command in examples_selection_cartpole with small explanation of the argument. Something like:
# To limit the number of worlds to render use the max-worlds argument.
# Command: python -m newton.examples selection_cartpole --num-worlds 16 --max-worlds 8
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.