From a88c07ba25ea3dc8c906ad97441eda7550195efa Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Thu, 23 Jun 2022 15:11:52 +0200 Subject: [PATCH] Add the AssetChanged query filter Solves #5069. `AssetChanged` is just a thin wrapper around the `&Handle` query, with additional logic to check in `Assets` if the corresponding `HandleId` was changed between the last and current runs of the system. This adds some overhead in the `asset_event_system` for keeping track of when which handle got updated. Otherwise, the overhead is constrained to the `AssetChanged` query, which is ripe for optimizations. NOTE: We chose to update the `change_ticks` hash map during asset event propagation rather than when accessing the asset, because we need access somehow to the current `change_tick`. Updating the tick hash map during modification would require passing the `change_tick` to all methods of `Assets` that accepts a `&mut self`. Which would be a huge ergonomic loss. As a result, the asset change expressed by `AssetChanged` is only effective after `asset_event_system` ran. This is in line with how `AssetEvents` currently works, but might cause a 1 frame lag for asset change detection. A potential future update could implement some sort of specialization so that when `Assets` is accessed through a `ResMut`, the called methods are called with the `ResMut`'s change_tick. But with current rust, this is either impossible or causing huge ergonomic loss on the `Assets` API. --- crates/bevy_asset/src/assets.rs | 25 +++- crates/bevy_asset/src/lib.rs | 3 + crates/bevy_asset/src/query.rs | 166 ++++++++++++++++++++++++ crates/bevy_ecs/src/change_detection.rs | 7 + examples/asset/hot_asset_reloading.rs | 7 + 5 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 crates/bevy_asset/src/query.rs diff --git a/crates/bevy_asset/src/assets.rs b/crates/bevy_asset/src/assets.rs index 23ec2cf3dee662..92298fdc9e1999 100644 --- a/crates/bevy_asset/src/assets.rs +++ b/crates/bevy_asset/src/assets.rs @@ -5,7 +5,7 @@ use crate::{ use bevy_app::App; use bevy_ecs::{ event::{EventWriter, Events}, - system::ResMut, + system::{ResMut, SystemChangeTick}, world::FromWorld, }; use bevy_utils::HashMap; @@ -65,6 +65,7 @@ impl Debug for AssetEvent { #[derive(Debug)] pub struct Assets { assets: HashMap, + pub(crate) change_ticks: HashMap, events: Events>, pub(crate) ref_change_sender: Sender, } @@ -73,6 +74,7 @@ impl Assets { pub(crate) fn new(ref_change_sender: Sender) -> Self { Assets { assets: HashMap::default(), + change_ticks: HashMap::default(), events: Events::default(), ref_change_sender, } @@ -239,13 +241,30 @@ impl Assets { } pub fn asset_event_system( - mut events: EventWriter>, + ticks: SystemChangeTick, + mut event_writer: EventWriter>, mut assets: ResMut>, ) { // Check if the events are empty before calling `drain`. // As `drain` triggers change detection. if !assets.events.is_empty() { - events.send_batch(assets.events.drain()); + let current_tick = ticks.change_tick(); + let Assets { + change_ticks, + events, + .. + } = &mut *assets; + let update_change_map = |event| { + use AssetEvent::{Created, Modified, Removed}; + match &event { + Removed { handle } => change_ticks.remove(&handle.id), + Created { handle } | Modified { handle } => { + change_ticks.insert(handle.id, current_tick) + } + }; + event + }; + event_writer.send_batch(events.drain().map(update_change_map)); } } diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index b5ba1a1854d025..a371cb0f287f4e 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -13,8 +13,11 @@ mod info; mod io; mod loader; mod path; +pub mod query; pub mod prelude { + #[doc(hidden)] + pub use crate::query::AssetChanged; #[doc(hidden)] pub use crate::{AddAsset, AssetEvent, AssetServer, Assets, Handle, HandleUntyped}; } diff --git a/crates/bevy_asset/src/query.rs b/crates/bevy_asset/src/query.rs new file mode 100644 index 00000000000000..ccd8a1252e7b5d --- /dev/null +++ b/crates/bevy_asset/src/query.rs @@ -0,0 +1,166 @@ +//! Filter query to limit iteration over [`Handle`]s which content was updated recently. +use std::marker::PhantomData; + +use bevy_ecs::{ + archetype::{Archetype, ArchetypeComponentId}, + change_detection::MAX_CHANGE_AGE, + component::ComponentId, + prelude::World, + query::{ + Access, ComponentIdState, Fetch, FetchState, FilteredAccess, QueryItem, ReadFetch, + ReadOnlyWorldQuery, WorldQuery, WorldQueryGats, + }, + storage::{Table, Tables}, +}; +use bevy_utils::HashMap; + +use crate::{Asset, Assets, Handle, HandleId}; + +struct AssetChangeCheck<'w> { + handle_change_map: Option<&'w HashMap>, + prev_system_tick: u32, + current_tick: u32, +} +impl<'w> AssetChangeCheck<'w> { + fn new(assets: Option<&'w Assets>, last_change: u32, current: u32) -> Self { + let handle_change_map = assets.map(|a| &a.change_ticks); + Self { + handle_change_map, + prev_system_tick: last_change, + current_tick: current, + } + } + fn ticks_since(&self, event_tick: u32) -> u32 { + self.current_tick + .wrapping_sub(event_tick) + .min(MAX_CHANGE_AGE) + } + // TODO: some sort of caching? Each check has two levels of indirection, + // which is not optimal. + fn has_changed(&self, handle: &Handle) -> bool { + let is_changed = || { + let map = self.handle_change_map?; + let handle_change_tick = *map.get(&handle.id)?; + // This replicates the logic used in `ComponentTicks::is_changed` + let ticks_since_change = self.ticks_since(handle_change_tick); + let ticks_since_system = self.ticks_since(self.prev_system_tick); + Some(ticks_since_system >= ticks_since_change) + }; + is_changed().unwrap_or(true) + } +} + +/// Query filter to get all entities which `Handle` component underlying asset changed. +pub struct AssetChanged(PhantomData); + +/// Fetch for [`AssetChanged`] +#[doc(hidden)] +pub struct AssetChangedFetch<'w, T: Asset> { + inner: ReadFetch<'w, Handle>, + check: AssetChangeCheck<'w>, +} + +/// State for [`AssetChanged`] +pub struct AssetChangedState { + inner: ComponentIdState>, +} + +// SAFETY: `ROQueryFetch` is the same as `QueryFetch` +unsafe impl WorldQuery for AssetChanged { + type ReadOnly = Self; + type State = AssetChangedState; + + fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { + item + } +} + +impl FetchState for AssetChangedState { + fn init(world: &mut World) -> Self { + Self { + inner: ComponentIdState::init(world), + } + } + + fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { + self.inner.matches_component_set(set_contains_id) + } +} + +impl<'w, T: Asset> WorldQueryGats<'w> for AssetChanged { + type Fetch = AssetChangedFetch<'w, T>; + type _State = AssetChangedState; +} + +// SAFETY: this reads the T component. archetype component access and component access are updated to reflect that +unsafe impl<'w, T: Asset> Fetch<'w> for AssetChangedFetch<'w, T> { + type State = AssetChangedState; + type Item = bool; + + unsafe fn init( + world: &'w World, + state: &AssetChangedState, + last_change_tick: u32, + change_tick: u32, + ) -> Self { + Self { + inner: ReadFetch::init(world, &state.inner, last_change_tick, change_tick), + check: AssetChangeCheck::new::(world.get_resource(), last_change_tick, change_tick), + } + } + + const IS_DENSE: bool = >>::IS_DENSE; + + // This Fetch filters more than just the archetype. + const IS_ARCHETYPAL: bool = false; + + unsafe fn set_table(&mut self, state: &Self::State, table: &'w Table) { + self.inner.set_table(&state.inner, table); + } + + unsafe fn set_archetype( + &mut self, + state: &Self::State, + archetype: &'w Archetype, + tables: &'w Tables, + ) { + self.inner.set_archetype(&state.inner, archetype, tables); + } + + unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { + let handle = self.inner.table_fetch(table_row); + self.check.has_changed(handle) + } + + unsafe fn archetype_fetch(&mut self, archetype_index: usize) -> Self::Item { + let handle = self.inner.archetype_fetch(archetype_index); + self.check.has_changed(handle) + } + + #[inline] + unsafe fn table_filter_fetch(&mut self, table_row: usize) -> bool { + self.table_fetch(table_row) + } + + #[inline] + unsafe fn archetype_filter_fetch(&mut self, archetype_index: usize) -> bool { + self.archetype_fetch(archetype_index) + } + + #[inline] + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + ReadFetch::>::update_component_access(&state.inner, access); + } + + #[inline] + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + ReadFetch::>::update_archetype_component_access(&state.inner, archetype, access); + } +} + +/// SAFETY: read-only access +unsafe impl ReadOnlyWorldQuery for AssetChanged {} diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 2b5ae2bd2f5cb2..20f768fd1dc884 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -206,6 +206,13 @@ change_detection_impl!(NonSendMut<'a, T>, T,); impl_into_inner!(NonSendMut<'a, T>, T,); impl_debug!(NonSendMut<'a, T>,); +// Note on how change detection works. +// +// The `World` maintains a `Table` for each `StorageType::Table` component, +// When accessing a `Mut` from a query, a somewhat mutable reference to the +// tick stored in the table is passed to the `Mut`'s `ticks.component_ticks` +// field, going from a `&UnsafeCell` to a `&mut ComponentTicks` +// using unsafe (in a context where the reference is guaranteed to be unique). /// Unique mutable borrow of an entity's component pub struct Mut<'a, T> { pub(crate) value: &'a mut T, diff --git a/examples/asset/hot_asset_reloading.rs b/examples/asset/hot_asset_reloading.rs index d96af65e59e65d..7254e3d3b3c5ca 100644 --- a/examples/asset/hot_asset_reloading.rs +++ b/examples/asset/hot_asset_reloading.rs @@ -12,10 +12,17 @@ fn main() { ..default() }) .add_plugins(DefaultPlugins) + .add_system(asset_updated) .add_startup_system(setup) .run(); } +fn asset_updated(query: Query<&Name, AssetChanged>) { + for name in query.iter() { + println!("{name} mesh got updated"); + } +} + fn setup(mut commands: Commands, asset_server: Res) { // Load our mesh: let scene_handle = asset_server.load("models/monkey/Monkey.gltf#Scene0");