DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets #597

Ryanf55 · 2024-04-20T18:44:42Z

🎉 New feature

Closes #596

Summary

Adds initial support to load super large DEM datasets by exposing an API to limit the size of the loaded DEM.
Adds an alternative way of loading datafiles that make use of features like vsizip and vsicurl that aren't true filepaths.
Adds a compiler warning for if you use new versions of GDAL, which will lose precision in the current implementation, and potentially misrepresent NoData values.
- This could be taken out, but could be an issue instead. A printed warning could be ok.
Add a few asserts for safety to catch coding errors earlier
The user-configurable size limits can be used to prevent segmentation faults by loading too big of a raster dataset
FIxed overflow and segfault in RasterIO for massive DEM's and overflow risk in vector resize operations

Test it

I added a unit test which is self explanatory.

./build/bin/UNIT_Dem_TEST

I also hacked a demo with one SRTMHGT file, and confirmed it can load. Gazebo does not perform well with this terrain size (3601 x 3601), but it does load. SDF needs ability to truncate the size of the heightmap in order to be usable, or perhaps decimate the data resolution.

Open Questions

How do we want the logs to look? Perhaps queue the errors and only report them if the DEM loading fails.
Do we need to solve user-configurable origin to load? The previous implementation loaded everything in the raster, so there was nothing to configure
- Not yet
Should we rely on ArduPilot's terrain server being up for the test, or generate a large in-memory dataset for testing large rasters?
- No, we've relocated a large VRT locally to avoid dependence on an external resource

Future Improvements

File an upstream GDAL bug about the segfault and ask for a recommendation how to prevent it: GDAL segfault loading massive VRT raster into std::vector OSGeo/gdal#9713
Expose an API to let users change which part of the large DEM is initially loaded
Expose an API to let users, after an inititial load, to re-load more terrain to handle a drone flying very far

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.

geospatial/include/gz/common/geospatial/Dem.hh

geospatial/src/Dem.cc

Ryanf55 · 2024-04-26T18:22:21Z

@ahcorde Would you be able to provide another round of feedback on the functionality of this PR. I'm looking for some direction on how to best contribute my time to Gazebo's support of terrain data. We can use Gazebo for outdoor 3D aerial robotics applications that the aerial working group is currently working on.

geospatial/src/Dem_TEST.cc

codecov · 2024-04-28T04:19:54Z

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.54%. Comparing base (2d06bf7) to head (7e8b197).
Report is 26 commits behind head on gz-common5.

Files	Patch %	Lines
geospatial/src/Dem.cc	90.90%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff               @@
##           gz-common5     #597      +/-   ##
==============================================
- Coverage       83.65%   80.54%   -3.11%     
==============================================
  Files              90       93       +3     
  Lines           10273    13617    +3344     
==============================================
+ Hits             8594    10968    +2374     
- Misses           1679     2649     +970

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

geospatial/src/Dem.cc

azeey

Looks great! Thanks for the contribution. I have a few comments, but they should all be easy to address.

geospatial/include/gz/common/geospatial/Dem.hh

azeey · 2024-05-21T02:47:23Z

geospatial/include/gz/common/geospatial/Dem.hh

@@ -134,6 +146,8 @@ namespace gz
                                 gz::math::Angle &_latitude,
                                 gz::math::Angle &_longitude) const;

+      private: [[nodiscard]] bool ConfigureLoadedSize();


Can this be placed in Dem::Implementation instead? Also can you document the function?

Yes, done! That made the code a lot cleaner too.

geospatial/src/Dem.cc

geospatial/include/gz/common/geospatial/Dem.hh

geospatial/src/Dem.cc

geospatial/src/Dem_TEST.cc

azeey · 2024-05-21T03:51:14Z

geospatial/src/Dem_TEST.cc

+  common::Dem dem;
+  auto const path = common::testing::TestFile("data", "ap_srtm1.vrt");
+  // We expect not to be able to allocate the data, so ensure we throw instead of segfault.
+  EXPECT_THROW(dem.Load(path), std::bad_alloc);


Is this actually causing Dem::Load to allocate more memory than available? I'm not sure if we want to do this in a unit test if it might badly affect other things running on CI.

Yes, it immediately throws on my machine, and then gtest catches it. I ran all the tests, and they seem reliable with it.
If you are concerned, what about a GTEST_SKIP so it's still compiled and able to be run manually if you desire?

CI is failing on macOS with a timeout so the behavior may not be the same on all platforms, so I'd prefer we use GTEST_SKIP as you suggested.

geospatial/src/Dem_TEST.cc

azeey · 2024-05-21T04:00:07Z

Also, please merge from gz-common5 to get the latest changes that fix CI failures.

azeey · 2024-07-29T20:53:46Z

@Ryanf55 we are now in Feature freeze for Gazebo Ionic. Since PR is already open, I've applied the beta label which means it's slotted for the Ionic release if we can get it merged before 08/28/2024. Do you think that's feasible? If not, please let me know and I'll remove the beta label.

Ryanf55 · 2024-07-29T20:56:46Z

Thanks for the heads up, I'll do my best to make time over the next 2 days.

Ryanf55

@azeey I've implemented all of your requests. This is ready for final approval, hopefully before the official release.

geospatial/include/gz/common/geospatial/Dem.hh

Ryanf55 · 2024-07-31T04:46:08Z

geospatial/include/gz/common/geospatial/Dem.hh

@@ -134,6 +146,8 @@ namespace gz
                                 gz::math::Angle &_latitude,
                                 gz::math::Angle &_longitude) const;

+      private: [[nodiscard]] bool ConfigureLoadedSize();


Yes, done! That made the code a lot cleaner too.

geospatial/src/Dem.cc

geospatial/src/Dem_TEST.cc

Ryanf55 · 2024-07-31T04:59:20Z

geospatial/src/Dem_TEST.cc

+  common::Dem dem;
+  auto const path = common::testing::TestFile("data", "ap_srtm1.vrt");
+  // We expect not to be able to allocate the data, so ensure we throw instead of segfault.
+  EXPECT_THROW(dem.Load(path), std::bad_alloc);


Yes, it immediately throws on my machine, and then gtest catches it. I ran all the tests, and they seem reliable with it.
If you are concerned, what about a GTEST_SKIP so it's still compiled and able to be run manually if you desire?

geospatial/src/Dem_TEST.cc

geospatial/src/Dem.cc

* Can now use GDAL vsicurl and GDAL vsizip features * There is a new API to set loaded size limits to prevent segfaults from loading too big of a raster terrain * For now, there is no configured way to change where in the large raster to load data from Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Ryan <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

* Add a VRT test file * Add a zipped VRT test file * Don't rely on internet for tests to pass Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Ryan <[email protected]>

* Enforce linter and naming conventions * Add missing getters for size limits and test them * Enforce 80char column limit * Move ConfigureLoadedSize to the implementation * Document all functions * Fix lambda name case Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 · 2024-07-31T05:07:33Z

gz-common5

Rebase complete.

azeey

LGTM with green CI!

Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Apr 20, 2024

Ryanf55 changed the title ~~Implement large vrt with vsicurl and vsizip support for loading more kinds of DEM data~~ DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets Apr 20, 2024

ahcorde reviewed Apr 22, 2024

View reviewed changes

geospatial/include/gz/common/geospatial/Dem.hh Outdated Show resolved Hide resolved

geospatial/src/Dem.cc Outdated Show resolved Hide resolved

Ryanf55 mentioned this pull request Apr 22, 2024

META - Ecosystem terrain standardization ROS-Aerial/community#65

Open

Ryanf55 marked this pull request as ready for review April 24, 2024 06:30

Ryanf55 requested a review from marcoag as a code owner April 24, 2024 06:30

azeey requested changes Apr 26, 2024

View reviewed changes

geospatial/src/Dem_TEST.cc Outdated Show resolved Hide resolved

Ryanf55 commented May 10, 2024

View reviewed changes

geospatial/src/Dem.cc Outdated Show resolved Hide resolved

Ryanf55 commented May 10, 2024

View reviewed changes

geospatial/src/Dem.cc Outdated Show resolved Hide resolved

Ryanf55 force-pushed the 596-implement-large-vrt-with-vsicurl-and-vsizip-support branch from c001ba1 to 9d02aba Compare May 10, 2024 18:06

azeey reviewed May 21, 2024

View reviewed changes

azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024

Ryanf55 commented Jul 31, 2024

