Added CoordinateVector3 and used it in SphericalCoordinates#616
Merged
arjo129 merged 4 commits intogazebosim:mainfrom Aug 20, 2024
Merged
Added CoordinateVector3 and used it in SphericalCoordinates#616arjo129 merged 4 commits intogazebosim:mainfrom
arjo129 merged 4 commits intogazebosim:mainfrom
Conversation
8 tasks
7079db9 to
7185850
Compare
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
7185850 to
a3911c1
Compare
8 tasks
Contributor
Author
|
I'll be adding tests later (hopefully today). But please, comment on the proposed interface design. |
arjo129
reviewed
Aug 16, 2024
…ad of throwing exceptions. Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
arjo129
requested changes
Aug 19, 2024
Contributor
arjo129
left a comment
There was a problem hiding this comment.
I've been through most of the code. This is looking pretty good. There is one minor knit I have about setting Nans without any logging.
arjo129
added a commit
to gazebosim/gz-msgs
that referenced
this pull request
Aug 19, 2024
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 <arjoc@intrinsic.ai>
8 tasks
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Contributor
Author
|
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). |
arjo129
approved these changes
Aug 20, 2024
Contributor
arjo129
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for iterating on this PR.
This was referenced Aug 21, 2024
Merged
azeey
pushed a commit
to gazebosim/gz-msgs
that referenced
this pull request
Aug 22, 2024
* 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 <arjoc@intrinsic.ai> * Fix deprecation warnings resulting from the deprecation of SphericalCoordinates enum field. Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz> --------- Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai> Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz> Co-authored-by: Arjo Chakravarty <arjoc@intrinsic.ai>
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎉 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
codecheckpassed (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-bymessages.