-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add tools to avoid unnecessary AssetEvent::Modified events that lead to rendering performance costs (#16751)
#22460
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
Changes from 4 commits
031302f
412b115
3b95149
1e900ae
77d13a1
4b19d64
27cee2c
9ea57ee
6c4eed9
9a2f557
57bb5c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ use bevy_reflect::{Reflect, TypePath}; | |
| use core::{any::TypeId, iter::Enumerate, marker::PhantomData, sync::atomic::AtomicU32}; | ||
| use crossbeam_channel::{Receiver, Sender}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::ops::{Deref, DerefMut}; | ||
| use thiserror::Error; | ||
| use uuid::Uuid; | ||
|
|
||
|
|
@@ -348,7 +349,7 @@ impl<A: Asset> Assets<A> { | |
| &mut self, | ||
| id: impl Into<AssetId<A>>, | ||
| insert_fn: impl FnOnce() -> A, | ||
| ) -> Result<&mut A, InvalidGenerationError> { | ||
| ) -> Result<AssetMut<'_, A>, InvalidGenerationError> { | ||
| let id: AssetId<A> = id.into(); | ||
| if self.get(id).is_none() { | ||
| self.insert(id, insert_fn())?; | ||
|
|
@@ -436,16 +437,20 @@ impl<A: Asset> Assets<A> { | |
| /// Retrieves a mutable reference to the [`Asset`] with the given `id`, if it exists. | ||
| /// Note that this supports anything that implements `Into<AssetId<A>>`, which includes [`Handle`] and [`AssetId`]. | ||
| #[inline] | ||
| pub fn get_mut(&mut self, id: impl Into<AssetId<A>>) -> Option<&mut A> { | ||
| pub fn get_mut(&mut self, id: impl Into<AssetId<A>>) -> Option<AssetMut<'_, A>> { | ||
| let id: AssetId<A> = id.into(); | ||
| let result = match id { | ||
| AssetId::Index { index, .. } => self.dense_storage.get_mut(index), | ||
| AssetId::Uuid { uuid } => self.hash_map.get_mut(&uuid), | ||
| }; | ||
| if result.is_some() { | ||
| self.queued_events.push(AssetEvent::Modified { id }); | ||
| } | ||
| result | ||
| Some(AssetMut { | ||
| asset: result?, | ||
| guard: AssetMutChangeNotifier { | ||
| changed: false, | ||
| asset_id: id, | ||
| queued_events: &mut self.queued_events, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| /// Retrieves a mutable reference to the [`Asset`] with the given `id`, if it exists. | ||
|
|
@@ -618,6 +623,62 @@ impl<A: Asset> Assets<A> { | |
| } | ||
| } | ||
|
|
||
| /// Unique mutable borrow of an asset. | ||
| /// | ||
| /// [AssetEvent::Modified] events will be only triggered if an asset itself is mutably borrowed. | ||
| /// | ||
| /// Just as an example, this allows checking if a material property has changed | ||
| /// before modifying it to avoid unnecessary material extraction down the pipeline. | ||
| pub struct AssetMut<'a, A: Asset> { | ||
| asset: &'a mut A, | ||
| guard: AssetMutChangeNotifier<'a, A>, | ||
| } | ||
|
|
||
| impl<'a, A: Asset> AssetMut<'a, A> { | ||
| /// Marks with inner asset as modified and returns reference to it. | ||
| pub fn into_inner(mut self) -> &'a mut A { | ||
| self.guard.changed = true; | ||
| self.asset | ||
| } | ||
|
|
||
| /// Returns reference to the inner asset but doesn't mark it as modified. | ||
| pub fn into_inner_untracked(self) -> &'a mut A { | ||
| self.asset | ||
| } | ||
| } | ||
|
|
||
| impl<'a, A: Asset> Deref for AssetMut<'a, A> { | ||
| type Target = A; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| self.asset | ||
| } | ||
| } | ||
|
|
||
| impl<'a, A: Asset> DerefMut for AssetMut<'a, A> { | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| self.guard.changed = true; | ||
| self.asset | ||
| } | ||
| } | ||
|
|
||
| /// Helper struct to allow safe destructuring of the [AssetMut::into_inner] | ||
| /// while also keeping strong change tracking guarantees. | ||
| struct AssetMutChangeNotifier<'a, A: Asset> { | ||
| changed: bool, | ||
| asset_id: AssetId<A>, | ||
| queued_events: &'a mut Vec<AssetEvent<A>>, | ||
| } | ||
|
|
||
| impl<'a, A: Asset> Drop for AssetMutChangeNotifier<'a, A> { | ||
| fn drop(&mut self) { | ||
| if self.changed { | ||
| self.queued_events | ||
| .push(AssetEvent::Modified { id: self.asset_id }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A mutable iterator over [`Assets`]. | ||
| pub struct AssetsMutIterator<'a, A: Asset> { | ||
| queued_events: &'a mut Vec<AssetEvent<A>>, | ||
|
|
@@ -673,7 +734,10 @@ pub enum InvalidGenerationError { | |
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use crate::AssetIndex; | ||
| use crate::{Asset, AssetApp, AssetEvent, AssetIndex, AssetPlugin, Assets}; | ||
| use bevy_app::{App, Last, Update}; | ||
| use bevy_ecs::prelude::*; | ||
| use bevy_reflect::TypePath; | ||
|
|
||
| #[test] | ||
| fn asset_index_round_trip() { | ||
|
|
@@ -684,4 +748,74 @@ mod test { | |
| let roundtripped = AssetIndex::from_bits(asset_index.to_bits()); | ||
| assert_eq!(asset_index, roundtripped); | ||
| } | ||
|
|
||
| #[test] | ||
| fn assets_mut_change_detection() { | ||
| #[derive(Asset, TypePath, Default)] | ||
| struct TestAsset { | ||
| value: u32, | ||
| } | ||
|
|
||
| #[derive(Resource, Default)] | ||
| struct TestState { | ||
| asset_target_value: u32, | ||
| asset_modified_counter: u32, | ||
| } | ||
|
|
||
| let mut app = App::new(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| app.add_plugins(AssetPlugin::default()); | ||
| app.init_asset::<TestAsset>(); | ||
| app.insert_resource(TestState::default()); | ||
|
|
||
| let mut assets = app.world_mut().resource_mut::<Assets<TestAsset>>(); | ||
| let my_asset_handle = assets.add(TestAsset::default()); | ||
| let my_asset_id = my_asset_handle.id(); | ||
|
|
||
| app.add_systems( | ||
| Update, | ||
| move |mut assets: ResMut<Assets<TestAsset>>, state: Res<TestState>| { | ||
| let mut asset = assets.get_mut(my_asset_id).unwrap(); | ||
|
|
||
| if asset.value != state.asset_target_value { | ||
| asset.value = state.asset_target_value; | ||
| } | ||
| }, | ||
| ); | ||
| app.add_systems( | ||
| Last, | ||
| move |mut reader: MessageReader<AssetEvent<TestAsset>>, | ||
| mut state: ResMut<TestState>| { | ||
| for event in reader.read() { | ||
| if event.is_modified(my_asset_id) { | ||
| state.asset_modified_counter += 1; | ||
| } | ||
| } | ||
| }, | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These don't need to be systems for this test. Can we just use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. it is really easier to track what happens without the systems. thanks for suggestion. |
||
|
|
||
| // check a few times just in case there are some unexpected leftover events from previous runs | ||
| for _ in 0..3 { | ||
| let mut state = app.world_mut().resource_mut::<TestState>(); | ||
| state.asset_target_value += 1; | ||
| state.asset_modified_counter = 0; | ||
|
|
||
| app.update(); | ||
|
|
||
| let mut state = app.world_mut().resource_mut::<TestState>(); | ||
| assert_eq!( | ||
| std::mem::take(&mut state.asset_modified_counter), | ||
| 1, | ||
| "Asset value was changed but AssetEvent::Modified was not triggered", | ||
| ); | ||
|
|
||
| app.update(); | ||
|
|
||
| let mut state = app.world_mut().resource_mut::<TestState>(); | ||
| assert_eq!( | ||
| std::mem::take(&mut state.asset_modified_counter), | ||
| 0, | ||
| "Asset value was not changed but AssetEvent::Modified was triggered", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.