Decide USD collider visibility from purpose, and keep the render mesh when approximating - #3712
Conversation
📝 WalkthroughWalkthroughChangesThe USD importer now derives collider visibility from USD purpose and visibility, preserves authored visual topology when approximating viewport-drawn meshes, and updates shape flags and counts accordingly. Regression tests, an example asset, and changelog entries reflect these semantics. USD import behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant USDStage
participant USDImporter
participant ModelBuilder
USDStage->>USDImporter: provide purpose, visibility, and mesh data
USDImporter->>ModelBuilder: create collider shapes and visual copies
ModelBuilder->>ModelBuilder: apply visibility flags and collision approximation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🤖 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 47: Add migration guidance to the changelog entry describing the
additional visual shape created by ModelBuilder.add_usd() for approximated
renderable colliders. Explain that shape_count and shape-array iteration now
include this non-colliding visual copy, and instruct consumers to filter
collision shapes using ShapeFlags.COLLIDE_SHAPES or otherwise account for the
visual shape.
In `@newton/_src/utils/import_usd.py`:
- Around line 1210-1214: Update the documentation around the guide-purpose
handling to remove the obsolete material-inherited visibility explanation.
Describe guide as controlling viewport drawability, and state that
force_show_colliders is the explicit override while preserving the independently
evaluated collider display policy.
- Around line 3831-3832: Update the mesh-loading logic around _is_viewport_drawn
and the related visual-copy path to use _get_mesh_with_visual_material()
whenever load_visual_shapes is enabled and the prim is viewport-drawn. Ensure
synthesized visual meshes retain source texture, roughness, and metallic
metadata, including the default-purpose physics:approximation case with hidden
collision shapes.
🪄 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: 0bc67b23-a382-436f-a4ed-f67a20f71756
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/utils/import_usd.pynewton/examples/assets/cartpole_single_pendulum.usdanewton/tests/test_import_usd.py
💤 Files with no reviewable changes (1)
- newton/examples/assets/cartpole_single_pendulum.usda
d13b026 to
9c4b872
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/tests/test_import_usd.py (1)
11250-11293: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the stale default-visibility comment.
Line 11242 says this collider should not be visible, contradicting the new assertion on Line 11251. Update it to describe default-purpose collider visibility.
🤖 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 `@newton/tests/test_import_usd.py` around lines 11250 - 11293, Update the stale comment immediately before the default-purpose visibility assertions in the USD import test to state that a default-purpose collider is visible when it is not invisible. Keep the assertions and behavior unchanged.Source: Path instructions
🤖 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.
Outside diff comments:
In `@newton/tests/test_import_usd.py`:
- Around line 11250-11293: Update the stale comment immediately before the
default-purpose visibility assertions in the USD import test to state that a
default-purpose collider is visible when it is not invisible. Keep the
assertions and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 485b9f93-a223-4cdb-b0fd-412fa377df4d
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/utils/import_usd.pynewton/examples/assets/cartpole_single_pendulum.usdanewton/tests/test_import_usd.py
💤 Files with no reviewable changes (1)
- newton/examples/assets/cartpole_single_pendulum.usda
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- newton/_src/utils/import_usd.py
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>
physics:approximation belongs to UsdPhysicsMeshCollisionAPI and is scoped to collision: it says which shape to collide against, not which to draw. The importer applied it destructively, so a Mesh that is both render geometry and a collider lost its authored topology and was drawn as its own collision hull. Approximating a prim that is viewport geometry now splits it into an approximated collider plus a visual carrying the authored mesh, via approximate_meshes(keep_visual_shapes=True). Whether a prim is viewport geometry follows USD purpose and visibility alone: an unauthored purpose composes to "default" and is drawable, and no bound material is required. The collider display policy that governs pure colliders does not decide this, since such a prim is render geometry in its own right. Colliders with no approximation are left alone -- their collision geometry is their authored geometry, so there is nothing to preserve -- as are guide-purpose and invisible prims, which have no render role. The visual carries zero density and no collision, and copies append after the originals, so masses, existing shape indices and path_shape_map entries are unchanged. Both spellings of the same intent now import alike: a guide-purpose collider beside a visual mesh, or one dual-purpose prim, each yielding one collider and one visual. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whether a collider is drawn was inferred from whether a render material with PBR cues happened to be bound to it. USD already states this declaratively: a prim whose purpose resolves to "default" or "proxy" and which is not invisible is viewport geometry, whether that purpose was authored or inherited from the fallback, and regardless of any material. The heuristic was not merely redundant, it was wrong in a way that got worse with more visuals in a scene. Because the collider display policy is model-wide, loading any visual geometry anywhere flipped unmaterialed colliders to hidden. With static visual shapes now imported by default, dropping a visual cube into a scene erased an unrelated collider entirely -- it rendered nothing at all. This subsumes the guide-purpose fix in the preceding commit by the more general rule: a guide collider is not viewport geometry, so it is not drawn, and force_show_colliders still reveals it. Two tests asserted the heuristic and are updated to the purpose-based contract, including the guide-purpose escape hatch that replaces the behaviour they were pinning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collider visibility had a fallback: when an import produced no visual shapes at all, every collider was given VISIBLE so the viewport would not come up empty. That overrode the one thing the asset had actually stated. A prim marked guide is not viewport geometry, and a scene whose geometry is all guide has no render geometry -- an empty viewport is the correct depiction of it, not a problem to paper over. The fallback also drew such colliders through the visual toggle, so Show Collision could not hide them and, once approximated, what it drew was the hull rather than the authored mesh. Visibility now follows the asset: purpose and visibility decide, with force_show_colliders as the explicit override for inspecting a collision-only scene. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These three boxes are the cartpole's only geometry -- they are its visuals as much as its colliders -- so marking them guide asserted something untrue about the asset. The importer previously compensated with a fallback that showed colliders when a scene had no visual shapes, which hid the mistake; without it the asset imported with nothing visible at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Port the regression test from @eric-heiden's PR newton-physics#3697, which found the same bug independently: one default-purpose prim serving as both visual and collider lost VISIBLE as soon as another body authored separate visual geometry. Passes here unchanged, and pins the shared-geometry contract more explicitly than the existing cases did. Co-Authored-By: Eric Heiden <eric-heiden@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The empty-viewport fallback removal had no regression test: the existing collision-only case authors a default-purpose sphere, so it exercises drawability rather than the fallback. Pin the case that actually changed -- every prim guide, nothing drawn, force_show_colliders to inspect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Load the render material for a mesh whenever something will draw it, not only when the collider itself is visible. hide_collision_shapes clears collider_is_visible while the visual copy is still produced, so that copy could be built from a mesh loaded without texture, roughness or metallic data. Update the purpose predicate's docstring, which still described gating material-inherited visibility after that heuristic was removed, and give the changelog entry migration guidance for the extra shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9c4b872 to
d8625bb
Compare
|
Rebased onto That merge changes this from a latent bug to a live one. #3694 loads static visual-only geometry by default, and because the old collider-visibility policy was model-wide, any loaded visual flips unmaterialed colliders to hidden. On current In practice that is a table, floor, or backdrop removing a robot's shared collision/visual geometry from the viewport. Previously it took a second rigid body authoring a visual to trigger; now ordinary scene content does it. 292 tests pass on the rebase, and all five authoring cases in the table above still hold. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_usd.py (1)
3298-3302: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid treating all static geometry as one visual body.
Loading any static visual records body
-1; then every static collider matches it and is hidden byhide_collision_shapes=True, even when unrelated or force-shown.Proposed fix
- has_body_visual_shapes = load_visual_shapes and body_id in bodies_with_visual_shapes + has_body_visual_shapes = load_visual_shapes and body_id >= 0 and body_id in bodies_with_visual_shapes🤖 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 `@newton/_src/utils/import_usd.py` around lines 3298 - 3302, Update the static-visual loading loop to avoid passing the shared body ID -1 to _load_visual_shapes_impl for every prim. Assign each unrelated static visual its own identity or otherwise prevent it from matching all static colliders, while preserving exclusions for deformable_visual_exclude_paths, path_shape_map, and rigid-body hierarchies.
🤖 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.
Outside diff comments:
In `@newton/_src/utils/import_usd.py`:
- Around line 3298-3302: Update the static-visual loading loop to avoid passing
the shared body ID -1 to _load_visual_shapes_impl for every prim. Assign each
unrelated static visual its own identity or otherwise prevent it from matching
all static colliders, while preserving exclusions for
deformable_visual_exclude_paths, path_shape_map, and rigid-body hierarchies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 41da0d2f-4a7d-4e5a-8829-d35d69a30e25
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/utils/import_usd.pynewton/examples/assets/cartpole_single_pendulum.usdanewton/tests/test_import_usd.py
💤 Files with no reviewable changes (1)
- newton/examples/assets/cartpole_single_pendulum.usda
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- newton/tests/test_import_usd.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
Decide what a USD collider draws from
purposeand visibility, and stopphysics:approximationfrom destroying the geometry it was only meant to collide with.Three related defects, all in how
parse_usd()assignsShapeFlags.VISIBLE:1. Drawability was inferred from a bound render material. A collider became visible only if it had PBR material cues (
texture/roughness/metallic) — a guess at something USD already states declaratively. Because the supporting policy was model-wide, loading any visual geometry anywhere flipped unmaterialed colliders to hidden: a default-purpose prim serving as both visual and collider vanished because some other body authored a separate visual. Drawability now followspurposeresolving todefault/proxyand the prim not being invisible.2.
physics:approximationreplaced the render mesh. That attribute belongs toUsdPhysicsMeshCollisionAPIand is scoped to collision — it says what to collide against, not what to draw. The importer applied it destructively, so a prim that was both render geometry and a collider lost its authored topology and was drawn as its own collision hull. Approximating a drawable prim now splits it into an approximated collider plus a visual carrying the authored mesh, via the existingapproximate_meshes(keep_visual_shapes=True).3. An empty-viewport fallback overrode
guide. When an import produced no visual shapes at all, every collider was givenVISIBLEso the viewport would not come up blank. That overrode the one thing the asset had stated, drewguidegeometry through the visual toggle, and — once approximated — drew the hull rather than the authored mesh. An asset whose geometry is allguidehas no render geometry; an empty viewport is the correct depiction.force_show_collidersremains for inspecting such a scene.Resulting contract
A(default)+B(guide, collider, approximated)A(default, collider, approximated)A(default, collider, not approximated)A(guide, collider, approximated)A(guide, collider, not approximated)Row 3 stays a single shape deliberately: with no approximation the two would carry identical geometry, so splitting buys no independent control and guarantees z-fighting on coincident surfaces. The split earns its keep precisely when the geometries differ.
Closes #3650 (superseded by the general rule — a
guidecollider is not viewport geometry, so it is not drawn, andforce_show_collidersstill reveals it).Effect on a real asset
A UR5e with a Robotiq 2F-85 gripper, identical camera, Show Collision off in both:
mainShapes: 392 visual: 384 collision: 362Shapes: 403 visual: 42 collision: 362Before:

After:

The gripper's link prims are dual-purpose — default
purpose,PhysicsCollisionAPIapplied, andphysics:approximationauthored (9convexHull, 2convexDecomposition). Onmainthe approximation replaced their render geometry, so 11 prims became 353 hull fragments and those are what the visual toggle drew. Collider count is unchanged at 362 either way: this is purely what reaches the renderer, not what is simulated.Worth noting the total shape count barely moves (392 → 403) while what is drawn goes 384 → 42. The totals alone do not show this, which is why the counts above are split (#3711).
Relationship to #3697
@eric-heiden diagnosed defect 1 independently in #3697 and reached the same predicate. This PR reaches the same result for shared geometry and additionally covers defects 2 and 3, which #3697 does not touch. Verified on the asset from that PR: the shared default-purpose collider is visible and the dedicated
guidecollider is not, identically on both branches. #3697's regression test is carried here unchanged, with credit — it pins the shared-geometry contract more explicitly than anything already in the suite.#3694 makes defect 1 live on
main#3694 landed as
e9aabe7d2, and it loads static visual-only geometry by default. Because the old policy was model-wide, more loaded visuals means more colliders silently hidden — so the failure is now reachable with ordinary scene content rather than requiring a second rigid body. On currentmain, adding a plain visual cube to a scene erases an unrelated dual-role collider:That is a table, a backdrop, or a floor removing a robot's shared collision/visual geometry from the viewport.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Each behavioural commit was verified failing without its fix before being committed.
Swept every asset in
newton/examples/assetsandnewton/tests/assetsfor shapes that stop being drawn. Exactly one changed:cartpole_single_pendulum.usdaimported with nothing visible, because its three collision boxes are the cartpole's only geometry — they are its visuals as much as its colliders — yet were markedpurpose="guide". The old fallback had been masking that. Fixed in the asset (final commit, three deleted lines) rather than by weakening the rule. No other bundled asset changes what it draws.Bug fix
Steps to reproduce:
purpose,PhysicsCollisionAPIapplied.physics:approximation = "convexHull".ModelBuilder.add_usd().Separately, with any second body authoring a visual shape, a default-purpose collider with no bound material loses
VISIBLEand is drawn by neither toggle.Minimal reproduction:
Migration
Assets relying on the removed behaviours need one attribute:
purpose = "guide". Previously a collider on a body with other visuals was hidden implicitly; that is no longer inferred.guide) now imports with nothing visible. Passforce_show_colliders=Trueto inspect it.hide_collision_shapesandforce_show_collidersare unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
Changed
guide-purpose handling.Bug Fixes
guide-purpose colliders with bound render materials from becoming visible unintentionally.Tests
Chores
guidepurpose where applicable.