Allow loading assets with custom async behavior#13700
Merged
alice-i-cecile merged 5 commits intobevyengine:mainfrom Jun 6, 2024
Merged
Allow loading assets with custom async behavior#13700alice-i-cecile merged 5 commits intobevyengine:mainfrom
alice-i-cecile merged 5 commits intobevyengine:mainfrom
Conversation
joseph-gio
added
A-Assets
alice-i-cecile
approved these changes
Jun 5, 2024
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Reasonable enough. I can see the use of this, and this seems reasonably constructed.
Agreed on the testing front: we need to sit down and figure out a testing strategy for bevy_assets more cohesively.
Member
Author
|
The |
This reverts commit e880938.
bushrat011899
approved these changes
Jun 5, 2024
Contributor
bushrat011899
left a comment
bushrat011899
left a comment
There was a problem hiding this comment.
Nice idea! There's just enough complexity here (IoTaskPool, the event_sender, etc.) that this encapsulation really improves the user experience, but is still overall pretty simple. I concur that a better testing story is needed for bevy_asset, but I don't think that is necessary to get this merged.
alice-i-cecile
added
the
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 10, 2024
# Objective The method `AssetServer::add_async` (added in #13700) requires a future that returns an `AssetLoadError` error, which was a bit of an oversight on my part, as that type of error only really makes sense in the context of bevy's own asset loader -- returning it from user-defined futures isn't very useful. ## Solution Allow passing custom error types to `add_async`, which get cast into a trait object matching the form of `AssetLoader::load`. If merged before the next release this will not be a breaking change
mockersf
pushed a commit
that referenced
this pull request
Jun 10, 2024
# Objective The method `AssetServer::add_async` (added in #13700) requires a future that returns an `AssetLoadError` error, which was a bit of an oversight on my part, as that type of error only really makes sense in the context of bevy's own asset loader -- returning it from user-defined futures isn't very useful. ## Solution Allow passing custom error types to `add_async`, which get cast into a trait object matching the form of `AssetLoader::load`. If merged before the next release this will not be a breaking change
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Currently, bevy supports custom asset loading via
AssetServer:;add, which allows you to add arbitrary assets to the asset system and returns a handle to it. However this only works for assets that have already been fully loaded. If your loading logic involves any async, you need to wait until the asset is done loading before adding it to the server. This is problematic, as theHandledoes not get allocated until the very end, which makes it very difficult to use and defeats the value of having handles for asynchronously-loaded assets.Solution
Add the method
AssetServer::add_async. This has the same behavior asAssetServer::add, only it accepts a future instead of a fully loaded asset.Testing
I added an identical method to my company's fork of bevy, which works in our app. I'm not quite sure how to go about adding an actual unit test for asset loading behvior, but I will note that
AssetServer::addalso does not appear to have any tests.Changelog
AssetServer::add_async, which allows adding assets with custom asynchronous loading behavior to theAssetServer