send Unused event when asset is actually unused

[robtfm]

Objective

fix #12344

Solution

use existing machinery in track_assets to determine if the asset is unused before firing Asset::Unused event

most extract functions use AssetEvent::Removed to schedule deletion of render world resources. RenderAssetPlugin was using AssetEvent::Unused instead.
Unused fires when the last strong handle is dropped, even if a new one is created. Removed only fires when a new one is not created.
as far as i can see, Unused is the same as Removed except for this "feature", and that it also fires early if all handles for a loading asset are dropped (Removed fires after the loading completes). note that in that case, processing based on Loaded won't have been done anyway.
i think we should get rid of Unused completely, it is not currently used anywhere (except here, previously) and i think using it is probably always a mistake.
i also am not sure why we keep loading assets that have been dropped while loading, we should probably drop the loader task as well and remove immediately.

[mockersf]

this was added in #10520 to allow removing assets from the cpu side and keeping them on the gpu side

[alice-i-cecile]

i think we should get rid of Unused completely, it is not currently used anywhere (except here, previously) and i think using it is probably always a mistake.

Agreed. Different PR of course, so then we can ship this in 0.13.1.

i also am not sure why we keep loading assets that have been dropped while loading, we should probably drop the loader task as well and remove immediately.

This makes sense to me too.

[robtfm]

@JMS55 on discord:

That doesn't seem right. The whole point was I added unused in 0.13 specifically so we would know when to drop a render asset without using removed, as we'll remove it even though it's still in use by the GPU ...

i missed that Removed is also fired on extract when RenderAssetUsages is !MAIN_WORLD.

so instead we can use the existing logic for processing the automatic removal (in track_assets) to determine whether to send the Unused event, as well as whether to attempt to unload the asset.

[bushrat011899]

Mostly just some minor style suggestions. Otherwise, makes sense!

[robtfm]

i think the resending of not_ready events might break this. i reworked it to use a local instead of resending, but that requires DropEvent to be pub or track_assets to be pub(crate) - is it a breaking change to make a type public?

[alice-i-cecile]

is it a breaking change to make a type public

No :)

[robtfm]

i think the resending of not_ready events might break this. i reworked it to use a local instead of resending, but that requires DropEvent to be pub or track_assets to be pub(crate) - is it a breaking change to make a type public?

nope, the code is fine, sorry. i think this is good as is

[JMS55]

Tbh I'm having a hard time reviewing this, as the code is a bit confusing. I'm not quite sure I understand the implications of moving it around like this.

[robtfm]

Tbh I'm having a hard time reviewing this, as the code is a bit confusing. I'm not quite sure I understand the implications of moving it around like this.

Have a look at the related issue. the important thing is that the process_handle_drop call checks whether new handles have been created since the drop event was fired, before removing the asset. We now perform that check before sending the unused event as well.

[JMS55]

Can we expand this comment a bit with the context you put in your last comment to me? That this is a check to see if a new handle has been created in the same frame after the last one was dropped, meaning it's not actually unused.

Also, what does "or the asset doesn't exist" mean here?

[robtfm]

it's what the comment in the process_handle_drop function says. i don't know a scenario where it would happen.

[JMS55]

Thanks that clears it up for me. Approved, but I'd like some more verbose doc comments.