Fix handles being split by asset_server_managed.

[andriyDev]

Objective

• Fixes Dropping a handle from Assets::get_strong_handle has different behaviour to one from AssetServer::load. #20651.
• Alternative to unifying Asset<A> / AssetServer handles (#20651, #20776) #20874.
• Also a step towards Do not unload subassets or the root asset unless all subassets and root asset handles have been dropped. #21878.

Solution

• When allocating a new handle (whether for an existing asset or a new asset), send a "new" message to the same channel as "drop" events.
  • This essentially means we're "leaning in" to the fact that we can have multiple live handles to the same asset.
• Assets::track_assets now increments when a "new" message is received, and decrements when a "drop" message is received. The asset entry is dropped if the counter is at 0 after processing all messages.
• Remove the asset_server_managed flag, since we can just lookup the index to see if it's present.
• Remove the handle_drops_to_skip counter since now we count all new handles, not just those in get_strong_handle.

Testing

• Unbroke our reproduction test.
• Added a new test to verify that "reacquiring" an asset doesn't drop the asset.

[kristoff3r]

LGTM, the logic in track_assets felt a bit complicated but I couldn't find a way to simplify it.

[kristoff3r]

I don't see why this import needs to be inside this function.

[andriyDev]

This file defines a type Entry, so the hashmap Entry collides with it. Scoping it here sidesteps that issue.

[janis-bhm]

I might be wrong here, but isn't this technically a race condition? There are a bunch of lock-shaped things here, the &mut Assets<A> (which prevents get_strong_handle from being called during track_assets) and the write guard on the infos (which prevents access to the server's handle provider list, as well as load_* calls), but none of those actually prevent new HandleEvent::New(_) messages being sent or get_handle from being called to create a new strong handle.
I'm not sure if this is an actual problem: I don't think this is currently possible because the HandleProvider/Sender isn't exposed beyond the pub(crate) scope.

[andriyDev]

Yup that's exactly correct, the only thing preventing us from a race condition here is the fact that we have exclusive access to Assets and are locking the asset server - there's no other way to get a new handle to an existing asset. The only other way to send a HandleEvent::new is with AssetHandleProvider::reserve_handle, but that only gives you handles to a new asset, so we can't have accidentally dropped it yet.

I will have added a comment here to explain that. The existing comment about the lock being transactional is insufficient.

[janis-bhm]

I think it would be nice to find a way to get rid of the weak handle reference in the asset info and use the Asset<A>s new "strong count"; the weak handle is still used in is_path_alive and get_index_handle for checking liveness and getting the asset's handle, which will fail if the original handle was dropped and a new handle was created by the handle provider.

The modified test below fails in this PR:

Details

    #[test]
    fn asset_is_live_after_reacquired() {
        let (mut app, dir) = create_app();

        dir.insert_asset_text(Path::new("dep.cool.ron"), SIMPLE_TEXT);

        app.init_asset::<CoolText>()
            .init_asset::<SubText>()
            .register_asset_loader(CoolTextLoader);

        let asset_server = app.world().resource::<AssetServer>().clone();

        let handle = asset_server.load::<CoolText>("dep.cool.ron");
        run_app_until(&mut app, |_| asset_server.is_loaded(&handle).then_some(()));

        // Dropping the handle and then reacquiring it through a load should **not** result in the
        // asset being dropped. Note: we won't reload the asset in any case since we only try to
        // drop on the next update.
        let new_handle = app
            .world_mut()
            .resource_mut::<Assets<CoolText>>()
            .get_strong_handle(handle.id())
            .expect("should yield strong handle");
        drop(handle);

        app.update();

        assert!(app
            .world()
            .resource::<Assets<CoolText>>()
            .contains(&new_handle));

        let asset_server = app.world().resource::<AssetServer>().clone();
        assert!(asset_server.read_infos().is_path_alive("dep.cool.ron"));
        let index =
            ErasedAssetIndex::try_from(new_handle.id().untyped()).expect("should yield index");
        assert!(asset_server.read_infos().get_index_handle(index).is_some());
    }

[andriyDev]

@janis-bhm That was a good catch! I didn't know these methods existed. Now when you call AssetServer::get_id_handle, it will allocate a new handle if possible. We now ignore the weak_count entirely and just say if the entry still exists, you can get a handle to it. I also entirely removed the is_path_alive, inlined it in the only place its used, and simplified it to assume that if the entry exists, the path still exists.

@kristoff3r I've added some not entirely trivial changes, so I'd like another review just in case.