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.

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

🌐 Parse spherical coordinates #685

Merged
merged 5 commits into from
Sep 10, 2021
Merged

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Sep 3, 2021

🎉 New feature

Part of gazebosim/gz-sim#981

Summary

Leverage ignition::math::SphericalCoordinates to hold the <spherical_coordinates> element.

Mismatches between the SDF spec and the ign-math class:

  • world_frame_orientation: that tag was never used by Gazebo (and maybe no other simulators either?). See discussion in Support spherical coordinates gz-sim#981 (comment). I updated the docs to mention only ENU is supported for now. The idea is that we update the docs as support for other conventions is added to math::SphericalCoordinates.
  • heading_deg: the docs say that the heading should be counter-clockwise, but according to the tests I added in 🌐 Spherical coordinates: bug fix, docs and sanity checks gz-math#235, I believe that's never been the case? It's also possible that I misunderstood something.

I should point out that ignition::math::SphericalCoordinates provides conversion and lookup functions that may not be as accurate or flexible as those provided by libraries such as proj. As far as SDFormat is concerned, that class is being used mainly as storage for the data, and downstream applications may choose to use the functions built into ign-math or use 3rd party applications to perform computation.

Test it

I'm working on an Ignition Gazebo PR that uses this, stay tuned! For now, look at the added tests 🙂

Used here: gazebosim/gz-sim#1008

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 3, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Sep 3, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Sep 3, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2021

Codecov Report

Merging #685 (c705e71) into main (f27e538) will decrease coverage by 0.05%.
The diff coverage is 76.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
- Coverage   87.99%   87.94%   -0.06%     
==========================================
  Files          73       73              
  Lines       11116    11166      +50     
==========================================
+ Hits         9782     9820      +38     
- Misses       1334     1346      +12     
Impacted Files Coverage Δ
src/World.cc 92.65% <76.00%> (-3.53%) ⬇️

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 f27e538...c705e71. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

All looks goods to me besides the <world_frame_orientation>. We're not doing anything with that parsed element but I think it's fine in case this changes in the future.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina
Copy link
Contributor Author

@scpeters , do you want to take a look at this before merging? I'm mainly concerned about the documentation changes.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
chapulina and others added 2 commits September 10, 2021 11:20
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Michael Carroll <michael@openrobotics.org>
@chapulina chapulina merged commit 9e5a18b into main Sep 10, 2021
Core development automation moved this from In review to Done Sep 10, 2021
@chapulina chapulina deleted the chapulina/12/spherical_coords branch September 10, 2021 21:19
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants