Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mesh optimization attribute to <mesh> #1382

Merged
merged 7 commits into from
Mar 21, 2024
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Mar 14, 2024

🎉 New feature

Replaces #1380

Implemented proposal in #68

Summary

Adds a new optional attribute called simplification optimization to the <mesh> sdf element. This allows users specify a method for simplifying the mesh, e.g.

<collision name="my_collision">
  <geometry>
    <mesh optimization="convex_decomposition">
      <uri>path_to_complex_mesh</uri>
    </mesh>
  </geometry>
</collision>

Also updated the Mesh DOM class to include this new attribute.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@iche033 iche033 requested review from azeey and scpeters as code owners March 14, 2024 01:36
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Mar 14, 2024
@iche033
Copy link
Contributor Author

iche033 commented Mar 14, 2024

copying the comment from #1380 for visibility

@scpeters:

I'm trying to decide if simplification is the right name for this attribute.

Alternatives (which might not be better):

//mesh/@transformation
//mesh/@convexity
with candidate values:

use_convex_hull
use_convex_decomposition

@iche033:

I think transformation is generic and would be another good candidate. So we don't limit this to just creating convex meshes

thoughts? @azeey

Signed-off-by: Ian Chen <[email protected]>
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.44%. Comparing base (b04828d) to head (1b0736e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
+ Coverage   92.42%   92.44%   +0.02%     
==========================================
  Files         134      134              
  Lines       17751    17812      +61     
==========================================
+ Hits        16406    16467      +61     
  Misses       1345     1345              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azeey
Copy link
Collaborator

azeey commented Mar 14, 2024

copying the comment from #1380 for visibility

@scpeters:

I'm trying to decide if simplification is the right name for this attribute.
Alternatives (which might not be better):
//mesh/@transformation
//mesh/@convexity
with candidate values:
use_convex_hull
use_convex_decomposition

@iche033:

I think transformation is generic and would be another good candidate. So we don't limit this to just creating convex meshes

thoughts? @azeey

transformation seems too generic. Would we including "scaling" for example?

Another option is "optimization"

@iche033
Copy link
Contributor Author

iche033 commented Mar 14, 2024

Another option is "optimization"

"optimization" sounds good.

@iche033
Copy link
Contributor Author

iche033 commented Mar 15, 2024

Another option is "optimization"

"optimization" sounds good.

checking to see if @scpeters if you're ok with the attribute name?

with candidate values:
use_convex_hull
use_convex_decomposition

Do we need the use_ prefix? I think it would only be needed if there's a case in which we plan to use a different verb?

@scpeters
Copy link
Member

Another option is "optimization"

"optimization" sounds good.

checking to see if @scpeters if you're ok with the attribute name?

//mesh/@optimization sounds great

with candidate values:
use_convex_hull
use_convex_decomposition

Do we need the use_ prefix? I think it would only be needed if there's a case in which we plan to use a different verb?

no, we don't need use_

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to expose the maxConvexHulls and voxelResolution parameters from gazebosim/gz-common#585 in the spec? if not, then perhaps we could read them using the gz: xml namespace in gz-physics

@iche033
Copy link
Contributor Author

iche033 commented Mar 15, 2024

do we want to expose the maxConvexHulls and voxelResolution parameters

I think the maxConvexHull param can be exposed as that's pretty generic. The voxel resolution on the other hand would only be applicable if the algorithm uses a voxel based approach for representing the mesh before decomposition. Another interesting library I found is CoACD that claims better results than V-HACD and uses 3D cutting plans for deciding how to split the mesh.

I think I could do some research first to see how we can generalize the params before adding them to the spec.

@iche033
Copy link
Contributor Author

iche033 commented Mar 15, 2024

ok updated to replace simplification with optimization. 75ca10e

think the maxConvexHull param can be exposed as that's pretty generic.

Updated mesh sdf spec with a new optional <convex_decomposition> element that exposes the max convex hull param. We can add other params later once we decide on them. b399bcd

<mesh optimization="convex_decomposition">
  <convex_decomposition>
    <max_convex_hulls>16u</max_convex_hulls>
  </convex_decomposition>
  ...
</mesh>

This PR is ready for review again.

@iche033 iche033 changed the title Add mesh simplification attribute to <mesh> Add mesh optimization attribute to <mesh> Mar 15, 2024
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few small suggestions to improve coverage

include/sdf/Mesh.hh Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Show resolved Hide resolved
src/Mesh_TEST.cc Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Outdated Show resolved Hide resolved
src/Mesh_TEST.cc Show resolved Hide resolved
Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Mar 20, 2024

just a few small suggestions to improve coverage

thanks, applied suggested changes in f82be1f

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member

I pushed one more improvement to code coverage in 1b0736e

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the patience with all my feedback, looks good!

@scpeters
Copy link
Member

FYI I opened an out-of-scope feature request to specify assert_convex for a mesh in #1384

@iche033
Copy link
Contributor Author

iche033 commented Mar 21, 2024

thanks for the reviews!

@iche033 iche033 merged commit 836dd42 into main Mar 21, 2024
10 checks passed
@iche033 iche033 deleted the mesh_simplification_15 branch March 21, 2024 00:41
aagrawal05 pushed a commit to aagrawal05/sdformat that referenced this pull request Apr 2, 2024

Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
iche033 added a commit that referenced this pull request Apr 8, 2024
iche033 added a commit that referenced this pull request Apr 10, 2024
aagrawal05 pushed a commit to aagrawal05/sdformat that referenced this pull request Aug 16, 2024
aagrawal05 pushed a commit to aagrawal05/sdformat that referenced this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants