Skip to content

Commit af25ffa

Browse files
cartjames7132
authored andcommitted
Revert "Recalculate entity aabbs when meshes change (bevyengine#4944)" (bevyengine#5489)
# Objective Sadly, bevyengine#4944 introduces a serious exponential despawn behavior, which cannot be included in 0.8. [Handling AABBs properly is a controversial topic](bevyengine#5423 (comment)) and one that deserves more time than the day we have left before release. ## Solution This reverts commit c2b332f.
1 parent c6c2b02 commit af25ffa

File tree

1 file changed

+38
-157
lines changed
  • crates/bevy_render/src/view/visibility

1 file changed

+38
-157
lines changed

crates/bevy_render/src/view/visibility/mod.rs

Lines changed: 38 additions & 157 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,15 @@
11
mod render_layers;
22

3-
use smallvec::SmallVec;
4-
53
pub use render_layers::*;
64

75
use bevy_app::{CoreStage, Plugin};
8-
use bevy_asset::{AssetEvent, Assets, Handle};
6+
use bevy_asset::{Assets, Handle};
97
use bevy_ecs::prelude::*;
108
use bevy_hierarchy::{Children, Parent};
119
use bevy_reflect::std_traits::ReflectDefault;
1210
use bevy_reflect::Reflect;
1311
use bevy_transform::components::GlobalTransform;
1412
use bevy_transform::TransformSystem;
15-
use bevy_utils::HashMap;
1613
use std::cell::Cell;
1714
use thread_local::ThreadLocal;
1815

@@ -130,11 +127,6 @@ pub struct VisibilityBundle {
130127
#[derive(Component)]
131128
pub struct NoFrustumCulling;
132129

133-
/// Use this component to opt-out of built-in [`Aabb`] updating for Mesh entities (i.e. to opt out
134-
/// of [`update_bounds`]).
135-
#[derive(Component)]
136-
pub struct NoAabbUpdate;
137-
138130
/// Collection of entities visible from the current view.
139131
///
140132
/// This component contains all entities which are visible from the currently
@@ -168,59 +160,9 @@ impl VisibleEntities {
168160
}
169161
}
170162

171-
/// Tracks which [`Entities`](Entity) have which meshes for entities whose [`Aabb`]s are managed by
172-
/// the [`calculate_bounds`] and [`update_bounds`] systems. This is needed because `update_bounds`
173-
/// recomputes `Aabb`s for entities whose mesh has been mutated. These mutations are visible via
174-
/// [`AssetEvent<Mesh>`](AssetEvent) which tells us which mesh was changed but not which entities
175-
/// have that mesh.
176-
#[derive(Debug, Default, Clone)]
177-
pub struct EntityMeshMap {
178-
entities_with_mesh: HashMap<Handle<Mesh>, SmallVec<[Entity; 1]>>,
179-
mesh_for_entity: HashMap<Entity, Handle<Mesh>>,
180-
}
181-
182-
impl EntityMeshMap {
183-
/// Register the passed `entity` as having the passed `mesh_handle`.
184-
fn register(&mut self, entity: Entity, mesh_handle: &Handle<Mesh>) {
185-
// Note that this list can have duplicates if an entity is registered for a mesh multiple
186-
// times. This should be rare and only cause an additional `Aabb.clone()` in
187-
// `update_bounds` so it is preferable to a `HashSet` for now.
188-
self.entities_with_mesh
189-
.entry(mesh_handle.clone_weak())
190-
.or_default()
191-
.push(entity);
192-
self.mesh_for_entity
193-
.insert(entity, mesh_handle.clone_weak());
194-
}
195-
196-
/// Deregisters the mapping between an `Entity` and `Mesh`. Used so [`update_bounds`] can
197-
/// track which mappings are still active so `Aabb`s are updated correctly.
198-
fn deregister(&mut self, entity: Entity) {
199-
let mut inner = || {
200-
let mesh = self.mesh_for_entity.remove(&entity)?;
201-
202-
// This lookup failing is _probably_ an error.
203-
let entities = self.entities_with_mesh.get_mut(&mesh)?;
204-
205-
// There could be duplicate entries in here if an entity was registered with a mesh
206-
// multiple times. It's important to remove all references so that if an entity gets a
207-
// new mesh and its old mesh is mutated, the entity doesn't get its old mesh's new
208-
// `Aabb`. Note that there _should_ only be one entity.
209-
for i in (0..entities.len()).rev() {
210-
if entities[i] == entity {
211-
entities.swap_remove(i);
212-
}
213-
}
214-
Some(())
215-
};
216-
inner();
217-
}
218-
}
219-
220163
#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
221164
pub enum VisibilitySystems {
222165
CalculateBounds,
223-
UpdateBounds,
224166
UpdateOrthographicFrusta,
225167
UpdatePerspectiveFrusta,
226168
UpdateProjectionFrusta,
@@ -236,121 +178,60 @@ impl Plugin for VisibilityPlugin {
236178
fn build(&self, app: &mut bevy_app::App) {
237179
use VisibilitySystems::*;
238180

239-
app.init_resource::<EntityMeshMap>()
240-
.add_system_to_stage(
241-
CoreStage::PostUpdate,
242-
calculate_bounds.label(CalculateBounds),
243-
)
244-
.add_system_to_stage(CoreStage::PostUpdate, update_bounds.label(UpdateBounds))
245-
.add_system_to_stage(
246-
CoreStage::PostUpdate,
247-
update_frusta::<OrthographicProjection>
248-
.label(UpdateOrthographicFrusta)
249-
.after(TransformSystem::TransformPropagate),
250-
)
251-
.add_system_to_stage(
252-
CoreStage::PostUpdate,
253-
update_frusta::<PerspectiveProjection>
254-
.label(UpdatePerspectiveFrusta)
255-
.after(TransformSystem::TransformPropagate),
256-
)
257-
.add_system_to_stage(
258-
CoreStage::PostUpdate,
259-
update_frusta::<Projection>
260-
.label(UpdateProjectionFrusta)
261-
.after(TransformSystem::TransformPropagate),
262-
)
263-
.add_system_to_stage(
264-
CoreStage::PostUpdate,
265-
visibility_propagate_system.label(VisibilityPropagate),
266-
)
267-
.add_system_to_stage(
268-
CoreStage::PostUpdate,
269-
check_visibility
270-
.label(CheckVisibility)
271-
.after(CalculateBounds)
272-
.after(UpdateBounds)
273-
.after(UpdateOrthographicFrusta)
274-
.after(UpdatePerspectiveFrusta)
275-
.after(UpdateProjectionFrusta)
276-
.after(VisibilityPropagate)
277-
.after(TransformSystem::TransformPropagate),
278-
);
181+
app.add_system_to_stage(
182+
CoreStage::PostUpdate,
183+
calculate_bounds.label(CalculateBounds),
184+
)
185+
.add_system_to_stage(
186+
CoreStage::PostUpdate,
187+
update_frusta::<OrthographicProjection>
188+
.label(UpdateOrthographicFrusta)
189+
.after(TransformSystem::TransformPropagate),
190+
)
191+
.add_system_to_stage(
192+
CoreStage::PostUpdate,
193+
update_frusta::<PerspectiveProjection>
194+
.label(UpdatePerspectiveFrusta)
195+
.after(TransformSystem::TransformPropagate),
196+
)
197+
.add_system_to_stage(
198+
CoreStage::PostUpdate,
199+
update_frusta::<Projection>
200+
.label(UpdateProjectionFrusta)
201+
.after(TransformSystem::TransformPropagate),
202+
)
203+
.add_system_to_stage(
204+
CoreStage::PostUpdate,
205+
visibility_propagate_system.label(VisibilityPropagate),
206+
)
207+
.add_system_to_stage(
208+
CoreStage::PostUpdate,
209+
check_visibility
210+
.label(CheckVisibility)
211+
.after(CalculateBounds)
212+
.after(UpdateOrthographicFrusta)
213+
.after(UpdatePerspectiveFrusta)
214+
.after(UpdateProjectionFrusta)
215+
.after(VisibilityPropagate)
216+
.after(TransformSystem::TransformPropagate),
217+
);
279218
}
280219
}
281220

282-
/// Calculates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es.
283221
pub fn calculate_bounds(
284222
mut commands: Commands,
285223
meshes: Res<Assets<Mesh>>,
286224
without_aabb: Query<(Entity, &Handle<Mesh>), (Without<Aabb>, Without<NoFrustumCulling>)>,
287-
mut entity_mesh_map: ResMut<EntityMeshMap>,
288225
) {
289226
for (entity, mesh_handle) in &without_aabb {
290227
if let Some(mesh) = meshes.get(mesh_handle) {
291228
if let Some(aabb) = mesh.compute_aabb() {
292-
entity_mesh_map.register(entity, mesh_handle);
293229
commands.entity(entity).insert(aabb);
294230
}
295231
}
296232
}
297233
}
298234

299-
/// Updates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es. This includes `Entities` that have
300-
/// been assigned new `Mesh`es as well as `Entities` whose `Mesh` has been directly mutated.
301-
///
302-
/// To opt out of bound calculation for an `Entity`, give it the [`NoAabbUpdate`] component.
303-
///
304-
/// NOTE: This system needs to remove entities from their collection in
305-
/// [`EntityMeshMap`] whenever a mesh handle is reassigned or an entity's mesh handle is
306-
/// removed. This may impact performance if meshes with many entities are frequently
307-
/// reassigned/removed.
308-
pub fn update_bounds(
309-
mut commands: Commands,
310-
meshes: Res<Assets<Mesh>>,
311-
mut mesh_reassigned: Query<
312-
(Entity, &Handle<Mesh>, &mut Aabb),
313-
(
314-
Changed<Handle<Mesh>>,
315-
Without<NoFrustumCulling>,
316-
Without<NoAabbUpdate>,
317-
),
318-
>,
319-
mut entity_mesh_map: ResMut<EntityMeshMap>,
320-
mut mesh_events: EventReader<AssetEvent<Mesh>>,
321-
entities_lost_mesh: RemovedComponents<Handle<Mesh>>,
322-
) {
323-
for entity in entities_lost_mesh.iter() {
324-
entity_mesh_map.deregister(entity);
325-
}
326-
327-
for (entity, mesh_handle, mut aabb) in mesh_reassigned.iter_mut() {
328-
entity_mesh_map.deregister(entity);
329-
if let Some(mesh) = meshes.get(mesh_handle) {
330-
if let Some(new_aabb) = mesh.compute_aabb() {
331-
entity_mesh_map.register(entity, mesh_handle);
332-
*aabb = new_aabb;
333-
}
334-
}
335-
}
336-
337-
let to_update = |event: &AssetEvent<Mesh>| {
338-
let handle = match event {
339-
AssetEvent::Modified { handle } => handle,
340-
_ => return None,
341-
};
342-
let mesh = meshes.get(handle)?;
343-
let entities_with_handle = entity_mesh_map.entities_with_mesh.get(handle)?;
344-
let aabb = mesh.compute_aabb()?;
345-
Some((aabb, entities_with_handle))
346-
};
347-
for (aabb, entities_with_handle) in mesh_events.iter().filter_map(to_update) {
348-
for entity in entities_with_handle {
349-
commands.entity(*entity).insert(aabb.clone());
350-
}
351-
}
352-
}
353-
354235
pub fn update_frusta<T: Component + CameraProjection + Send + Sync + 'static>(
355236
mut views: Query<(&GlobalTransform, &T, &mut Frustum)>,
356237
) {

0 commit comments

Comments
 (0)