Make `IPrepareRendererResources::prepareInLoadThread` asynchronous, return a Future
This is a PR into #554.
Previously, prepareInLoadThread was invoked in the load thread (as is hopefully obvious from the name), but it had no option to do any truly asynchronous work. Once the function returned, the load-thread part of the process was considered complete.
This PR changes the paradigm so that prepareInLoadThread now returns a Future, and the load-thread part of the process isn't complete until the Future resolves. This allows additional network requests, work in other threads, etc. without requiring the load thread to block while those other operations are in progress. For example, it could be used to dispatch part of the work to a render thread, thread pool, or game thread.
Thanks @kring, this looks good to me, I'll merge this and #554 once we get the comment in the other PR resolved!
@kring Since some of the fixes for issues discussed in #554 are committed here, feel free to self-merge this branch into that one. I'm happy with the future based loading changes, so up to you if you want to just merge it into the other PR to be reviewed altogether when it's done - whatever is more convenient for you!
@nithinp7 I'm going to merge this into the other PR as you suggested, but it may be useful to see what's new here since you last looked. The diff is here: https://github.com/CesiumGS/cesium-native/compare/cc75a31...ffaf65b
Noteworthy changes:
- Implemented the "async destruction complete event" for RasterOverlay. It's a little different from the Tileset one in that the user must pass an AsyncSystem (because RasterOverlay doesn't have one itself). It asserts that equivalent AsyncSystems are provided in every call.
- Added
operator==andoperator!=for AsyncSystem, so the above equivalence can be determined. - Changed
RasterOverlay::createTileProviderto return errors in its Future, rather than reporting them and returning nullptr. This allowed some code to be deleted from every raster overlay type. Load errors are reported byRasterOverlayCollectioninstead of by eachRasterOverlay. - Added a new third-party library.
expected-lite, which is used for the above.expected-liteis an implementation ofstd::expectedwhich will be available in future C++ standards (23 I think?). It's also roughly an implementation of the Either monad from Haskell 🤓 and a nice way for a single return value to contain one set of data on success and another on failure. expected-lite is already used in at least one internal Cesium project. - Removed
pOverlayfromRasterOverlayLoadFailureDetails. - Fixed a couple of cases (
DebugColorizeTilesRasterOverlayandRasterizedPolygonsOverlay) where an overlay was creating its tile provider withthisas its parent overlay, rather than the passed-inpOwner. - Added debug code to
ReferenceCountedNoThreadSafeto assert that it's not used from multiple threads, because that would be unsafe.
Funny story: after all the above, I was still sometimes seeing deadlocks at Tileset or RasterOverlay destruction. I poured through code looking for the reference counting error or race condition, but found nothing. I tried adding some code to help trap the condition, but it was a Heisenbug.
Eventually I got to the point where I couldn't reproduce it at all anymore and had nearly convinced myself I had imagined it, when I started doing some other stuff and it reappeared. I finally realized the cause: while waiting in IsReadyForFinishDestroy, I was dispatching main thread tasks, but I wasn't ticking the asset accessor. So, if an HTTP request was in flight when the Cesium3DTileset destruction was started, that request would never finish, and the Tileset could never be destroyed. But if all the requests were already cached, no asset accessor ticking was necessary in order for the requests to be fulfilled, and so the bug didn't appear. Basically, the more consistent I was about the steps I used to try to reproduce the bug, the less likely it was that the bug would appear. 😆 The good news: it was an easy fix once I realized what was happening. I'm just embarrassed that it took so long!
Sounds good I'll review it there! Also didn't think I'd ever hear the word monad again 😂
Regarding the http asset accessor tick, I was looking at that IsReadyForFinishDestroy code for quite a while trying to confirm to myself that loads were progressing as they should through dispatchInMainThread - I had completely forgotten that the asset accessor was tick based! Wish I could have caught it earlier too hahaha. It might be useful to add a debug option to bypass cache at some point.