Arc-d assets

[andriyDev]

Objective

Fixes #15723. This is an alternative to #15346 (and the associated PR #15359) obsolete. Note if we decide asset locking is preferable, there are learnings from this PR we can apply to #15359.

Solution

1. Assets are now stored as Arc<A> instead of just A.

2. get remains the same, but I've added an alternative called get_arc which returns a clone of the Arc that stores an asset.

3. Replaced the get_mut with one of get_cloned_mut, get_inplace_mut, and get_reflect_cloned_mut (this last one is just because it's necessary for reflection, but not necessarily needed for users).

4. Added add_arc and insert_arc to allow adding Arc types directly.

   1. add currently takes Into<A>. Trying to change this to Into<Arc<A>> breaks a lot of callsites like meshes.add(Rectangle::new(1.0, 2.0)) (since this requires two Into calls, which Rust can't figure out).
   2. As a result, making add_arc take Into<Arc<A>> is a fine middle ground to allow both Arc-ed assets and owned assets.
   3. For completeness, I also added insert_arc.

5. RenderAsset now takes an Arc<Self::SourceAsset>. This means in most cases, cloning the asset can be skipped when passing data to the render world. Data only needs to be cloned if you mutate an asset.

Testing

• Added an example that shows off using an Arc-d asset in an async context.

---

Showcase

Assets can now be used in asynchronous contexts! By using Assets<A>::get_arc, you can get a clone of the underlying Arc of an asset, and pass it to an async closure.

fn my_task_launcher(configs: Res<Assets<ProceduralGenerationConfig>>, selected_config: Res<SelectedConfigHandle>) {
    let config = configs.get_arc(&selected_config.0).expect("The asset is loaded");
    
    AsyncComputeTaskPool::get().spawn(move async || {
        // Do something with the `config` in a task!
        std::thread::sleep(std::time::Duration::from_secs(config.time_it_takes_to_generate));
        println!("Done generating a level!");
    });
}

Migration Guide

• Assets::get_mut has been removed. Use get_cloned_mut for Clone assets, or get_in_place_mut for non-Clone assets (and handle the case where the asset is aliased).
• Assets::iter_mut now returns an iterator of &mut Arc<A>. Consider using Arc::make_mut for Clone assets, or Arc::get_mut for non-Clone assets (and handle the case where the asset is aliased).
• RenderAssets now take an Arc<Self::SourceAsset>. Consider using Arc::try_unwrap() and falling back to a clone if the asset is aliased (try_unwrap returns an error). Otherwise, prefer borrowing the source asset's data instead of moving it.

[BenjaminBrienen]

Can you fix the inconsistency between "[Arc]-ed" and "[Arc]d"? I prefer the latter.

[aecsocket]

I really like the concept of storing assets in an Arc. I find that I very rarely mutate existing assets, only add/remove/swap, so Arcs for assets make perfect sense to me. I am somewhat worried about a lot of usages of get_cloned_mut that this causes in the rest of the codebase though.

[andriyDev]

Can you fix the inconsistency between "[Arc]-ed" and "[Arc]d"? I prefer the latter.

Done!

[andriyDev]

No-op rebase to handle conflicts with #16368, and updated error to using thiserror after #16684.

[andriyDev]

@ElliottjPierce First, technically a file is not strictly necessary, however it is motivating. That is, if it wasn't a file, a reader might be confused - why are we making this an asset if we could have just passed this around as an Arc<LinearInterpolation> in a resource? So while I could technically make this work without a file, I think it explains the concept much better: we can load assets and still pass them around in async. That said, I did rename the file to match the name of the example.

Second, I am skeptical that assets as entities will really change much in terms of async. If anything, I expect it to make it harder to access assets in async, since now you need to synchronize with the whole world (or the whole asset world or something). I'm not saying we shouldn't do assets-as-entities (I very much want it), but I believe it is gonna end up being mostly orthogonal.

Next, this is absolutely a risk. However, I think the splitting of get_mut into get_cloned_mut and get_inplace_mut (and their respective doc comments will likely give people enough info on what's going on (I hope!). The other thing to note: both get_*_mut versions still require &mut Assets<A>. So it is still not possible to have two changes happen at the same time. So at least that is not at risk.

And yes, we are losing the "dense" nature of assets. But unlike components, I would expect most assets are fairly big anyway, and so won't benefit much from cache locality. Plus, assets are accessed in a more "random-access" way anyway. I think it's pretty rare to iterate over all assets. Users can still mimic the RwLock setup by just making their asset a RwLock wrapper.

I don't think this is the final solution - I agree, assets-as-entities may change things. But we should still support assets in async. This is necessary for asset saving (which I want to work on next).

Thanks for your review!!

[ElliottjPierce]

technically a file is not strictly necessary, however it is motivating.

Makes sense. Definitely worth keeping then, good call.

I am skeptical that assets as entities will really change much in terms of async.

Me too. I just mean if/when we have assets as entities in their own world, and that world is referenced by both the main and render world, etc, the cloning and sharing of assets between contexts won't be much of a motivation anymore. The async stuff is still a very strong motivation, but only for a subset of asset types that need to be referenced in a task or something. And it might be easier to do that with incremental systems or tasks in the asset world anyway. I know that context is hypothetical; just thinking ahead.

And yes, we are losing the "dense" nature of assets.

Good points here too. You've convinced me.

Users can still mimic the RwLock setup by just making their asset a RwLock wrapper.

I know we aren't loosing anything here. Just feels odd that we are standardizing the Arc pattern instead of leaving it up to the user whether to do A , Arc<A>, or Arc<RwLock<A>>. But I suppose Arc is the most common.

I don't think this is the final solution - I agree, assets-as-entities may change things.

Still, that's in the future, and as long as we're ok re-evaluating this in like a year, I do agree that this is the best solution until then.

Next, this is absolutely a risk.

I'm ok with the additional foot guns here. We just need a really good introduction of the concept. If you have label permissions, it might be worth adding needs-release-note, but IDK how Bevy does that process.

Thanks for your review!!

You've reviewed two of mine, so I still owe you one lol. And I'm excited for that saving work. Arcs certainly are the right choice for that, and I suppose it's a reasonable assumption that all assets should be "savable". It's always confused me why game engines have great systems for scriptable objects (unity), resources (godot), etc, but never make them savable at runtime. So I'm excited to see this come to Bevy, IIUC. Thanks!

[mockersf]

did you check the impact of this on rendering an asset heavy scene?

[mockersf]

the focus of the example should not be "arc_asset", it should be "async_access_to_asset" or something. arc is the technical detail, not the interesting feature

[andriyDev]

Good point! Done.

[mockersf]

really dislike the public api change, but not sure there's a better way to do it

[kristoff3r]

I don't understand how get_cloned_mut is ever the right thing to do. In case it's aliased you clone the asset, make your changes, but unless you store it somewhere or re-register with the asset system won't it just be dropped again? But none of the uses in the PR seems to do that.

I haven't read the entire PR so I might have missed something.

[andriyDev]

@kristoff3r get_cloned_mut uses Arc::make_mut. It takes a mutable reference to an arc (in this case, the arc that is stored in Assets). If the arc is aliases, it clones the data, makes a new arc, and then stores that arc in the mutable reference. This means the arc stored in Assets is replaced when you call get_cloned_mut for an aliased asset.

[kristoff3r]

@kristoff3r get_cloned_mut uses Arc::make_mut. It takes a mutable reference to an arc (in this case, the arc that is stored in Assets). If the arc is aliases, it clones the data, makes a new arc, and then stores that arc in the mutable reference. This means the arc stored in Assets is replaced when you call get_cloned_mut for an aliased asset.

Ah that makes total sense, thanks for explaining.

[andriyDev]

@mockersf Sorry I realized I didn't respond to your other question: I have not tested this on an asset heavy scene. Does Bevy have an example or stress test of this?

It is possible that the extra dereferences hurt performance, but that might not be too big a deal. We already put render assets in a hashmap so cache locality might already be hurting? Unsure.

[djeedai]

I'm not sure Arc is the right tool for the job. Sure, it solves the issue of thread safety and concurrent (read-only) access. But by design Arc and most shared pointers do not assign any preferred role to their owners. That is, all owners of an Arc<A> have the same "status" regarding the ownership and usage of A. In many usages that's a good thing, but it's not in the case with assets. The asset management system (for lack of a better name) which is in charge of loading (and saving; mostly in Editor) assets, and maintaining them resident in memory based on usage (under constrained memory budget) should have a much stronger role than all other "users" of an asset. With Assets<A> we have most of this, because Assets owns the asset and nobody else. With Arc we lose this, because by design the ownership becomes shared. And the fact anyone can keep an old version of an asset alive by mistake scares me; for having done that countless time by mistake with bind groups in rendering, this leads to many bugs which are often hard to track, because the asset may have changed only in subtle ways, and looking at the data in a debugger often tricks you into thinking it's still the correct one.

I would suggest investigating a custom smart pointer (or Arc with some additional wrapper which derefs to A?) which does something similar to Arc, but on top of that can mark the asset as "old"/"deprecated", so that any other user holding an Arc<A> can immediately realize that the asset is outdated and they need to pool Assets to refresh it. In fact, ideally that would happen automatically under the hood, if we can "repoint" the smart pointer to the newer asset. That way you cannot, by design, use an outdated asset. This may require a read-only pointer (no mutable access), which as mentioned should be the dominant use case anyway; in that case you'd have to go back to Assets<A> to mutate, but I think that's reasonable?

Thoughts?

[JMS55]

Fyi MeshletMesh is an example of a read-only asset that I use Arc to share with the render world cheaply.

In general, I wish we could share assets between the main/render world without any copies, in the same way that bevy schedules systems across multiple threads to avoid conflicts.

[alice-i-cecile]

@cart @andriyDev I'm going to cut arc'd assets PR [from the 0.16 milestone]. I know that this is overdue for a review (and generally I'm supportive of it), but I don't think it's high enough priority to ship something high risk at the last minute or delay the release candidate

Me on Discord

[ElliottjPierce]

I made a discussion post #18949 that is an alternative to this. I don't have a crazy strong opinion, but I think my proposal is worth investigating before merging this. Just in case.

[ElliottjPierce]

Just a head's up that @andriyDev and I have come up with a followup to this PR. See here.

I think we need to start with merging this pr, but I think in the end, we'll have a less breaking api change and a much more flexible system.