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

Support lunar coordinate transformations #377

Merged
merged 18 commits into from
Jul 21, 2022
Merged

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Jun 24, 2022

🎉 New feature

Summary

This PR adds a method to accept the spherical coordinates from the world, and calculate DEM sizes accordingly.
Logic is added to GeoReference() method to perform appropriate transformation for lunar surface.

Extending the same logic as lunar coordinate transformations, we can support transformations for custom surfaces as well.

Test it

Added a simple test to Dem_TEST.cc. The GeoReference method is private currently, and I'm not sure how to verify those coordinate transformations externally.

TODO :

  • I think the isNonEarthDem flag should be renamed to isUnknownDem or something, since we do support lunar dems now. Open to suggestions !

Depends on :

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 the 🌱 garden Ignition Garden label Jun 24, 2022
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.

Document new method

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #377 (ba01d65) into main (4136c0a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
+ Coverage   77.82%   77.85%   +0.03%     
==========================================
  Files          85       85              
  Lines       10776    10783       +7     
==========================================
+ Hits         8386     8395       +9     
+ Misses       2390     2388       -2     
Impacted Files Coverage Δ
geospatial/src/Dem.cc 92.37% <100.00%> (+1.17%) ⬆️

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 4136c0a...ba01d65. Read the comment docs.

@adityapande-1995 adityapande-1995 force-pushed the aditya/lunar_dem_load branch from de85ea3 to 5bb4c5b Compare July 6, 2022 08:59
@adityapande-1995 adityapande-1995 marked this pull request as ready for review July 6, 2022 09:02
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc 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.

Are the SetUnknownDEM and UknownDEM functions needed anymore? I believe with your gz-math and sdformat updates, these methods aren't necessary anymore and can be removed

Also, can you update the comments for both the GeoReference* functions, removing WGS84?

/// \brief Get the georeferenced coordinates (lat, long) of the terrain's
/// origin in WGS84.

geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved
geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved
geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved
geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
@adityapande-1995
Copy link
Contributor Author

Are the SetUnknownDEM and UknownDEM functions needed anymore? I believe with your gz-math and sdformat updates, these methods aren't necessary anymore and can be removed

Also, can you update the comments for both the GeoReference* functions, removing WGS84?

/// \brief Get the georeferenced coordinates (lat, long) of the terrain's
/// origin in WGS84.

Done 97b1c6d

Signed-off-by: Aditya <[email protected]>
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.

LGTM, nice work! One minor comment

geospatial/src/Dem_TEST.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

This looks good to me as well, thanks @jennuine for taking the lift on reviews here!

geospatial/src/Dem.cc Outdated Show resolved Hide resolved
Signed-off-by: Aditya <[email protected]>
@adityapande-1995 adityapande-1995 merged commit 17e2c6a into main Jul 21, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/lunar_dem_load branch July 21, 2022 20:34
luca-della-vedova pushed a commit that referenced this pull request Jul 27, 2022
* Expose method to set spherical coordinates, added transformation logic for moon and custom surfaces.

Signed-off-by: Aditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants