mapbox / mapbox-scenekit

Other
231 stars 51 forks source link

Height map doesn't load for certain bounding boxes. #41

Closed natalia-osa closed 5 years ago

natalia-osa commented 6 years ago

If you create the terrain node with following coordinates (not working example, there are many more such coordinates):

let terrainNode = TerrainNode(minLat: 54.434409000000002, maxLat: 54.563648999999998, minLon: -3.1655532000000002, maxLon: -3.0641508000000002)

The map appears flat and fetching the tiles returns error 404. Also, the drawn route doesn't fit into place, but that's the fault of invalid height.

Output from console:

2018-08-31 22:40:12.977944+0200 Examples[4666:2503897] Non-OK response from server: 404
2018-08-31 22:40:12.978579+0200 Examples[4666:2503897] Error downloading tile: 404
2018-08-31 22:40:12.978871+0200 Examples[4666:2503897] Couldn't get image for tile {12,2011,1304}
2018-08-31 22:40:13.247122+0200 Examples[4666:2503909] Non-OK response from server: 404
2018-08-31 22:40:13.247187+0200 Examples[4666:2503909] Error downloading tile: 404
2018-08-31 22:40:13.247375+0200 Examples[4666:2503909] Couldn't get image for tile {12,2011,1305}
2018-08-31 22:40:13.290805+0200 Examples[4666:2503921] Non-OK response from server: 404
2018-08-31 22:40:13.291009+0200 Examples[4666:2503921] Error downloading tile: 404
2018-08-31 22:40:13.291365+0200 Examples[4666:2503908] Couldn't get image for tile {12,2011,1306}
2018-08-31 22:40:13.291897+0200 Examples[4666:2503692] Terrain load complete
2018-08-31 22:40:13.334467+0200 Examples[4666:2503692] Texture load complete

I will look at the issue on Monday and try to fix it, but if you have any suggestions (like "there must not be imagery for that tile set at that zoom level" - @avi-c suggestion) please let me know to speed up the process. For us, this one is a deal breaker and I have a red flag to deploy our app till this one is fixed. Then we're ready to go.

avi-c commented 6 years ago

In this case, the issue is that there is no tile imagery at that zoom level. In order to protect against this you’d have to try the tile fetch and if it fails, back the zoom level out. Then repeat that until it works. Should be handled at the SDK level so we’ll deal with it there as soon as possible.

natalia-osa commented 6 years ago

@avi-c Hi Avi, I'm working on the fix for it. It comes in 2 parts, the first one is to simply provide errors from the web because currently they're removed after leaving the HTTP managing class. Usually, frameworks inform about such errors, so this part is useful anyway I guess. Above you can see a PR with appropriate changes.

The second part is still WIP and I need some theoretical help to be able to solve it. Basically, I've done this: https://github.com/OrdnanceSurvey/mapbox-scenekit/pull/1. I'm sure it isn't working as expected because the applied texture is of lower quality. I completely don't get it, because I thought this call is for fetching height map and call fetchTerrainTexture fetches the texture. Can you please give me some light over here?

Left: before changes, high resolution without height data Right: after changes, low resolution with height data (Please open the right image in a separate window to see how bad is it)

avi-c commented 6 years ago

I'm not totally sure what is happening with the texture seeming to be a worse quality. One issue I did see in the commented out code is that your retry logic is adding 1 to the zoom level when it tries the fetch again. You actually want to decrease the zoom level (zoomLevel - 1) and not increase. Increasing it adds detail and reducing it fetches lower res tiles.

natalia-osa commented 6 years ago

I did it intentionally, because I was afraid of a crash due to memory pressure with a bigger zoom. Also, due to the fact that it seems to be connected with image quality (???), I'd completely be afraid of decreasing, as it would cause to fetch more tiles, so opposite to the current situation.

Maybe you apply the images with the image bound position based on the heightmap? And due to the fact I'm fetching fewer points, the whole output is worse quality..?

avi-c commented 6 years ago

The visual issue you are seeing is related to you increasing the zoom level instead of dropping it. If you spin the terrain around, you will notice that the pixel rows alternate between having a height of 0 and an actual height. This gives the terrain a pixelated look when you view it from an angle. Visually, it appears to have half the vertical resolution that it should.

There is a related issue that happens if you decrease the zoom level for the heights where only part of the terrain will have heights.

The basic issue is the SDK isn't handling the case where the zoom level of the terrain heights doesn't match the zoom level of the terrain image texture. I'm looking into this now.

natalia-osa commented 6 years ago

That sounds like there is no easy workaround for this. Can you give me any estimations? Our marketing team was already working on the release, and that is the only issue stopping the release, so it's quite important for us.

avi-c commented 6 years ago

I'm hoping to have something soon.

I'll describe the approach I am taking now as a potential solution in case you want to pursue something similar in parallel.

I'm trying to make sure that the heightmap image that is used for all the z lookups is the same size as the texture image. To achieve that, I am scaling the heightmap image from whatever bounds it has come back as into a new UIImage with bounds that match the texture image bounds. Then I'll need to make sure that the z lookup finds the correct pixel of the heightmap image when doing the lookup.

avi-c commented 6 years ago

Finished the pull request and marking for review.

natalia-osa commented 5 years ago

Fixed by #39 and #45