Description
openedon Sep 21, 2024
What problem does this solve or what need does it fill?
Today, there is no way to access an asset from a thread/async block. The best you can do is clone the asset entirely, or make your asset a wrapper around an Arc with all the data. This is especially painful for #11216, which needs to be able to send assets (and their subassets) away to be serialized.
What solution would you like?
Asset locking! A user can lock an asset, which puts the asset into an Arc. This blocks mutating the asset until the asset is unlocked. Now the asset can be passed to threads and read from there.
Non-goal: being able to mutate an asset from an async block. While we could support this, I don't think we should. This would mean requiring a Mutex or similar around the asset while it's locked, which would slow down everything accessing the asset, not just the async block.
Expected usage
- A system takes a
ResMut<Assets<A>>
. - The system calls
let asset_arc = assets.lock(my_handle)
. - A task or similar is spawned that is given the
Arc
. - The task reads the
Arc
to do its processing. - The task finishes its work and drops the
Arc
. - The asset is automatically (after a world update) unlocked, returning it to normal asset storage.
Design proposal
-
Both dense and hash storage store an
AssetPresence
(bikeshedding ready), which looks like:enum AssetPresence<A> { None, Unlocked(A), Locked(Arc<A>), }
When inserting an asset into
Assets<A>
, it begins in theUnlocked
state. Note, it's actually totally fine to replace a locked asset. We're not locking theAssetId
, we're locking the actual data. So just throwing way the existingArc
and replacing it is fine. This does lead to the slightly odd case that a locked asset may still trigger anAssetEvent::Modified
. This seems fine, since relying on anAssetId
remaining locked is weird and point 6 also makes this some more reasonable. -
Two new functions are added to
Assets<A>
to lock and unlock assets:pub fn lock(&mut self, id: impl Into<AssetId<A>>) -> Option<Arc<A>> { // Lock the asset if it exists. }
This either returns
None
(if the asset doesn't exist of course), moves the asset to the locked state (if the asset was unlocked), or just clones the existingArc
(if the asset was already locked).pub fn try_unlock(&mut self, id: impl Into<AssetId<A>>) -> Result<(), UnlockAssetError> { // Unlock the asset if it exists, and there is only one strong reference to the Arc. }
This either returns Ok if the asset was successfully, an error if the asset doesn't exist, or an error if the asset is still being used. Unfortunately, this has no way of telling you what is keeping the asset locked. This is because cloning the
Arc
has no way of reporting "why" it was cloned. We could make our own wrapper aroundArc
to also record its reason for being cloned so here we can figure out what is locking an asset, but the complexity of this seems non-trivial and I'm not convinced the value is worth it (how often will you really not know why an asset is locked?).The reason for the unlock function being public is because in theory a system could lock an asset and then unlock the asset in the same frame (if they don't need the lock). Otherwise, the system could only lock the asset, and then have to wait a frame for the asset to become unlocked again. This is likely a rare case, but if we have to implement the next system anyway, this is a nice function to do so.
Optional:
try_unlock
could also fail if the asset is already unlocked. It's not clear though how this information will benefit you. Maybe you could filter an asset iterator by whether the asset is unlocked? -
A new system is added to
Assets<A>
to automatically unlock assets.fn unlock_assets(mut assets: ResMut<Self>) { // Try unlocking all assets. }
This system will iterate through all assets and try to unlock them. This system will be automatically added when adding asset types. This way, you don't need to worry about unlocking assets, you just lock them and then some time in the future, they are unlocked.
-
Assets::get_mut
can now return an error (not justNone
).pub fn get_mut(&mut self, id: impl Into<AssetId<A>>) -> Result<&mut A, MutableAssetError>
There are now two reasons you can't get an asset mutably: either it's not present, or it's locked. In either case, returning a value doesn't really make sense. This does mean, if you want to handle the case where the asset is locked, you have to do an
Assets::get
call to get the locked asset, but I bet most of the time you want toget_mut
there is no fallback.Note
Assets::get
is pretty much unchanged (from an API point of view). If theAssetPresence
isNone
returnOption::None
. If the asset is unlocked, get a borrow to the asset. If the asset is locked, get a borrow to the value in theArc
. Nothing fancy! -
Assets::iter_mut
now returnsAssetPresenceMut
(bikeshedding intensifies).pub enum AssetPresenceMut<'a, A> { Unlocked(&'a mut A), Locked(Arc<A>), }
While for
Assets::get_mut
I don't think it makes sense to return theArc
d value, for an iterator I think we should provide theArc
d value. It would be incredibly confusing if an asset ID was missing from this iterator if the asset was just locked. -
Assets::remove
now returns anAssetPresence
.Removing an asset no longer gives you an owned asset. It may give you a locked asset. Therefore, we need to be able to return either the owned asset or the
Arc
d asset.We could choose to prevent removing locked assets (and similarly prevent inserting over locked assets), but it's not clear what we're winning here and is just adding more ways to fail.
-
ReflectAsset
needs to match.-
get_unchecked_mut
andget_mut
both need to returnResult<&mut dyn Reflect, MutableAssetError>
to matchAssets::get_mut
. -
remove
needs to return aReflectedAssetPresence
pub enum ReflectedAssetPresence { None, Unlocked(Box<dyn Reflect>), Locked(Arc<dyn Reflect>), }
-
What alternative(s) have you considered?
As discussed at the start, you can clone your data or Arc your data. Both of those are impractical. Assets can be large, so cloning the data is not practical. Arc-ing your data may not be possible if you don't own the type (e.g., Bevy's Mesh
asset can't be Arc'ed externally without rewriting ~all of Bevy rendering), and also leads to your data being permanently immutable.
Additional context
This idea came from the Bevy discord!