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

Adding cone primitives. #1418

Merged
merged 13 commits into from
May 24, 2024
Merged

Adding cone primitives. #1418

merged 13 commits into from
May 24, 2024

Conversation

bperseghetti
Copy link
Member

@bperseghetti bperseghetti commented May 15, 2024

🦟 Bug fix

Summary

This helps add the missing cone geometry for primitive/basic parametric shapes:

conetopple
cone

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:

drone_attack

Associated PRs:

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

sdf/1.11/cone_shape.sdf Outdated Show resolved Hide resolved
src/Geometry.cc Outdated Show resolved Hide resolved
@bperseghetti bperseghetti requested a review from azeey May 16, 2024 02:04
Copy link
Collaborator

@ahcorde ahcorde left a 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.

include/sdf/Cone.hh Outdated Show resolved Hide resolved
include/sdf/Cone.hh Outdated Show resolved Hide resolved
src/Cone.cc Show resolved Hide resolved
src/Cone.cc Outdated Show resolved Hide resolved
src/Cone.cc Outdated Show resolved Hide resolved
@ahcorde
Copy link
Collaborator

ahcorde commented May 16, 2024

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 ?

@bperseghetti
Copy link
Member Author

Do you mind to add the Python binding ? or if you dont' have time just open an issue.

Added all the pybind and a bunch of tests for everything as well:
d9154be

@bperseghetti bperseghetti requested a review from ahcorde May 17, 2024 05:23
@bperseghetti bperseghetti requested a review from scpeters May 18, 2024 19:06
Copy link
Collaborator

@ahcorde ahcorde left a 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

src/Cone.cc Show resolved Hide resolved
src/Cone_TEST.cc Outdated Show resolved Hide resolved
src/Geometry_TEST.cc Outdated Show resolved Hide resolved
@azeey
Copy link
Collaborator

azeey commented May 21, 2024

Can you merge from main to fix CI?

bperseghetti and others added 9 commits May 21, 2024 14:00
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]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti
Copy link
Member Author

Can you merge from main to fix CI?

Done!

Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 98.24561% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.48%. Comparing base (7af63cb) to head (07c1573).
Report is 16 commits behind head on main.

Current head 07c1573 differs from pull request most recent head 3cd4b3b

Please upload reports for the commit 3cd4b3b to get more accurate results.

Files Patch % Lines
python/src/sdf/pyCone.cc 94.11% 1 Missing ⚠️
python/src/sdf/pyGeometry.cc 75.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@azeey
Copy link
Collaborator

azeey commented May 21, 2024

Has the issue with COM being different from the geometric center been addressed?

@bperseghetti
Copy link
Member Author

Has the issue with COM being different from the geometric center been addressed?

Not sure, @scpeters any particular thoughts?

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.

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):

  1. 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 to L/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).

  2. Shape origin at the centroid of a cone with uniform density. For a uniform-density cone with length L, the centroid is L/4 above the base, or L/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).

test/sdf/basic_shapes.sdf Show resolved Hide resolved
test/sdf/shapes.sdf Show resolved Hide resolved
test/sdf/shapes_world.sdf Show resolved Hide resolved
src/Cone.cc Show resolved Hide resolved
Signed-off-by: Benjamin Perseghetti <[email protected]>
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.

sorry I missed updating one test

src/Geometry_TEST.cc Outdated Show resolved Hide resolved
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@bperseghetti
Copy link
Member Author

sorry I missed updating one test

No worries, thanks for helping!

src/Geometry_TEST.cc Outdated Show resolved Hide resolved
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
@scpeters scpeters merged commit e6adcd3 into gazebosim:main May 24, 2024
10 checks passed
bperseghetti added a commit to rudislabs/sdformat that referenced this pull request Jun 18, 2024
bperseghetti added a commit to rudislabs/sdformat that referenced this pull request Jun 19, 2024
bperseghetti added a commit to rudislabs/sdformat that referenced this pull request Jun 19, 2024
caguero pushed a commit that referenced this pull request Jun 20, 2024
* Backport:  Add cone shape to SDFormat spec (#1418)

Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@bperseghetti bperseghetti deleted the pr-cone-main branch June 21, 2024 01:19
aagrawal05 pushed a commit to aagrawal05/sdformat that referenced this pull request Aug 16, 2024
* Backport:  Add cone shape to SDFormat spec (gazebosim#1418)

Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
aagrawal05 pushed a commit to aagrawal05/sdformat that referenced this pull request Aug 16, 2024
* Backport:  Add cone shape to SDFormat spec (gazebosim#1418)

Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
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.

4 participants