Skip to content

Viewer visibility improvements#801

Merged
eric-heiden merged 8 commits into
newton-physics:mainfrom
mmacklin:viewer-improvements
Sep 23, 2025
Merged

Viewer visibility improvements#801
eric-heiden merged 8 commits into
newton-physics:mainfrom
mmacklin:viewer-improvements

Conversation

@mmacklin

@mmacklin mmacklin commented Sep 22, 2025

Copy link
Copy Markdown
Member

Description

  • Avoids repopulating the viewer instances when toggling visibility
  • Allows showing both collision and visual together to see correspondence
  • Use a per-shape type color instead of per-instance for collision geometries
  • Update the allegro example to show the palm

Newton Migration Guide

Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.

  • The migration guide in docs/migration.rst is up-to date

Before your PR is "Ready for review"

  • Necessary tests have been added and new examples are tested (see newton/tests/test_examples.py)
  • Documentation is up-to-date
  • Code passes formatting and linting checks with pre-commit run -a

Summary by CodeRabbit

  • New Features

    • Added display toggles: Show Collision, Show Visual, and Show Static.
    • Added an optional hidden parameter to instance/logging APIs to control per-batch visibility.
  • Refactor

    • Centralized shape visibility and hashing for consistent batching and rendering.
  • Style

    • Darkened the viewer sky gradient and refined color mapping for shapes.
  • Chores

    • Simplified collision visibility handling in the Allegro Hand example.

…s in the viewer

Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
… make allegro example show the palm

Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
@coderabbitai

coderabbitai Bot commented Sep 22, 2025

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Centralizes shape visibility and batching in the viewer: adds show_collision/show_visual/show_static toggles, a shape-hash batching model (ShapeInstances), and threads a new hidden flag through logging APIs; syncs backend signatures; updates GL UI and example usage; tweaks OpenGL sky_lower color constant.

Changes

Cohort / File(s) Summary of changes
Viewer core: visibility, hashing, batching
newton/_src/viewer/viewer.py
Adds show_collision, show_visual, show_static; introduces ShapeInstances (replacing Instances), _hash_shape, _should_show_shape; switches batching to shape-hash keyed _shape_instances; threads hidden through log_shapes/log_instances/log_geo/log_mesh; updates _populate_geometry, caching, and color selection.
OpenGL backend & UI
newton/_src/viewer/viewer_gl.py
log_instances(..., hidden=False) added; computes needs_update from hidden, updates instancer transforms only when needed, sets object.hidden; replaces single “Show Collision Geometry” toggle with “Show Collision” and “Show Visual”; removes show_collision_geometry attr and _update_geometry_visibility.
Other backends (API signature sync)
newton/_src/viewer/viewer_file.py , newton/_src/viewer/viewer_null.py , newton/_src/viewer/viewer_rerun.py , newton/_src/viewer/viewer_usd.py
Each log_instances signature extended to accept hidden=False; implementations remain no-ops or unchanged aside from the new parameter.
OpenGL renderer constant
newton/_src/viewer/gl/opengl.py
Changed RendererGL.sky_lower color from (139/255,151/255,186/255) to (40/255,44/255,55/255).
Example adjustment
newton/examples/robot/example_robot_allegro_hand.py
Removed hide_collision_shapes=False argument from add_usd; reduced ignore_paths and simplified collision-hiding logic (removed palm_link exception).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Viewer as ViewerBase
  participant Cache as GeometryCache
  participant Batches as _shape_instances (ShapeInstances)
  participant Backend as Viewer Backend (GL/US D/File/Null)

  User->>Viewer: log_shapes(..., hidden)
  Viewer->>Viewer: _hash_shape(geo_hash, static, flags)
  Viewer->>Cache: _populate_geometry(geo_type, ...)
  Cache-->>Viewer: mesh_ref
  Viewer->>Viewer: _should_show_shape(flags, is_static)
  alt batch exists
    Viewer->>Batches: reuse/update ShapeInstances
  else create batch
    Viewer->>Batches: create ShapeInstances(static, flags, mesh)
  end
  Viewer->>Backend: log_instances(name, mesh, xforms, scales, colors, materials, hidden)
  Backend-->>Viewer: ack
Loading
sequenceDiagram
  autonumber
  actor User
  participant UI as ViewerGL UI
  participant State as ViewerGL State (show_collision/show_visual/show_static)
  participant Instanced as Instanced Objects

  User->>UI: Toggle Show Collision / Show Visual
  UI->>State: update boolean
  UI->>Instanced: set object.hidden per batch
  note over Instanced: hidden=true -> skip transform pushes / mark invisible
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • preist-nvidia
  • eric-heiden

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Viewer visibility improvements" is concise and directly reflects the primary intent of the changes in the PR (visibility controls like show_collision/show_visual/show_static and improved visibility toggling behavior); it is clear and relevant for a teammate scanning history. It avoids noisy details and does not mislead about the contents. Implementation specifics (API signature changes, hashing, caching) are appropriately left to the PR description.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Miles Macklin <mmacklin@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
newton/_src/viewer/viewer.py (1)

718-727: Dynamic color generation improves visual distinction.

The switch from a fixed palette to golden-ratio-based hue generation provides better visual separation between shapes. The pastel HLS values (L=0.6, S=0.65) work well on dark backgrounds.

The variable name l on line 725 could be renamed to lightness for better readability:

-        l, s = 0.6, 0.65
-        r, g, b = colorsys.hls_to_rgb(h, l, s)
+        lightness, saturation = 0.6, 0.65
+        r, g, b = colorsys.hls_to_rgb(h, lightness, saturation)
newton/examples/robot/example_robot_allegro_hand.py (1)

103-107: Tighten the collision regex; viewer logs hidden shapes so toggles will work if flags/overrides are changed

Verified: viewer.log_instances accepts a hidden flag and viewers use it (GL sets objects[name].hidden; USD sets instancer visibility). viewer computes visible from ShapeFlags.VISIBLE and calls log_instances(hidden=not visible), so clearing VISIBLE will log collisions as hidden but they can be re‑shown if a runtime toggle updates shape_flags or viewer overrides.

  • Replace the regex with a precompiled, anchored pattern (optional refactor — diff below).
  • Ensure any viewer collision toggle updates shape_flags or the viewer's visibility overrides at runtime (otherwise collisions will remain hidden).
-        for i, key in enumerate(allegro_hand.shape_key):
-            if re.match(".*Robot/.*?/collision", key):
-                allegro_hand.shape_flags[i] &= ~newton.ShapeFlags.VISIBLE
+        pattern = re.compile(r'/Robot/[^/]+/collision(?:$|/)')
+        for i, key in enumerate(allegro_hand.shape_key):
+            if pattern.search(key):
+                allegro_hand.shape_flags[i] &= ~newton.ShapeFlags.VISIBLE
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c054658 and a569125.

📒 Files selected for processing (8)
  • newton/_src/viewer/gl/opengl.py (1 hunks)
  • newton/_src/viewer/viewer.py (7 hunks)
  • newton/_src/viewer/viewer_file.py (1 hunks)
  • newton/_src/viewer/viewer_gl.py (3 hunks)
  • newton/_src/viewer/viewer_null.py (1 hunks)
  • newton/_src/viewer/viewer_rerun.py (1 hunks)
  • newton/_src/viewer/viewer_usd.py (1 hunks)
  • newton/examples/robot/example_robot_allegro_hand.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
newton/_src/viewer/viewer_file.py (5)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer.py (1)
  • log_instances (385-386)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_null.py (5)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer.py (1)
  • log_instances (385-386)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_usd.py (5)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer.py (1)
  • log_instances (385-386)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_rerun.py (5)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer.py (1)
  • log_instances (385-386)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer.py (8)
newton/_src/sim/builder.py (3)
  • flags (176-182)
  • flags (185-190)
  • color (4015-4056)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/geometry/flags.py (1)
  • ShapeFlags (30-42)
