Skip to content

Commit

Permalink
Revert "Recalculate entity aabbs when meshes change (#4944)" (#5489)
Browse files Browse the repository at this point in the history 
# Objective

Sadly, #4944 introduces a serious exponential despawn behavior, which cannot be included in 0.8. [Handling AABBs properly is a controversial topic](#5423 (comment)) and one that deserves more time than the day we have left before release.

## Solution

This reverts commit c2b332f.
  • Loading branch information
@cart
cart committed Jul 29, 2022
1 parent 4078273 commit 418beff
Showing 1 changed file with 38 additions and 157 deletions.
195 changes: 38 additions & 157 deletions crates/bevy_render/src/view/visibility/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
mod render_layers;

use smallvec::SmallVec;

pub use render_layers::*;

use bevy_app::{CoreStage, Plugin};
use bevy_asset::{AssetEvent, Assets, Handle};
use bevy_asset::{Assets, Handle};
use bevy_ecs::prelude::*;
use bevy_hierarchy::{Children, Parent};
use bevy_reflect::std_traits::ReflectDefault;
use bevy_reflect::Reflect;
use bevy_transform::components::GlobalTransform;
use bevy_transform::TransformSystem;
use bevy_utils::HashMap;
use std::cell::Cell;
use thread_local::ThreadLocal;

Expand Down Expand Up @@ -130,11 +127,6 @@ pub struct VisibilityBundle {
#[derive(Component)]
pub struct NoFrustumCulling;

/// Use this component to opt-out of built-in [`Aabb`] updating for Mesh entities (i.e. to opt out
/// of [`update_bounds`]).
#[derive(Component)]
pub struct NoAabbUpdate;

/// Collection of entities visible from the current view.
///
/// This component contains all entities which are visible from the currently
Expand Down Expand Up @@ -168,59 +160,9 @@ impl VisibleEntities {
}
}

/// Tracks which [`Entities`](Entity) have which meshes for entities whose [`Aabb`]s are managed by
/// the [`calculate_bounds`] and [`update_bounds`] systems. This is needed because `update_bounds`
/// recomputes `Aabb`s for entities whose mesh has been mutated. These mutations are visible via
/// [`AssetEvent<Mesh>`](AssetEvent) which tells us which mesh was changed but not which entities
/// have that mesh.
#[derive(Debug, Default, Clone)]
pub struct EntityMeshMap {
entities_with_mesh: HashMap<Handle<Mesh>, SmallVec<[Entity; 1]>>,
mesh_for_entity: HashMap<Entity, Handle<Mesh>>,
}

impl EntityMeshMap {
/// Register the passed `entity` as having the passed `mesh_handle`.
fn register(&mut self, entity: Entity, mesh_handle: &Handle<Mesh>) {
// Note that this list can have duplicates if an entity is registered for a mesh multiple
// times. This should be rare and only cause an additional `Aabb.clone()` in
// `update_bounds` so it is preferable to a `HashSet` for now.
self.entities_with_mesh
.entry(mesh_handle.clone_weak())
.or_default()
.push(entity);
self.mesh_for_entity
.insert(entity, mesh_handle.clone_weak());
}

/// Deregisters the mapping between an `Entity` and `Mesh`. Used so [`update_bounds`] can
/// track which mappings are still active so `Aabb`s are updated correctly.
fn deregister(&mut self, entity: Entity) {
let mut inner = || {
let mesh = self.mesh_for_entity.remove(&entity)?;

// This lookup failing is _probably_ an error.
let entities = self.entities_with_mesh.get_mut(&mesh)?;

// There could be duplicate entries in here if an entity was registered with a mesh
// multiple times. It's important to remove all references so that if an entity gets a
// new mesh and its old mesh is mutated, the entity doesn't get its old mesh's new
// `Aabb`. Note that there _should_ only be one entity.
for i in (0..entities.len()).rev() {
if entities[i] == entity {
entities.swap_remove(i);
}
}
Some(())
};
inner();
}
}

#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
pub enum VisibilitySystems {
CalculateBounds,
UpdateBounds,
UpdateOrthographicFrusta,
UpdatePerspectiveFrusta,
UpdateProjectionFrusta,
Expand All @@ -236,121 +178,60 @@ impl Plugin for VisibilityPlugin {
fn build(&self, app: &mut bevy_app::App) {
use VisibilitySystems::*;

app.init_resource::<EntityMeshMap>()
.add_system_to_stage(
CoreStage::PostUpdate,
calculate_bounds.label(CalculateBounds),
)
.add_system_to_stage(CoreStage::PostUpdate, update_bounds.label(UpdateBounds))
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<OrthographicProjection>
.label(UpdateOrthographicFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<PerspectiveProjection>
.label(UpdatePerspectiveFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<Projection>
.label(UpdateProjectionFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
visibility_propagate_system.label(VisibilityPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
check_visibility
.label(CheckVisibility)
.after(CalculateBounds)
.after(UpdateBounds)
.after(UpdateOrthographicFrusta)
.after(UpdatePerspectiveFrusta)
.after(UpdateProjectionFrusta)
.after(VisibilityPropagate)
.after(TransformSystem::TransformPropagate),
);
app.add_system_to_stage(
CoreStage::PostUpdate,
calculate_bounds.label(CalculateBounds),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<OrthographicProjection>
.label(UpdateOrthographicFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<PerspectiveProjection>
.label(UpdatePerspectiveFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<Projection>
.label(UpdateProjectionFrusta)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
visibility_propagate_system.label(VisibilityPropagate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
check_visibility
.label(CheckVisibility)
.after(CalculateBounds)
.after(UpdateOrthographicFrusta)
.after(UpdatePerspectiveFrusta)
.after(UpdateProjectionFrusta)
.after(VisibilityPropagate)
.after(TransformSystem::TransformPropagate),
);
}
}

/// Calculates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es.
pub fn calculate_bounds(
mut commands: Commands,
meshes: Res<Assets<Mesh>>,
without_aabb: Query<(Entity, &Handle<Mesh>), (Without<Aabb>, Without<NoFrustumCulling>)>,
mut entity_mesh_map: ResMut<EntityMeshMap>,
) {
for (entity, mesh_handle) in &without_aabb {
if let Some(mesh) = meshes.get(mesh_handle) {
if let Some(aabb) = mesh.compute_aabb() {
entity_mesh_map.register(entity, mesh_handle);
commands.entity(entity).insert(aabb);
}
}
}
}

/// Updates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es. This includes `Entities` that have
/// been assigned new `Mesh`es as well as `Entities` whose `Mesh` has been directly mutated.
///
/// To opt out of bound calculation for an `Entity`, give it the [`NoAabbUpdate`] component.
///
/// NOTE: This system needs to remove entities from their collection in
/// [`EntityMeshMap`] whenever a mesh handle is reassigned or an entity's mesh handle is
/// removed. This may impact performance if meshes with many entities are frequently
/// reassigned/removed.
pub fn update_bounds(
mut commands: Commands,
meshes: Res<Assets<Mesh>>,
mut mesh_reassigned: Query<
(Entity, &Handle<Mesh>, &mut Aabb),
(
Changed<Handle<Mesh>>,
Without<NoFrustumCulling>,
Without<NoAabbUpdate>,
),
>,
mut entity_mesh_map: ResMut<EntityMeshMap>,
mut mesh_events: EventReader<AssetEvent<Mesh>>,
entities_lost_mesh: RemovedComponents<Handle<Mesh>>,
) {
for entity in entities_lost_mesh.iter() {
entity_mesh_map.deregister(entity);
}

for (entity, mesh_handle, mut aabb) in mesh_reassigned.iter_mut() {
entity_mesh_map.deregister(entity);
if let Some(mesh) = meshes.get(mesh_handle) {
if let Some(new_aabb) = mesh.compute_aabb() {
entity_mesh_map.register(entity, mesh_handle);
*aabb = new_aabb;
}
}
}

let to_update = |event: &AssetEvent<Mesh>| {
let handle = match event {
AssetEvent::Modified { handle } => handle,
_ => return None,
};
let mesh = meshes.get(handle)?;
let entities_with_handle = entity_mesh_map.entities_with_mesh.get(handle)?;
let aabb = mesh.compute_aabb()?;
Some((aabb, entities_with_handle))
};
for (aabb, entities_with_handle) in mesh_events.iter().filter_map(to_update) {
for entity in entities_with_handle {
commands.entity(*entity).insert(aabb.clone());
}
}
}

pub fn update_frusta<T: Component + CameraProjection + Send + Sync + 'static>(
mut views: Query<(&GlobalTransform, &T, &mut Frustum)>,
) {
Expand Down

0 comments on commit 418beff

Please sign in to comment.