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

TransectStyleComplexItem.cc: Fix bug generating empty surveys when no terrain data #10678

Conversation

Davidsastresas
Copy link
Member

@Davidsastresas Davidsastresas commented May 7, 2023

Selecting terrain data on surverys was generating an empty survey if the polygon was on an area where QGC doesn't have terrain height info about.

This is because QGC was waiting on terrain info to build the path on such surveys, which is not needed at all, because terrain frame just specifies an altitude above terrain, and relies on the vehicle for being able to calculate that height, by terrain database or by altitude sensors, having terrain height information is not needed at all to generate this kind of surveys.

I think this is heritage of a few time ago where QGC didn't support terrain frame for missions, and it was supporting only what we call now AltitudeModeCalcAboveTerrain, so this comprobation was needed.

I double checked what happens on this situation selecting AltitudeModeCalcAboveTerrain instead, and it works correctly, instead of uploading an empty survey it throws an error:

"Plan is waiting on terrain data from server for correct altitude values."

So I think everything should be working correctly now. In any case, I would appreciate if @DonLakeFlyer and @booo can double check this makes sense.

Thank you very much @AndKe for reporting this issue #10557. Please let me know if this fixes the issue for you.

… terrain data:

Selecting terrain data on surverys was generating an empty survey if the polygon
was on an area where QGC doesn't have terrain height info about.

This is because QGC was waiting on terrain info to build the path on such surveys,
which is not needed at all, because terrain frame just specifies an altitude above
terrain, and relies on the vehicle for being able to calculate that height, by terrain
database or by altitude sensors, having terrain height information is not needed at all
to generate this kind of surveys
@AndKe
Copy link
Contributor

AndKe commented May 8, 2023

@Davidsastresas - well done ! It works as expected - thank you.

@Davidsastresas Davidsastresas merged commit 72f10e5 into mavlink:master May 17, 2023
@Davidsastresas
Copy link
Member Author

3 users reported this commit to fix this issue for them ( included myself ). As I didn't see opposition to this I merged this one.

@AndKe
Copy link
Contributor

AndKe commented May 17, 2023

@Davidsastresas Thank you - this was a major issue for radar use with QGC.

@DonLakeFlyer
Copy link
Contributor

Sorry folks, been out of town for a while. I think the reason for terrain frame also waiting to query terrain data was so that the terrain graph would show correctly. Although the flight plan itself doesn't really need terrain data since the vehicle is in control with terrain frame the Plan UI needs it to display the terrain graphs doesn't it?

I haven't been in this code for a while, so I don't quite remember how all this works. With this change does the terrain graph work?

@AndKe
Copy link
Contributor

AndKe commented May 21, 2023

