bevy_ecs_tilemap
bevy_ecs_tilemap copied to clipboard
StarArawn
Track changed Image assets and use them to update image caches.
Previously, since the bind groups do not get updated every frame, modified image assets would not be rebound. This caused hot reloading to no longer work. Similarly, the TextureArrayCache (when not using atlas) would not invalidate modified assets, so the textures would become out-of-date.
Note there can be other reasons that an asset can be modified, so this should keep the assets more up-to-date in general.
Fixes #430.
Rebased onto main to fix cargo clippy which was broken at HEAD at the time.
Can it be merged to main? This commit fixes the issue with updating texture on a map.
I will rebase this once #440 goes through. I still need a review though for the overall structure of this solution.
I've rebased this onto main after #440. I tested this with a little example and it still works.
@andriyDev is it possible to use https://docs.rs/bevy/latest/bevy/render/render_asset/struct.ExtractedAssets.html instead of doing the manual collecting/extraction?
It seems like the answer to the above is probably no.
Sorry, but I don't think I can provide a meaningful review. I don't think I'll be able to get up-to-speed on this area of bevy_ecs_tilemap / bevy any time soon.
But this does fix hot reloading for me and doesn't seem to cause other issues as far as I can tell.
In theory I think I could extract the EventReader<AssetEvent<Image>> resource, then do the collection in a render system, but since we need the modified images in queue_material_tilemap_meshes and remove_modified_textures, we'd still need the ModifiedImageHandles resource. It's not clear to me that that would be worth it? But if that seems nicer, I can make that change!
Looks like CI is just failing due to new let-else formatting in rust 1.72 in an unrelated file.
What should I do in regards to https://github.com/StarArawn/bevy_ecs_tilemap/pull/431#issuecomment-1732363616 ? I'm happy to change it if that looks cleaner. They both look roughly the same to me.
@andriyDev Can we get the conflicts fixed up so we can get this merged in? :+1:
Alright I rebased onto main. I also used AssetIds in place of Handles, since AssetIds seem to be the more "canonical" identifier for assets. I tested this locally and it still works for hot reloading!
@StarArawn I think this is good to go :)
Hey @andriyDev, could I bug you to update the branch again? It looks like a relatively straightforward rebase. We should be in a better position to try to get this merged now.
If not, I'd be happy to adopt the PR.