mapbox / mapbox-scenekit

Other
231 stars 51 forks source link

Dynamically calculate the zoom level for tile fetching. #39

Closed avi-c closed 6 years ago

avi-c commented 6 years ago

Dynamically calculate the tile zoom level for fetching tiles of a TerrainNode. Balance the detail with the data transfer amount and resulting CGImage size. Use 1MB as the limit for now.

natalia-osa commented 6 years ago

We want to release our app this week, and we're waiting for your changes (both this and updated route drawing). You said this should fix the issue but is WIP. Could you please explain why? I'm not sure whether to merge this branch as it is now to our fork master, because I need it as good as possible up to Friday. So a few questions:

1) do you think you'll finish this fix by Friday? 2) if not, do you think it's better to deploy the app with the current master branch or to merge this branch in our fork and release with this unfinished fix (master is crashing for us sometimes, so I'd rather merge it even if it's not fully tested but you expect it to be working better). 3) do you think you'll finish updated line drawing by Friday?

Kindly thank you for your response.

Also for the far future, will you upgrade it so that the images are loaded and removed based on the zoom of the model and visible part of the model? That will surely fix all of such issues, but it's quite a lot of work. Workaround like this in your PR is surely needed ASAP, so I'm eager to integrate it at least in our fork (asap ;-)).

natalia-osa commented 6 years ago

Our tester did some tests and even though in our fork I've included your code, the app is still crashing. I don't claim they're all coming from this exact source, but considering the fact it's crashing only with larger maps (what should be easy to test on your example just with bigger bounds) I suppose it's the same issue.

We've noticed also that if you open 3 small routes and the big one, it's crashing, while if you open just the big one it's fine, what can suggest that not everything is freed. If you put a general objc and general swift breakpoint, on dealloc the app is crashing, what is also indicating there may be a problem. Probably checking what's happening with Apple Analyse tool would help, but it also takes some time. It may be due to my drawing method, which is taking more memory than the one you've created. Do you have any rough estimation of merging it? I don't care about iOS 8 (but there probably will be people who will care) but we still support iOS 9.

I've noticed that sometimes it is crashing in Math.h latLng2tile, which you use in MapboxImageAPI.swift tiles:zoom:latBounds:lonBounds:tileSize: method.

So probably there are following issues: 1) the tiles sometimes do not appear, whole area is gray - if it happens, if you close this route and open any (eg 1km square, so very tiny one) other - it won't load as well. There is an output in the console "Error creating CGContext"
2) the module is crashing with larger bounding boxes. I couldn't reproduce it on your example, because i'm always getting issue 1 and the terrain is not drawing at all with larger maps.

I am now trying out how it works with the newest changes. Thank you for your hard work!

natalia-osa commented 6 years ago

Ok, so with the new change I didn't spot any crash yet, but all bigger routes are without texture (grey).

avi-c commented 6 years ago

can you give me a sample bounding region that produces this result? Any errors in the console?

natalia-osa commented 6 years ago

Sure!

let terrainNode = TerrainNode(minLat: 51.394747374316, maxLat: 52.8120267, minLon: -0.23018147712619, maxLon: 1.4424401)

and the console output is following (I wrote above the most important thing about unability to draw CGContext).

2018-08-31 19:58:18.480613+0200 Examples[4461:2414836] [DYMTLInitPlatform] platform initialization successful
2018-08-31 19:58:18.905698+0200 Examples[4461:2414620] Metal GPU Frame Capture Enabled
2018-08-31 19:58:18.906566+0200 Examples[4461:2414620] Metal API Validation Enabled
2018-08-31 19:58:19.172596+0200 Examples[4461:2414823] [INFO] {}[Database](283): recovered 630 frames from WAL file /private/var/mobile/Containers/Data/Application/C1D1E369-4BA8-4EF1-B349-7DCACA653B5B/Library/Caches/com.mapbox.scenekit.Examples/Cache.db-wal
2018-08-31 19:58:21.360378+0200 Examples[4461:2414620] Texture load complete
2018-08-31 19:58:33.491367+0200 Examples[4461:2414824] TIC Read Status [1:0x0]: 1:57
2018-08-31 19:58:33.491513+0200 Examples[4461:2414824] TIC Read Status [1:0x0]: 1:57
2018-08-31 19:59:00.012197+0200 Examples[4461:2414620] Status bar could not find cached time string image. Rendering in-process.
2018-08-31 19:59:01.876876+0200 Examples[4461:2414620] [MC] System group container for systemgroup.com.apple.configurationprofiles path is /private/var/containers/Shared/SystemGroup/systemgroup.com.apple.configurationprofiles
2018-08-31 19:59:01.878082+0200 Examples[4461:2414620] [MC] Reading from public effective user settings.
2018-08-31 19:59:07.494461+0200 Examples[4461:2415178] [Unknown process name] CGBitmapContextInfoCreate: unable to allocate 8558477312 bytes for bitmap data
2018-08-31 19:59:07.494921+0200 Examples[4461:2415178] Error creating CGContext
2018-08-31 19:59:12.840982+0200 Examples[4461:2414620] Terrain load complete
avi-c commented 6 years ago

@natalia-osa - Try it again now. I had not changed it to use the dynamic zoom level when fetching the terrain height image, so it was still using a fixed value of 14. This resulted in a image that was far too large for larger bounds like you are trying.

avi-c commented 6 years ago

Ooops. Wrong button pressed and I accidentally closed the issue. Reopened!

natalia-osa commented 6 years ago

Yes, now it's much better. The texture is drawn. But terrain heights are... completely flat? While I understand it's quite a flat terrain over there, it's exactly like a piece of paper, impossible... Do you suggest the Earth actually is flat ;-)? We actually had a lot of such issues around Lake District National Park and I thought it's a bug on my side, but it seems it's visible here as well. Height maps is a stopper for release for us.

I've waited for both logs:

Examples[4476:2429018] Texture load complete
Examples[4476:2429018] Terrain load complete

Attaching screenshot:

mbsk_flatearth

avi-c commented 6 years ago

@natalia-osa - When I put your bounds into the DemoARViewController sample, I also see it pretty flat. I need to boost the value returned from TerrainNode.heightFromImage() in order to see any reasonable terrain in relief with bounds that large. I know you proposed having a multiplier in there in another PR. Did you try turning that up? Screenshot attached: img_0026

natalia-osa commented 6 years ago

Hmm, weird. Your "flat" is normal. My "flat" is completely flat, look at the image, even the water is on the same level as forest. Let me debug it more.

avi-c commented 6 years ago

That screenshot is with the terrain height multiplied a lot just to show what it does.

natalia-osa commented 6 years ago

Ok, I've checked it on more variated terrain and looks perfect. Anyway, for that previous flat terrain, there is minimum wall height defined as 50, shouldn't everything be set to 50 (so we'd see that grey underneath) instead of just seeing it flat without anything below?

avi-c commented 6 years ago

Merging based on a review and testing by @natalia-osa