@DonLakeFlyer his change does not affect how the elevation profile is displayed (it's all at zero altitude when I checked) for two reasons:
The current DEM source does not include my latitude (69.6°) or where the relevant missions I had were >78°.

The elevation graph is kind of broken anyway because it's missing color coding of relative/ABS/terr/ etc.
I made a mockup of a color-coded suggestion but failed to find it now.
Flying a lot, I can tell that it's a higher value to clearly distinguish between altitude-types, than the altitudes - or to put it other way : takeoff to rel50m then flying to an abs150m waypoint can mean flying into the ground. (as the takeoff site may very well be at much higher altitude). Hence: before the graph is of much use (for those that have the DEM) - the altitude types are super-essencial.

As for DEM for the northern hemisphere around Norway - there are open/free DEM sources of very high quality for our latitude ..except there is no way for QGC to use them.

@DonLakeFlyer
Copy link
Contributor

I get it doesn't work for the area you are flying but it should work if there is terrain available.

I don't get your complaint about the current display not being valuable. There are basically to lines:

  • The flight path altitude (whatever frame you are using)
  • The terrain altitude

If those two lines cross the segment turns red to indicate you will hit terrain and the Plan won't be uploaded.

Maybe as you say a picture which show the problems would help me understand.

@AndKe
Copy link
Contributor

AndKe commented May 21, 2023

@DonLakeFlyer
Please see attached a mixed mode mission: (had to compress it to make github happy)

Bosleyfjellet.zip

it looks like:
Screenshot from 2023-05-21 22-28-56

having different color dots/lines for different terrain frames would be helpful.

@Davidsastresas Davidsastresas deleted the master-PR/Fix-offline-terrain-frame branch May 22, 2023 12:33
@Davidsastresas
Copy link
Member Author

Thank you for commenting @DonLakeFlyer . Indeed what you point out might be an issue that should be handled. I have noted it down to double check and fix whatever is necessary.

As far as I know, the terrain graph is cool and it works, might be in the area where @AndKe points out there is not height information? not sure, I will look at it at some point.

Thanks!

@AndKe
Copy link
Contributor

AndKe commented May 22, 2023

Yes, my bitching about terrain is kind of unfair, because I always thought of it as a local issue, but given that https://terrain.ardupilot.org provides coverage up to 84°N - I start to suspect a bug more than a real limitation of QGC.

I would absolutely love to see the terrain-graph working as intended. It would make it easier to validate a mission near mountainous terrain. (basically everywhere in norther Norway)

@DonLakeFlyer
Copy link
Contributor

Terrain data comes from Airmap. QGC can only work with what they have. At one point Auterion has talked about taking over the terrain server support and providing new data. Didn’t get anywhere though.

As far as showing terrain frame modes in the graph that would be nice additional information. But that isn't what the initial concept was for. The goal was to show whether you were going to fly into the ground or not. Which can be done without that level of display.

As you can see from @AndKe's example the terrain data is yellow and 0 which means unavailable. My worry is this pull causes that to happen also in areas which have terrain data available to it when you are using terrain frame.

@AndKe
Copy link
Contributor

AndKe commented May 22, 2023

Would't it make sense to get the data from https://terrain.ardupilot.org ? (or the same source)
Airmap's world view of 50°N to 50°S may be suffient for flat-earthers, but otherwise .... it's simply inadequate.
(comment based on https://developers.airmap.com/docs/elevation-api)

@DonLakeFlyer
Copy link
Contributor

"Would't it make sense..." Only if it didn't require rewriting the terrain server query code, which it does. So unless someone wants to write the code ...

@DonLakeFlyer
Copy link
Contributor

I don't remember exactly because I didn't write this code Gus Grubba did. But I think Airmap funded the development of it.

@AndKe
Copy link
Contributor

AndKe commented May 23, 2023

Who's involved in ArduPilot's terrain service? Maybe that person can tell if that server can serve the same kind of elevation data.
Also: maybe mapproxy together with terrain.ardupilot.com could translate the format.
Maybe this could be discussed at next Ardupilot devcall?

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented May 24, 2023 via email

@Davidsastresas
Copy link
Member Author

Davidsastresas commented May 28, 2023

Sorry folks, been out of town for a while. I think the reason for terrain frame also waiting to query terrain data was so that the terrain graph would show correctly. Although the flight plan itself doesn't really need terrain data since the vehicle is in control with terrain frame the Plan UI needs it to display the terrain graphs doesn't it?

I haven't been in this code for a while, so I don't quite remember how all this works. With this change does the terrain graph work?

I just tested this and it works as it should. If no terrain data available the graph is not drawn, and as soon as terrain data keeps coming the graph is shown perfectly, so I think we could forget about this.

Regarding the discussion about other elevation map providers, it is interesting and I think it has value. Maybe we should move the conversation elsewhere, to an issue?

I am thinking maybe there are other providers that could serve the terrain data similarly to how we are getting it now from airmap. Maybe some of those servers have nicer resolution, or resolution at all at extreme latitudes. It is worth checking, and if we find a similar service provider the implementation could be much more straighforward than adapting the whole tile server to Ardupilot data, which as @DonLakeFlyer said, was taken from a flight simulator a long time ago, I am sure we can get some providers with updated and nice data, although maybe they require subscription and api key, but it could be justified for some users.

I did a quick search and I found this:

https://docs.mapbox.com/data/tilesets/reference/mapbox-terrain-rgb-v1/

I think something like that, providing the altitude in rgb tiles could be doable, it seems it has good worldwide coverage. The current elevation is retrieved in .json format, as a "carpet" as they call it in Airmap documentation, so it is quite different, but we have already the part of the code to receive the image based tiles, although not for elevation of course. So maybe making something there to translate between the color and the elevation given the provided formulae could address this issue.

Anyway, lets open that conversation in an issue to see if more people is interested on it.

@AndKe
Copy link
Contributor

AndKe commented May 28, 2023

@Davidsastresas Done: #10702 :)

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