Skip to content

Change RenderAsset::unload_asset to be called whenever RenderAsset would be dropped#24250

Open
larsraph wants to merge 5 commits into
bevyengine:mainfrom
larsraph:render-asset-unload-on-drop
Open

Change RenderAsset::unload_asset to be called whenever RenderAsset would be dropped#24250
larsraph wants to merge 5 commits into
bevyengine:mainfrom
larsraph:render-asset-unload-on-drop

Conversation

@larsraph
Copy link
Copy Markdown

Objective

  • Currently RenderAsset::unload_asset (internally used only for morph) is called when the source asset is removed from the MainWorld. I beleive this functionality is limiting and should be called whenever the RenderAsset is about to be dropped (but giving ECS access).
  • The reason I think that is because I think the idiomatic pattern for indirectly owning GPU resources is to receive and store a Handle inside the RenderAsset then free the indirect resource when the RenderAsset is dropped through the unload_asset function.

Solution

  • Change the call location of unload_asset.
  • make unload_asset take self param
  • Change the MorphTargetsAllocator to properly handle the new system.
    • Currently it maps AssetID -> MorphTargetsImage.
    • The main issue is that on Update we call RenderMorphTargetsAllocator::allocate again in prepare_resources without calling RenderMorphTargetsAllocator::free (b/c free is only called when the source asset is removed) inserting the new value into the map and implicitly replacing and dropping the old value. BUT with our new system where free would be called after every update (when the old asset is dropped) then keying the allocation with AssetID the above will still happen followed by the RenderAsset::unload_asset impl which drops the allocation we just inserted. In order to fix this I stopped keying the RenderMorphTargetsAllocator and stored MorphTargetsImageHandle in RenderMesh.
      • A possible downside to this change:
        There is not much of a difference between this system and just storing the MorphTargetsImage directly in the RenderMesh. So if the goal in using the RenderMorphTargetsAllocator in the first place was to avoid bloating the RenderMesh with conditional data (RenderMorphTargetsAllocator and the MorphTargetsImage is only actually active when certain features are enabled on the target device) then the new system mostly fails. (although the Option<Handle> takes 8 bytes vs however many bytes MorphTargetsImage takes)

I also want to eventually change RenderMesh to store a handle to MeshAllocator but is unneccecary for this PR and also complicated due to batching.

Testing

  • I've run it through the ci crate.

Notes

  1. I used a pattern where the Handle is not Clone or Copy + free() taking ownership of the handle. This means we can read from the Handle w/o checking if it has an associated item (it cannot have been freed) and we can free from it w/o worrying abt double free. However I think this pattern might not be suited for a large project such as bevy. I can change it relatively easily to do checked access instead. Going the other direction if we want to double down on this pattern we can do unchecked access instead of an expect("").
  2. I added an error!() logging for a MorphBuildError that was ignored. If this PR doesn't get merged a separate PR should add that.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant