Add Show Ground visualization toggle to the viewer#3626
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a per-layer ChangesGround visibility toggle
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ViewerGui
participant ViewerBase
participant ViewerGL
participant ViewerRTX
ViewerGui->>ViewerBase: update show_ground
ViewerBase->>ViewerGL: evaluate geo_type-aware visibility
ViewerBase->>ViewerRTX: expose updated toggle state
ViewerRTX->>ViewerRTX: queue plane-instance visibility updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/viewer/viewer_rtx.py`:
- Around line 1298-1300: Update the show_ground change handling around
_pending_instance_visibility to check whether shapes.geo_type is present before
converting it to int and comparing with newton.GeoType.PLANE. Preserve the
existing visibility update for plane shapes while safely ignoring batches
without a geometry type, consistent with the canonical visibility helper.
- Around line 1298-1300: Update the show_ground_changed handling for plane
shapes in the viewer visibility flow to avoid assigning show_ground directly to
_pending_instance_visibility. Recompute the plane’s visibility using the same
predicate as the base render path, preserving show_visual, show_collision, and
static-shape visibility rules when planes are re-enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 932275ab-77a3-4676-b5aa-18b0a9436406
📒 Files selected for processing (1)
newton/_src/viewer/viewer_rtx.py
444be6b to
c3eb4c9
Compare
Add a global "Show Ground" checkbox to the viewer visualization panel, backed by ViewerBase.show_ground (default on). When disabled, plane shapes (e.g. the ground plane) are hidden while all other geometry is unaffected. The gate lives in _should_show_shape() so both the base render loop and the ViewerGL packed-instancer path honor it. Add a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RTX viewer's render phase bypasses the _shape_instances loop that drives _should_show_shape(), so plane visibility never updated when show_ground was toggled. Track _last_show_ground and push visibility updates for PLANE batches into _pending_instance_visibility on change; end_frame() already flushes these to OVRTX each frame. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
c3eb4c9 to
10337d8
Compare
There was a problem hiding this comment.
I don't think we need a separate test suite for just this feature? We can skip these kinds of mock tests altogether I think.
There was a problem hiding this comment.
Removed in e5481f1 — agreed, the mock-style suite was not earning its keep.
Flagging the consequence so it is a deliberate choice rather than an accident: nothing else in newton/tests touches _should_show_shape or show_ground, so the toggle now has no direct test coverage. Happy to fold a case into an existing viewer suite if you would rather it not go fully uncovered.
eric-heiden
left a comment
eric-heiden
left a comment
There was a problem hiding this comment.
I think generally this is a useful feature, maybe later we can have something like a scene graph GUI or so to enable better control of toggling visibility for individual bodies/shapes. But in the meantime it should be good to have this.
Assigning show_ground straight into _pending_instance_visibility made every plane batch visible when the toggle was switched back on, even when show_visual, show_collision, static-shape, or layer rules should still have hidden it. Re-derive through _should_show_shape(), the same predicate the build path uses. Drop the standalone show_ground test suite at review request. This leaves the toggle without direct test coverage: nothing else in newton/tests exercises _should_show_shape or show_ground. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed e5481f1 addressing review:
Verified: @eric-heiden this should cover your review — re-requesting when you have a moment. |
2 participants
Description
Ground plane can get in the way of viewing certain assets. Most other things can be hidden in the viewer, so I figured the ground should be too.
Add a global "Show Ground" checkbox to the viewer visualization panel, backed by ViewerBase.show_ground (default on). When disabled, plane shapes (e.g. the ground plane) are hidden while all other geometry is unaffected.
The gate lives in _should_show_shape() so both the base render loop and the ViewerGL packed-instancer path honor it. Add a regression test.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Summary by CodeRabbit