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<Mesh>` 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<Entity>` 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<HashMap>` when handles change? Does the `Changed` event give us the old mesh handle? If not we might need to have a `HashMap<Entity, Handle<Mesh>>` 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<Mesh>` 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: <details> <summary>Test Script</summary> ```rust use bevy::prelude::*; use bevy::render:📷: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<Handle<Mesh>>); fn main() { App::new() .add_plugins(DefaultPlugins) .init_resource::<MutateMeshState>() .init_resource::<Meshes>() .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<Assets<Mesh>>, mut global_meshes: ResMut<Meshes>, mut materials: ResMut<Assets<StandardMaterial>>, ) { 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<Mesh>, &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<Assets<Mesh>>, time: Res<Time>, global_meshes: Res<Meshes>, mut mutate_mesh_state: ResMut<MutateMeshState>, ) { let mutated = mutate_mesh_state.0; if time.seconds_since_startup() > 4.5 && !mutated { println!("Mutating {:?}", global_meshes.0[0]); let m = meshes.get_mut(&global_meshes.0[0]).unwrap(); let mut p = m.attribute(Mesh::ATTRIBUTE_POSITION).unwrap().clone(); use bevy::render::mesh::VertexAttributeValues; match &mut p { VertexAttributeValues::Float32x3(v) => { v[0] = [10.0, 10.0, 10.0]; } _ => unreachable!(), } m.insert_attribute(Mesh::ATTRIBUTE_POSITION, p); mutate_mesh_state.0 = true; } } /// For testing the first part - assigning a new handle. fn assign_new_mesh( mut query: Query<&mut Handle<Mesh>, With<Aabb>>, time: Res<Time>, global_meshes: Res<Meshes>, ) { let s = time.seconds_since_startup() as usize; let idx = s % global_meshes.0.len(); for mut handle in query.iter_mut() { *handle = global_meshes.0[idx].clone_weak(); } } ``` </details> ## Changelog ### Fixed Entity `Aabb`s not updating when meshes are mutated or re-assigned.
This commit is contained in:
parent
959f3b1186
commit
c2b332f98a
@ -1,15 +1,18 @@
|
|||||||
mod render_layers;
|
mod render_layers;
|
||||||
|
|
||||||
|
use smallvec::SmallVec;
|
||||||
|
|
||||||
pub use render_layers::*;
|
pub use render_layers::*;
|
||||||
|
|
||||||
use bevy_app::{CoreStage, Plugin};
|
use bevy_app::{CoreStage, Plugin};
|
||||||
use bevy_asset::{Assets, Handle};
|
use bevy_asset::{AssetEvent, Assets, Handle};
|
||||||
use bevy_ecs::prelude::*;
|
use bevy_ecs::prelude::*;
|
||||||
use bevy_hierarchy::{Children, Parent};
|
use bevy_hierarchy::{Children, Parent};
|
||||||
use bevy_reflect::std_traits::ReflectDefault;
|
use bevy_reflect::std_traits::ReflectDefault;
|
||||||
use bevy_reflect::Reflect;
|
use bevy_reflect::Reflect;
|
||||||
use bevy_transform::components::GlobalTransform;
|
use bevy_transform::components::GlobalTransform;
|
||||||
use bevy_transform::TransformSystem;
|
use bevy_transform::TransformSystem;
|
||||||
|
use bevy_utils::HashMap;
|
||||||
use std::cell::Cell;
|
use std::cell::Cell;
|
||||||
use thread_local::ThreadLocal;
|
use thread_local::ThreadLocal;
|
||||||
|
|
||||||
@ -127,6 +130,11 @@ pub struct VisibilityBundle {
|
|||||||
#[derive(Component)]
|
#[derive(Component)]
|
||||||
pub struct NoFrustumCulling;
|
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.
|
/// Collection of entities visible from the current view.
|
||||||
///
|
///
|
||||||
/// This component contains all entities which are visible from the currently
|
/// 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<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)]
|
#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
|
||||||
pub enum VisibilitySystems {
|
pub enum VisibilitySystems {
|
||||||
CalculateBounds,
|
CalculateBounds,
|
||||||
|
UpdateBounds,
|
||||||
UpdateOrthographicFrusta,
|
UpdateOrthographicFrusta,
|
||||||
UpdatePerspectiveFrusta,
|
UpdatePerspectiveFrusta,
|
||||||
UpdateProjectionFrusta,
|
UpdateProjectionFrusta,
|
||||||
@ -178,10 +236,12 @@ impl Plugin for VisibilityPlugin {
|
|||||||
fn build(&self, app: &mut bevy_app::App) {
|
fn build(&self, app: &mut bevy_app::App) {
|
||||||
use VisibilitySystems::*;
|
use VisibilitySystems::*;
|
||||||
|
|
||||||
app.add_system_to_stage(
|
app.init_resource::<EntityMeshMap>()
|
||||||
|
.add_system_to_stage(
|
||||||
CoreStage::PostUpdate,
|
CoreStage::PostUpdate,
|
||||||
calculate_bounds.label(CalculateBounds),
|
calculate_bounds.label(CalculateBounds),
|
||||||
)
|
)
|
||||||
|
.add_system_to_stage(CoreStage::PostUpdate, update_bounds.label(UpdateBounds))
|
||||||
.add_system_to_stage(
|
.add_system_to_stage(
|
||||||
CoreStage::PostUpdate,
|
CoreStage::PostUpdate,
|
||||||
update_frusta::<OrthographicProjection>
|
update_frusta::<OrthographicProjection>
|
||||||
@ -209,6 +269,7 @@ impl Plugin for VisibilityPlugin {
|
|||||||
check_visibility
|
check_visibility
|
||||||
.label(CheckVisibility)
|
.label(CheckVisibility)
|
||||||
.after(CalculateBounds)
|
.after(CalculateBounds)
|
||||||
|
.after(UpdateBounds)
|
||||||
.after(UpdateOrthographicFrusta)
|
.after(UpdateOrthographicFrusta)
|
||||||
.after(UpdatePerspectiveFrusta)
|
.after(UpdatePerspectiveFrusta)
|
||||||
.after(UpdateProjectionFrusta)
|
.after(UpdateProjectionFrusta)
|
||||||
@ -218,20 +279,78 @@ impl Plugin for VisibilityPlugin {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Calculates [`Aabb`]s for [`Entities`](Entity) with [`Mesh`]es.
|
||||||
pub fn calculate_bounds(
|
pub fn calculate_bounds(
|
||||||
mut commands: Commands,
|
mut commands: Commands,
|
||||||
meshes: Res<Assets<Mesh>>,
|
meshes: Res<Assets<Mesh>>,
|
||||||
without_aabb: Query<(Entity, &Handle<Mesh>), (Without<Aabb>, Without<NoFrustumCulling>)>,
|
without_aabb: Query<(Entity, &Handle<Mesh>), (Without<Aabb>, Without<NoFrustumCulling>)>,
|
||||||
|
mut entity_mesh_map: ResMut<EntityMeshMap>,
|
||||||
) {
|
) {
|
||||||
for (entity, mesh_handle) in &without_aabb {
|
for (entity, mesh_handle) in &without_aabb {
|
||||||
if let Some(mesh) = meshes.get(mesh_handle) {
|
if let Some(mesh) = meshes.get(mesh_handle) {
|
||||||
if let Some(aabb) = mesh.compute_aabb() {
|
if let Some(aabb) = mesh.compute_aabb() {
|
||||||
|
entity_mesh_map.register(entity, mesh_handle);
|
||||||
commands.entity(entity).insert(aabb);
|
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>(
|
pub fn update_frusta<T: Component + CameraProjection + Send + Sync + 'static>(
|
||||||
mut views: Query<(&GlobalTransform, &T, &mut Frustum)>,
|
mut views: Query<(&GlobalTransform, &T, &mut Frustum)>,
|
||||||
) {
|
) {
|
||||||
|
Loading…
Reference in New Issue
Block a user