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

Compute distance between points on a general celestial body #434

Merged
merged 32 commits into from
Jul 5, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented May 27, 2022

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

🎉 New feature

Summary

The current SphericalCoordinates::Distance function uses hardcoded Earth's radius for its calculation. In order to compute distances between points for any non-Earth celestial body, we would need to consider its radius as well. This PR overloads the Distance() function to add an additional parameter, _radius.

Test it

Added a simple test case : Consider a hypothetical body with its radius half as that of the earth. The distance between 2 points on that body should be half as the distance between them on Earth.

Implementation variation

I could also implement radius as an optional parameter in the original SphericalCoordinates::Distance() function.

Another idea is to keep a small database of radii of common celestial bodies, so "MOON" should automatically get you a radius of 1.7374 * 10^6 m. This way it makes sense to keep overloads of the Distance method, we can have a third one with the signature Distance(Angle, Angle, Angle, Angle, string _celestialBody).

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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

@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels May 27, 2022
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #434 (b8c4887) into main (d15c9b2) will decrease coverage by 0.04%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
- Coverage   99.69%   99.65%   -0.05%     
==========================================
  Files          73       73              
  Lines        6567     6641      +74     
==========================================
+ Hits         6547     6618      +71     
- Misses         20       23       +3     
Impacted Files Coverage Δ
src/SphericalCoordinates.cc 99.05% <96.00%> (-0.95%) ⬇️

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 d15c9b2...b8c4887. Read the comment docs.

@adityapande-1995 adityapande-1995 changed the base branch from ign-math6 to main May 27, 2022 00:35
@adityapande-1995 adityapande-1995 changed the title Compute distance between points on a sphere Compute distance between points on a general celestial body May 27, 2022
include/gz/math/SphericalCoordinates.hh Outdated Show resolved Hide resolved
@adityapande-1995 adityapande-1995 marked this pull request as draft June 2, 2022 05:47
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Jun 2, 2022

Making changes to the SetType method would've required changes to a lot of methods. In the constructors, the SetElevation and UpdateTransformation methods expect the ellipsoid parameters like ellA to be set already. I thought overloading the constructor to force the user to supply radius, equatorial and polar axes was the easiest way to include custom surfaces.

Latitude and logitudinal references can be updated using the Set... helper functions, so I did not overload that constructor.

@adityapande-1995 adityapande-1995 marked this pull request as ready for review June 4, 2022 00:53
Signed-off-by: Aditya <[email protected]>
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I think we should deprecate the existing static Distance function and rename it to DistanceWGS84 or something like that. We could add the same function alias to ign-math6

include/gz/math/SphericalCoordinates.hh Outdated Show resolved Hide resolved
include/gz/math/SphericalCoordinates.hh Outdated Show resolved Hide resolved
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Initial pass, +1 to deprecate the existing static Distance() function

include/gz/math/SphericalCoordinates.hh Show resolved Hide resolved
src/SphericalCoordinates.cc Outdated Show resolved Hide resolved
src/SphericalCoordinates.cc Outdated Show resolved Hide resolved
src/SphericalCoordinates.cc Outdated Show resolved Hide resolved
src/SphericalCoordinates.cc Show resolved Hide resolved
@adityapande-1995
Copy link
Contributor Author

I messed up the commit history here. Fixing that.

chapulina and others added 3 commits June 22, 2022 01:21
Signed-off-by: methylDragon <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
* Suppress zero variadic macro args message
* Remove unused clang flag, suppress ruby warnings

Signed-off-by: Michael Carroll <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I saw that you added the Python interface, do you mind to add the tests too ? otherwise can you add an issue ?

@adityapande-1995
Copy link
Contributor Author

Ported test cases to python bindings, passing in CI, cc @ahcorde :

145: Test command: /usr/bin/python3.8 "/home/jenkins/workspace/ignition_math-ci-pr_any-ubuntu_auto-amd64/ign-math/src/python_pybind11/test/SphericalCoordinates_TEST.py"
145: Environment variables: 
145:  PYTHONPATH=/home/jenkins/workspace/ignition_math-ci-pr_any-ubuntu_auto-amd64/build/fake/install/lib/python/
145:  LD_LIBRARY_PATH=/home/jenkins/workspace/ignition_math-ci-pr_any-ubuntu_auto-amd64/build/fake/install/lib:
145: Test timeout computed to be: 1500
145: .Invalid coordinate type[7]
145: Unknown coordinate type[6]
145: Unknown coordinate type[7]
145: Unknown coordinate type[7]
145: .Unknown surface type[3]
145: ..SurfaceType string not recognized, EARTH_WGS84 returned by default
145: ..For custom surfaces, use SetSurface(type, radius,axisEquatorial, axisPolar, flattening)
145: ..Value of _axisEquatorial should be greater than zero  defaulting to Earth's equatorial radius.
145: Value of _axisPolar should be greater than zero  defaulting to Earth's polar radius.
145: Value of _flattening should be greater than  or equal to zero, defaulting to Earth's flattening value.
145: Value of _radius should be greater than zero  defaulting to Earth's radius.
145: ......
145: ----------------------------------------------------------------------
145: Ran 14 tests in 0.001s
145: 
145: OK
145: NEW POS[ 2.0   -4.0   -6379647.88 ]
145: 
145/170 Test #145: SphericalCoordinates_TEST.py ......................   Passed    0.08 sec

include/gz/math/SphericalCoordinates.hh Outdated Show resolved Hide resolved
include/gz/math/SphericalCoordinates.hh Outdated Show resolved Hide resolved
// https://en.wikipedia.org/wiki/Eccentricity_(mathematics)#Ellipses
this->dataPtr->ellP = sqrt(
std::pow(this->dataPtr->ellA, 2) / std::pow(this->dataPtr->ellB, 2) -
1.0);
Copy link
Member

Choose a reason for hiding this comment

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

nit: the ellE and ellP computations are repeated between this MOON_SCS case and the prior EARTH_WGS84 case. We could potentially move them outside of this switch block if the CUSTOM_SERVICE and default cases had a return instead of break, but this isn't a big deal, so it's not required

@adityapande-1995 adityapande-1995 dismissed stale reviews from chapulina and ahcorde July 5, 2022 16:54

Addressed changes, dismissing so that github allows merging

@adityapande-1995 adityapande-1995 merged commit 33a4827 into main Jul 5, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/distance_with_radius branch July 5, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants