Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite AssetStorage to the new ktx-async API #182

Closed
5 tasks done
czyzby opened this issue Feb 5, 2019 · 8 comments · Fixed by #258
Closed
5 tasks done

Rewrite AssetStorage to the new ktx-async API #182

czyzby opened this issue Feb 5, 2019 · 8 comments · Fixed by #258
Assignees
Labels
assets Issues of the ktx-assets module async Issues of the ktx-async module freetype Issues of the ktx-freetype and ktx-freetype-async modules
Milestone

Comments

@czyzby
Copy link
Member

czyzby commented Feb 5, 2019

  • Add ktx-assets-async module.
  • AssetStorage rewrite.
  • Usage documentation.
  • Restore ktx-freetype-async, update to new API.
  • Restore TextAssetLoader, move to ktx-assets.

AssetStorage was removed during the ktx-async overhaul in 1.9.9-b1 due to reliance on old coroutines API and threading issues. The goal of this task is to rewrite AssetStorage in a fully thread-safe way, without reliance on specific coroutines dispatchers for asynchronous tasks.

@czyzby czyzby added assets Issues of the ktx-assets module async Issues of the ktx-async module freetype Issues of the ktx-freetype and ktx-freetype-async modules labels Feb 5, 2019
@czyzby czyzby added this to the 1.9.9 milestone Feb 5, 2019
@czyzby czyzby self-assigned this Feb 5, 2019
@czyzby czyzby changed the title AssetStorage rewrite to new ktx-async API Rewrite AssetStorage to the new ktx-async API Feb 5, 2019
@czyzby czyzby pinned this issue Feb 5, 2019
@czyzby czyzby mentioned this issue Feb 5, 2019
10 tasks
czyzby added a commit that referenced this issue Feb 7, 2019
czyzby added a commit that referenced this issue Feb 7, 2019
@czyzby czyzby modified the milestones: 1.9.9, 1.9.10 Jul 22, 2019
@amitkot
Copy link

amitkot commented Jul 24, 2019

I'm making changes to a project I created two years ago in which I used the previous version of ktx-async which depended on the experimental coroutines, and specifically AssetStorage.

I see that AssetStorage is no longer available in KTX.

Would you say the wise move right now is to switch to using ktx-assets's AssetManager instead of AssetStorage?

@czyzby
Copy link
Member Author

czyzby commented Jul 24, 2019

@amitkot For now, yes. There were some issues with thread safety and unstable coroutines API usage, and I'm afraid AssetStorage needs a complete rewrite. Sorry.

@czyzby
Copy link
Member Author

czyzby commented Mar 29, 2020

@Quillraven @cypherdare @Jkly @maltaisn @jcornaz @amitkot

I finally decided to rewrite the good old AssetStorage. While I still have to write usage examples and update ktx-freetype-async, at this point I'm happy with the AssetStorage implementation and its test suite.

To be honest, this is probably among top 5 most fun, challenging and complex pieces of code I've written. The initial implementation would definitely not pass the extensive tests I've written this time, including concurrent assets loading and unloading on multiple threads and coroutines.

I think the current implementation is fully thread-safe and non-blocking. Unless the user of the API blocks the rendering or loading threads manually with runBlocking and whatnot, it should handle anything you throw at it without deadlocks or unexpected exceptions. I've run hundreds of thousands of concurrent loading and unloading requests of assets with nested dependencies - I encourage you to play with should handle stress test test in storageTest.kt to try to break it.

Anyway, I would appreciate it very much if you took the time to try AssetStorage out as a replacement for AssetManager or even go through the sources and do some code review. I know it's a huge pull request, but even superficial review would help a lot. If you're interested and have some time:

I'll leave the pull request open awaiting suggestions while I focus on the documentation and ktx-freetype-async.

Thanks.

@czyzby
Copy link
Member Author

czyzby commented Mar 29, 2020

ktx-freetype-async is now updated to the new AssetStorage API and available via snapshot releases.

@czyzby
Copy link
Member Author

czyzby commented Mar 30, 2020

Ironically, after implementing this I found this obscure feature called LoadedCallback in AssetLoaderParameters, where you can actually add a callback that will be invoked after the asset is loaded by a AssetManager, which means coroutines support can be added to the original LibGDX manager relatively easy (well, easier than writing AssetStorage was).

AssetStorage still makes sense, of course, as it allows for true parallel asset loading, but I'm thinking we could also add coroutines support to AssetManager. loadAsync extension could return a Deferred reference to an asset that you can await for.


Edit: other than having to make the parameters nullable and create them dynamically for supported asset types, this would be as easy as:

import com.badlogic.gdx.assets.AssetLoaderParameters
import com.badlogic.gdx.assets.AssetLoaderParameters.LoadedCallback
import com.badlogic.gdx.assets.AssetManager
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.Deferred

inline fun <reified T> AssetManager.loadAsync(
  path: String, parameters: AssetLoaderParameters<T>
): Deferred<T> {
  val result = CompletableDeferred<T>()
  parameters.loadedCallback = LoadedCallback { assetManager, fileName, type ->
    @Suppress("UNCHECKED_CAST") val assetClass = type as Class<T>
    try {
      result.complete(assetManager.get(fileName, assetClass) as T)
    } catch (exception: Throwable) {
      result.completeExceptionally(exception)
    }
  }
  load(path, T::class.java, parameters)
  return result
}

Oh well.

czyzby added a commit that referenced this issue Apr 1, 2020
@czyzby czyzby linked a pull request Apr 2, 2020 that will close this issue
czyzby added a commit that referenced this issue Apr 5, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
Asynchronous coroutines-based asset manager: AssetLoader #182
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
czyzby added a commit that referenced this issue Apr 10, 2020
* Excluded OpenAL tests from Travis. #182
* Updated Travis test configuration. #182
* Eased off loading progress tests. #182
@czyzby
Copy link
Member Author

czyzby commented Apr 10, 2020

I believe the current AssetStorage available on develop is feature-complete and can be used as a replacement for AssetManager for the majority of use cases. @Quillraven also reported notable performance improvements, especially on mobile (see discussion in #258).

I want to thank @Quillraven again for testing and review of the new asset manager. I'd ship a lot less features and a clunky API if not for the input and feature requests.

Since there were no further comments, I'm closing this issue for now. If you'd like you make a feature request or comment regarding the AssetStorage API, use issue #259.

@cypherdare
Copy link
Contributor

@czyzby I have been on a bit of LibGDX hiatus for the past three months. I looked this over briefly and it looks slick. I don't currently have a project in need of this feature to try it out in currently, though.

czyzby added a commit that referenced this issue Feb 23, 2021
czyzby added a commit that referenced this issue Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets Issues of the ktx-assets module async Issues of the ktx-async module freetype Issues of the ktx-freetype and ktx-freetype-async modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants