🌐 Spherical coordinates: bug fix, docs and sanity checks #235

chapulina · 2021-09-03T00:58:32Z

🦟 Bug fix

Fixes gazebosim/gazebo-classic#2022
Part of gazebosim/gz-sim#981

Summary

I believe that gazebosim/gazebo-classic#2022 is only an issue with the *FromLocal* functions. While the heading rotation from GLOBAL to LOCAL seems to be correct, the rotation from LOCAL to GLOBAL was not the inverse of that, which resulted in incoherent behaviour.

In order to preserve behaviour on version 6, which is being used by a lot of people who compensate for the bug, I introduced a new frame, LOCAL2, which can be used to get the correct calculation.

I also added several tests with sanity checks (i.e. higher latitude means further North...).

It's my first time playing with this class, so I'd appreciate it if people paid close attention to the math and test cases, in case I'm misunderstanding something.

Checklist

Signed all commits for DCO
Added tests
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>

codecov · 2021-09-03T01:27:09Z

Codecov Report

Merging #235 (b447b94) into ign-math6 (0d884db) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b447b94 differs from pull request most recent head c64b173. Consider uploading reports for the commit c64b173 to get more accurate results

@@            Coverage Diff             @@
##           ign-math6     #235   +/-   ##
==========================================
  Coverage      99.39%   99.40%           
==========================================
  Files             66       66           
  Lines           6162     6185   +23     
==========================================
+ Hits            6125     6148   +23     
  Misses            37       37

Impacted Files	Coverage Δ
src/SphericalCoordinates.cc	`100.00% <100.00%> (ø)`

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 0d884db...c64b173. Read the comment docs.

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

scpeters · 2021-09-07T17:16:07Z

src/SphericalCoordinates_TEST.cc

+    EXPECT_GT(xyzOrigin.Y(), xyz.Y());
+  }
+
+  // Decrease longitude == go West == go -X (and a bit -Y)


it seems weird that a change to longitude in either direction causes Y to decrease slightly. I would have thought it would be linearized

I was also surprised to see this, I assumed it was an artifact of the projection and didn't look further into it.

Ok, so I spent some time looking into this and I think I understand what's happening. This image should help:

Note that the ENU plane is tangent to the ellipsoid.

What this test is doing is changing lambda while keeping phi fixed. As you do that, the projection of the point onto the plane will only move on a straight line if you're exactly on the equator. If you're on the northern hemisphere, it goes further North (+Y) and if you're in the southern hemisphere, like on the test, it goes further South (-Y).

Here's a handy way of visualizing it, see how the projection of the blue line is not linear:

https://demonstrations.wolfram.com/TangentPlaneToASphere/

src/SphericalCoordinates_TEST.cc

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

src/SphericalCoordinates_TEST.cc

mjcarroll

Maybe consider adding +/- altitude tests as well for completeness.

src/SphericalCoordinates_TEST.cc

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

chapulina · 2021-09-10T18:19:12Z

Maybe consider adding +/- altitude tests as well for completeness.

Done in a8933ca

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

* Migration guide and LOCAL2 Signed-off-by: Louise Poubel <louise@openrobotics.org> Co-authored-by: Carlos Agüero <caguero@osrfoundation.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org> Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>

peci1 · 2024-01-04T18:04:47Z

include/ignition/math/SphericalCoordinates.hh

+      /// There's a known bug with this computation that can't be fixed on
+      /// version 6 to avoid behaviour changes. Directly call


@scpeters The comments indicate that the behavior was only intended to be kept for gz-math6. However, it slipped through to gz-math7. Is there some will to straighten this up in 8, or will we live with this forever?

sure, we can fix it in gz-math8

can you open an issue or pull request to main?

Good, I'll try the PR.

I've noticed another discrepancy in the interface: SphericalFromLocalPosition returns spherical coordinates in degrees, while PositionTransform uses radians. Shouldn't this be also united to using degrees everywhere? Or was there a reason for keeping it this way?

I think consistency would be preferred; it was probably oversight in the first place that caused them to diverge. A std::vector<math::Angle> type would handle the units more clearly but it a little more overhead than Vector3d

Okay, I've sent the PRs: #596, #597.

chapulina added 2 commits September 2, 2021 17:17

Spherical coordinates: bug fix, docs and sanity checks

81b3353

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

Migration guide and LOCAL2

60b9785

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

