Keep guide-purpose colliders off the visual toggle - #3650
Conversation
A collider with a bound render material was given the VISIBLE flag regardless of its purpose. Viewers draw a shape when (COLLIDE and show_collision) or (VISIBLE and show_visual), so such a collider was drawn by the visual toggle and could not be hidden by the collision toggle -- it occluded the very visual shapes it sat next to. "guide" is the conventional purpose for authored collision geometry, so honour it: purpose now gates only whether a collider inherits visibility from its render material. force_show_colliders is checked independently and still reveals guide colliders, which is what the previous behaviour was protecting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe USD importer now requires mesh colliders to be viewport-drawn before bound materials mark them visible. Guide-purpose colliders remain collision shapes without ChangesUSD collider visibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Line 81: Update the CHANGELOG entry describing ModelBuilder.add_usd() so it
documents material-inherited VISIBLE removal for both guide- and render-purpose
mesh colliders, while preserving the existing note about force_show_colliders.
🪄 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: 8b0ad439-6a66-4590-a702-096ef5dae1d2
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/utils/import_usd.pynewton/tests/test_import_usd.py
|
|
||
| ### Fixed | ||
|
|
||
| - Fix `ModelBuilder.add_usd()` marking a `guide`-purpose collider visible when it has a bound render material. Such a collider is not viewport geometry, and the extra `VISIBLE` flag left it drawn by the viewer's visual toggle instead of its collision toggle. `force_show_colliders` still reveals it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the render-purpose behavior too.
The new gate in newton/_src/utils/import_usd.py removes material-inherited VISIBLE for both guide and render mesh colliders, but this entry mentions only guide. Update the entry to cover both purposes, or narrow the code if render was not intended to change.
🤖 Prompt for 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.
In `@CHANGELOG.md` at line 81, Update the CHANGELOG entry describing
ModelBuilder.add_usd() so it documents material-inherited VISIBLE removal for
both guide- and render-purpose mesh colliders, while preserving the existing
note about force_show_colliders.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Closing in favour of a single consolidated PR. @eric-heiden asked for the visual/collider visibility work to land together rather than piecemeal, and while investigating this it became clear the guide-purpose fix here is one facet of a broader issue: whether a collider is drawn should follow USD purpose and visibility, not whether a render material happens to be bound to it. That same question governs approximated colliders (which currently discard their authored render mesh) and interacts with #3694. Rolling this commit, the approximation fix, and the remaining drawability work into a |
Description
A collider with a bound render material was given the
VISIBLEflag regardless of itspurpose. Viewers draw a shape when
(COLLIDE and show_collision) or (VISIBLE and show_visual), so such a collider was drawn by the visual toggle and could not behidden by the collision toggle — it occluded the very visual shapes it sat beside.
guideis the conventional purpose for authored collision geometry (the MuJoCo USDexporter emits it), so honour it: purpose now gates only whether a collider inherits
visibility from its bound render material.
force_show_collidersis checkedindependently and still reveals
guidecolliders, which is what the previousbehaviour was protecting — that concern is preserved, it was just conflated with
material-inherited visibility.
Note for reviewers: this changes behaviour for existing assets. A
guide-purposecollider with a render material used to render and now does not, unless
force_show_colliders=True. Filed underFixedsince the old behaviour made theviewer's collision toggle inoperative on such assets, but it is worth a second
opinion if anyone is relying on it.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Verified the new test fails without the fix (
AssertionError: 1 is not false) andpasses with it.
Bug fix
Steps to reproduce:
uniform token purpose = "guide"and bind a rendermaterial to it.
ModelBuilder.add_usd()and inspect the collider's shape flags, orview it with
show_visual=True, show_collision=False.VISIBLE, so it is drawn by the visual toggle and thecollision toggle cannot hide it.
Minimal reproduction:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests