feat: replace Airmap terrain data with Copernicus #10740

leonardosimovic · 2023-07-20T17:31:00Z

The Problem

The Airmap terrain servers (and possibly other Airmap services) are shutting down. The docs https://developers.airmap.com/docs/elevation-api are already down while the servers seem still to be running for the time being.

The Solution

This PR replaces the Airmap query with an alternative provider, hosting the Copernicus GLO-30 which was compiled and provided by © Airbus Defence and Space GmbH dataset. The Copernicus dataset has generally better global coverage and accuracy than the previous SRTM dataset.

Followup or Incremental Work

Remove Airmap references in the code.
Give credit to © Airbus Defence and Space GmbH also in the UI.

Elevation Provider Credit

Added a small note in the bottom center of terrain plot:

Comparison

Latitude zone I: 0-50 degrees north and south

Airmap

Copernicus

Latitude zone II: 50-70 degrees north and south:

Airmap

Copernicus

Latitude zone III: 70-75 degrees north and south:

Airmap
Note: the SRTM dataset only covers land surface between 60° north and 56° south latitude

Copernicus

Latitude zone IV: 75-80 degrees north and south:

Airmap
Note: the SRTM dataset only covers land surface between 60° north and 56° south latitude

Copernicus

Latitude zone V: 80-90 degrees north and south:

Airmap

Note: the SRTM dataset only covers land surface between 60° north and 56° south latitude

Copernicus

Raw Images and Mission Plans

qgc-airmap-replacement.zip

src/QtLocationPlugin/QGCMapEngine.cpp

leonardosimovic · 2023-07-20T17:38:29Z

src/TerrainTile.cc

-        // Calc the elevation as the average across the four known points
-        double known00      = _data[latIndex][lonIndex];
-        double known01      = _data[latIndex][lonIndex+1];
-        double known10      = _data[latIndex+1][lonIndex];
-        double known11      = _data[latIndex+1][lonIndex+1];
-        double lonValue1    = known00 + ((known01 - known00) * lonFraction);
-        double lonValue2    = known10 + ((known11 - known10) * lonFraction);
-        double latValue     = lonValue1 + ((lonValue2 - lonValue1) * latFraction);


Dropped the bi-linear interpolation since it doesn't necessarily yield better results than NN. More advanced interpolation algorithms based on statistical methods, e.g. Kriging or Natural Neighbor, would be ideal.
Sources:

https://www.sciencedirect.com/science/article/pii/S1110982313000276

http://www.geocomputation.org/1999/082/gc_082.htm

Davidsastresas · 2023-07-24T14:21:02Z

Thanks @leonardosimovic

I took a quick look, and regarding the implementation I would only change all the references to Airmap to whatever we want to call this provider. Not 100% sure right now, but doing this also we can probably avoid bumping cache version right?

Regarding the bi-linear approximation or not, what is the gridSize of your tiles right now? The graphs on your comparisons look great, so probably we are good, but I think it would be good to specify somewhere the gridSize of your tiles so we can understand the error we are assuming not doing interpolation, or doing it.

I tested real quick your branch and it works great for me, but I am getting some errors when I create a polygon and then I move it. When creating a survey polygon everything is smooth, I only get the errors when click and drag vertices:

TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates Internal Error: missing elevation in tile cache
TerrainQueryLog: addPathQuery: signalling failure due to internal error
TerrainTileLog: 0x5580d4118738 Internal error: coordinate outside tile bounds: QGeoCoordinate(40.479999573, -3.3279448085)

I don't think I will be able to spend much more time this week on this matter, so if you could investigate the above it would be great.

Thank you very much!

leonardosimovic · 2023-07-25T13:13:08Z

Hey @Davidsastresas

Thank you for the feedback!

Not 100% sure right now, but doing this also we can probably avoid bumping cache version right

I would also prefer not upgrading the cache. However, it is necessary because the new data has a different format and therefore is not compatible. Previously, with Airmap, it was assumed that each carpet query would return a fixed array size n_lat x n_lon, regardless of the geolocation, and this has worked well. Our service accounts for converging longitudinal lines as we go to higher latitudes, hence the array size will vary in longitudinal direction.

Regarding the bi-linear approximation or not, what is the gridSize of your tiles right now?

