Skip to content

Commit

Permalink
Fix infinite asset preparation due to undrained AssetEvent events (be…
Browse files Browse the repository at this point in the history
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
  • Loading branch information
brianreavis authored and Maximetinu committed Feb 12, 2024
1 parent 7e6d817 commit 73a687c
Showing 1 changed file with 57 additions and 35 deletions.
92 changes: 57 additions & 35 deletions crates/bevy_render/src/render_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use bevy_ecs::{
prelude::{Commands, EventReader, IntoSystemConfigs, ResMut, Resource},
schedule::SystemConfigs,
system::{StaticSystemParam, SystemParam, SystemParamItem, SystemState},
world::{FromWorld, Mut},
};
use bevy_reflect::Reflect;
use bevy_utils::{thiserror::Error, HashMap, HashSet};
Expand Down Expand Up @@ -88,6 +89,7 @@ impl<A: RenderAsset, AFTER: RenderAssetDependency + 'static> Plugin
for RenderAssetPlugin<A, AFTER>
{
fn build(&self, app: &mut App) {
app.init_resource::<CachedExtractRenderAssetSystemState<A>>();
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
.init_resource::<ExtractedAssets<A>>()
Expand Down Expand Up @@ -176,49 +178,69 @@ impl<A: RenderAsset> RenderAssets<A> {
}
}

#[derive(Resource)]
struct CachedExtractRenderAssetSystemState<A: RenderAsset> {
state: SystemState<(
EventReader<'static, 'static, AssetEvent<A>>,
ResMut<'static, Assets<A>>,
)>,
}

impl<A: RenderAsset> FromWorld for CachedExtractRenderAssetSystemState<A> {
fn from_world(world: &mut bevy_ecs::world::World) -> Self {
Self {
state: SystemState::new(world),
}
}
}

/// This system extracts all created or modified assets of the corresponding [`RenderAsset`] type
/// into the "render world".
fn extract_render_asset<A: RenderAsset>(mut commands: Commands, mut main_world: ResMut<MainWorld>) {
let mut system_state: SystemState<(EventReader<AssetEvent<A>>, ResMut<Assets<A>>)> =
SystemState::new(&mut main_world);
let (mut events, mut assets) = system_state.get_mut(&mut main_world);

let mut changed_assets = HashSet::default();
let mut removed = Vec::new();
for event in events.read() {
#[allow(clippy::match_same_arms)]
match event {
AssetEvent::Added { id } | AssetEvent::Modified { id } => {
changed_assets.insert(*id);
}
AssetEvent::Removed { .. } => {}
AssetEvent::Unused { id } => {
changed_assets.remove(id);
removed.push(*id);
}
AssetEvent::LoadedWithDependencies { .. } => {
// TODO: handle this
main_world.resource_scope(
|world, mut cached_state: Mut<CachedExtractRenderAssetSystemState<A>>| {
let (mut events, mut assets) = cached_state.state.get_mut(world);

let mut changed_assets = HashSet::default();
let mut removed = Vec::new();

for event in events.read() {
#[allow(clippy::match_same_arms)]
match event {
AssetEvent::Added { id } | AssetEvent::Modified { id } => {
changed_assets.insert(*id);
}
AssetEvent::Removed { .. } => {}
AssetEvent::Unused { id } => {
changed_assets.remove(id);
removed.push(*id);
}
AssetEvent::LoadedWithDependencies { .. } => {
// TODO: handle this
}
}
}
}
}

let mut extracted_assets = Vec::new();
for id in changed_assets.drain() {
if let Some(asset) = assets.get(id) {
if asset.persistence_policy() == RenderAssetPersistencePolicy::Unload {
if let Some(asset) = assets.remove(id) {
extracted_assets.push((id, asset));
let mut extracted_assets = Vec::new();
for id in changed_assets.drain() {
if let Some(asset) = assets.get(id) {
if asset.persistence_policy() == RenderAssetPersistencePolicy::Unload {
if let Some(asset) = assets.remove(id) {
extracted_assets.push((id, asset));
}
} else {
extracted_assets.push((id, asset.clone()));
}
}
} else {
extracted_assets.push((id, asset.clone()));
}
}
}

commands.insert_resource(ExtractedAssets {
extracted: extracted_assets,
removed,
});
commands.insert_resource(ExtractedAssets {
extracted: extracted_assets,
removed,
});
cached_state.state.apply(world);
},
);
}

// TODO: consider storing inside system?
Expand Down

0 comments on commit 73a687c

Please sign in to comment.