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

Expose API to set non earth DEMs flag #353

Merged
merged 5 commits into from
May 17, 2022

Conversation

adityapande-1995
Copy link
Contributor

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

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

🎉 New feature

Summary

This PR exposes the isNonEarthDEM member so that it can be set before the Load function, if required.
Needed for this PR : gazebosim/gz-sim#1478

Test it

Added some lines to the existing Moon DEM load test case.

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 🏯 fortress Ignition Fortress label May 10, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/disable_GeoRef_console_prints branch May 10, 2022 00:59
@adityapande-1995 adityapande-1995 restored the aditya/disable_GeoRef_console_prints branch May 10, 2022 01:01
@adityapande-1995 adityapande-1995 changed the base branch from ign-common4 to main May 10, 2022 01:01
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #353 (e79008f) into main (d5f1c97) will not change coverage.
The diff coverage is n/a.

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

@@     Coverage Diff     @@
##   main   #353   +/-   ##
===========================
===========================

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 d5f1c97...600670b. Read the comment docs.

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.

test?

geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
Signed-off-by: Aditya <[email protected]>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review May 13, 2022 19:18
@adityapande-1995 adityapande-1995 added 🌱 garden Ignition Garden and removed 🏯 fortress Ignition Fortress labels May 13, 2022
@mjcarroll
Copy link
Contributor

Was there a decision to revisit this variable naming?

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented May 17, 2022

Was there a decision to revisit this variable naming?

That was for using this flag later in Physics.cc, based on an sdf parameter which I had originally planned to be <non_earth_dem>, but which was decided to be changed to <surface_model> I'll change that here : gazebosim/gz-sim#1478.

For this PR, in the GetGeoRef call, we compare the transformations with the WGS84 earth model and only when that fails we can set the isNonEarthDEM flag. We cannot know which celestial body it belongs to, just that it is not from the earth :

targetCs.SetWellKnownGeogCS("WGS84");
. So we can change the sdf param to expose what celestial body a DEM belongs to, but for this use case we can only use the information whether the DEM is from the Earth or not.

For e.g. in this test, the GetGeoRef call fails in Load and sets the isNonEarthDEM flag :

auto path = common::testing::TestFile("data", "dem_moon.tif");
EXPECT_EQ(dem.Load(path), 0);
EXPECT_EQ(33, static_cast<int>(dem.Height()));
EXPECT_EQ(33, static_cast<int>(dem.Width()));
EXPECT_FLOAT_EQ(-212.29616f, dem.MinElevation());
EXPECT_FLOAT_EQ(-205.44009f, dem.MaxElevation());
// unable to determne world width/height of non earth DEM
EXPECT_FLOAT_EQ(-1, dem.WorldHeight());
EXPECT_FLOAT_EQ(-1, dem.WorldWidth());
// unable to get coordinates in WGS84
ignition::math::Angle latitude, longitude;
EXPECT_FALSE(dem.GeoReferenceOrigin(latitude, longitude));
// The Load() method in Dem.cc should set the
// isNonEarthDEM flag.
EXPECT_TRUE(dem.GetNonEarthDEM());
, and we cannot know what exact celestial body it belongs to, at least for now.

Signed-off-by: Aditya <[email protected]>
@adityapande-1995 adityapande-1995 force-pushed the aditya/disable_GeoRef_console_prints branch from e79008f to 8bcc9d0 Compare May 17, 2022 13:25
@adityapande-1995 adityapande-1995 merged commit e93afaa into main May 17, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/disable_GeoRef_console_prints branch May 17, 2022 17:14
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.

4 participants