Skip to content

Asset locking #15346

Open
Open

Description

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

  1. A system takes a ResMut<Assets<A>>.
  2. The system calls let asset_arc = assets.lock(my_handle).
  3. A task or similar is spawned that is given the Arc.
  4. The task reads the Arc to do its processing.
  5. The task finishes its work and drops the Arc.
  6. The asset is automatically (after a world update) unlocked, returning it to normal asset storage.

Design proposal

  1. 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 the Unlocked state. Note, it's actually totally fine to replace a locked asset. We're not locking the AssetId, we're locking the actual data. So just throwing way the existing Arc and replacing it is fine. This does lead to the slightly odd case that a locked asset may still trigger an AssetEvent::Modified. This seems fine, since relying on an AssetId remaining locked is weird and point 6 also makes this some more reasonable.

  2. 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 existing Arc (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 around Arc 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?

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

  4. Assets::get_mut can now return an error (not just None).

    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 to get_mut there is no fallback.

    Note Assets::get is pretty much unchanged (from an API point of view). If the AssetPresence is None return Option::None. If the asset is unlocked, get a borrow to the asset. If the asset is locked, get a borrow to the value in the Arc. Nothing fancy!

  5. Assets::iter_mut now returns AssetPresenceMut (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 the Arcd value, for an iterator I think we should provide the Arcd value. It would be incredibly confusing if an asset ID was missing from this iterator if the asset was just locked.

  6. Assets::remove now returns an AssetPresence.

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

  7. ReflectAsset needs to match.

    • get_unchecked_mut and get_mut both need to return Result<&mut dyn Reflect, MutableAssetError> to match Assets::get_mut.

    • remove needs to return a ReflectedAssetPresence

      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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    A-AssetsLoad files from disk to use for things like images, models, and soundsLoad files from disk to use for things like images, models, and soundsC-FeatureA new feature, making something new possibleA new feature, making something new possibleD-ModestA "normal" level of difficulty; suitable for simple features or challenging fixesA "normal" level of difficulty; suitable for simple features or challenging fixesS-Ready-For-ImplementationThis issue is ready for an implementation PR. Go for it!This issue is ready for an implementation PR. Go for it!

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions