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 <voxel_resolution> SDF element to <convex_decomposition> #1403

Merged
merged 7 commits into from
May 28, 2024

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 16, 2024

🎉 New feature

Summary

Adds a new <voxel_resolution> parameter. This is only applicable to voxel based convex decomposition methods.

See below for alternative proposals that I have considered. I'm open to suggestions.

Alternatives considered

  1. Use a more generic parameter <resolution> instead of <voxel_resolution> so that it's applicable to a wider range of convex decomposition methods. e.g.
<convex_decomposition>
   <resolution>100000</resolution>
</convex_decomposition>

One example of non-voxel based method is the CoACD method which offers a couple resolution configuration params (resolution and manifold preprocess resolution).

  1. Have specific SDF elements for each different method / library (similar to how there is <bullet>, <ode>, and <dart> specific params):
<convex_decomposition>
  <VHACD>
    <voxel_resolution>200000</voxel_resolution>
  <VHACD>
  <some_other_method>
    <another_param>50</another_param>
  <some_other_method>
</convex_decomposition>

This PR chose an approach that's kind of half way between the 2 above alternatives. I added the <voxel_resolution> SDF element to be more explicit in limiting this element to voxel based methods but not overly specific to a particular convex decomposition library.

One option for extending the spec in the future would be:

<!-- `method` can be VHACD, CoACD, etc and this determines the corresponding resolution param to use --> 
<convex_decomposition method="VHACD>
  <voxel_resolution>200000</voxel_resolution>
  <manifold_resolution>50</manifold_resolution>
</convex_decomposition>

Test it

Run the tests: UNIT_Mesh_TEST and INTEGRATION_geometry_dom

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.

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Apr 16, 2024
@@ -12,6 +12,9 @@
<element name="max_convex_hulls" type="unsigned int" default="16" required="0">
<description>Maximum number of convex hulls to decompose into. This sets the maximum number of submeshes that the final decomposed mesh will contain.</description>
</element>
<element name="voxel_resolution" type="unsigned int" default="200000" required="0">
<description>Voxel resolution to use for representing the mesh before decomposition. Applicable only to voxel based convex decomposition methods.</description>
Copy link
Member

Choose a reason for hiding this comment

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

what are the units of the voxel resolution parameter? is it a volumetric density or a number of elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated description to say that resolution is the number of voxels. 5416ba5

@ahcorde ahcorde merged commit e2e72ae into main May 28, 2024
10 checks passed
@ahcorde ahcorde deleted the voxel_resolution branch May 28, 2024 08:57
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