From c2b332f98a0bcab7390e4b184099202cfb4fbbe1 Mon Sep 17 00:00:00 2001 From: Mark Lodato Date: Wed, 20 Jul 2022 07:05:29 +0000 Subject: [PATCH] Recalculate entity aabbs when meshes change (#4944) # Objective Update the `calculate_bounds` system to update `Aabb`s for entities who've either: - gotten a new mesh - had their mesh mutated Fixes https://github.com/bevyengine/bevy/issues/4294. ## Solution There are two commits here to address the two issues above: ### Commit 1 **This Commit** Updates the `calculate_bounds` system to operate not only on entities without `Aabb`s but also on entities whose `Handle` has changed. **Why?** So if an entity gets a new mesh, its associated `Aabb` is properly recalculated. **Questions** - This type is getting pretty gnarly - should I extract some types? - This system is public - should I add some quick docs while I'm here? ### Commit 2 **This Commit** Updates `calculate_bounds` to update `Aabb`s of entities whose meshes have been directly mutated. **Why?** So if an entity's mesh gets updated, its associated `Aabb` is properly recalculated. **Questions** - I think we should be using `ahash`. Do we want to do that with a direct `hashbrown` dependency or an `ahash` dependency that we configure the `HashMap` with? - There is an edge case of duplicates with `Vec` in the `HashMap`. If an entity gets its mesh handle changed and changed back again it'll be added to the list twice. Do we want to use a `HashSet` to avoid that? Or do a check in the list first (assuming iterating over the `Vec` is faster and this edge case is rare)? - There is an edge case where, if an entity gets a new mesh handle and then its old mesh is updated, we'll update the entity's `Aabb` to the new geometry of the _old_ mesh. Do we want to remove items from the `Local` when handles change? Does the `Changed` event give us the old mesh handle? If not we might need to have a `HashMap>` or something so we can unlink entities from mesh handles when the handle changes. - I did the `zip()` with the two `HashMap` gets assuming those would be faster than calculating the Aabb of the mesh (otherwise we could do `meshes.get(mesh_handle).and_then(Mesh::compute_aabb).zip(entity_mesh_map...)` or something). Is that assumption way off? ## Testing I originally tried testing this with `bevy_mod_raycast` as mentioned in the original issue but it seemed to work (maybe they are currently manually updating the Aabbs?). I then tried doing it in 2D but it looks like `Handle` is just for 3D. So I took [this example](https://github.com/bevyengine/bevy/blob/main/examples/3d/pbr.rs) and added some systems to mutate/assign meshes:
Test Script ```rust use bevy::prelude::*; use bevy::render::camera::ScalingMode; use bevy::render::primitives::Aabb; /// Make sure we only mutate one mesh once. #[derive(Eq, PartialEq, Clone, Debug, Default)] struct MutateMeshState(bool); /// Let's have a few global meshes that we can cycle between. /// This way we can be assigned a new mesh, mutate the old one, and then get the old one assigned. #[derive(Eq, PartialEq, Clone, Debug, Default)] struct Meshes(Vec>); fn main() { App::new() .add_plugins(DefaultPlugins) .init_resource::() .init_resource::() .add_startup_system(setup) .add_system(assign_new_mesh) .add_system(show_aabbs.after(assign_new_mesh)) .add_system(mutate_meshes.after(show_aabbs)) .run(); } fn setup( mut commands: Commands, mut meshes: ResMut>, mut global_meshes: ResMut, mut materials: ResMut>, ) { let m1 = meshes.add(Mesh::from(shape::Icosphere::default())); let m2 = meshes.add(Mesh::from(shape::Icosphere { radius: 0.90, ..Default::default() })); let m3 = meshes.add(Mesh::from(shape::Icosphere { radius: 0.80, ..Default::default() })); global_meshes.0.push(m1.clone()); global_meshes.0.push(m2); global_meshes.0.push(m3); // add entities to the world // sphere commands.spawn_bundle(PbrBundle { mesh: m1, material: materials.add(StandardMaterial { base_color: Color::hex("ffd891").unwrap(), ..default() }), ..default() }); // new 3d camera commands.spawn_bundle(Camera3dBundle { projection: OrthographicProjection { scale: 3.0, scaling_mode: ScalingMode::FixedVertical(1.0), ..default() } .into(), ..default() }); // old 3d camera // commands.spawn_bundle(OrthographicCameraBundle { // transform: Transform::from_xyz(0.0, 0.0, 8.0).looking_at(Vec3::default(), Vec3::Y), // orthographic_projection: OrthographicProjection { // scale: 0.01, // ..default() // }, // ..OrthographicCameraBundle::new_3d() // }); } fn show_aabbs(query: Query<(Entity, &Handle, &Aabb)>) { for thing in query.iter() { println!("{thing:?}"); } } /// For testing the second part - mutating a mesh. /// /// Without the fix we should see this mutate an old mesh and it affects the new mesh that the /// entity currently has. /// With the fix, the mutation doesn't affect anything until the entity is reassigned the old mesh. fn mutate_meshes( mut meshes: ResMut>, time: Res
## Changelog ### Fixed Entity `Aabb`s not updating when meshes are mutated or re-assigned. --- crates/bevy_render/src/view/visibility/mod.rs | 195 ++++++++++++++---- 1 file changed, 157 insertions(+), 38 deletions(-) diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index eb14f94f1ad2a..314ebe56c0bdf 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -1,15 +1,18 @@ mod render_layers; +use smallvec::SmallVec; + pub use render_layers::*; use bevy_app::{CoreStage, Plugin}; -use bevy_asset::{Assets, Handle}; +use bevy_asset::{AssetEvent, 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; @@ -127,6 +130,11 @@ 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 @@ -160,9 +168,59 @@ 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`](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, SmallVec<[Entity; 1]>>, + mesh_for_entity: HashMap>, +} + +impl EntityMeshMap { + /// Register the passed `entity` as having the passed `mesh_handle`. + fn register(&mut self, entity: Entity, mesh_handle: &Handle) { + // 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, @@ -178,60 +236,121 @@ impl Plugin for VisibilityPlugin { fn build(&self, app: &mut bevy_app::App) { use VisibilitySystems::*; - app.add_system_to_stage( - CoreStage::PostUpdate, - calculate_bounds.label(CalculateBounds), - ) - .add_system_to_stage( - CoreStage::PostUpdate, - update_frusta:: - .label(UpdateOrthographicFrusta) - .after(TransformSystem::TransformPropagate), - ) - .add_system_to_stage( - CoreStage::PostUpdate, - update_frusta:: - .label(UpdatePerspectiveFrusta) - .after(TransformSystem::TransformPropagate), - ) - .add_system_to_stage( - CoreStage::PostUpdate, - update_frusta:: - .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), - ); + app.init_resource::() + .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:: + .label(UpdateOrthographicFrusta) + .after(TransformSystem::TransformPropagate), + ) + .add_system_to_stage( + CoreStage::PostUpdate, + update_frusta:: + .label(UpdatePerspectiveFrusta) + .after(TransformSystem::TransformPropagate), + ) + .add_system_to_stage( + CoreStage::PostUpdate, + update_frusta:: + .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), + ); } } +/// Calculates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es. pub fn calculate_bounds( mut commands: Commands, meshes: Res>, without_aabb: Query<(Entity, &Handle), (Without, Without)>, + mut entity_mesh_map: ResMut, ) { 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>, + mut mesh_reassigned: Query< + (Entity, &Handle, &mut Aabb), + ( + Changed>, + Without, + Without, + ), + >, + mut entity_mesh_map: ResMut, + mut mesh_events: EventReader>, + entities_lost_mesh: RemovedComponents>, +) { + 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| { + 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( mut views: Query<(&GlobalTransform, &T, &mut Frustum)>, ) {