-
Notifications
You must be signed in to change notification settings - Fork 67
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
Added CoordinateVector3 and used it in SphericalCoordinates #616
Conversation
7079db9
to
7185850
Compare
Signed-off-by: Martin Pecka <[email protected]>
7185850
to
a3911c1
Compare
I'll be adding tests later (hopefully today). But please, comment on the proposed interface design. |
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.
Not done a super thorough review, but have a minor knit about throwing exceptions.
…ad of throwing exceptions. Signed-off-by: Martin Pecka <[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.
I've been through most of the code. This is looking pretty good. There is one minor knit I have about setting Nan
s without any logging.
With the introduction of gazebosim/gz-math#616. We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As part of this change, its probably a good idea to update `gz-msgs` as well. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Martin Pecka <[email protected]>
I've implemented what was requested in the last review. I also finished the test suite (with Ruby tests being more smoke tests than real tests). |
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.
Looks good to me. Thanks for iterating on this PR.
* Deprecate LOCAL2 in `SphericalCoordinates` With the introduction of gazebosim/gz-math#616. We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As part of this change, its probably a good idea to update `gz-msgs` as well. Signed-off-by: Arjo Chakravarty <[email protected]> * Fix deprecation warnings resulting from the deprecation of SphericalCoordinates enum field. Signed-off-by: Martin Pecka <[email protected]> --------- Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Martin Pecka <[email protected]> Co-authored-by: Arjo Chakravarty <[email protected]>
🎉 New feature
Summary
As discussed in #597 (comment) , it seems that gz-math would benefit from having a CoordinateVector3 class that explicitly holds either spherical or metric coordinates.
This PR merges the fixes from #596 and #597 - it deprecates LOCAL2 frame and provides an unambiguous interface regarding angular units.
Test it
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.