-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adopted: Track changed Image assets and use them to update image caches #547
Conversation
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.
Handles have some tie to lifetimes of assets. AssetIds don't!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I spot-checked the basic example with and without atlas.
screenshot-2024-07-31-at-20.38.08.mp4
.retain(|texture, _| texture_is_unmodified(texture)); | ||
texture_cache.prepare_queue.retain(texture_is_unmodified); | ||
texture_cache.queue_queue.retain(texture_is_unmodified); | ||
texture_cache.bad_flag_queue.retain(texture_is_unmodified); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only strange thing I noticed was we don't seem to actually be using bad_flag_queue
anywhere. What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fascinating. I don't think it has ever been used. It seems to have originated here: 020d10e
src/tiles/storage.rs
Outdated
@@ -47,7 +47,7 @@ impl TileStorage { | |||
/// Gets a tile entity for the given tile position, if: | |||
/// 1) the tile position lies within the underlying tile map's extents *and* | |||
/// 2) there is an entity associated with that tile position; | |||
/// otherwise it returns `None`. | |||
/// otherwise it returns `None`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is supposed to be a second paragraph, not a continuation of the second item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good catch.
Original PR text:
Fixes #430
Alternative to #431
Testing
I modified
tiles.png
and observed hot reloading occurring with:cargo run --example basic --features=bevy/file_watcher
cargo run --example basic --features=atlas --features=bevy/file_watcher