Closed iBicha closed 6 years ago
Wow! This is a treasure trove of great feedback. Thank you so much for researching this so thoroughly. We will address many of these in the upcoming version.
You're very welcome! Thank you for releasing a really cool tool!
Also, CheckResponseForError
is doing parsing on the main thread, which is causing a huge spike (with over 20000 GC.Alloc
calls)
Just to keep track of things, let me enumerate the items:
Yes, I should have posted in a similar format. These are the most important ones as far as I remember.
Also, there's the Json decoding that shouldn't be done on the main thread (CheckResponseForError
) because Newtonsoft's really expensive.
Ok, moved the decoding/parsing of the response to the background thread too.
I wanted to share my experience with PolyToolKit integration in an ARCore app.
To be honest PolyToolkit added a lot of overhead, due to the number of MonoBehaviours it adds, and also a lot of the importing is done on the main thread (even with
clientThrottledMainThread
, and limiting models complexity to medium), which can make the frame freeze for a few seconds, which can totally mess up tracking on ARCore.Other than this I discovered a noticeable, and reproducible lag, caused by this which is reading the encoding from the request and encoding the whole buffer (causes a huge spike in the profiler)
A call to
webRequest.downloadHandler.text
is doing all thisAlso the
webRequest.downloadHandler.data
getter is a fresh copy of the data with each call (so this second call is redundant, and this copy is not needed)Also, creating a texture with a specific size before loading the image does not preserve the size. It overrides whatever's in the texture and creates a new one (so one could either create a really small texture, or use the texture download handler, etc). This was also a pain point, because these textures are loaded on the main thread, and usually we load a bunch of them, so even with throttling with coroutines, the frame rate still drops, because the thumbnails are simply too big. After some digging, I discovered that the web server for the thumbnail can resize images if the thumbnail ends with
=s<SIZE>
(for example, I append a=s150
to the thumbnail url before fetching the texture, which worked as expected, returning an image with width and height no bigger than 150) This is not documented, and might break in the future, so it would be nice to include it as an option in the Thumbnail Fetcher API.There are few optimisations that can be done here and there (and I probably forgot to mention more than half of the edits made internally), but overall, in a project with many libraries and SDKs, PolyToolkit was the biggest overhead, and required a lot of optimisations to get anywhere decent performance.
Obviously no contributor can do anything about this, because you guys don't accept PRs, and I hate keeping fixes in my own fork/version, because I will have to maintain that when the main repo gets an update.
My suggestion is a serious benchmark of the toolkit (in a restricted environment such as mobile AR/VR), optimising as much as possible, since the main target would be mobile AR and VR, which needs to be efficient. Taking advantage of the new Job System in Unity can be an answer to few issues in this repo, and definitely reducing the number of MonoBehaviours needed (I'd say one is enough, the rest of the components can be regular classes)
Cheers