chapulina added the bug Something isn't working label Sep 3, 2021

chapulina requested a review from caguero September 3, 2021 00:58

chapulina requested a review from scpeters as a code owner September 3, 2021 00:58

github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Sep 3, 2021

chapulina mentioned this pull request Sep 3, 2021

Support spherical coordinates gazebosim/gz-sim#981

Closed

codecheck

009cc49

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

chapulina mentioned this pull request Sep 3, 2021

Default SphericalCoordinates frame should be East-North-Up (ENU) gazebosim/gazebo-classic#2022

Open

chapulina mentioned this pull request Sep 3, 2021

🌐 Parse spherical coordinates gazebosim/sdformat#685

Merged

8 tasks

chapulina added 2 commits September 2, 2021 23:37

convert back

42dbfb5

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

fix warning

083216a

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

chapulina mentioned this pull request Sep 4, 2021

🌐 Spherical coordinates gazebosim/gz-sim#1008

Merged

11 tasks

scpeters reviewed Sep 7, 2021

View reviewed changes

src/SphericalCoordinates_TEST.cc Outdated Show resolved Hide resolved

src/SphericalCoordinates_TEST.cc Outdated Show resolved Hide resolved

chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021

chapulina added 2 commits September 9, 2021 18:05

more tests, explain Y change

c8465db

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

Merge branch 'ign-math6' into chapulina/6/spherical

c925cf0

mjcarroll reviewed Sep 10, 2021

View reviewed changes

src/SphericalCoordinates_TEST.cc Show resolved Hide resolved

mjcarroll reviewed Sep 10, 2021

View reviewed changes

src/SphericalCoordinates_TEST.cc Outdated Show resolved Hide resolved

Z tests

a8933ca

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

caguero mentioned this pull request Sep 11, 2021

Toward consistent local (Gazebo) and geographic (GPS) coordinates. osrf/vrx#358

Closed

chapulina and others added 3 commits September 14, 2021 19:46

Document radians vs deg

e16fce4

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

Merge branch 'ign-math6' into chapulina/6/spherical

0d74ad9

Merge branch 'ign-math6' into chapulina/6/spherical

c64b173

scpeters approved these changes Sep 16, 2021

View reviewed changes

scpeters merged commit 46d5324 into ign-math6 Sep 16, 2021

scpeters deleted the chapulina/6/spherical branch September 16, 2021 18:34

ViVittori mentioned this pull request Oct 27, 2021

Negative GPS coordinate uuvsimulator/uuv_simulator#390

Open

chapulina mentioned this pull request Dec 27, 2021

Bumps in garden: use ign-math7 gazebosim/sdformat#803

Merged

caguero mentioned this pull request Feb 7, 2022

Fix spherical coordinates FromLocal calls osrf/vrx#428

Merged

peci1 reviewed Jan 4, 2024

View reviewed changes

azeey mentioned this pull request Feb 28, 2024

Compile error in the World.cc of sdformat12-12.6.0 gazebosim/sdformat#1361

Closed

This was referenced May 26, 2024

Removed the workaround LOCAL2 coordinate system from SphericalCoordinates. #596

Closed

SphericalCoordinates: Added SPHERICAL_DEG and SPHERICAL_RAD, deprecated SPHERICAL #597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌐 Spherical coordinates: bug fix, docs and sanity checks #235

🌐 Spherical coordinates: bug fix, docs and sanity checks #235

chapulina commented Sep 3, 2021

codecov bot commented Sep 3, 2021 •

edited

Loading

scpeters Sep 7, 2021

chapulina Sep 7, 2021

chapulina Sep 10, 2021

mjcarroll left a comment

chapulina commented Sep 10, 2021

peci1 Jan 4, 2024

scpeters Jan 4, 2024

peci1 Jan 4, 2024

scpeters Jan 5, 2024

peci1 May 26, 2024

		/// There's a known bug with this computation that can't be fixed on
		/// version 6 to avoid behaviour changes. Directly call

🌐 Spherical coordinates: bug fix, docs and sanity checks #235

🌐 Spherical coordinates: bug fix, docs and sanity checks #235

Conversation

chapulina commented Sep 3, 2021

🦟 Bug fix

Summary

Checklist

codecov bot commented Sep 3, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjcarroll left a comment

Choose a reason for hiding this comment

chapulina commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 3, 2021 •

edited

Loading