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

Fix UNIT_Dem_TEST failure on Ubuntu 20.04 #3275

Open
wants to merge 11 commits into
base: gazebo11
Choose a base branch
from

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Nov 8, 2022

Signed-off-by: Aditya aditya050995@gmail.com

🦟 Bug fix

Fixes ##2828

Summary

Differences in versions of libproj leads to different results in coordiante transformation. This PR proposes worksarounds for that.

Checklist

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

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

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review November 9, 2022 02:01
@adityapande-1995 adityapande-1995 marked this pull request as draft November 9, 2022 17:12
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review November 10, 2022 16:34
Comment on lines +47 to +50
public: double worldWidth = 0;

/// \brief Real height of the world in meters.
public: double worldHeight;
public: double worldHeight = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

= 0 isn't necessary

Comment on lines +183 to +194
// This DEM model is from the moon. Older versions
// of libproj will calculate the size assuming it
// is of the Earth, unless we specify the surface.
bool sizeSameAsEarth =
(std::abs(293.51089 - dem.GetWorldWidth()) < 0.1)
&& (std::abs(293.51068 - dem.GetWorldHeight()) < 0.1);
// Newer versions give invalid sizes, 0 in this case.
bool invalidSize =
(dem.GetWorldHeight() < 0.001) &&
(dem.GetWorldWidth() < 0.001);

EXPECT_TRUE(sizeSameAsEarth || invalidSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use the libproj macros? https://github.com/OSGeo/PROJ/blob/35e4207a9d9eb86b4e5188f00eae1f152306dad3/src/proj.h#L175-L177

or

https://github.com/OSGeo/PROJ/blob/35e4207a9d9eb86b4e5188f00eae1f152306dad3/src/proj.h#L188

eg.

#if PROJ_AT_LEAST_VERSION(<ubuntu 18.04 version>)
  EXPECT_FLOAT_EQ(293.51068, dem.GetWorldHeight());
  EXPECT_FLOAT_EQ(293.51089, dem.GetWorldWidth());
#else
  ...
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try that initially (82b92ca), but the CI was complaining that it couldn't find "proj.h". Worked for me locally though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that will be resolved if you add find_package(PROJ REQUIRED) then add ${PROJ_INCLUDE_DIRS} to include_directories, and ${PROJ_LIBRARIES} to target_link_libraries

https://github.com/OSGeo/PROJ/blob/65f60f6e27482920aa5007af5357d1f37b220b90/cmake/CMakeLists.txt#L14-L21

eg.,

Update:

if (HAVE_GDAL)
include_directories(${GDAL_INCLUDE_DIR})
endif()

To be:

if (HAVE_GDAL)
  include_directories(${GDAL_INCLUDE_DIR})
  if (PROJ_FOUND)
    include_directories(${PROJ_INCLUDE_DIRS})
  endif()
endif()

I'm not sure what version of PROJ is being used but it may need PROJ4 instead: https://github.com/OSGeo/PROJ/blob/65f60f6e27482920aa5007af5357d1f37b220b90/cmake/CMakeLists.txt#L51-L56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants