🌐 Spherical coordinates conversion and commands #177
Conversation
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| { | ||
| out.SetSurface(math::SphericalCoordinates::EARTH_WGS84); | ||
| } | ||
| out.SetLatitudeReference(IGN_DTOR(_sc.latitude_deg())); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yup, makes sense. Ticketed gazebosim/gz-math#237.
| { | ||
| _sc->set_surface_model(msgs::SphericalCoordinates::EARTH_WGS84); | ||
| } | ||
| _sc->set_latitude_deg(_m.LatitudeReference().Degree()); |
There was a problem hiding this comment.
See my previous comment about unsuported surface model.
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
chapulina
changed the title
🎉 New feature
Part of gazebosim/gz-sim#981
Summary
Test it
See tests, and
ign-gazeboPR: gazebosim/gz-sim#1008Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge
🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