It has the same resolution as Airmap i.e. 30 meters, hence I did not change the static variables or the line sampling method.

I tested real quick your branch and it works great for me, but I am getting some errors when I create a polygon and then I move it. When creating a survey polygon everything is smooth, I only get the errors when click and drag vertices:

I will have a look!
Edit: yup, can reproduce 👍🏼

Davidsastresas · 2023-07-25T23:39:46Z

Great to hear you can also reproduce those warnings!

Regarding Cache, I am not 100% sure of this, but isn't the cache system based on the hash for each tile, and isn't this hash dependant on the name? So, changing the name of the provider, shouldn't the tiles have a different hash and thus not having need to bump cache version? I am not 100% sure though.

Thanks!

leonardosimovic · 2023-07-26T06:01:28Z

Hey @Davidsastresas

I looked into the warnings some more and traced it back to rounding errors in the bounds fields for the request:
https://terrain-ce.suite.auterion.com/api/v1/carpet?points=40.42,-3.23,40.43,-3.22.
We will soon deploy an updated service, I will include a fix for this issue.

Regarding Cache, I am not 100% sure of this, but isn't the cache system based on the hash for each tile, and isn't this hash dependant on the name? So, changing the name of the provider, shouldn't the tiles have a different hash and thus not having need to bump cache version? I am not 100% sure though.

This sounds right, but I'm not 100% sure either. When I tested it the cache needed to be bumped otherwise still the perviously cached tiles were loaded 🤔

Edit
Of course! The provider key is still the same i.e. Airmap Elevation. This I wanted to address in an incremental PR anyway (removing all the airmap elevation references). However it makes sense to get it in all at once so we can avoid bumping the cache version.

leonardosimovic · 2023-07-26T08:29:32Z

@Davidsastresas

I pushed a couple of commits, addressing the Airmap hard-coding + some unrelated performance improvements. I tried to keep the changes to a minimum in order to reduce conflicts with your PR. Let me know what you think!

Also, I will let you know as soon as we have deployed our updated service.

Davidsastresas · 2023-07-26T13:03:47Z

Thank you, sounds great!

I won't have time to take a look at it until next week I think. Thanks!

DonLakeFlyer · 2023-08-05T20:07:07Z

Thanks so, so much for this work. I took a quick look through and it looks fine. But at this point @Davidsastresas knows way more about this than I do since he was recently in this code.

My one complaint is in the license string being part of the terrain status display. That takes up really (really) critical vertical real estate on small screens, like small tablets or say a Herelink. Can that go someplace else? Couple options:

Where the QGC version number is:

The help screen in Application settings:

leonardosimovic · 2023-08-07T12:24:15Z

Hey @DonLakeFlyer! No problem, we're happy to help :)

My one complaint is in the license string being part of the terrain status display. That takes up really (really) critical vertical real estate on small screens, like small tablets or say a Herelink. Can that go someplace else?

That's a valid point! My only concern is with the visibility of the copyright notice if we place it in the menus. Let me check with the team and I'll come back to you.

DonLakeFlyer · 2023-08-07T18:27:25Z

Is there some legal thing with respect to where it needs to be?

DonLakeFlyer · 2023-08-07T18:28:14Z

The other options is show it in terrain status for say a few seconds and then hide it.

Davidsastresas · 2023-08-07T19:44:11Z

Hi!, I think I will be able to check this tomorrow. Thanks for the work!

About the label, I agree with Don, it is really not good there, in devices like Herelink screen real state is precious. The idea about showing it for a few seconds I think is a good compromise. Something like 5 seconds will really be noticed. On the draft PR we were making for supporting several elevation providers we will have a dedicated option in general settings for it. It could be another nice place, in a way that the copyright only shows if we select this provider.

Also, It would be good to somehow link that license to the elevationmap provider, instead of being in QGCUrlmapengine as a global variable. Just thinking on the near future when we support more elevation providers. We can manage that later as well, I don't think that should hold back the PR.

Thanks!

Davidsastresas · 2023-08-10T13:29:24Z

I just spared some time to check this out. It looks good, but that error when creating a polygon and moving it around is still there:

addPathQuery: signalling failure due to internal error
TerrainTileLog: 0x559173b25078 Internal error: coordinate QGeoCoordinate(-33.380000755, 122.5906058) outside tile bounds
TerrainQueryLog: TerrainTileManager::getAltitudesForCoordinates Internal Error: missing elevation in tile cache