View reviewed changes

Ryanf55 and others added 11 commits July 30, 2024 23:07

Improve comments

ef9d50f

Signed-off-by: Ryan Friedman <[email protected]>

Avoid overflow and add test

fad4e18

Signed-off-by: Ryan Friedman <[email protected]>

Variable naming

1512c4e

Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Ryan <[email protected]>

Whitespace

d39e578

Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Ryan <[email protected]>

Add better overflow protection

a395db7

Signed-off-by: Ryan Friedman <[email protected]>

Use correct types in assert

294b7ec

Signed-off-by: Ryan <[email protected]>

Remove warning about 64 bit datasets

2479a10

Signed-off-by: Ryan Friedman <[email protected]>

Convert to use a local VRT file

395721a

* Add a VRT test file * Add a zipped VRT test file * Don't rely on internet for tests to pass Signed-off-by: Ryan Friedman <[email protected]>

Remove bugged static assert

5352a04

Signed-off-by: Ryan Friedman <[email protected]>

Brace alignment

71bbd64

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Ryan <[email protected]>

Ryanf55 added 2 commits July 30, 2024 23:07

PR feedback

0397e4b

* Enforce linter and naming conventions * Add missing getters for size limits and test them * Enforce 80char column limit * Move ConfigureLoadedSize to the implementation * Document all functions * Fix lambda name case Signed-off-by: Ryan Friedman <[email protected]>

Replace reinterpret_cast with ToHandle

b722c57

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 force-pushed the 596-implement-large-vrt-with-vsicurl-and-vsizip-support branch from 5e03fc3 to b722c57 Compare July 31, 2024 05:07

azeey approved these changes Aug 6, 2024

View reviewed changes

Ryanf55 and others added 2 commits August 6, 2024 08:56

Skip test for throwing because it doesn't work on Mac

4cc0fa1

Signed-off-by: Ryan Friedman <[email protected]>

Fix GTEST_SKIP usage

1457fc9

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey merged commit 85b12c6 into gazebosim:gz-common5 Aug 6, 2024
12 checks passed

azeey mentioned this pull request Aug 13, 2024

Remove debug logging introduced accidentally in PR review #622

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets #597

DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets #597

Ryanf55 commented Apr 20, 2024 •

edited

Loading

Ryanf55 commented Apr 26, 2024

codecov bot commented Apr 28, 2024 •

edited

Loading

azeey left a comment

azeey May 21, 2024

Ryanf55 Jul 31, 2024

azeey May 21, 2024

Ryanf55 Jul 31, 2024

azeey Aug 6, 2024

azeey commented May 21, 2024

azeey commented Jul 29, 2024

Ryanf55 commented Jul 29, 2024

Ryanf55 left a comment

Ryanf55 Jul 31, 2024

Ryanf55 Jul 31, 2024

Ryanf55 commented Jul 31, 2024

azeey left a comment

DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets #597

DEM: Add support for GDAL vsicurl, vsizip support and avoid segfaults with huge VRT datasets #597

Conversation

Ryanf55 commented Apr 20, 2024 • edited Loading

🎉 New feature

Summary

Test it

Open Questions

Future Improvements

Checklist

Ryanf55 commented Apr 26, 2024

codecov bot commented Apr 28, 2024 • edited Loading

Codecov Report

azeey left a comment

Choose a reason for hiding this comment

azeey May 21, 2024

Choose a reason for hiding this comment

Ryanf55 Jul 31, 2024

Choose a reason for hiding this comment

azeey May 21, 2024

Choose a reason for hiding this comment

Ryanf55 Jul 31, 2024

Choose a reason for hiding this comment

azeey Aug 6, 2024

Choose a reason for hiding this comment

azeey commented May 21, 2024

azeey commented Jul 29, 2024

Ryanf55 commented Jul 29, 2024

Ryanf55 left a comment

Choose a reason for hiding this comment

Ryanf55 Jul 31, 2024

Choose a reason for hiding this comment

Ryanf55 Jul 31, 2024

Choose a reason for hiding this comment

Ryanf55 commented Jul 31, 2024

azeey left a comment

Choose a reason for hiding this comment

Ryanf55 commented Apr 20, 2024 •

edited

Loading

codecov bot commented Apr 28, 2024 •

edited

Loading