newton/_src/geometry/types.py (1)
  • GeoType (25-64)
newton/_src/viewer/viewer_gl.py (6)
newton/_src/viewer/viewer.py (1)
  • log_instances (385-386)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/gl/opengl.py (2)
  • MeshInstancerGL (528-755)
  • update_from_transforms (673-696)
🪛 Ruff (0.13.1)
newton/_src/viewer/viewer_usd.py

187-187: Unused method argument: materials

(ARG002)


187-187: Unused method argument: hidden

(ARG002)

newton/_src/viewer/viewer_rerun.py

133-133: Unused method argument: materials

(ARG002)


133-133: Unused method argument: hidden

(ARG002)

newton/_src/viewer/viewer.py

718-718: Unused noqa directive (non-enabled: PLC0415)

Remove unused noqa directive

(RUF100)


725-725: Ambiguous variable name: l

(E741)

⏰ 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). (4)
  • GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
  • GitHub Check: Run GPU Benchmarks (Pull Request)
  • GitHub Check: run-newton-tests / newton-unittests (windows-latest)
  • GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (13)
newton/_src/viewer/gl/opengl.py (1)

777-777: Update the sky_lower color change looks good.

The darker tone (40, 44, 55) provides better contrast for visualization, which aligns well with the broader viewer improvements in the PR.

newton/_src/viewer/viewer_file.py (1)

106-106: LGTM!

The signature update to include the hidden parameter aligns perfectly with the API changes across all viewer implementations.

newton/_src/viewer/viewer_rerun.py (1)

133-133: LGTM!

The signature update correctly extends the API to match other viewers, maintaining consistency across the codebase.

newton/_src/viewer/viewer_usd.py (1)

187-187: LGTM!

The signature update properly extends the public API to include the hidden parameter, consistent with the other viewer implementations.

newton/_src/viewer/viewer_null.py (1)

68-68: LGTM!

The signature update to include the hidden parameter maintains API consistency across viewer implementations.

newton/_src/viewer/viewer_gl.py (2)

230-257: Good implementation of the hidden parameter logic.

The implementation correctly avoids updating transforms when hidden=True, which prevents unnecessary GPU work for non-visible instances. The logic also ensures that newly created instancers are always updated initially.


891-897: UI toggles are properly separated.

The replacement of the single "Show Collision Geometry" toggle with separate "Show Collision" and "Show Visual" toggles provides better control over visibility, aligning with the PR objectives.

newton/_src/viewer/viewer.py (5)

63-65: Well-structured visibility control attributes.

The addition of show_collision, show_visual, and show_static provides clear and intuitive control over shape visibility. The defaults (visual on, collision/static off) are sensible for typical use cases.


111-124: Efficient visibility handling in log_state.

The implementation correctly applies visibility checks before updating transforms, avoiding unnecessary GPU work for hidden shapes. The hidden parameter is properly passed to log_instances.


416-421: Good refactor of the ShapeInstances class.

The addition of static and flags attributes to ShapeInstances enables proper visibility determination and shape batching. This design cleanly separates shape properties from rendering logic.


465-466: Proper shape hashing for batching.

The _hash_shape method correctly includes static and flags in the hash, ensuring that shapes with different visibility properties are batched separately.


468-489: Well-designed visibility logic.

The _should_show_shape method provides clear precedence rules: static override → collision toggle → visual toggle → shape flags. This gives users intuitive control over visibility.

newton/examples/robot/example_robot_allegro_hand.py (1)

99-101: Palm visuals now included — filter update LGTM.

Dropping ".*palm_link/visuals" aligns with the PR goal to show the palm. Expect a small increase in draw calls/GPU memory.

Please sanity‑check that the palm visuals appear as intended with the default viewer toggles (both visual+collision), and that no unintended prims slipped through the filters.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
newton/_src/viewer/viewer.py (2)

720-720: Remove unnecessary noqa directive.

The # noqa: PLC0415 comment is no longer needed since the import is now at the function level and doesn't trigger the linting rule.

Apply this fix:

-        import colorsys  # noqa: PLC0415
+        import colorsys

727-727: Use more descriptive variable name.

The variable name l (lowercase L) is ambiguous and could be confused with 1 (one) or I (capital i). Consider using lightness for clarity.

Apply this fix:

-        h = (i * _PHI) % 1.0
-        l, s = 0.6, 0.65
-        r, g, b = colorsys.hls_to_rgb(h, l, s)
+        h = (i * _PHI) % 1.0
+        lightness, s = 0.6, 0.65
+        r, g, b = colorsys.hls_to_rgb(h, lightness, s)
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c054658 and 9fdf6cb.

📒 Files selected for processing (8)
  • newton/_src/viewer/gl/opengl.py (1 hunks)
  • newton/_src/viewer/viewer.py (10 hunks)
  • newton/_src/viewer/viewer_file.py (1 hunks)
  • newton/_src/viewer/viewer_gl.py (3 hunks)
  • newton/_src/viewer/viewer_null.py (1 hunks)
  • newton/_src/viewer/viewer_rerun.py (1 hunks)
  • newton/_src/viewer/viewer_usd.py (1 hunks)
  • newton/examples/robot/example_robot_allegro_hand.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
newton/_src/viewer/viewer_null.py (5)
newton/_src/viewer/viewer.py (1)
  • log_instances (387-388)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_usd.py (5)
newton/_src/viewer/viewer.py (1)
  • log_instances (387-388)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_rerun.py (5)
newton/_src/viewer/viewer.py (1)
  • log_instances (387-388)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_file.py (5)
newton/_src/viewer/viewer.py (1)
  • log_instances (387-388)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_gl.py (6)
newton/_src/viewer/viewer.py (1)
  • log_instances (387-388)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/gl/opengl.py (2)
  • MeshInstancerGL (528-755)
  • update_from_transforms (673-696)
newton/_src/viewer/viewer.py (8)
newton/_src/sim/builder.py (3)
  • flags (176-182)
  • flags (185-190)
  • color (4015-4056)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/geometry/flags.py (1)
  • ShapeFlags (30-42)
newton/_src/geometry/types.py (1)
  • GeoType (25-64)
🪛 Ruff (0.13.1)
newton/_src/viewer/viewer_usd.py

187-187: Unused method argument: materials

(ARG002)


187-187: Unused method argument: hidden

(ARG002)

newton/_src/viewer/viewer_rerun.py

133-133: Unused method argument: materials

(ARG002)


133-133: Unused method argument: hidden

(ARG002)

newton/_src/viewer/viewer.py

720-720: Unused noqa directive (non-enabled: PLC0415)

Remove unused noqa directive

(RUF100)


727-727: Ambiguous variable name: l

(E741)

🔇 Additional comments (18)
newton/_src/viewer/gl/opengl.py (1)

777-777: Minor visual refinement.

The sky lower color has been changed from a lighter blue-gray to a darker, more muted color. This aligns with the broader viewer improvements mentioned in the PR summary and creates a more subdued background that likely enhances visibility of collision and visual geometries.

newton/examples/robot/example_robot_allegro_hand.py (2)

99-99: Simplified ignore paths for better palm visibility.

The removal of ".*palm_link/visuals" from the ignore paths enables showing the palm, which aligns with the PR's goal of improving viewer visibility and allowing users to observe correspondence between collision and visual geometries.


105-106: Simplified collision visibility logic.

The updated condition removes the palm_link exclusion, applying visibility masking consistently to all collision shapes. This change aligns with the centralized visibility control introduced in the viewer modules.

newton/_src/viewer/viewer_null.py (1)

68-68: API consistency maintained.

The signature extension with hidden=False maintains consistency across all viewer backends while preserving the no-op behavior appropriate for the null viewer.

newton/_src/viewer/viewer_usd.py (1)

187-187: API signature extended for consistency.

The hidden parameter has been added to maintain API consistency across all viewer backends. The parameter is currently unused in the implementation but provides a foundation for future visibility control in USD output.

newton/_src/viewer/viewer_rerun.py (1)

133-133: API signature standardized.

The hidden parameter addition ensures consistent API surface across all viewer backends. While unused in the current implementation, this prepares the ReRun backend for unified visibility control.

newton/_src/viewer/viewer_file.py (1)

106-106: Consistent API extension.

The hidden parameter addition maintains API compatibility across all viewer backends. The no-op implementation is appropriate for the file viewer which focuses on data recording rather than rendering.

newton/_src/viewer/viewer_gl.py (2)

230-257: Enhanced visibility control with smart update logic.

The new hidden parameter enables fine-grained visibility control while optimizing performance by skipping unnecessary updates for hidden instances. The logic correctly forces updates when creating new instancers and respects the hidden state for existing ones.


892-897: Improved geometry visibility toggles.

The replacement of a single "Show Collision Geometry" toggle with separate "Show Collision" and "Show Visual" toggles provides better granular control. This allows users to toggle collision and visual shapes independently, addressing the PR objective of allowing both to be shown together for correspondence observation.

newton/_src/viewer/viewer.py (9)

63-65: Enhanced visibility control attributes.

The new toggles (show_collision, show_visual, show_static) provide granular control over shape visibility, replacing the previous per-shape inference with centralized policy. This supports the PR's goal of allowing collision and visual geometries to be shown together.


111-124: Centralized visibility decision with performance optimization.

The new _should_show_shape method centralizes visibility logic, and the conditional update prevents unnecessary computation for hidden shapes. The hidden parameter threading through log_instances enables backends to optimize rendering accordingly.


418-423: Enhanced shape instance data structure.

The renamed and expanded ShapeInstances class now includes static and flags attributes, enabling the new visibility policy. This supports per-batch visibility decisions based on shape properties rather than individual shape flags.


467-468: Shape batching with visibility-aware hashing.

The new _hash_shape method enables batching shapes by geometry, static state, and flags. This supports the centralized visibility policy by grouping shapes with similar rendering characteristics.


470-491: Comprehensive visibility policy implementation.

The _should_show_shape method implements the new visibility logic with clear precedence: static override → collision override → visual override → fallback to shape visibility flag. This provides users with intuitive control over what gets rendered.


584-610: Optimized shape population with improved batching.

The updated shape population logic uses the geometry cache effectively and creates shape batches based on the new hashing strategy. This avoids repopulating viewer instances when toggling visibility, addressing a key PR objective.


617-617: Per-shape-type color scheme for collision geometries.

The new _shape_color_map function generates colors based on geometry type rather than instance, implementing the PR's objective of using a per-shape-type color scheme for better visual distinction.


719-729: Improved color generation with mathematical distribution.

The new color mapping uses the golden ratio for hue distribution, creating visually distinct pastel colors that work well on dark backgrounds. This replaces hard-coded colors with a mathematically sound approach for better visual differentiation.


387-387: API signature updated consistently.

The hidden parameter addition maintains consistency across all viewer backend signatures and enables the new visibility control features.

Comment thread newton/_src/viewer/viewer.py Outdated
…etry

Signed-off-by: Miles Macklin <mmacklin@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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)

228-233: Fix isinstance union usage (will raise TypeError at runtime).

isinstance(..., tuple | list | np.ndarray) is invalid at runtime. Use a tuple of types.

Apply this diff:

-        def _as_float_list(value):
-            if isinstance(value, tuple | list | np.ndarray):
+        def _as_float_list(value):
+            if isinstance(value, (tuple, list, np.ndarray)):
                 return [float(v) for v in value]
             else:
                 return [float(value)]
-        # normalize
-        if isinstance(geo_scale, list | tuple | np.ndarray):
+        # normalize
+        if isinstance(geo_scale, (list, tuple, np.ndarray)):
             scale_list = [float(v) for v in geo_scale]
         else:
             scale_list = [float(geo_scale)]

Also applies to: 507-511

🧹 Nitpick comments (3)
newton/_src/viewer/viewer.py (3)

100-102: Remove duplicate is_paused definition.

There are two identical is_paused() methods; keep one.

Apply this diff to remove the later duplicate:

-    def is_paused(self):
-        return False

190-194: Contact color comment mismatch.

Comment says orange-red but code uses green.

Pick one. Example to match the comment:

-        # Use orange-red color for contact normals
-        colors = (0.0, 1.0, 0.0)
+        # Use orange-red color for contact normals
+        colors = (1.0, 0.35, 0.0)

444-451: Parent index dtype: consider explicit 32-bit.

Kernel/index buffers are typically 32-bit. Using bare int may vary by platform.

If kernels expect 32-bit, apply:

-            self.parents = wp.array(self.parents, dtype=int, device=self.device)
+            self.parents = wp.array(self.parents, dtype=wp.int32, device=self.device)
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9fdf6cb and a6d8b39.

📒 Files selected for processing (1)
  • newton/_src/viewer/viewer.py (9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/viewer/viewer.py (8)
newton/_src/sim/builder.py (3)
  • flags (176-182)
  • flags (185-190)
  • color (4015-4056)
newton/_src/viewer/viewer_file.py (1)
  • log_instances (106-108)
newton/_src/viewer/viewer_null.py (1)
  • log_instances (68-80)
newton/_src/viewer/viewer_rerun.py (1)
  • log_instances (133-203)
newton/_src/viewer/viewer_usd.py (1)
  • log_instances (187-271)
newton/_src/viewer/viewer_gl.py (1)
  • log_instances (230-257)
newton/_src/geometry/flags.py (1)
  • ShapeFlags (30-42)
newton/_src/geometry/types.py (1)
  • GeoType (25-64)
⏰ 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). (3)
  • GitHub Check: Run GPU Benchmarks (Pull Request)
  • GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
  • GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (5)
newton/_src/viewer/viewer.py (5)

524-536: Unsupported geo types: ELLIPSOID/HFIELD not mapped.

_populate_geometry() lacks entries for GeoType.ELLIPSOID and GeoType.HFIELD. If the model uses them, this will raise.

Please confirm these types cannot appear here. If they can, add mappings or guard earlier to skip with a clear message.


325-356: log_geo doesn’t handle ELLIPSOID/HFIELD.

log_geo() raises for these types.

Confirm they’re unreachable. If not, either implement or explicitly skip them in _populate_shapes() with a user-facing warning.


614-618: Per-hash collider coloring — confirm palette and intent.

Switch to Paul Tol palette and hash-based grouping addresses earlier “too few colors” concern. Looks good; just confirm this matches desired UX for Allegro.


111-124: Visibility-driven updates: behavior is consistent with “no repopulate”.

Updating transforms only when visible and passing hidden=not visible aligns with GL backend’s needs_update logic.

Toggle test recommended to verify no one-frame lag: hide → simulate → show; instances should snap to current pose on the reveal frame.


387-388: API change LGTM — viewers & caller updated.

Confirmed log_instances signatures and caller accept hidden=False in: newton/_src/viewer/viewer.py, viewer_gl.py, viewer_usd.py, viewer_rerun.py, viewer_file.py, viewer_null.py.

Comment thread newton/_src/viewer/viewer.py
@eric-heiden eric-heiden enabled auto-merge (squash) September 23, 2025 05:04
@eric-heiden eric-heiden merged commit 1c8a2e4 into newton-physics:main Sep 23, 2025
12 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Sep 25, 2025
4 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Nov 21, 2025
3 tasks
eric-heiden pushed a commit to eric-heiden/newton that referenced this pull request Jan 28, 2026
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
mmacklin added a commit to mmacklin/newton that referenced this pull request Apr 7, 2026
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants