Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

brianreavis · 2024-01-16T09:12:05Z

Objective

This adds events for assets that fail to load along with minor utility methods to make them useful. This paves the way for users writing their own error handling and retry systems, plus Bevy including robust retry handling: #11349.

Addresses Include errors in AssetEvent #11288
Needed for Asset Load Retry Plugin #11349

Solution

/// An event emitted when a specific [`Asset`] fails to load.
#[derive(Event, Clone, Debug)]
pub struct AssetLoadFailedEvent<A: Asset> {
    pub id: AssetId<A>,
    /// The original handle returned when the asset load was requested.
    pub handle: Option<Handle<A>>,
    /// The asset path that was attempted.
    pub path: AssetPath<'static>,
    /// Why the asset failed to load.
    pub error: AssetLoadError,
}

I started implementing AssetEvent::Failed like suggested in #11288, but decided it was better as its own type because:

I think it makes sense for AssetEvent to only refer to assets that actually exist.
In order to return AssetLoadError in the event (which is useful information for error handlers that might attempt a retry) we would have to remove Copy from AssetEvent.
There are numerous places in the render app that match against AssetEvent, and I don't think it's worth introducing extra noise about assets that don't exist.

I also introduced UntypedAssetLoadErrorEvent, which is very useful in places that need to support type flexibility, like an Asset-agnostic retry plugin.

Changelog

Added: AssetLoadFailedEvent<A>
Added: UntypedAssetLoadFailedEvent
Added: AssetReaderError::Http for status code information on HTTP errors. Before this, status codes were only available by parsing the error message of generic Io errors.
Added: asset_server.get_path_id(path). This method simply gets the asset id for the path. Without this, one was left using get_path_handle(path), which has the overhead of returning a strong handle.
Fixed: Made AssetServer loads return the same handle for assets that already exist in a failed state. Now, when you attempt a load that's in a LoadState::Failed state, it'll re-use the original asset id. The advantage of this is that any dependent assets created using the original handle will "unbreak" if a retry succeeds.

alice-i-cecile · 2024-01-16T13:44:38Z

crates/bevy_asset/src/event.rs

+#[derive(Event, Clone, Debug)]
+pub struct AssetLoadFailedEvent<A: Asset> {
+    pub id: AssetId<A>,
+    /// The original handle returned when the asset load was requested.


When will this be None?

To be totally honest, I can't say I have a firm grasp on that. It maps 1:1 with InternalAssetEvent::Failed – which in some cases doesn't have a resolved handle.

I'd be okay with dropping this field for the time being. Users can use the asset path to lookup the asset id / handle from the asset server if needed and there's no real reason to keep the handle alive via the event. The handle should be in the world already from the original load() if it's useful to retain the asset (and potentially retry).

What do you think?

The current event API only delivers IDs (weak handles), I think the contract is that you are responsible for keeping handles from load requests alive - the event system is not going to do that for you. Unless there's some other circumstances that could arise from handling the error, we should probably follow the same pattern with the error events and keep the actual event as small as possible while still containing all necessary information.

I think dropping the field makes sense.

crates/bevy_asset/src/event.rs

alice-i-cecile · 2024-01-16T13:46:13Z

crates/bevy_asset/src/io/mod.rs

    task::Poll,
 };
 use thiserror::Error;

 /// Errors that occur while loading assets.
-#[derive(Error, Debug)]
+#[derive(Error, Debug, Clone)]


Are we able to reasonably derive PartialEq here? If so, please do: it's really useful for writing tests on error types.

Unfortunately not due to the std::io::Error in the Io variant

alice-i-cecile · 2024-01-16T13:46:49Z

crates/bevy_asset/src/io/source.rs

@@ -569,22 +569,22 @@ impl AssetSources {
 }

 /// An error returned when an [`AssetSource`] does not exist for a given id.
-#[derive(Error, Debug)]
+#[derive(Error, Debug, Clone)]


PartialEq on all these error types too please.

alice-i-cecile · 2024-01-16T13:48:56Z

crates/bevy_asset/src/server/mod.rs

@@ -775,12 +811,20 @@ impl AssetServer {
        self.data.infos.read().contains_key(id.into())
    }

+    /// Returns an active untyped asset id for the given path, if the asset at the given path has already started loading,
+    /// or is still "alive".


Suggested change

/// or is still "alive".

/// or is still "alive".

///

/// Equivalently, returns [`None`] if the asset has not yet started loading, or the asset was loaded but all strong handles were dropped.

Is this true? I had trouble understanding what "still alive" meant.

I looked through the asset server code. Unless I missed something, "Still alive" just means that it has a strong handle somewhere. An asset's UntypedAssetId is inserted into the asset server's HashMap of AssetPaths to UntypedAssetIds when a handle for it is created (which happens very soon after it starts loading), and it is removed when all strong handles are dropped.

@thepackett That's my reading of it too. The comment I have there is just a copy/paste of another method's documentation with "handle" replaced with "id".

alice-i-cecile · 2024-01-16T13:49:22Z

crates/bevy_asset/src/server/mod.rs

@@ -1109,7 +1187,7 @@ pub enum RecursiveDependencyLoadState {
 }

 /// An error that occurs during an [`Asset`] load.
-#[derive(Error, Debug)]
+#[derive(Error, Debug, Clone)]


PartialEq here too if possible.

We unfortunately can't easily add PartialEq here because of AssetLoadError::AssetLoaderError holding dyn std::error::Error. Adding a PartialEq trait bound to would create a headache for loaders. If you'd like, I can add a manual implementation of PartialEq for AssetLoadError that stringifies the error and tests equality on that... but that seems kinda sketchy.

alice-i-cecile

Just nits: nothing blocking but it would be nice to improve the docs and UX a bit.

Thanks for explaining your reasoning about the separate event vs same event in the PR description: it's really useful for reviewers to see why the "obvious" path isn't the best one. I'm convinced by the argument :)

thepackett

I concur with @alice-i-cecile 's findings. The suggested changes would be nice, but are not worth blocking on.

thepackett · 2024-01-16T21:41:37Z

crates/bevy_asset/src/io/mod.rs

+    Io(Arc<std::io::Error>),
+
+    /// The HTTP request completed but returned an unhandled [HTTP response status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status).
+    /// If the request fails before getting a status code (e.g. request timeout, interrupted connection, etc), expect [`AssetReaderError::Io`].


Looking at this my first impression is that something like an HttpError, which is only relevant for some AssetReaders, should only be implemented for those particular readers. However, neither AssetReader nor AssetWriter allow defining custom error types like AssetLoader and AssetSaver do.

This is probably something that ought to be looked at in the future, but I think it is also outside the scope of this PR, so this is good for now.

I can see the case for that. I do think having well-defined variants is useful for allowing downstream code to handle errors however desireable. For example: https://github.com/bevyengine/bevy/pull/11349/files#diff-b21bad69535483663d48f483c940909cef359606f3fc1094508dc25c3ea86327R83-R102:

if let AssetLoadError::AssetReaderError(read_error) = &event.error { match read_error { AssetReaderError::NotFound(_) => {} AssetReaderError::Io(io_error) => match io_error.kind() { ErrorKind::InvalidInput | ErrorKind::InvalidData | ErrorKind::NotFound | ErrorKind::PermissionDenied | ErrorKind::Unsupported => {} _ => return self.retry_settings, }, AssetReaderError::HttpError(status_code) => { match status_code { // Retry after server errors and rate limits 500..=599 | 429 => return self.retry_settings, _ => {} } } } }

I'm having a hard time imagining anything other than general Io and Http errors, but I could be missing something.

It wouldn't be the end of the world to end up at what you're suggesting:

pub trait AssetReader: Send + Sync + 'static { type Error: Into<Box<dyn std::error::Error + Send + Sync + AssetReaderError + 'static>>; // ... } pub trait AssetReaderError { fn is_retryable(&self) -> bool { false } }

If there are generally accepted error types for I/O and HTTP errors, one could use downcasting to get at any low-level information needed. This does seem outside the scope of this PR, though.

thepackett · 2024-01-16T22:20:03Z

crates/bevy_asset/src/server/mod.rs

@@ -775,12 +811,20 @@ impl AssetServer {
        self.data.infos.read().contains_key(id.into())
    }

+    /// Returns an active untyped asset id for the given path, if the asset at the given path has already started loading,
+    /// or is still "alive".


I looked through the asset server code. Unless I missed something, "Still alive" just means that it has a strong handle somewhere. An asset's UntypedAssetId is inserted into the asset server's HashMap of AssetPaths to UntypedAssetIds when a handle for it is created (which happens very soon after it starts loading), and it is removed when all strong handles are dropped.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

brianreavis · 2024-01-17T00:59:21Z

Thanks for the review @alice-i-cecile and @thepackett! I tried to address the comments; let me know if there's anything else you need from me to get this merged.

alice-i-cecile · 2024-01-17T14:09:36Z

Let's drop the handle field from AssetLoadFailedEvent and then get this merged.

Shame about the PartialEq, but that's the way it is sometimes.

bevyengine#11369 (comment)

brianreavis · 2024-01-17T20:44:04Z

Sounds good. All set 👍

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent

ae6d118

brianreavis mentioned this pull request Jan 16, 2024

Asset Load Retry Plugin #11349

Open

alice-i-cecile 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 Jan 16, 2024

alice-i-cecile added this to the 0.13 milestone Jan 16, 2024

alice-i-cecile reviewed Jan 16, 2024

View reviewed changes

crates/bevy_asset/src/event.rs Show resolved Hide resolved

alice-i-cecile reviewed Jan 16, 2024

View reviewed changes

alice-i-cecile approved these changes Jan 16, 2024

View reviewed changes

thepackett approved these changes Jan 16, 2024

View reviewed changes

Added doc mention of untyped variant of AssetLoadFailedEvent

3a4c0be

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Removed comment reference to retry plugin

8937079

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 17, 2024

brianreavis added 2 commits January 17, 2024 12:38

Removed handle from asset load failed events (bevyengine#11369)

12d9b64

bevyengine#11369 (comment)

Formatting on dependency_failed_event_sender

e449b4e

alice-i-cecile added this pull request to the merge queue Jan 17, 2024

Merged via the queue into bevyengine:main with commit c9e1fcd Jan 17, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

brianreavis commented Jan 16, 2024 •

edited

Loading

alice-i-cecile Jan 16, 2024

brianreavis Jan 17, 2024

siler Jan 17, 2024 •

edited

Loading

alice-i-cecile Jan 17, 2024

alice-i-cecile Jan 16, 2024 •

edited

Loading

brianreavis Jan 17, 2024

alice-i-cecile Jan 16, 2024

alice-i-cecile Jan 16, 2024

thepackett Jan 16, 2024

brianreavis Jan 17, 2024

alice-i-cecile Jan 16, 2024

brianreavis Jan 17, 2024

alice-i-cecile left a comment

thepackett left a comment

thepackett Jan 16, 2024

brianreavis Jan 17, 2024 •

edited

Loading

thepackett Jan 16, 2024

brianreavis commented Jan 17, 2024

alice-i-cecile commented Jan 17, 2024

brianreavis commented Jan 17, 2024

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

Conversation

brianreavis commented Jan 16, 2024 • edited Loading

Objective

Solution

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siler Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

thepackett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianreavis Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianreavis commented Jan 17, 2024

alice-i-cecile commented Jan 17, 2024

brianreavis commented Jan 17, 2024

brianreavis commented Jan 16, 2024 •

edited

Loading

siler Jan 17, 2024 •

edited

Loading

alice-i-cecile Jan 16, 2024 •

edited

Loading

brianreavis Jan 17, 2024 •

edited

Loading