Skip to content

Commit

Permalink
Add small optimization for AssetChanged
Browse files Browse the repository at this point in the history
If there were no asset updates at all, The `AssetChanged` filter now
does not iterates over anything. This improves over the base case of
always iterating and checking over every entity with a `Handle<T>`
component.
  • Loading branch information
nicopap committed Jun 28, 2022
1 parent 6ceebf2 commit f6998ca
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 18 deletions.
3 changes: 3 additions & 0 deletions crates/bevy_asset/src/assets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ use std::{fmt::Debug, marker::PhantomData};

pub struct AssetChanges<T: Asset> {
pub(crate) change_ticks: HashMap<HandleId, u32>,
pub(crate) last_change_tick: u32,
_marker: PhantomData<T>,
}
impl<T: Asset> Default for AssetChanges<T> {
fn default() -> Self {
Self {
change_ticks: Default::default(),
last_change_tick: 0,
_marker: PhantomData,
}
}
Expand Down Expand Up @@ -274,6 +276,7 @@ impl<T: Asset> Assets<T> {
}
Created { handle } | Modified { handle } => {
asset_changes.change_ticks.insert(handle.id, current_tick);
asset_changes.last_change_tick = current_tick;
}
};
event
Expand Down
48 changes: 30 additions & 18 deletions crates/bevy_asset/src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ use bevy_utils::HashMap;

use crate::{Asset, AssetChanges, Handle, HandleId};

fn ticks_since(event: u32, current: u32) -> u32 {
current.wrapping_sub(event).min(MAX_CHANGE_AGE)
}
// This replicates the logic used in `ComponentTicks::is_changed`
fn is_tick_between(begin_period: u32, event: u32, end_period: u32) -> bool {
let ticks_since_event = ticks_since(event, end_period);
let ticks_since_begin = ticks_since(begin_period, end_period);
ticks_since_begin > ticks_since_event
}

struct AssetChangeCheck<'w> {
handle_change_map: &'w HashMap<HandleId, u32>,
prev_system_tick: u32,
Expand All @@ -29,19 +39,11 @@ impl<'w> AssetChangeCheck<'w> {
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<T: Asset>(&self, handle: &Handle<T>) -> bool {
if let Some(&handle_change_tick) = self.handle_change_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);
ticks_since_system >= ticks_since_change
is_tick_between(self.prev_system_tick, handle_change_tick, self.current_tick)
} else {
false
}
Expand Down Expand Up @@ -92,7 +94,7 @@ pub struct AssetChanged<T>(PhantomData<T>);
/// Fetch for [`AssetChanged`].
#[doc(hidden)]
pub struct AssetChangedFetch<'w, T: Asset> {
inner: ReadFetch<'w, Handle<T>>,
inner: Option<ReadFetch<'w, Handle<T>>>,
check: AssetChangeCheck<'w>,
}

Expand Down Expand Up @@ -154,9 +156,11 @@ unsafe impl<'w, T: Asset> Fetch<'w> for AssetChangedFetch<'w, T> {
change_tick: u32,
) -> Self {
let err_msg = format!("AssetChanges<{ty}> was not registered, yet AssetChanged<{ty}> is used in a query. Please register AssetChanges<{ty}> with app.add_asset::<{ty}>()", ty = std::any::type_name::<T>());
let changes = world.get_resource().expect(&err_msg);
let changes: &AssetChanges<_> = world.get_resource().expect(&err_msg);
let has_updates = is_tick_between(last_change_tick, changes.last_change_tick, change_tick);
Self {
inner: ReadFetch::init(world, &state.inner, last_change_tick, change_tick),
inner: has_updates
.then(|| ReadFetch::init(world, &state.inner, last_change_tick, change_tick)),
check: AssetChangeCheck::new::<T>(changes, last_change_tick, change_tick),
}
}
Expand All @@ -167,7 +171,9 @@ unsafe impl<'w, T: Asset> Fetch<'w> for AssetChangedFetch<'w, T> {
const IS_ARCHETYPAL: bool = false;

unsafe fn set_table(&mut self, state: &Self::State, table: &'w Table) {
self.inner.set_table(&state.inner, table);
if let Some(inner) = &mut self.inner {
inner.set_table(&state.inner, table);
}
}

unsafe fn set_archetype(
Expand All @@ -176,17 +182,23 @@ unsafe impl<'w, T: Asset> Fetch<'w> for AssetChangedFetch<'w, T> {
archetype: &'w Archetype,
tables: &'w Tables,
) {
self.inner.set_archetype(&state.inner, archetype, tables);
if let Some(inner) = &mut self.inner {
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)
self.inner.as_mut().map_or(false, |inner| {
let handle = 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)
self.inner.as_mut().map_or(false, |inner| {
let handle = inner.archetype_fetch(archetype_index);
self.check.has_changed(handle)
})
}

#[inline]
Expand Down

0 comments on commit f6998ca

Please sign in to comment.