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

Add AsyncSeek trait to Reader to be able to seek inside asset loaders #12547

Merged
merged 8 commits into from
Mar 30, 2024

Conversation

BeastLe9enD
Copy link
Contributor

@BeastLe9enD BeastLe9enD commented Mar 17, 2024

Objective

For some asset loaders, it can be useful not to read the entire asset file and just read a specific region of a file. For this, we need a way to seek at a specific position inside the file

Solution

I added support for AsyncSeek to Reader. In my case, I want to only read a part of a file, and for that I need to seek to a specific point.

Migration Guide

Every custom reader (which previously only needed the AsyncRead trait implemented) now also needs to implement the AsyncSeek trait to add the seek capability.

@james7132 james7132 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 Mar 18, 2024
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Thanks for properly handling the errors here!

@BeastLe9enD
Copy link
Contributor Author

do we still need to change something here?

@james7132
Copy link
Member

We require at least two reviews before merging. Requesting to get a few more eyes on this.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Just a minor question around the bounds checking and marking this as a breaking change. Otherwise I think this looks good! I like the addition of a slice reader, nice touch.

};

if let Ok(new_pos) = result {
if new_pos < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there also be a check to ensure new_pos is before the end of the Reader? I can see the AsyncRead implementation is already checking so I don't think this is a safety issue, just want to flag it just in case it's a mistake.


impl<T: AsyncRead + AsyncSeek> AsyncReadAndSeek for T {}

pub type Reader<'a> = dyn AsyncReadAndSeek + Unpin + Send + Sync + 'a;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is a breaking change for any third party Reader types (if any exist).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I added a point to the migration guide :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 27, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile
Copy link
Member

@viridia I recall you asking after this sort of functionality.

@viridia
Copy link
Contributor

viridia commented Mar 27, 2024

@alice-i-cecile This was less of an "ask" and more of an "observation", but here's some more context:

Traditionally, games have used various means of bundling resources - .zip files, .wad files and so on - to avoid the overhead of managing lots of small files. Doing this efficiently requires some means of reading parts of a file without reading the entire thing.

Zip files are a good example of this. The directory index of a zip file is a table which is located at the end of the file (because it is written last). There's an algorithm which can be used to determine the offset of the start of the index. Once this offset has been determined, the index can be loaded as a single block read. Afterwards, entries can be looked up in the directory by name, which gives the file offset and length of each compressed block.

In other words, zip files are designed to allow efficient random access. However, not every kind of hierarchical asset format supports this: for example, JSON and YAML files are also hierarchical, but must be read and parsed sequentially in order to make any sense. For these latter kinds of files, there's no benefit to seeking.

There are two ways that hierarchical asset structures can be modeled in Bevy:

  • As assets containing sub-assets.
  • As asset sources / asset readers.

I think that traditionally, random access files like .zip and .wad files have been modeled as asset sources rather than as assets. This is because in most cases, zip files are treated semantically as collections of assets rather than as assets themselves.

Non-random access files, on the other hand, tend to be modeled as assets with sub-assets.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 28, 2024
@BeastLe9enD
Copy link
Contributor Author

@viridia in my case I use it to load multiple mesh lod levels from one file without having to split it up into multiple files. But because I don't want all to be loaded at the same time, I cannot load them at once, so loading the total file multiples times would be really wasteful.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 29, 2024
@BeastLe9enD
Copy link
Contributor Author

Test failed because a missing trait impl of AsyncSeek in sync_file_asset.rs, I added a fix.
Unfortunately, I can't run the tests locally because it uses far too much hard drive space :/ Hope it works now :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 30, 2024
Merged via the queue into bevyengine:main with commit eb44db4 Mar 30, 2024
27 checks passed
@cart
Copy link
Member

cart commented Apr 4, 2024

I'm concerned that this will prevent us from writing AssetReaders (and therefore loaders) that truly "stream" the bytes. For example, the http reader used by WASM currently collects bytes into a VecReader, which is why this works. But if we wanted to skip the Vec to avoid the up front allocation of the entire asset bytes, we now literally cannot do that because AssetReader requires seek, which an http request does not support.

@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1294 if you'd like to help out.

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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants