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

Add ellipsoid shape to sdf #434

Merged
merged 6 commits into from
Dec 29, 2020
Merged

Add ellipsoid shape to sdf #434

merged 6 commits into from
Dec 29, 2020

Conversation

brawner
Copy link
Collaborator

@brawner brawner commented Dec 11, 2020 •
edited
Loading

For the second part of #376 and following up #389, this PR adds the ellipsoid simple shape. It is defined by a 3-component vector of doubles for its radii.

This wasn't branched off from #389, but it will need to be rebased on top of it to address a couple of related changes.

Depends directly on gazebosim/gz-math#182

Signed-off-by: Stephen Brawner [email protected]

@brawner brawner requested review from azeey and scpeters December 11, 2020 01:59
@brawner brawner mentioned this pull request Dec 11, 2020
Closed
scpeters
scpeters reviewed Dec 14, 2020
View reviewed changes
include/sdf/Geometry.hh Outdated Show resolved Hide resolved
@azeey azeey added this to the SDFormat 1.8 / libsdformat11 milestone Dec 17, 2020
@chapulina chapulina mentioned this pull request Dec 17, 2020
Closed
@brawner brawner force-pushed the brawner/ellipsoid-simple-shape branch from a24644e to 6208c2c Compare December 18, 2020 00:04
@brawner
Copy link
Collaborator Author

brawner commented Dec 18, 2020

Rebasing onto master and resolving conflicts

scpeters added a commit to gazebo-tooling/gzdev that referenced this pull request Dec 19, 2020
@scpeters
sdformat11: use prereleases for now
f01563b 
gazebosim/sdformat#434 needs ignition-math 6.8.0~pre1

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters mentioned this pull request Dec 19, 2020
Merged
@scpeters
Copy link
Member

the following is needed for Ubuntu CI: gazebo-tooling/gzdev#26

j-rivero pushed a commit to gazebo-tooling/gzdev that referenced this pull request Dec 21, 2020
@scpeters
sdformat11: use prereleases for now (#26)
6a0e370 
gazebosim/sdformat#434 needs ignition-math 6.8.0~pre1

Signed-off-by: Steve Peters <[email protected]>
@brawner brawner force-pushed the brawner/ellipsoid-simple-shape branch from 6208c2c to 62bfcd8 Compare December 21, 2020 21:29
@scpeters
Copy link
Member

@osrf-jenkins run tests please

@scpeters
Copy link
Member

we should require version 6.8 of ignition-math6 in cmake/SearchForStuff.cmake

I guess we also aren't using gzdev in the GitHub workflow for this package, so it's not picking up the prereleases

azeey
azeey approved these changes Dec 23, 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 once CI is green

@brawner
Add ellipsoid shape to sdf
b1abc79 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
Set geometry enum to 8
a3ced20 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
Linter cleanup
fa181c5 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
Update ign-math dependency to 6.8
8939e3b 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner
Add ubuntu-prerelease for apt
5eb55d5 
Signed-off-by: Stephen Brawner <[email protected]>
chapulina
chapulina approved these changes Dec 29, 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 minor comments about documentation.

Because this is targeting master, I think it can get in using ign-math's pre-release, just like how other libraries are using some nightlies. We just need to make sure we make the stable ign-math 6.8 release before libSDFormat 11.0.0 is released.

include/sdf/Ellipsoid.hh Outdated Show resolved Hide resolved
include/sdf/Geometry.hh Outdated Show resolved Hide resolved
src/Ellipsoid.cc Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 29, 2020 •
edited
Loading

Codecov Report

Merging #434 (7e95873) into master (e0e09f9) will increase coverage by 0.02%.
The diff coverage is 91.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
+ Coverage   87.63%   87.65%   +0.02%     
==========================================
  Files          63       64       +1     
  Lines        9622     9681      +59     
==========================================
+ Hits         8432     8486      +54     
- Misses       1190     1195       +5
Impacted Files Coverage Δ
src/Geometry.cc 91.26% <84.61%> (-0.77%) ⬇️
src/Ellipsoid.cc 93.47% <93.47%> (ø)

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 e0e09f9...7e95873. Read the comment docs.

@brawner
PR Fixup
7e95873 
Signed-off-by: Stephen Brawner <[email protected]>
@brawner brawner force-pushed the brawner/ellipsoid-simple-shape branch from c223cc3 to 7e95873 Compare December 29, 2020 20:31
brawner
brawner commented Dec 29, 2020
View reviewed changes
Copy link
Collaborator Author

@brawner brawner left a comment

Choose a reason for hiding this comment

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

Rebased onto master and addressed pr feedback. The last build succeeded, so I'm pretty confident about this one.

include/sdf/Ellipsoid.hh Outdated Show resolved Hide resolved
include/sdf/Geometry.hh Outdated Show resolved Hide resolved
src/Ellipsoid.cc Outdated Show resolved Hide resolved
@brawner brawner merged commit f6e029b into master Dec 29, 2020
@brawner brawner deleted the brawner/ellipsoid-simple-shape branch December 29, 2020 22:02
@brawner
Copy link
Collaborator Author

brawner commented Dec 29, 2020

This added the prereleases in the github action workflow, which should be removed before a stable release is created.

@azeey azeey mentioned this pull request Jan 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters left review comments

@chapulina chapulina chapulina approved these changes

@azeey azeey azeey 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.
5 participants
@brawner @scpeters @codecov-io @azeey @chapulina