Add mesh approximation methods, physics:approximation handling in USD importer#430
Conversation
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>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
📝 Walkthrough## Walkthrough
This update introduces new utility functions for axis-aligned and oriented bounding box computation, extends mesh approximation capabilities in the model builder to include bounding sphere and box methods, and enables USD import to select mesh approximation strategies via USD attributes. The changes also adjust collision flag handling during USD import and add in-place remeshing support. Additionally, a `copy` method was added to the `Mesh` class, and new tests for mesh approximation were added to both USD import and model tests.
## Changes
| File(s) | Change Summary |
|---------------------------------|------------------------------------------------------------------------------------------------------------|
| `newton/geometry/utils.py` | Added `compute_aabb` and `compute_obb` for bounding box calculations; updated `remesh_mesh` with `inplace` parameter; added `create_box_mesh` and `transform_points` utilities; improved `remesh_convex_hull` vertex indexing. |
| `newton/sim/builder.py` | Renamed `simplify_meshes` to `approximate_meshes`; added support for "vhacd", "bounding_sphere", and "bounding_box" approximation methods; updated imports to include `compute_obb`. |
| `newton/utils/import_usd.py` | Extended `parse_usd` with `skip_mesh_approximation` parameter; added mesh approximation dispatch based on USD `physics:approximation` attribute; updated collision flag handling for disabled shapes. |
| `newton/geometry/types.py` | Added `copy` method to `Mesh` class for creating mesh copies with optional inertia recomputation. |
| `newton/tests/test_model.py` | Added `test_mesh_approximation` verifying mesh approximation methods; added imports for box mesh creation and transformation utilities; simplified unittest main invocation. |
| `newton/tests/test_import_usd.py`| Added `test_mesh_approximation` USD import test with box mesh and multiple approximation methods; added imports for utilities and assertions. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant USD File
participant parse_usd
participant ModelBuilder
participant GeometryUtils
USD File->>parse_usd: Provide mesh and shape data
parse_usd->>parse_usd: Read physics:approximation attribute
alt skip_mesh_approximation is False and mesh shape
parse_usd->>ModelBuilder: Queue shape indices by approximation method
loop For each approximation method
ModelBuilder->>GeometryUtils: Call compute_obb or other utility as needed
GeometryUtils-->>ModelBuilder: Return bounding box or mesh data
ModelBuilder->>ModelBuilder: Replace or add shape(s) as per method
end
end
parse_usd->>ModelBuilder: Update shape collision flags as neededEstimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
newton/sim/builder.py (1)
2171-2238: Consider applying scale before decomposition for non-uniform scaling.The convex decomposition is performed on the unscaled mesh vertices, but the scale is only applied when creating new mesh shapes. For non-uniform scaling, this could lead to incorrect decompositions.
Consider scaling the vertices before decomposition:
if hash_m in decompositions: decomposition = decompositions[hash_m] else: + # Apply scale to vertices if non-uniform + scaled_vertices = mesh.vertices * np.array([*scale]) if scale != (1.0, 1.0, 1.0) else mesh.vertices if method == "coacd": - cmesh = coacd.Mesh(mesh.vertices, mesh.indices.reshape(-1, 3)) + cmesh = coacd.Mesh(scaled_vertices, mesh.indices.reshape(-1, 3)) coacd_settings = { "threshold": 0.5, "mcts_nodes": 20, "mcts_iterations": 5, "mcts_max_depth": 1, "merge": False, "max_convex_hull": mesh.maxhullvert, } coacd_settings.update(remeshing_kwargs) decomposition = coacd.run_coacd(cmesh, **coacd_settings) else: - tmesh = trimesh.Trimesh(mesh.vertices, mesh.indices.reshape(-1, 3)) + tmesh = trimesh.Trimesh(scaled_vertices, mesh.indices.reshape(-1, 3)) vhacd_settings = { "maxNumVerticesPerCH": mesh.maxhullvert, } vhacd_settings.update(remeshing_kwargs) decomposition = trimesh.decomposition.convex_decomposition(tmesh, **vhacd_settings) decomposition = [(d["vertices"], d["faces"]) for d in decomposition]Then adjust the scale parameter when creating new shapes:
self.add_shape_mesh( body=body, xform=xform, cfg=cfg, mesh=Mesh(decomposition[i][0], decomposition[i][1]), key=f"{self.shape_key[shape]}_convex_{i}", - scale=scale, + scale=(1.0, 1.0, 1.0) if scale != (1.0, 1.0, 1.0) else scale, )
🧹 Nitpick comments (1)
newton/utils/import_usd.py (1)
129-138: Consider making the approximation mapping configurable.The hardcoded mapping from USD
physics:approximationvalues to remeshing methods is comprehensive but inflexible.Consider making this mapping configurable through a parameter or builder configuration:
+def get_default_approximation_mapping(): + return { + "convexdecomposition": "coacd", + "convexhull": "convex_hull", + "boundingsphere": "bounding_sphere", + "boundingcube": "bounding_box", + "meshSimplification": "quadratic", + } def parse_usd( source, builder: ModelBuilder, # ... other parameters skip_mesh_approximation: bool = False, + approximation_mapping: dict[str, str] | None = None, ): # ... - approximation_to_remeshing_method = { - "convexdecomposition": "coacd", - "convexhull": "convex_hull", - "boundingsphere": "bounding_sphere", - "boundingcube": "bounding_box", - "meshSimplification": "quadratic", - } + approximation_to_remeshing_method = approximation_mapping or get_default_approximation_mapping()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/generated/core/newton.Mesh.rstis excluded by!**/generated/**docs/generated/core/newton.ModelBuilder.rstis excluded by!**/generated/**docs/generated/renderers/newton.utils.SimRendererOpenGL.rstis excluded by!**/generated/**docs/generated/renderers/newton.utils.SimRendererUsd.rstis excluded by!**/generated/**
📒 Files selected for processing (4)
newton/geometry/utils.py(1 hunks)newton/sim/builder.py(4 hunks)newton/tests/test_import_usd.py(1 hunks)newton/utils/import_usd.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/geometry/utils.py (1)
newton/geometry/types.py (2)
vertices(137-138)vertices(141-143)
⏰ 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 (9)
newton/geometry/utils.py (1)
62-66: LGTM: Simple and efficient AABB computation.The function correctly computes axis-aligned bounding boxes using numpy's min/max operations.
newton/tests/test_import_usd.py (1)
159-159: Minor cleanup: Kernel cache clearing disabled.The change to comment out
wp.clear_kernel_cache()appears to be intentional cleanup, possibly reflecting changes in Warp usage patterns or test environment setup.newton/utils/import_usd.py (3)
52-52: Well-designed feature toggle parameter.The
skip_mesh_approximationparameter provides good control over the new functionality with a sensible default value and clear documentation.Also applies to: 78-78
930-942: Robust mesh approximation queuing logic.The implementation correctly handles the
physics:approximationattribute processing with proper error handling and case-insensitive matching.
969-969: Correct collision flag handling.The bitwise operation properly clears the collision flag for shapes with disabled collision.
newton/sim/builder.py (4)
66-66: LGTM!The import statement correctly includes the new
compute_obbfunction needed for the bounding box approximation method.
2133-2163: Well-structured method signature and documentation!The method signature properly uses type hints and the documentation clearly explains all supported approximation methods with a well-formatted table.
2249-2259: LGTM!The bounding box approximation correctly scales vertices before computing the OBB and properly composes the transforms.
2260-2273: LGTM!The remeshing fallback implementation correctly uses caching and provides clear error messages for unknown methods.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/geometry/utils.py (1)
69-136: Good improvements addressing past feedback, but add shape validation.The function now handles most edge cases well, addressing previous review concerns about empty inputs, single vertices, and degenerate cases. However, input shape validation is still missing.
def compute_obb(vertices: nparray) -> tuple[wp.transform, wp.vec3]: """Compute the oriented bounding box of a set of vertices. Args: vertices: A numpy array of shape (N, 3) containing the vertex positions. Returns: A tuple containing: - transform: The transform of the oriented bounding box - extents: The half-extents of the box along its principal axes """ - if len(vertices) == 0: + if len(vertices) == 0 or vertices.shape[1] != 3: return wp.transform_identity(), wp.vec3(0.0, 0.0, 0.0)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/geometry/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/geometry/utils.py (2)
newton/geometry/types.py (6)
vertices(157-158)vertices(161-163)Mesh(67-228)indices(166-167)indices(170-172)copy(136-154)newton/geometry/inertia.py (1)
compute_mesh_inertia(292-381)
⏰ 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 (1)
newton/geometry/utils.py (1)
464-491: Well-implemented in-place modification support.The addition of the
inplaceparameter provides good flexibility. The implementation correctly handles both in-place modification and copy scenarios, with appropriate inertia recomputation when requested.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_model.py (2)
256-260: Bounding box validation needs clarification on rotation comparison.The test correctly validates that the shape type changes to
GEO_BOXand the source mesh is cleared. However, the comment mentions that rotation comparison is skipped because "the rotation is not guaranteed to be the same." This could benefit from a brief explanation of why the rotation might differ.Consider adding a more detailed comment explaining why the rotation component of the transform might vary for oriented bounding boxes:
- # only compare the position since the rotation is not guaranteed to be the same + # only compare the position since the OBB rotation depends on the mesh orientation + # and may not match the original transform rotation
251-268: Consider adding edge case testing.The current test covers the happy path well, but consider adding tests for edge cases such as:
- Empty or invalid meshes
- Meshes that are already convex
- Very small or degenerate meshes
- Error handling for unsupported approximation methods
You could add a separate test method for edge cases:
def test_mesh_approximation_edge_cases(self): builder = ModelBuilder() # Test with invalid method with self.assertRaises(ValueError): builder.approximate_meshes(method="invalid_method") # Test with empty shape_indices builder.approximate_meshes(method="convex_hull", shape_indices=[]) # Should complete without error
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/geometry/utils.py(3 hunks)newton/tests/test_model.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/geometry/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/tests/test_model.py (3)
newton/geometry/utils.py (2)
create_box_mesh(501-563)transform_points(566-569)newton/sim/builder.py (3)
ModelBuilder(83-3572)add_shape_mesh(2067-2100)approximate_meshes(2133-2280)newton/tests/unittest_utils.py (1)
assert_np_equal(240-246)
⏰ 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 (9)
newton/tests/test_model.py (9)
24-24: Import statement looks good.The import of
create_box_meshandtransform_pointsfromnewton.geometry.utilsis correctly added to support the new test functionality.
231-235: Helper functionbox_meshis well-designed.The function correctly creates a box mesh using
create_box_meshand optionally applies a transformation. The implementation is clean and reusable within the test.
237-238: Helper functionnpsortedis appropriate for the test.This simple utility function helps compare arrays regardless of element order, which is useful for comparing scale vectors where order might not be guaranteed.
240-247: Test setup creates appropriate test data.The test creates a mesh with non-uniform scaling and translation, which provides good coverage for testing the approximation methods. Setting
mesh.maxhullvert = 5is a smart way to control and verify the convex hull approximation result.
248-250: Approximation method calls are correctly structured.Each approximation method is tested on a separate shape instance, which allows for independent validation of each method without interference.
252-254: Convex hull validation is thorough.The test correctly verifies that the convex hull reduces the vertex count to the expected value (5) and maintains the identity transform, which aligns with the expected behavior of the convex hull approximation.
262-265: Bounding sphere validation is mathematically correct.The test properly validates that the sphere radius equals
wp.length(scale)(the magnitude of the scale vector) and that the transform is preserved. This aligns with the expected behavior for bounding sphere approximation.
267-268: Original mesh preservation check is essential.This validation ensures that the approximation methods don't modify the original mesh data, which is crucial for maintaining data integrity when the same mesh might be used elsewhere.
272-272: Simplified unittest.main() call is appropriate.Removing the explicit verbosity parameter aligns with the changes mentioned in the AI summary and makes the test invocation more standard.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
newton/tests/test_import_usd.py (3)
177-178: Remove unused variabletfThe variable
tfis defined but never used in the test. Based on the pattern intest_model.py, it seems this was intended to transform the mesh, but the transformation is handled differently here.Remove the unused variable:
- tf = wp.transform(wp.vec3(1.0, 2.0, 3.0), wp.quat_identity()) scale = wp.vec3(1.0, 3.0, 0.2)
195-195: Remove unused variablenormalsThe variable
normalsis initialized but never populated or used.# Fill in VtArrays points = [] - normals = [] indices = [] vertex_counts = []
162-169: Consider extracting duplicate helper functions to test utilitiesThe
box_meshandnpsortedhelper functions are duplicated fromtest_model.py. Consider moving these to a shared test utilities module to avoid duplication.These helper functions could be moved to
newton.tests.unittest_utilsor a new test utilities module for mesh-related tests.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_import_usd.py(2 hunks)
🧠 Learnings (1)
newton/tests/test_import_usd.py (1)
Learnt from: dylanturpin
PR: #394
File: newton/tests/test_ik.py:119-119
Timestamp: 2025-07-15T21:00:03.709Z
Learning: In the Newton physics engine codebase, it's acceptable to test private API functions (those prefixed with underscore) in unit and component tests, as the maintainers consider this appropriate for testing internal functionality.
🧬 Code Graph Analysis (1)
newton/tests/test_import_usd.py (4)
newton/geometry/utils.py (2)
create_box_mesh(501-563)transform_points(566-569)newton/tests/test_model.py (3)
test_mesh_approximation(230-268)box_mesh(231-235)npsorted(237-238)newton/sim/builder.py (1)
ModelBuilder(83-3572)newton/utils/import_usd.py (1)
parse_usd(34-1122)
🪛 Ruff (0.12.2)
newton/tests/test_import_usd.py
160-160: import should be at the top-level of a file
(PLC0415)
177-177: Local variable tf is assigned to but never used
Remove assignment to unused variable tf
(F841)
195-195: Local variable normals is assigned to but never used
Remove assignment to unused variable normals
(F841)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/tests/test_import_usd.py
[error] 162-162: NameError: name 'wp' is not defined in test_mesh_approximation test function.
🪛 GitHub Actions: Pull Request
newton/tests/test_import_usd.py
[error] 162-162: NameError: name 'wp' is not defined in test_mesh_approximation function.
🧰 Additional context used
🧠 Learnings (1)
newton/tests/test_import_usd.py (1)
Learnt from: dylanturpin
PR: #394
File: newton/tests/test_ik.py:119-119
Timestamp: 2025-07-15T21:00:03.709Z
Learning: In the Newton physics engine codebase, it's acceptable to test private API functions (those prefixed with underscore) in unit and component tests, as the maintainers consider this appropriate for testing internal functionality.
🧬 Code Graph Analysis (1)
newton/tests/test_import_usd.py (4)
newton/geometry/utils.py (2)
create_box_mesh(501-563)transform_points(566-569)newton/tests/test_model.py (3)
test_mesh_approximation(230-268)box_mesh(231-235)npsorted(237-238)newton/sim/builder.py (1)
ModelBuilder(83-3572)newton/utils/import_usd.py (1)
parse_usd(34-1122)
🪛 Ruff (0.12.2)
newton/tests/test_import_usd.py
160-160: import should be at the top-level of a file
(PLC0415)
177-177: Local variable tf is assigned to but never used
Remove assignment to unused variable tf
(F841)
195-195: Local variable normals is assigned to but never used
Remove assignment to unused variable normals
(F841)
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/tests/test_import_usd.py
[error] 162-162: NameError: name 'wp' is not defined in test_mesh_approximation test function.
🪛 GitHub Actions: Pull Request
newton/tests/test_import_usd.py
[error] 162-162: NameError: name 'wp' is not defined in test_mesh_approximation function.
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>
…-approximation Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
|
I didn't go through all the changes but tested this on some of my assets and things are working as expected. Thanks Eric |
…SD importer (newton-physics#430) Signed-off-by: Eric Heiden <eric-heiden@outlook.com> Signed-off-by: Eric Heiden <eheiden@nvidia.com>
# Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> Fixes the gif links for Mimic docs so that they render. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Documentation change - ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
…SD importer (newton-physics#430) Signed-off-by: Eric Heiden <eric-heiden@outlook.com> Signed-off-by: Eric Heiden <eheiden@nvidia.com>
Description
parse_usd()now considers thephysics:approximationattributeModelBuilder.approximate_meshes()to support all USD approximation methods (and other remeshing algorithms)ToDo's:
scaleshape attribute in convex decomposition of meshesNewton 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
Other