-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Implementation of temp://
Asset Source
#13406
Conversation
67e7bad
to
a728ac6
Compare
4a1e474
to
6ead01d
Compare
temp://
Asset Sourcetemp://
Asset Source
Converting to draft as I have a rough implementation of this now working on WASM as well as desktop (mobile is not something I am equipped to test so that is still outside of my scope). WASM support is built on top of the Origin Private File System, which provides a file-level API in the browser. I have implemented an I'm marking this as draft because while I have these implementations working in the browser (tested on Firefox using I would encourage anyone with |
To aid with discoverability of the temp folder. Note that the directory is explicitly _not_ logged (e.g., `trace!`) to avoid logging sensitive data (e.g., user names) Co-Authored-By: Ricky Taylor <[email protected]>
Allows configuration of the temp directory after startup, and the retrieval of the `Path` for 3rd party use.
Initial support for WASM `temp://` using OPFS.
90ec52c
to
4ffc2bb
Compare
Split out certain utility methods which are more general WASM items.
4ffc2bb
to
98b21d5
Compare
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.
I think that OPFS is the right approach for this, and broadly it looks good, however there's a lot of bridging async code that I need to review more closely and I have a couple of broader (hopefully) useful suggestions.
} | ||
} | ||
|
||
pub(crate) fn get_temp_source( |
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.
I'm not sure that the TempDirectory
struct makes much sense on the web. If people want an abstraction over temp directories & OPFS, we're providing one with temp://
. Not being able to fetch a path to it seems like a relevant platform-specific caveat. I would probably check the target in lib.rs
and register the temp directory / OPFS source as relevant.
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.
I do agree there's not a lot of utility to the TempDirectory
type on web. I think I'll leave it in place tho just as it's part of the user-facing configuration of the temp://
source, and I want to leave the OPFS independent of the temp://
feature, since users may find it useful to manage assets in the browser as well.
} | ||
|
||
/// Get the [`FileSystemDirectoryHandle`] for the root directory pointed to by `self.root`. | ||
pub(crate) async fn shadow_root(&self) -> std::io::Result<FileSystemDirectoryHandle> { |
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.
This is executed frequently. Would it make more sense to cache the root directory handle?
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.
I do agree, but I'm concerned about storing the handle (which is !Send
) in such a way that it won't negatively impact the ergonomics of working with OriginPrivateFileSystem
as an AssetReader
/AssetWriter
. I'm going to leave a TODO here for now.
} | ||
|
||
/// Uses channels to create a [`Send`] + [`Sync`] wrapper around a [`Stream`]. | ||
pub(crate) struct IndirectStream<T> { |
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.
FutureExt::boxed
is enough to make the stream Send
. Is Sync
necessary?
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.
Unfortunately boxed
won't work in this case, since it requires the inner stream be Send
. The issue we have is the read_directory
method requires a stream which implements Stream<Item=PathBuf> + Send + Unpin
, but I can only produce a Stream<Item=PathBuf> + Unpin
because the inner JS handles are explicitly !Send
.
As such, the workaround I use here is to use channels to leave the !Send
resources in-place (thrown onto the JS task queue using spawn_local
), letting us send a dummy Stream
object that just wraps the channel.
I find this particularly frustrating as most of futures in WASM is !Send
already, so I'm half-tempted to make PathStream
ConditionalSend
as a workaround, but I want to avoid changing any existing APIs as much as possible.
)) | ||
} | ||
|
||
mod utils { |
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.
There's a lot of relatively generic WASM-async bridging code here in an inline submodule of opfs
- it rubs me the wrong way a little.
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.
I agree. Part of the reason why I've moved so much into here is to help split out the more generic code and (hopefully) replace it or give it a better home (maybe bevy_utils
with a WASM section?). I am hoping to remove at least some of these.
Co-Authored-By: Ricky Taylor <[email protected]>
Objective
temp:://
AssetSource
for Temporary Assets #13403Solution
Created a new
temp://
asset source which can be configured through theAssetPlugin
via a new optional fieldtemporary_file_path
. If a path is provided, it will be considered as a persistent temporary directory, otherwise a directory is created in a platform appropriate location which will be automatically deleted on application exit.Example Usage
A typical use for a temporary directory is the storage of data which you may need at some point, but don't right now (e.g., pre-fetching a map from a game server). In the below example, we create an asset
my_text_asset
, save it to thetemp://
source, and later retrieve it.Since the
temp://
source is on-disk, the unloaded asset will free memory for the rest of the application to use. In the above example, the memory used is trivial, but in a more complex game, a pre-fetched level could be significantly larger.Testing
Changelog
temp://
Asset Sourcepub temporary_file_path: Option<String>
toAssetPlugin
(defaults toNone
)temp_asset
, which demonstrates how to save and load arbitrary assets in the new temporary asset source.OriginPrivateFileSystem
for the WASM platform as anAssetReader
/AssetWriter
for the Origin Private File System APIMigration Guide
temp://
asset source ID, please use an alternate ID (e.g.,my_temp://
)AssetPlugin
manually, use..default()
to populate the new field with an appropriate default, or configure it yourself.