-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
copying the comment from #1380 for visibility
thoughts? @azeey |
Signed-off-by: Ian Chen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Another option is "optimization" |
"optimization" sounds good. |
checking to see if @scpeters if you're ok with the attribute name?
Do we need the |
no, we don't need |
There was a problem hiding this 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
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. |
Signed-off-by: Ian Chen <[email protected]>
…ulls property Signed-off-by: Ian Chen <[email protected]>
ok updated to replace
Updated mesh sdf spec with a new optional <mesh optimization="convex_decomposition">
<convex_decomposition>
<max_convex_hulls>16u</max_convex_hulls>
</convex_decomposition>
...
</mesh> This PR is ready for review again. |
There was a problem hiding this 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
Signed-off-by: Ian Chen <[email protected]>
thanks, applied suggested changes in f82be1f |
Signed-off-by: Steve Peters <[email protected]>
I pushed one more improvement to code coverage in 1b0736e |
There was a problem hiding this 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!
FYI I opened an out-of-scope feature request to specify |
thanks for the reviews! |
Signed-off-by: Ian Chen <[email protected]> Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
🎉 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.Also updated the Mesh DOM class to include this new attribute.
Checklist
codecheck
passed (See contributing)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.