unifying Asset<A> / AssetServer handles (#20651, #20776)#20874
unifying Asset<A> / AssetServer handles (#20651, #20776)#20874janis-bhm wants to merge 7 commits intobevyengine:mainfrom
Asset<A> / AssetServer handles (#20651, #20776)#20874Conversation
|
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. |
|
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. |
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". |
|
would this be needed with the move towards assets as entities? |
|
@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. |
|
main...janis-bhm:bevy:asset-strong-handle-lockless |
crates/bevy_asset/src/lib.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn uuid_asset_strong_handle() { |
There was a problem hiding this comment.
This test name confused me slightly because I didn't realize this PR actually removes the ability get a handle that can drop the UUID asset - what is absolutely a pro! Can we rename this to get_strong_handle_for_uuid_asset?
As an aside, we may want to rename get_strong_handle to get_handle_from_id - since weak handles no longer exist! That should be left to another PR though I think.
- test against asset duplication when dropping asset server handle only - test assets<A> handles can keep asset info alive
…setServer`'s `AssetInfo`
…flag in drop event
9330734 to
b37b832
Compare
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