Load visual-only shapes from USD#473
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant parse_usd
participant add_body
participant load_visual_shapes
participant builder
User->>parse_usd: Call with load_non_physics_prims=True
parse_usd->>add_body: For each body prim
add_body->>builder: Add body
add_body->>load_visual_shapes: If load_non_physics_prims, load visual shapes for children
load_visual_shapes->>builder: Add visual shape (zero density, no collision)
parse_usd->>add_body: For each articulation body prim
add_body->>builder: Add articulation body
add_body->>load_visual_shapes: Load visual shapes for articulation children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
newton/utils/import_usd.py (2)
303-450: Consider performance impact and add tests.The recursive traversal in
load_visual_shapescould impact loading performance for complex USD scenes. Consider:
- Adding performance metrics or warnings for scenes with many non-physics prims
- Documenting the performance implications in the function docstring
- Adding unit tests to verify visual shape loading behavior
52-52: Ensure migration guide is updated.As mentioned in the PR objectives, the migration guide for warp.sim users should be updated to reflect the removal of
only_load_warp_sceneparameter and the addition ofload_non_physics_prims.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/utils/import_usd.py(6 hunks)
⏰ 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 GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (6)
newton/utils/import_usd.py (6)
52-52: Function signature change looks good.The addition of
load_non_physics_primsparameter with a default value ofTruemaintains backward compatibility while providing the new functionality to load visual-only shapes.Also applies to: 78-78
297-301: Visual shape configuration is appropriate.The configuration correctly sets up shapes with zero density and disabled collisions, ensuring they serve only as visual elements without affecting physics simulation.
410-434: Mesh loading implementation is correct.The code properly handles mesh triangulation and is consistent with the existing mesh handling patterns in the file.
481-481: Parse body refactoring is correct.The changes properly integrate the new
add_bodyhelper and maintain compatibility with the deferred body creation pattern.Also applies to: 484-487
814-818: Verbose logging enhancement is helpful.The additional logging for articulation bodies improves debugging capabilities when verbose mode is enabled.
877-877: Articulation body creation properly uses the new helper.The changes correctly use the
add_bodyhelper to ensure visual shapes are loaded for articulation bodies whenload_non_physics_primsis enabled.Also applies to: 882-882
|
Thanks Eric! Works great! |
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Looks good, just one question. And this needs some testing.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
This is not needed for simulation and may slow down MuJoCo so I don't think we should do this. The Newton visualizer should be enough to debug the simulation eventually so we should not expect the MuJoCo visualizer to have everything in it, even things that are unrelated to the simulation. |
|
Do we have a ticket for the tests? |
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Description
load_non_physics_primsprovided toparse_usdis Trueonly_load_warp_sceneargument and filtering by special "warp" attributesNewton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
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
Improvements
Removed
Tests