Mesh simplification, convex decomposition#407
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes simplify the handling of convex hulls in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Mesh
participant Remeshing
participant ModelBuilder
participant MuJoCoSolver
User->>Mesh: Create Mesh(vertices, indices, ...)
Mesh-->>User: Mesh instance (no convex_hull argument)
User->>Mesh: compute_convex_hull(replace=False)
Mesh->>Remeshing: remesh_convex_hull(maxhullvert)
Remeshing-->>Mesh: Convex hull mesh
User->>Remeshing: remesh_mesh(mesh, method, ...)
Remeshing->>Remeshing: remesh(vertices, faces, method, ...)
Remeshing-->>Remeshing: (verts, faces)
Remeshing-->>User: New Mesh
User->>ModelBuilder: simplify_meshes(method, shape_indices, ...)
ModelBuilder->>Remeshing: remesh_mesh or coacd decomposition
Remeshing-->>ModelBuilder: Simplified meshes
ModelBuilder-->>User: Updated model with simplified meshes
User->>MuJoCoSolver: convert_to_mjc(model)
MuJoCoSolver->>Mesh: Get vertices, indices
MuJoCoSolver-->>User: MuJoCo model with mesh geoms (no convex hull fallback)
Possibly related issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 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
|
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: 3
🧹 Nitpick comments (1)
newton/geometry/types.py (1)
156-156: Remove unused parameter.The
replaceparameter is defined but never used in the method body, which can be confusing for callers.- def compute_convex_hull(self, replace: bool = False) -> "Mesh": + def compute_convex_hull(self) -> "Mesh":
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/geometry/types.py(3 hunks)newton/geometry/utils.py(3 hunks)newton/solvers/mujoco/solver_mujoco.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
newton/solvers/mujoco/solver_mujoco.py (1)
Learnt from: Kenny-Vilella
PR: newton-physics/newton#398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.659Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
🧬 Code Graph Analysis (2)
newton/geometry/types.py (1)
newton/geometry/utils.py (1)
remesh_convex_hull(308-336)
newton/geometry/utils.py (1)
newton/geometry/types.py (1)
Mesh(67-178)
🔇 Additional comments (8)
newton/solvers/mujoco/solver_mujoco.py (1)
1478-1482: LGTM: Simplified mesh handling by removing convex hull fallback logic.The changes correctly remove the conditional logic that previously checked for pre-computed convex hull meshes, now consistently using the original mesh data directly. This aligns with the overall design goal of removing cached convex hull handling.
newton/geometry/types.py (1)
128-128: Good addition for hash caching optimization.The new
_cached_hashattribute follows Python naming conventions and provides a foundation for optimizing hash computation performance.newton/geometry/utils.py (6)
19-19: Good addition for type safety.Adding
Literalimport supports the newRemeshingMethodtype alias, improving type safety.
24-24: Improved import organization.Consolidating the imports from
..core.typesis cleaner and more maintainable.
338-338: Excellent type safety improvement.The
RemeshingMethodtype alias provides clear constraints on valid remeshing methods and improves API usability.
340-340: Enhanced type annotations improve API clarity.The explicit typing with
RemeshingMethodand return type annotation makes the function interface clearer and more maintainable.
377-386: Well-documented function with clear interface.The updated function signature, parameter typing, and comprehensive docstring significantly improve the API usability and maintainability.
389-393: Solid implementation of mesh updating logic.The mesh vertex/index updating, index flattening, and conditional inertia recomputation provide a complete and flexible remeshing solution.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/geometry/types.py (1)
186-186: Verify function signature compatibility.The past review comment flagged that
remesh_convex_hulldoesn't accept themaxhullvertparameter. However, the enriched summary suggests this function was updated. Let's verify the current function signature.#!/bin/bash # Description: Check if remesh_convex_hull function accepts maxhullvert parameter # Expected: Function signature should include maxhullvert parameter # Search for the function definition ast-grep --pattern 'def remesh_convex_hull($$$)'
🧹 Nitpick comments (1)
newton/sim/builder.py (1)
2177-2178: Add logging for empty decompositions and clarify type checking.Two suggestions for improvement:
- Empty decomposition results (lines 2177-2178) should be logged as they might indicate an issue with the mesh.
- The type checking pattern on line 2206 is unusual and could be more explicit.
if len(decomposition) == 0: + print(f"Warning: CoACD decomposition returned no results for shape {shape}") continueFor better clarity, consider using a more explicit type check or enum membership test instead of
RemeshingMethod.__args__if possible.Also applies to: 2206-2206
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/geometry/types.py(6 hunks)newton/geometry/utils.py(7 hunks)newton/sim/builder.py(4 hunks)newton/solvers/mujoco/solver_mujoco.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/solvers/mujoco/solver_mujoco.py
- newton/geometry/utils.py
🧰 Additional context used
🪛 GitHub Actions: Pull Request
newton/geometry/types.py
[error] 1-1: Sphinx warning: duplicate object description of newton.Mesh.vertices, other instance in generated/core/newton.Mesh, use :no-index: for one of them
[error] 1-1: Sphinx warning: duplicate object description of newton.Mesh.indices, other instance in generated/core/newton.Mesh, use :no-index: for one of them
⏰ 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/sim/builder.py (2)
25-25: LGTM!The imports are correctly added to support the new mesh simplification functionality.
Also applies to: 66-66
177-181: Good type safety improvements.The explicit
int()casting ensures proper bitwise operations, and the specific return type annotation improves clarity.newton/geometry/types.py (7)
21-21: LGTM: Import addition is correct.The
overrideimport is properly added and used later in the__hash__method.
100-104: LGTM: Type annotations enhance code clarity.The type annotations properly specify the accepted parameter types and improve static type checking capabilities.
121-122: LGTM: Good encapsulation with private attributes.Converting vertices and indices to private attributes with proper numpy array conversion is a solid design choice.
127-127: LGTM: Hash caching initialization.The
_cached_hashattribute initialization supports the performance improvement mentioned in the PR objectives.
187-201: LGTM: Well-designed replace parameter logic.The implementation provides both in-place modification and new instance creation options, with proper attribute preservation for the new mesh case.
206-212: LGTM: Effective hash caching implementation.The hash caching mechanism directly addresses the PR objective of reducing mesh computation time from 200+ seconds to 200 milliseconds. Cache invalidation on data modification is properly handled.
136-157: Ensure consistent dtype handling for vertices and resolve Sphinx documentation conflicts.
- Remove
dtype=np.float32in theverticessetter so it preserves the input’s precision and matches the__init__behavior:- self._vertices = np.array(value, dtype=np.float32).reshape(-1, 3) + self._vertices = np.array(value).reshape(-1, 3)- Retain
dtype=np.int32in theindicessetter to enforce a valid integer type for mesh indices.- Investigate the Sphinx build warnings about duplicate descriptions for
Mesh.verticesandMesh.indices. To eliminate conflicts, consider one of the following:
- Move both attribute descriptions into the class docstring under an Attributes section and remove the individual property docstrings.
- Annotate the property docstrings with
:noindex:(or similar) so Sphinx doesn’t generate duplicate entries.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
On the USD importing side (maybe as part of a different PR), does it make sense to choose mesh simplification based on what the asset specifies through |
That's a good point! I will look into this. I think we can do this in a separate PR. I've created an issue for this here: #409. |
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Description
ModelBuilder.simplify_meshes()which remeshes mesh shapes or computes convex decompositions using CoACDMesh.maxhullvertattribute when generating the convex hull of meshesWork in progress on supporting convex decomposition and other mesh simplification algorithms in
ModelBuilder.The following example shows how convex decomposition via CoACD allows the sphere to fall into the torus which is otherwise not possible since MuJoCo uses the convex hull of a mesh for collision handling:

Newton 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
Summary by CodeRabbit
New Features
Refactor