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

return type-erased Errors in AssetLoader::load #10463

Closed
wants to merge 2 commits into from

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Nov 8, 2023

Fixes #10350

ping @bushrat011899, @Roryl-c (I'm not actually using custom assets, so this needs to be reviewed by someone who does)

Objective

Want to allow people to use anyhow as the return error value of AssetLoader.

Solution

ErasedAssetLoader::load already returns type-erased errors. Why not return the same errors in AssetLoader::load?

Also, I aliased bevy::asset::Error to Box<dyn Error + Send + Sync + 'static> to make changes more backward compatible in the future + easier to type. This was suggested earlier in #5359.

Changelog

AssetLoader::load now returns type-erased Errors.

Migration Guide

Remove an associated type Error from AssetLoader and AssetSaver. Return error value is now Box<dyn Error> to allow for unsized user types.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 8, 2023
this fixes bevyengine#10350

Previous change made in bevyengine#10003
resulted in inability for users to have ?Sized errors, which caused
usability issues with crates like anyhow.
@Runi-c
Copy link

Runi-c commented Nov 10, 2023

I tried this out and was met with a very confusing type error.
I re-read the PR and saw that I had to change the return type to:
BoxedFuture<'a, Result<Self::Asset, BoxedError>>
instead of using anyhow::Error as the error type. This is fine, but I would guess others might be similarly confused when they migrate.

I tried #10493 as well and that one was trivial for me as I was able to set the associated type directly to anyhow::Error as I was instructed by the original migration guide. So that might be a preferred fix for this case assuming it doesn't have other problems.

@alice-i-cecile
Copy link
Member

Closing in favor of #10493. Thank you!

@rlidwka rlidwka closed this Nov 11, 2023
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-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use anyhow when writing custom AssetLoaders or AssetSavers
3 participants