-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
bevy_asset: support upgrading Reader to SeekableReader #22182
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
base: main
Are you sure you want to change the base?
Conversation
andriyDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we need to delete the remnants: android, wasm, and web modules need to be cleaned up, custom_asset_reader example also need to be cleaned up. Also some migration guides we need to update (we can leave this until later).
I updated these (and release notes) right before you posted 😄 |
andriyDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but for the doctest, which I've made a PR against this branch for: cart#50
Thanks for taking the time to fix this!!
janhohenheim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner to me, good job! The PR introduces some new public API that is missing documentation. Approving after that :)
| /// An [`AsyncRead`] implementation capable of reading a [`Vec<u8>`]. | ||
| pub struct VecReader { | ||
| bytes: Vec<u8>, | ||
| pub bytes: Vec<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding docs to this struct now that it's pub? How is a user meant to mutate this field? When? Is there anything to keep in mind, or can the user just freely mutate the Vec? Does read_bytes get affected by this?
| fn seekable(&mut self) -> Result<&mut dyn SeekableReader, ReaderNotSeekableError>; | ||
| } | ||
|
|
||
| pub trait SeekableReader: Reader + AsyncSeek {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some documentation and breadcrumbs to other related traits? Nothing big.
| #[derive(Error, Debug, Copy, Clone)] | ||
| #[error( | ||
| "The `Reader` returned by the current `AssetReader` does not support `AsyncSeek` behavior." | ||
| )] | ||
| pub struct ReaderNotSeekableError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[derive(Error, Debug, Copy, Clone)] | |
| #[error( | |
| "The `Reader` returned by the current `AssetReader` does not support `AsyncSeek` behavior." | |
| )] | |
| pub struct ReaderNotSeekableError; | |
| /// Error returned by [`Reader::seekable`] when the reader implementation does not support [`AsyncSeek`] behavior. | |
| #[derive(Error, Debug, Copy, Clone)] | |
| #[error( | |
| "The `Reader` returned by the current `AssetReader` does not support `AsyncSeek` behavior." | |
| )] | |
| pub struct ReaderNotSeekableError; |
Objective
#22104 added
AsyncSeeksupport, but it has some downsides:AssetLoaderto "attest" that it needs the seek feature. However, AssetLoaders both have access to theAsyncSeekAPI no matter what (it is a supertrait of Reader), and AssetReaders provideAsyncSeekbehavior even if it isn't requested. In practice this is likely to create situations where AssetLoaders don't requestAsyncSeekand just rely on it being provided by default, causing unexpected incompatibilities when AssetLoader consumers try to use other AssetReaders that don't support AsyncSeek.Solution
ReaderRequiredFeaturesand associated functionalitySeekableReadertrait, whereSeekableReader: Reader + AsyncSeek.Reader::seekable(&mut self) -> Result<&mut dyn SeekableReader, ReaderNotSeekableError>, which can either fail or cast toSeekableReader, if that is supported.This gives
AssetLoaderimplementers more clarity when it comes toReaderfeature support, gives them more autonomy over fallback behavior, makes our APIs more static, and cuts down on the complexity of the system as a whole.