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

Added AssetLoadFailedEvent, UntypedAssetLoadFailedEvent #11369

Merged
merged 5 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions crates/bevy_asset/src/event.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,47 @@
use crate::{Asset, AssetId};
use crate::{Asset, AssetId, AssetLoadError, AssetPath, UntypedAssetId};
use bevy_ecs::event::Event;
use std::fmt::Debug;

/// Events that occur for a specific [`Asset`], such as "value changed" events and "dependency" events.
/// An event emitted when a specific [`Asset`] fails to load.
brianreavis marked this conversation as resolved.
Show resolved Hide resolved
///
/// For an untyped equivalent, see [`UntypedAssetLoadFailedEvent`].
#[derive(Event, Clone, Debug)]
pub struct AssetLoadFailedEvent<A: Asset> {
pub id: AssetId<A>,
/// The asset path that was attempted.
pub path: AssetPath<'static>,
/// Why the asset failed to load.
pub error: AssetLoadError,
}

impl<A: Asset> AssetLoadFailedEvent<A> {
/// Converts this to an "untyped" / "generic-less" asset error event that stores the type information.
pub fn untyped(&self) -> UntypedAssetLoadFailedEvent {
self.into()
}
}

/// An untyped version of [`AssetLoadFailedEvent`].
#[derive(Event, Clone, Debug)]
pub struct UntypedAssetLoadFailedEvent {
pub id: UntypedAssetId,
/// The asset path that was attempted.
pub path: AssetPath<'static>,
/// Why the asset failed to load.
pub error: AssetLoadError,
}

impl<A: Asset> From<&AssetLoadFailedEvent<A>> for UntypedAssetLoadFailedEvent {
fn from(value: &AssetLoadFailedEvent<A>) -> Self {
UntypedAssetLoadFailedEvent {
id: value.id.untyped(),
path: value.path.clone(),
error: value.error.clone(),
}
}
}

/// Events that occur for a specific loaded [`Asset`], such as "value changed" events and "dependency" events.
#[derive(Event)]
pub enum AssetEvent<A: Asset> {
/// Emitted whenever an [`Asset`] is added.
Expand Down
20 changes: 16 additions & 4 deletions crates/bevy_asset/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,32 @@ use futures_lite::{ready, Stream};
use std::{
path::{Path, PathBuf},
pin::Pin,
sync::Arc,
task::Poll,
};
use thiserror::Error;

/// Errors that occur while loading assets.
#[derive(Error, Debug)]
#[derive(Error, Debug, Clone)]
Copy link
Member

@alice-i-cecile alice-i-cecile Jan 16, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

pub enum AssetReaderError {
/// Path not found.
#[error("path not found: {0}")]
#[error("Path not found: {0}")]
NotFound(PathBuf),

/// Encountered an I/O error while loading an asset.
#[error("encountered an io error while loading asset: {0}")]
Io(#[from] std::io::Error),
#[error("Encountered an I/O error while loading asset: {0}")]
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`].
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@brianreavis brianreavis Jan 17, 2024

Choose a reason for hiding this comment

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

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.

#[error("Encountered HTTP status {0:?} when loading asset")]
HttpError(u16),
}

impl From<std::io::Error> for AssetReaderError {
fn from(value: std::io::Error) -> Self {
Self::Io(Arc::new(value))
}
}

pub type Reader<'a> = dyn AsyncRead + Unpin + Send + Sync + 'a;
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_asset/src/io/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

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

PartialEq on all these error types too please.

#[error("Asset Source '{0}' does not exist")]
pub struct MissingAssetSourceError(AssetSourceId<'static>);

/// An error returned when an [`AssetWriter`] does not exist for a given id.
#[derive(Error, Debug)]
#[derive(Error, Debug, Clone)]
#[error("Asset Source '{0}' does not have an AssetWriter.")]
pub struct MissingAssetWriterError(AssetSourceId<'static>);

/// An error returned when a processed [`AssetReader`] does not exist for a given id.
#[derive(Error, Debug)]
#[derive(Error, Debug, Clone)]
#[error("Asset Source '{0}' does not have a processed AssetReader.")]
pub struct MissingProcessedAssetReaderError(AssetSourceId<'static>);

/// An error returned when a processed [`AssetWriter`] does not exist for a given id.
#[derive(Error, Debug)]
#[derive(Error, Debug, Clone)]
#[error("Asset Source '{0}' does not have a processed AssetWriter.")]
pub struct MissingProcessedAssetWriterError(AssetSourceId<'static>);

Expand Down
5 changes: 1 addition & 4 deletions crates/bevy_asset/src/io/wasm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ impl HttpWasmAssetReader {
Ok(reader)
}
404 => Err(AssetReaderError::NotFound(path)),
status => Err(AssetReaderError::Io(std::io::Error::new(
std::io::ErrorKind::Other,
format!("Encountered unexpected HTTP status {status}"),
))),
status => Err(AssetReaderError::HttpError(status as u16)),
}
}
}
Expand Down
Loading