Skip to content

Improve error handling for AssetServer::add_async#13745

Merged
alice-i-cecile merged 4 commits intobevyengine:mainfrom
joseph-gio:add-async-error
Jun 10, 2024
Merged

Improve error handling for AssetServer::add_async#13745
alice-i-cecile merged 4 commits intobevyengine:mainfrom
joseph-gio:add-async-error

Conversation

@joseph-gio
Copy link
Member

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

@joseph-gio joseph-gio added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 8, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 8, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 8, 2024
@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 8, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile
Copy link
Member

I've added this to the 0.14 milestone: it's a breaking change compared to the release candidate, but I think this is important to make the added functionality actually useful.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
Merged via the queue into bevyengine:main with commit 0dfdd87 Jun 10, 2024
@joseph-gio joseph-gio deleted the add-async-error branch June 10, 2024 17:05
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants