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

🌐 Spherical coordinates conversion and commands #177

Merged
merged 5 commits into from
Sep 16, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Sep 4, 2021

🎉 New feature

Part of gazebosim/gz-sim#981

Summary

  • Provides conversions between ign-math and ign-msgs for spherical coordinates.
  • Adds the possibility of spawning new entities at given spherical coordinates
  • Adds the possibility of moving an entity at a given spherical coordinate

Test it

See tests, and ign-gazebo PR: 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 <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 4, 2021
@codecov
Copy link

codecov bot commented Sep 4, 2021

Codecov Report

Merging #177 (6cc6783) into main (1275b5d) will decrease coverage by 0.22%.
The diff coverage is 77.77%.

❗ Current head 6cc6783 differs from pull request most recent head 9ca5628. Consider uploading reports for the commit 9ca5628 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   84.78%   84.56%   -0.23%     
==========================================
  Files           7        7              
  Lines         828      855      +27     
==========================================
+ Hits          702      723      +21     
- Misses        126      132       +6     
Impacted Files Coverage Δ
src/Utility.cc 82.14% <77.77%> (-0.20%) ⬇️

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 1275b5d...9ca5628. Read the comment docs.

@chapulina chapulina marked this pull request as ready for review September 8, 2021 14:00
{
out.SetSurface(math::SphericalCoordinates::EARTH_WGS84);
}
out.SetLatitudeReference(IGN_DTOR(_sc.latitude_deg()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to continue doing the conversion if the surface_model isn't supported? I think we shouldn't do it as the values will be probably incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ign-math class is populated with default values, so the caller wouldn't know if we're populating the message or leaving the default values. So instead of skipping the rest of the message, I'm printing a warning on c11ebb3.

For the messages, the field is left empty, and I also printed a warning, but still populated the rest of the function.

I think that ultimately, it shouldn't be the conversion function's responsibility to decide what are valid combinations. It should convert as much as possible and the caller should be the one deciding what is acceptable. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Practically speaking I don't see any problems right now because we only have one surface type in Ignition Msgs, so it's impossible to receive an unsupported type. I agree that potentially receiving a default math::SphericalCoordinates is ambiguous but returning a math::SphericalCoordinates with default surface type (WGS_84) but potentially lat/lon, heading and elevation references coming from a different surface type could be hard to debug. The error message definitely helps.

I'm approving but as an idea, we could create a new type in ignition::math::SurfaceType that is Unsupported or similar. We could use this type in this conversion for example, when we detect an unsupported type set in ignition::msgs::SphericalCoordinates. This way, after calling convert() we can programmatically check if the conversion was right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense. Ticketed gazebosim/gz-math#237.

{
_sc->set_surface_model(msgs::SphericalCoordinates::EARTH_WGS84);
}
_sc->set_latitude_deg(_m.LatitudeReference().Degree());
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my previous comment about unsuported surface model.

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

@caguero , I added some message changes in b64edb8

@chapulina chapulina changed the title 🌐 Spherical coordinates conversion 🌐 Spherical coordinates conversion and commands Sep 14, 2021
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.

2 participants