Skip to content
This repository has been archived by the owner on Nov 25, 2019. It is now read-only.

Threaded tile fetching 2nd iteration #46

Open
5 tasks
wilhelmberg opened this issue Feb 7, 2017 · 7 comments
Open
5 tasks

Threaded tile fetching 2nd iteration #46

wilhelmberg opened this issue Feb 7, 2017 · 7 comments
Assignees
Milestone

Comments

@wilhelmberg
Copy link
Contributor

refs #11 #37

Refactor multithreaded tile fetching after merge of #40 and standalone mapbox-sdk-cs:

@david-rhodes
Copy link
Contributor

@BergWerkGIS Sort of related: threaded deserialization/parsing of fetched data. I was watching this Unity optimization video and it is suggested to thread stuff like this. Not sure how applicable this is for us, given we do lazy deserialization, but it's worth thinking about.

@david-rhodes
Copy link
Contributor

@BergWerkGIS Not sure if this is the most recent ticket for threading, but here are some notes about: https://github.com/mapbox/mapbox-sdk-cs/tree/BergWerkGIS-multithread-filesource.

  • Works for iOS
  • Works for OSX
  • I don't like that the developers have to bring a prefab into their scene for this to work. I tried to make UnityDispatcher inherit from Singleton, but it's too late at that point and instantiation of the gameobject is attempted on another thread (not possible). Any ideas here?
  • Completely untested, but this actually seems slower than without threading? We should profile? That said, I don't have the most reliable internet connection.

@wilhelmberg
Copy link
Contributor Author

wilhelmberg commented Apr 25, 2017

  • Works for iOS

🎉

  • Works for OSX

🎉

  • I don't like that the developers have to bring a prefab into their scene for this to work. I tried to make UnityDispatcher inherit from Singleton, but it's too late at that point and instantiation of the gameobject is attempted on another thread (not possible). Any ideas here?

Prefab is gone now, https://github.com/PimDeWitte/UnityMainThreadDispatcher has been updated 5 days ago to not need a prefab anymore.

I just dragged the new UnityMainThreadDispatcher.cs onto Main Camera (don't know if this is the right place to put it, but it worked).
If you try it you have to update in mapbox-sdk-cs as well (update-sdk.sh) as UnityMainThreadDispatcher methods have changed a bit.

  • Completely untested, but this actually seems slower than without threading? We should profile? That said, I don't have the most reliable internet connection.

oh, really?
We should definitely profile.
Still on my todo: limit the number of parallel requests (depending on OS, less threads on devices).

Two ideas that come to mind why this implementation might be slower:

That said, I don't have the most reliable internet connection.

Maybe it's because all requests are fired almost at once and thus clog your connection?
Testing on my laptop with the StylingDemoMeshGeneration scene all satellite images pop up at once and then shortly after that all buildings.
Video: StylingDemoMeshGeneration.zip

Do you feel that it is slower on your laptop as well as on devices?
If it is just devices it might be running out of available threads.

Talking about this code fragment:

To work around the limitation that even the async HttpWebRequest.BeginGetResponse() might block for several minutes I'm kicking off a thread via actionWrapper.BeginInvoke() which then kicks off another thread via request.BeginGetResponse().

@wilhelmberg
Copy link
Contributor Author

@david-rhodes I think you were testing with the Slippy example, right?

I think there is another reason why this is slow.

Video: ThreadedSlippyTest.zip

Tiles (Texture2d) don't get removed when they are out of sight and worldRoot grows longer and longer.
If you follow the video you can see that tile fetching and display can keep up with scrolling until memory usage reaches ~570MB on my system.
I've tried this 6 times now and always got similar results.
I think depending on your system/device and available memory tile fetching might seem to be slow, but the real cause might be memory issues.

image

@wilhelmberg
Copy link
Contributor Author

wilhelmberg commented Apr 25, 2017

@david-rhodes latest changes here mapbox-unity-sdk/BergWerkGIS-multithread-filesource.

EDIT: don't forget to run update.sh again.

This includes latest UnityMainThreadDispatcher.cs:

  • no prefab needed
  • no manual attaching needed

It uses attribute [PostProcessScene] from namespace UnityEditor.Callbacks to automagically insert itself into the scene.

Unfortunately this seems to work with Editor only and not when building a player (tried with Windows Native, Android and Windows Store):

Assets/ThirdPartyAssets/UnityMainThreadDispatcher/UnityMainThreadDispatcher.cs(21,7): 
error CS0246: 
The type or namespace name `UnityEditor' could not be found. Are you missing an assembly reference?

Any ideas?
Maybe remove [PostProcessScene] and attach UnityMainThreadDispatcher.cs manually to every scene that needs it?
Or are you aware of another attribute that both works in Editor and exported players?

@david-rhodes
Copy link
Contributor

@BergWerkGIS

It uses attribute [PostProcessScene] from namespace UnityEditor.Callbacks to automagically insert itself into the scene.

Anything that references UnityEditor must be placed within an Editor folder. Unfortunately, this also means that these are unavailable when building for any platform. This doesn't sound like a solution that will work for us. That said, this isn't a blocker right now.

Tiles (Texture2d) don't get removed when they are out of sight and worldRoot grows longer and longer.

This is true, but on iOS, for example, the demo should crash well before there's a performance issue. Also, I've tested this specific example on device before (without threading) and it seemed faster. I will try to profile on iOS later today.

Do you feel that it is slower on your laptop as well as on devices?

Yes, seems slower on laptop as well.

@david-rhodes
Copy link
Contributor

david-rhodes commented Apr 25, 2017

@BergWerkGIS Just found this, but editor requests (outside of play mode) are broken with this implementation. This is because the previous HTTPRequest had the ability to execute with a Runnable.

Check GeocodeAttributeSearchWindow for reference.

Edit: one more reason to support using different IAsyncRequest implementations?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants