Skip to content

Added capsule and ellipsoid geom msgs#128

Merged
ahcorde merged 2 commits intomainfrom
ahcorde/add_geom/capsule_ellipsoid
Jan 26, 2021
Merged

Added capsule and ellipsoid geom msgs#128
ahcorde merged 2 commits intomainfrom
ahcorde/add_geom/capsule_ellipsoid

Conversation

@ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jan 25, 2021

Added capsule and ellipsoid msgs.

Not really sure If you should target main or ign-msgs7

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Jan 25, 2021
@ahcorde ahcorde requested a review from caguero as a code owner January 25, 2021 20:02
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jan 25, 2021
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #128 (6573212) into main (b581eb1) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   84.59%   84.78%   +0.18%     
==========================================
  Files           7        7              
  Lines         818      828      +10     
==========================================
+ Hits          692      702      +10     
  Misses        126      126              
Impacted Files Coverage Δ
src/Utility.cc 82.34% <100.00%> (+0.29%) ⬆️

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 b581eb1...6573212. Read the comment docs.

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.

Not really sure If you should target main or ign-msgs7

main is correct, adding fields breaks ABI.


LGTM, let's just make sure all fields are documented before merging.

Header header = 1;

double radius = 2;
double length = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document all fields? (We're doing this for new messages / fields)

/// \brief Optional header data
Header header = 1;

Vector3d radii = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document radii

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde merged commit 249f1ec into main Jan 26, 2021
@ahcorde ahcorde deleted the ahcorde/add_geom/capsule_ellipsoid branch January 26, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏢 edifice Ignition Edifice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants