Allow `AssetServer::load` to acquire a guard item. #13051

mintlu8 · 2024-04-21T13:32:04Z

Objective

Supercedes #12881 . Added a simple implementation that allows the user to react to multiple asset loads both synchronously and asynchronously.

Solution

Added load_acquire, that holds an item and drops it when loading is finished or failed.

When used synchronously

Hold an Arc<()>, check for Arc::strong_count() == 1 when all loading completed.

When used asynchronously

Hold a SemaphoreGuard, await on acquire_all for completion.

This implementation has more freedom than the original in my opinion.

alice-i-cecile

Could you add an example demonstrating how one might use this? This seems like a really neat design, but making the jump from "cool API" to "practical use" has been a perpetual challenge for users of bevy_asset.

crates/bevy_asset/src/server/mod.rs

github-actions · 2024-05-17T01:47:59Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

github-actions · 2024-05-17T01:59:03Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

mintlu8 · 2024-05-17T02:20:45Z

Could you add an example demonstrating how one might use this? This seems like a really neat design, but making the jump from "cool API" to "practical use" has been a perpetual challenge for users of bevy_asset.

Added an example to show how this works.

examples/asset/multi_asset_sync.rs

alice-i-cecile

Okay, that example was super useful! I now understand the point of this PR much better, and now it can be more easily taught and tested.

I've left a few comments for suggestions on how to make the example a bit gentler on beginners :) I think that I would also hook into Bevy's states to define whether or not asset loading is done, but I'm happy to get pushback if you feel otherwise.

mintlu8 · 2024-05-18T01:46:43Z

Okay, that example was super useful! I now understand the point of this PR much better, and now it can be more easily taught and tested.

I've left a few comments for suggestions on how to make the example a bit gentler on beginners :) I think that I would also hook into Bevy's states to define whether or not asset loading is done, but I'm happy to get pushback if you feel otherwise.

Thanks! I made a few more updates to the example. I think it is now more useful as a template or as a guide to beginners.

examples/asset/multi_asset_sync.rs

alice-i-cecile

Much improved! It's nice to see Bevy's states playing nicely with this style of logic.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Added dev-dependency to event_listener. This is probably fine since `bevy_asset` and most of the `smol` ecosystem depend on it.

bushrat011899

This is a nice PR overall, thanks for writing it up! I just have a couple of feedback items around the example which I think will help others understand what's happening.

Cargo.toml

bushrat011899 · 2024-05-23T05:33:22Z

examples/asset/multi_asset_sync.rs

+    }
+
+    /// Wait for all [`AssetBarrierGuard`]s to be dropped asynchronously.
+    pub fn wait_async(&self) -> impl Future<Output = ()> + 'static {


Nit: this should probably just be wait since there won't be a sync alternative (a blocking wait would be an anti-pattern).

I think this is needed since impl Future<Output = ()> + 'static is not 100% obvious to a new rust user that this is an async function.

That is also a fair point, happy with that as a reason!

Cargo.toml

examples/asset/multi_asset_sync.rs

Co-authored-by: Zachary Harrold <zac@harrold.com.au>

ricky26

This is cool. Sorry for getting in the way! I'm happy to help with the builder-related shenanigans.

ricky26 · 2024-05-23T10:07:23Z

crates/bevy_asset/src/server/mod.rs

+    ///
+    /// The asset load will fail and an error will be printed to the logs if the asset stored at `path` is not of type `A`.
+    #[must_use = "not using the returned strong handle may result in the unexpected release of the asset"]
+    pub fn load_acquire<'a, A: Asset, G: Send + Sync + 'static>(


~~I didn't expect the builder-pattern to land quite so quickly (sorry about that), but it might be worth replacing these two functions with one in NestedLoader.~~ Ignore that, I misread which file this was in!

It might also make sense to support it for untyped handles, but I don't think that has to happen now.

Yeah I think getting these ready is more important to keep the scope of this PR small.
There is already a public async function for loading untyped assets so half of the use cases are already covered.

ricky26 · 2024-05-23T10:23:22Z

examples/asset/multi_asset_sync.rs

+        // This showcases how to wait for assets synchronously.
+        .add_systems(Update, wait_on_load)
+        // This showcases how to wait for assets asynchronously.


nit: Both of these are asynchronous (in the more general sense), they even both poll. Might be worth phrasing it more like "how to poll until assets are loaded" and "how to wait for assets to load in a background task".

Rephrased. I think "sync code" and "async code" are better descriptors than "synchronously" and "asynchronously". Added some explanations on polling as well.

Moved `main` on top. Used run condition to check whether assets are loaded. Separated setup methods for scene, ui and assets.

JMS55 added C-Enhancement A new feature A-Assets Load files from disk to use for things like images, models, and sounds labels Apr 21, 2024

JMS55 added this to the 0.14 milestone Apr 21, 2024

alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 16, 2024

alice-i-cecile requested changes May 16, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 16, 2024

alice-i-cecile removed this from the 0.14 milestone May 16, 2024

mockersf reviewed May 17, 2024

View reviewed changes

crates/bevy_asset/src/server/mod.rs Outdated Show resolved Hide resolved