Slippy Threaded - Alpha version

wilhelmberg commented 7 years ago

Hey @mapbox/games

could you do me a favor and check this unitypackage: SlippyThreaded-01.zip

It just contains the Slippy demo. Please leave all settings alone (not fully implemented yet) except those two:

^^ These settings are for PC only. In case you are exporting to a mobile player, reduce cached tiles and number of threads as you see fit. I think the number of cached tiles can stay pretty high tough as this is the raw binary vector tile data and under normal circumstances they ~~are~~ should be pretty small - only a few KB.

I'm primarily interested in

[ ] Does it run at all on your platform of choice (Linux/Mac)? 😏
[ ] Does it work on iOS, Android? When exporting to WebGL I get: Can't add component because class 'MeshCollider' doesn't exist
[ ] On Windows from within the Unity Editor I'm seeing a constant increase in memory usage the longer I pan around. This doesn't seem to be the case with the exported Windows .exe player - except the spikes when textures are created. Do you notice a similar memory usage?
[ ] Intermittently I also saw an error because IFileSource was null. Does this happen on your platform?
[ ] Intermittently Unity Editor crashes when stopping play mode. Does this happen on your platform?
[ ] How smooth is the panning?

david-rhodes commented 7 years ago

@BergWerkGIS your package is corrupt or something. Did you right click -> export package from Unity?

isiyu commented 7 years ago

Looks like it has to be unzipped on the command line, unzipping from finder unzips into a directory and unzipping on command line unzips to a .unitypackage

matt-2:Downloads matt$ unzip SlippyThreaded-01.zip 
Archive:  SlippyThreaded-01.zip
  inflating: SlippyThreaded-01.unitypackage

@BergWerkGIS but it does look like the SlippyThreaded-01.zip is missing the slippy scene and prefabs;

I tried loading the existing slippy scene but wasn't able to get it to run; seems like there are some new ThirdParty libraries as well? @BergWerkGIS Could you export a new package with your updates to the slippy scenes and prefab included?

wilhelmberg commented 7 years ago

Thanks for looking into it. Sorry - seems I created a faulty unitypackage and didn't double check.

Now a plain ZIP containing just the Mapbox folder. Extract it into the Assets folder of a new project and you should be good to go - open the Slippy scene. I tested on Linux and it worked nicely.

Download: SlippyThreaded.zip

david-rhodes commented 7 years ago

@BergWerkGIS First note: I had to change to .Net 2.0, rather that subset, just to compile for iOS. I think it's because of System.web floating around.

david-rhodes commented 7 years ago

@BergWerkGIS Took me about 3 minutes of scrolling to crash on iOS. Memory grew from about 290mb (starting) to 500+ in that time. Also, on iOS, I was getting some weird unresponsive behavior when trying to drag after completing a the previous drag. I don't know if this is because of our dragging code, or GC spikes from Resources.UnloadUnusedAssets(). In any case, we should definitely try to avoid using that call and disposing of those objects differently.

Does it work on iOS, Android? When exporting to WebGL I get: Can't add component because class 'MeshCollider' doesn't exist

You can fix this by instantiating a plane prefab instead of using primitives. I saw this on iOS as well.

This is more of a design problem that a technical challenge, but for this to be a good slippy map, we should be requesting tiles as the map moves, not just when the drag is finished. You can observe the problem by starting a drag at top of screen and going all the way down without releasing your finger. Also, as previously mentioned, there is a weird delay in dragging twice in a row, which makes it feel unresponsive. This could just be a bug, rather than a performance issues.

On a side note, if we make panning the map faster (more sensitive dragging), it will be easier to observe tile loading speed and such.

Memory as reported from Xcode:

wilhelmberg commented 7 years ago

@david-rhodes thanks for looking into this:

First note: I had to change to .Net 2.0, rather that subset, just to compile for iOS. I think it's because of System.web floating around.

Same on Windows when exporting to standalone player.

Took me about 3 minutes of scrolling to crash on iOS. Memory grew from about 290mb (starting) to 500+ in that time.

Ok, still more memory profiling necessary ☹️

Also, on iOS, I was getting some weird unresponsive behavior when trying to drag after completing a the previous drag. I don't know if this is because of our dragging code, or GC spikes from Resources.UnloadUnusedAssets(). In any case, we should definitely try to avoid using that call and disposing of those objects differently.

