-
Notifications
You must be signed in to change notification settings - Fork 98
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
Adding cone primitives. #1418
Adding cone primitives. #1418
Conversation
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 you mind to add the Python binding ? or if you dont' have time just open an issue.
And additional note. Probably you should close all PR which are targeting harmonic, then we will backport them as required. @azeey. other thoughts on this ? |
Added all the pybind and a bunch of tests for everything as well: |
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 some small nits, but LGTM
8bea333
to
341727d
Compare
Can you merge from |
Squashing commits to make requested target of main with backports to harmonic. Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Steve Peters <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
341727d
to
07c1573
Compare
Done! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1418 +/- ##
==========================================
+ Coverage 92.45% 92.48% +0.03%
==========================================
Files 134 137 +3
Lines 17820 17954 +134
==========================================
+ Hits 16475 16605 +130
- Misses 1345 1349 +4 ☔ View full report in Codecov by Sentry. |
Has the issue with COM being different from the geometric center been addressed? |
Not sure, @scpeters any particular thoughts? |
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.
Has the issue with COM being different from the geometric center been addressed?
Not sure, @scpeters any particular thoughts?
apologies, I was working on it and then decided I wanted to implement gz sdf --print --expand-auto-inertials
in #1422 instead of manually computing a bunch of inertial values
I see two options for defining the origin of the cone shape for purposes of object placement in SDFormat (a different origin may be assumed in other parts of our API, but this is just focusing on SDFormat):
-
Shape origin at the center of the cone's bounding box (or bounding cylinder). In this case, placing a cone with length
L
with base down on a ground plane at Z=0 can be done by setting the initial Z pose of the cone toL/2
. This would allow the cone to be placed in the same way as a cylinder. (Example world file placing multiple unit shapes with the same initial Z poses in scpeters@3ad5655). -
Shape origin at the centroid of a cone with uniform density. For a uniform-density cone with length
L
, the centroid isL/4
above the base, orL/4
towards the base from the center of the bounding cylinder.
I prefer option 1 for the following reasons:
-
I think option 1 is more intuitive for placing geometry since it allows similar placement of a cone and cylinder with the same poses (it will place the base of a cone in the same location as a cylinder base).
-
While option 2 does not require specifying an
//inertial/pose
value when the code has uniform density, I think is not worth the trade-off for making geometry placement less intuitive by not following the pattern used by the cylinder, the most similar shape to a cone. This has little value for static shapes (which have no inertial properties) and for objects with non-uniform density (which will need the//inertial/pose
to be specified anyway).
Signed-off-by: Benjamin Perseghetti <[email protected]>
9e23185
to
ec0f10f
Compare
Signed-off-by: Steve Peters <[email protected]>
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.
sorry I missed updating one test
Co-authored-by: Steve Peters <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
No worries, thanks for helping! |
Co-authored-by: Steve Peters <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
* Backport: Add cone shape to SDFormat spec (#1418) Signed-off-by: Benjamin Perseghetti <[email protected]> Co-authored-by: Steve Peters <[email protected]>
* Backport: Add cone shape to SDFormat spec (gazebosim#1418) Signed-off-by: Benjamin Perseghetti <[email protected]> Co-authored-by: Steve Peters <[email protected]>
* Backport: Add cone shape to SDFormat spec (gazebosim#1418) Signed-off-by: Benjamin Perseghetti <[email protected]> Co-authored-by: Steve Peters <[email protected]>
🦟 Bug fix
Summary
This helps add the missing cone geometry for primitive/basic parametric shapes:
And is also valuable for visualizations of emitters/source that typically have conic-based spread as seen in this acoustic attack on an IMU by showing the affected area:
Associated PRs:
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.