Skip to content

Conversation

janis-bhm
Copy link
Contributor

Objective

fixes #20651, #20776

Solution

the AssetHandleProvider gets a mutex'd hashmap of all handles it has given out and the Assets and AssetServer use this to synchronise their respective assets and asset state.
Even though this introduces a lock to an otherwise lockless type, after trying to share the handle between AssetServer and Assets via the asset events I believe that this is the better and simpler approach.
Opening this as a draft though because I think it's controversial.

calling get_strong_handle() with a UUID asset now also returns a UUID handle so that track_assets doesn't get a drop even for UUID assets which don't have automatic dropping behaviour.

Testing

ci doesn't work on my machine, so I did cargo test --workspace --lib --tests

@andriyDev
Copy link
Contributor

Don't keep this as a draft just because it's controversial please. That's what the controversial tag is for! However if it's unfinished, then keeping it in draft is correct.

@andriyDev
Copy link
Contributor

One thing I want to note here: Assets is a lockless type, however AssetServer is not! Loading assets already locks the entire AssetServer, so we'd just be acquiring an extra lock which would be largely uncontested - get_strong_handle is likely a very rarely used function.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 5, 2025
@janis-bhm janis-bhm marked this pull request as ready for review September 5, 2025 12:16
@janis-bhm
Copy link
Contributor Author

Don't keep this as a draft just because it's controversial please. That's what the controversial tag is for! However if it's unfinished, then keeping it in draft is correct.

Alright, I'm pretty confident this is complete. I don't think I can add tags myself, so I wasn't sure how to best communicate the unreadiness in the sense of "I definitely want people to voice their opinions on this before considering accepting it".

@hukasu
Copy link
Contributor

hukasu commented Sep 5, 2025

would this be needed with the move towards assets as entities?

@andriyDev
Copy link
Contributor

@hukasu I think so. Nothing here actually cares about how the asset is stored, the problem is synchronizing between the system dropping the assets (which could be depending entities instead) and the asset server which is doing everything outside the ECS through locks. The system doing the drop needs to make sure there isn't a race condition with the asset server where it hands out a handle to an asset that has just been despawned.

@janis-bhm
Copy link
Contributor Author

main...janis-bhm:bevy:asset-strong-handle-lockless
alternate solution without lock but which keeps the handles separate, meaning multiple StrongHandles may point at a single asset at any point in time.

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-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropping a handle from Assets::get_strong_handle has different behaviour to one from AssetServer::load.
4 participants