As far as I've been monitoring memory usage it increases heavily when the Texture2Ds are assigned the image data. Also, I've chosen a rather heavy test case (if you haven't changed the zoom level): It's set to zoom level 13 and 120 tiles are requested: 60 pngs and 60 vector tiles.

unresponsive behavior when trying to drag after completing a the previous drag My hunch is that's because of GC kicking in and at the same time creation of new tiles happens. That's a catch 22:

we could delay GC, but might run into memory issues earlier: existing tiles + new tiles would be in memory at the same time
we could delay tile fetching until Resources.UnloadUnusedAssets() is done, but that would create an even worse user experience.

I'm not happy with Resources.UnloadUnusedAssets() either and would happily not use it, but it was the only way I found to get the textures unloaded.

From my point of view I tried all the other methods known to me:

manually Destroy()ing all Unity objects
manually Dispose()ing and nulling each and every .Net object involved: eg byte arrays with tile data
creating deep copies of byte arrays just before applying them as textures, nulling them afterwards.

This is more of a design problem that a technical challenge, but for this to be a good slippy map, we should be requesting tiles as the map moves, not just when the drag is finished.

💯% agreed. Two reasons I changed it:

make it as simple for me as possible - still learning Unity 😅
Previously the tile fetching code was called on every Update() which used quite a lot CPU (at least on my machine)

wilhelmberg commented 7 years ago

Finally managed to export SlippyThreaded to Android.

I exported via Mono2 backend, not IL2CPP as I don't have the NDK yet.

On my devices (OnePlus One 3 & Samsung Galaxy Tab S2 8.0) SlippyThreaded didn't crash. Even after about ~7minutes of continuous panning.

Also, on iOS, I was getting some weird unresponsive behavior when trying to drag after completing a the previous drag. I don't know if this is because of our dragging code

I noticed that too, but it only happened when there was less than ~1sec between my touches. Even with all tiles already displayed and having waited long enough that GC should have done its job. When I waited about ~1 sec between my touches panning was smooth - even when tiles were continuously loaded/disposed.

So, part of the problem is likely to be caused by the current dragging code.

david-rhodes commented 7 years ago

@BergWerkGIS Ok, I think we ignore dragging code for now. I can do a pass on that logic once we integrate into SDK.

For the Resources.UnloadUnusedAssets() fix, I suggest we bring @parahunter in to help us solve this and the memory leaks. Alex, think you can jump in here tomorrow and help track down memory leaks/solve this resource unloading issue, please? I'd say this takes priority over any demo work.

parahunter commented 7 years ago

@BergWerkGIS Okay I have had a look at this but please bear in mind I'm not very familiar with threading and have only used it in one Unity project beforehand. Here's the ideas I have so far:

One is the use of UnityToolbag.Dispatcher.Invoke instead of InvokeAsync. This instantiates a lambda each time it is called https://github.com/nickgravelyn/UnityToolbag/tree/master/Dispatcher I tried to change the code to use InvokeAsync myself and it seemed to help a little bit.
On Slippy.cs line 319 a new SlippyTile is created for each new tile id. Problem is SlippyTile inherits from ScriptableObject and they are notorious for causing memory leaks when created at runtime since they exist outside of the scene so Unity won't do normal GC for them. The only reason you have for inheriting from ScriptableObject is if the object needs to be serialized in Unity as either part of the object hierarchy inside a MonoBehaviour, as a .asset file in the project, or inside another ScriptableObject. So looks like to me you can safely change the type of SlippyTile to be a normal object. It does look like you do the cleanup correctly by calling DestroyImmediate but in any case this is not the right place to use a ScriptableObject.
When I did profiling what went up for me was the ManagedHeap size in the profiler and not the allocation of of Texture2Ds. I'm not sure the problem is with Unity centric objects but is actually other objects allocated on the threads. That said I also think this could be an issue with creating a canvas for each tile since Unity's UI is known to not be very well optimized. I'll do a test tomorrow where I will disable the creation of any objects in the scene and still see if we have leaks.

I also think in order to really get a deeper understanding of this you will have to write a more fixed test case since from run to run depending on how quickly I would swipe I would get quite different results. Maybe this would be a keyboard shortcut that jumps to the same location on the map each time? I guess we need enough tiles to get loaded in order to see enough of a difference as well.

wilhelmberg commented 7 years ago

@parahunter Wow, that's a lot of valuable information packed into one comment - thanks.

@1: going to try that too @2: that's super helpful, going to change that @3: with Resources.UnloadUnusedAssets() I got rid of the Texture2D and realized that's the heap space now. Sorry forgot to reference: https://github.com/mapbox/mapbox-sdk-unity-core/issues/31#issuecomment-274507106

parahunter commented 7 years ago

Okay tried out not making the labels and still got the memory leak in the mono heap. I would look into all allocations made in the threading code and minimize or remove them. There's a fair amount of EventArgs being created which you could probably get rid off. I am not a fan of EventArgs anyways since you end up creating a wrapper object so you have more to GC.

I mentioned this in the first feedback on the SDK back at the start of January but the code base should move towards using object pooling. I would use this both for objects existing in the scene but also for other objects used by the threads for loading in the map to prevent allocations. If all the tiles are pooled you wouldn't have to call UnloadUnusedAssets for instance.

david-rhodes commented 7 years ago

@BergWerkGIS Just saw this: https://docs.unity3d.com/ScriptReference/Networking.UnityWebRequest.Dispose.html

You must call Dispose once you have finished using a [UnityWebRequest] object, regardless of whether the request succeeded or failed.

wilhelmberg commented 7 years ago

As mentioned in https://github.com/mapbox/mapbox-sdk-unity-core/pull/40#issuecomment-277254648 I couldn't spot any differences with/without Dispose(), but added it again.

wilhelmberg commented 7 years ago

Closing here as #40 is about to be merged.

To be continued #46

mapbox / mapbox-sdk-cs

Slippy Threaded - Alpha version #37

I'm primarily interested in