Don't panic on error when loading assets #1286
Conversation
| @@ -295,7 +306,9 @@ impl AssetServer { | |||
| self.server | |||
| .task_pool | |||
| .spawn(async move { | |||
| server.load_async(owned_path, force).await.unwrap(); | |||
| if let Err(err) = server.load_async(owned_path, force).await { | |||
| warn!("Asset failed to load: {:?}", err); | |||
There was a problem hiding this comment.
I think its probably better to rely on the error's "display" implementation here:
warn!("{}", err);But that requires some changes to the error in question:
- rename PathLoader to AssetIo (this was already out of sync. PathLoader was an old name for the AssetIo abstraction)
- add the actual AssetIo error to the message
#[error("AssetIo encountered an error: {0:?}")]
AssetIoError(#[from] AssetIoError),There was a problem hiding this comment.
And if you're feeling generous, you could update the other error messages in that enum to also include their contents (where relevant).
There was a problem hiding this comment.
Arg actually maybe including the "Asset failed to load" message is a good idea because the error unfortunately doesn't encapsulate that information.
The current Error impl is shared everywhere, which is nice for code size / error handling ergonomics, but makes cases like this nastier.
Feel free to leave this as-is for now (although renaming PathLoader to AssetIo would be nice 😄 )
There was a problem hiding this comment.
Good suggestions, I updated the error text of AssetServerError and AssetIoError to include the contents and use Display where appropriate. Now, I get a warnings that look like:
Jan 23 11:24:25.658 WARN bevy_asset::asset_server: encountered an error while reading an asset: path not found: /Users/will/Code/crateton/assets/models/monkey/config.json
willcrichton
force-pushed
the
handle-asset-failures
branch
from
39a8a6e to
22e0c99
Compare
|
Love the changes. Thanks! |
* Don't panic on IO errors * Better formatting for asset server errors
Right now, loading an asset that doesn't exist causes the IO task to panic. Instead, we can toss a warning and set the load state to
Failedso applications can decide how to handle the failure. As @cart mentioned here, eventually the reason for failing can get broadcast as an event. But this PR is a step towards that.