Some notes:

I think we could squash the first and second commits, as the second only fixes some stuff over TerrainTile, in order to keep the git tree more compact.
Regarding the 3rd commit, when you declare that kOnlineElevationProviderKey, I think we could do something there that leaves it nicer for future support for more elevation map providers. It could be added as a setting, so the name of the map provider is available globally by QGroundControl settings. Please see this commit to see how it was done on the work we were doing:

Davidsastresas@6b1817c

and then it could be accessed like this:

Davidsastresas@ed2dff4

For the copyright notice, besides where to place it, I think it would be good to move it to elevationmapprovider files, so that string can be retrieved from a mapprovider method. I think that way it could be nicer, more normalized. I really don't like to use global variables like that.

In any case, I understand you probably don't have a lot of time left to spend into this, and we are very grateful that you managed to spend the time in the first place. So maybe we can push this to a separate branch, not master, and rework it from there, if you can not put the time to fix the above. But it would be good to sort out that error before considering this.

Of course this is only my opinion, and if the rest of developers disagree and we decide to take a different approach I am totally happy with it too.

Thanks!

mrpollo · 2023-08-10T14:25:24Z

Hey @leonardosimovic thanks for your work we are excited to have this feature in. Is there a way we can get this finalized soon we would like to promote a stable release to deprecate Airmap and bring in a few other fixes

leonardosimovic · 2023-08-21T06:46:52Z

Hey everyone, sorry for the delay!

The other options is show it in terrain status for say a few seconds and then hide it.

I'm not sure about hiding the label after x seconds from a UX point of view. I don't think the label is super noticeable as it is now, but it's disappearing will cause the plot to resize / elements to shift, this will be noticed for sure by which time the label is already gone.

Other suggestion would be to move outside and on top of the plot. This won't increase terrain plot background size while the label remains visible.

On the draft PR we were making for supporting several elevation providers we will have a dedicated option in general settings for it. It could be another nice place, in a way that the copyright only shows if we select this provider.

This would be a very nice solution!

I just spared some time to check this out. It looks good, but that error when creating a polygon and moving it around is still there

Yeah we haven't yet deployed the fix to the terrain-service. I will let you know as soon as it's in.

Regarding the 3rd commit, when you declare that kOnlineElevationProviderKey, I think we could do something there that leaves it nicer for future support for more elevation map providers. It could be added as a setting, so the name of the map provider is available globally by QGroundControl settings.

That's definitely the way to go and you already implemented in your draft I believe. Hence I went with the bare minimum so it's a) less cumbersome to rebase for you and b) we can safe time by avoid duplicate work.

DonLakeFlyer · 2023-08-21T16:10:40Z

Other suggestion would be to move outside and on top of the plot.

I like that much better. If it's just text you can still see the map behind it and make sure it in the zorder below the plan items on the map.

leonardosimovic · 2023-08-25T13:41:39Z

Hey everyone, I cleaned up the commit history and moved the label ontop of the terrain plot:

It's not the prettiest solution but I hope it's acceptable for the time being, until there's a dedicated terrain settings section which @Davidsastresas proposed.

DonLakeFlyer · 2023-08-25T20:15:38Z

It's not the prettiest solution but I hope it's acceptable for the time being

I think it's good enough for now. @Davidsastresas Want to give one final look see an then you can merge.

Davidsastresas · 2023-08-28T14:36:43Z

@DonLakeFlyer as said on my previous comment, if the rest of you are fine with this I am fine myself too, so I am happy with this getting merged as well!

Thanks!

leonardosimovic · 2023-08-29T08:42:05Z

Awesome, thank you all for the reviews!

@Davidsastresas we have redeployed our terrain service, so you shouldn't see the error any more after clearing the map cache 😄

leonardosimovic commented Jul 20, 2023

View reviewed changes

leonardosimovic changed the title ~~[WIP] feat: replace Airmap terrain data with Copernicus~~ feat: replace Airmap terrain data with Copernicus Jul 21, 2023

leonardosimovic marked this pull request as ready for review July 21, 2023 06:43

leonardosimovic force-pushed the airmap-terrain-replacement branch from 6da7f0e to e42ebd5 Compare July 26, 2023 08:21

DonLakeFlyer self-requested a review August 5, 2023 20:00

DonLakeFlyer added the Release: v4.3 label Aug 10, 2023

leonardosimovic added 2 commits August 25, 2023 15:15

perf(MapEngine): avoid some unnecessary string copies

14f1810

feat: replace airmap terrain with copernicus

ab9913c

leonardosimovic force-pushed the airmap-terrain-replacement branch from e42ebd5 to fcc8873 Compare August 25, 2023 13:34

feat: give credit to elevation provider

534e33f

leonardosimovic force-pushed the airmap-terrain-replacement branch from fcc8873 to 534e33f Compare August 28, 2023 10:05

DonLakeFlyer merged commit f658b74 into mavlink:master Aug 28, 2023
6 checks passed

dayjaby mentioned this pull request Feb 13, 2024

Please add a decent DEM source (the current ends at 50°) #10702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace Airmap terrain data with Copernicus #10740

feat: replace Airmap terrain data with Copernicus #10740

leonardosimovic commented Jul 20, 2023 •

edited

Loading

leonardosimovic Jul 20, 2023 •

edited

Loading

Davidsastresas commented Jul 24, 2023 •

edited

Loading

leonardosimovic commented Jul 25, 2023 •

edited

Loading

Davidsastresas commented Jul 25, 2023

leonardosimovic commented Jul 26, 2023 •

edited

Loading

leonardosimovic commented Jul 26, 2023

Davidsastresas commented Jul 26, 2023

DonLakeFlyer commented Aug 5, 2023

leonardosimovic commented Aug 7, 2023

DonLakeFlyer commented Aug 7, 2023

DonLakeFlyer commented Aug 7, 2023

Davidsastresas commented Aug 7, 2023

Davidsastresas commented Aug 10, 2023

mrpollo commented Aug 10, 2023

leonardosimovic commented Aug 21, 2023 •

edited

Loading

DonLakeFlyer commented Aug 21, 2023

leonardosimovic commented Aug 25, 2023

DonLakeFlyer commented Aug 25, 2023

Davidsastresas commented Aug 28, 2023 •

edited

Loading

leonardosimovic commented Aug 29, 2023

feat: replace Airmap terrain data with Copernicus #10740

feat: replace Airmap terrain data with Copernicus #10740

Conversation

leonardosimovic commented Jul 20, 2023 • edited Loading

The Problem

The Solution

Followup or Incremental Work

Elevation Provider Credit

Comparison

Latitude zone I: 0-50 degrees north and south

Latitude zone II: 50-70 degrees north and south:

Latitude zone III: 70-75 degrees north and south:

Latitude zone IV: 75-80 degrees north and south:

Latitude zone V: 80-90 degrees north and south:

Raw Images and Mission Plans

leonardosimovic Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Davidsastresas commented Jul 24, 2023 • edited Loading

leonardosimovic commented Jul 25, 2023 • edited Loading

Davidsastresas commented Jul 25, 2023

leonardosimovic commented Jul 26, 2023 • edited Loading

leonardosimovic commented Jul 26, 2023

Davidsastresas commented Jul 26, 2023

DonLakeFlyer commented Aug 5, 2023

leonardosimovic commented Aug 7, 2023

DonLakeFlyer commented Aug 7, 2023

DonLakeFlyer commented Aug 7, 2023

Davidsastresas commented Aug 7, 2023

Davidsastresas commented Aug 10, 2023

mrpollo commented Aug 10, 2023

leonardosimovic commented Aug 21, 2023 • edited Loading

DonLakeFlyer commented Aug 21, 2023

leonardosimovic commented Aug 25, 2023

DonLakeFlyer commented Aug 25, 2023

Davidsastresas commented Aug 28, 2023 • edited Loading

leonardosimovic commented Aug 29, 2023

leonardosimovic commented Jul 20, 2023 •

edited

Loading

leonardosimovic Jul 20, 2023 •

edited

Loading

Davidsastresas commented Jul 24, 2023 •

edited

Loading

leonardosimovic commented Jul 25, 2023 •

edited

Loading

leonardosimovic commented Jul 26, 2023 •

edited

Loading

leonardosimovic commented Aug 21, 2023 •

edited

Loading

Davidsastresas commented Aug 28, 2023 •

edited

Loading