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.

Sign up for GitHub

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

Jump to bottom

sdf 1.8: add capsule geometry type #389

Merged
merged 14 commits into from
Dec 15, 2020
Merged

sdf 1.8: add capsule geometry type #389

merged 14 commits into from
Dec 15, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Oct 13, 2020 •
edited
Loading

This adds a capsule geometry type to the SDFormat 1.8 spec. This shape is a cylinder with hemispheres capping each end. Due to the similarity to a cylinder, the same parameters length and radius are used. The Cylinder DOM class was copied and modified mainly with search and replace s/ylinder/apsule/ to support the new shape. This requires gazebosim/gz-math#163, so this will be a Draft PR until that is merged and released.

Part of #376.

Update: ignition-math 6.7.0 has been released, so this is now ready for review.

@scpeters
Add capsule type to sdf/1.8/geometry.sdf
e1846db 
Signed-off-by: Steve Peters <[email protected]>
@scpeters
Add Capsule DOM class based on Cylinder
febd668 
Copy Cylinder class and unit tests to create Capsule.
Add Capsule to Geometry enum, logic, and integration
test.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Require ignition-math6 6.7
a5d381c 
Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters added this to the SDFormat 1.8 / libsdformat11 milestone Oct 13, 2020
This was referenced Oct 13, 2020
Closed
Merged
@codecov-io
Copy link

codecov-io commented Nov 24, 2020 •
edited
Loading

Codecov Report

Merging #389 (d9f11c4) into master (28e16c8) will increase coverage by 0.07%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
+ Coverage   87.52%   87.60%   +0.07%     
==========================================
  Files          62       63       +1     
  Lines        9542     9616      +74     
==========================================
+ Hits         8352     8424      +72     
- Misses       1190     1192       +2
Impacted Files Coverage Δ
src/Geometry.cc 92.03% <84.61%> (-0.97%) ⬇️
src/Capsule.cc 96.61% <96.61%> (ø)
src/Cylinder.cc 96.66% <100.00%> (+3.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28e16c8...d9f11c4. Read the comment docs.

@scpeters scpeters marked this pull request as ready for review November 24, 2020 06:22
azeey
azeey requested changes Nov 24, 2020
View reviewed changes
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM. Just left some minor comments

include/sdf/Geometry.hh Show resolved Hide resolved
src/Capsule.cc Outdated Show resolved Hide resolved
src/Capsule.cc Outdated Show resolved Hide resolved
src/Capsule.cc Outdated Show resolved Hide resolved
test/sdf/shapes_world.sdf Show resolved Hide resolved
scpeters
scpeters commented Nov 25, 2020
View reviewed changes
include/sdf/Capsule.hh Outdated
public: Capsule(Capsule &&_capsule) noexcept;

/// \brief Destructor
public: virtual ~Capsule();
Copy link
Member Author

Choose a reason for hiding this comment

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

is there any point to having a virtual destructor? I copied it from the other classes, but would it be better without the virtual?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to leave a comment about that but didn't because the other classes use virtual destructors. IMO, these classes are not meant to be used polymorphically (they don't have any virtual functions), so there is no need for a virtual destructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

made non-virtual in 1c09156

chapulina
chapulina approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, I just have some minor comments and questions.

sdf/1.8/capsule_shape.sdf Show resolved Hide resolved
src/Capsule.cc Outdated Show resolved Hide resolved
src/Cylinder.cc Outdated Show resolved Hide resolved
@scpeters scpeters mentioned this pull request Dec 2, 2020
Open
@scpeters
Make ~Capsule non-virtual
1c09156 
Signed-off-by: Steve Peters <[email protected]>
@scpeters
Don't check for missing required elements
6169fec 
The parser will populate them with default values.

Signed-off-by: Steve Peters <[email protected]>
@brawner brawner mentioned this pull request Dec 11, 2020
Merged
@scpeters scpeters merged commit fc4dc8c into gazebosim:master Dec 15, 2020
@scpeters scpeters deleted the capsule branch December 15, 2020 05:23
@chapulina chapulina mentioned this pull request Dec 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
SDFormat 1.8 / libsdformat11
Development

Successfully merging this pull request may close these issues.
4 participants
@scpeters @codecov-io @azeey @chapulina