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

Implementation of temp:// Asset Source #13406

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented May 17, 2024

Objective

Solution

Created a new temp:// asset source which can be configured through the AssetPlugin via a new optional field temporary_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 the temp:// source, and later retrieve it.

// This is the asset we will attempt to save.
let my_text_asset = TextAsset("Hello World!".to_owned());

// To ensure the `Task` can outlive this function, we must provide owned versions
// of the `AssetServer` and our desired path.
let path = AssetPath::from("temp://message.txt").into_owned();
let server = assets.clone();

// Pass to the IoTaskPool to complete in the background
IoTaskPool::get().spawn(async move {
    save_asset(my_text_asset, path, server, TextSaver)
        .await
        .unwrap();
}).detach();

// ...

// Can later retrieve the saved asset
let my_text_asset = assets.load("temp://message.txt");

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

  • Tested the included example on my local devices (Windows and Linux).
  • Have not tested on mobile (I suspect iOS may be problematic here but cannot confirm).
  • Tested in Firefox when compiling to WASM.

Changelog

  • Added temp:// Asset Source
  • Added pub temporary_file_path: Option<String> to AssetPlugin (defaults to None)
  • Added a new example, temp_asset, which demonstrates how to save and load arbitrary assets in the new temporary asset source.
  • Added OriginPrivateFileSystem for the WASM platform as an AssetReader / AssetWriter for the Origin Private File System API

Migration Guide

  • If you were using the temp:// asset source ID, please use an alternate ID (e.g., my_temp://)
  • If constructing AssetPlugin manually, use ..default() to populate the new field with an appropriate default, or configure it yourself.

@bushrat011899 bushrat011899 added C-Enhancement A new feature A-Assets Load files from disk to use for things like images, models, and sounds C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 17, 2024
crates/bevy_asset/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_asset/src/temp.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/temp.rs Outdated Show resolved Hide resolved
examples/asset/temp_asset.rs Outdated Show resolved Hide resolved
examples/asset/temp_asset.rs Show resolved Hide resolved
@bushrat011899 bushrat011899 force-pushed the TemporaryAssetSource branch 3 times, most recently from 4a1e474 to 6ead01d Compare May 20, 2024 07:32
@bushrat011899 bushrat011899 changed the title Initial Implementation of temp:// Asset Source Implementation of temp:// Asset Source May 20, 2024
@bushrat011899 bushrat011899 marked this pull request as ready for review May 20, 2024 07:38
crates/bevy_asset/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/temp.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/temp.rs Outdated Show resolved Hide resolved
@bushrat011899 bushrat011899 marked this pull request as draft May 26, 2024 12:41
@bushrat011899
Copy link
Contributor Author

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 AssetReader and an AssetWriter which are suitable for use with the proposed temp:// asset source.

I'm marking this as draft because while I have these implementations working in the browser (tested on Firefox using temp_asset example), I believe there is room for improvement in the exact implementations, and the potential for an AssetWatcher as well for completeness.

I would encourage anyone with wasm-bindgen experience (especially if you've worked with the OPFS API!) to leave any remarks as I will gladly accept and greatly appreciate any feedback!

bushrat011899 and others added 4 commits May 27, 2024 22:15
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 <262786+ricky26@users.noreply.github.com>
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.
@bushrat011899 bushrat011899 force-pushed the TemporaryAssetSource branch 2 times, most recently from 90ec52c to 4ffc2bb Compare May 27, 2024 12:27
Split out certain utility methods which are more general WASM items.
Copy link
Contributor

@ricky26 ricky26 left a 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(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

crates/bevy_asset/src/temp.rs Outdated Show resolved Hide resolved
examples/asset/temp_asset.rs Outdated Show resolved Hide resolved
}

/// Get the [`FileSystemDirectoryHandle`] for the root directory pointed to by `self.root`.
pub(crate) async fn shadow_root(&self) -> std::io::Result<FileSystemDirectoryHandle> {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

crates/bevy_asset/src/io/wasm/opfs.rs Outdated Show resolved Hide resolved
}

/// Uses channels to create a [`Send`] + [`Sync`] wrapper around a [`Stream`].
pub(crate) struct IndirectStream<T> {
Copy link
Contributor

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?

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 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.

crates/bevy_asset/src/io/wasm/opfs.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/wasm/opfs.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/wasm/opfs.rs Outdated Show resolved Hide resolved
))
}

mod utils {
Copy link
Contributor

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.

Copy link
Contributor Author

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 <262786+ricky26@users.noreply.github.com>
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add temp::// AssetSource for Temporary Assets
2 participants