Skip to content

bevy_render: Improve Aabb maintenance from O(n^2) to O(n) for n meshes #5423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

bevy_render: Improve Aabb maintenance from O(n^2) to O(n) for n meshes #5423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/bevy_pbr/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ impl Plugin for PbrPlugin {
check_light_mesh_visibility
.label(SimulationLightSystems::CheckLightVisibility)
.after(TransformSystem::TransformPropagate)
.after(VisibilitySystems::CalculateBounds)
.after(SimulationLightSystems::UpdateLightFrusta)
// NOTE: This MUST be scheduled AFTER the core renderer visibility check
// because that resets entity ComputedVisibility for the first view
Expand Down
197 changes: 86 additions & 111 deletions crates/bevy_render/src/view/visibility/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
mod render_layers;

use smallvec::SmallVec;

pub use render_layers::*;

use bevy_app::{CoreStage, Plugin};
use bevy_asset::{AssetEvent, Assets, Handle};
use bevy_derive::{Deref, DerefMut};
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 bevy_utils::{HashMap, HashSet};
use std::cell::Cell;
use thread_local::ThreadLocal;

Expand Down Expand Up @@ -168,58 +167,16 @@ 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();
}
/// Tracks the [`Aabb`]s that have been provided by users or computed automatically for [`Mesh`]
/// assets. Maintenance of the mapping is carried out by the [`update_bounds`] system. This is
/// needed to avoid computing [`Aabb`]s per-[`Entity`] where they could be calculated per-[`Mesh`].
#[derive(Debug, Default, Clone, Deref, DerefMut)]
pub struct MeshAabbMap {
map: HashMap<Handle<Mesh>, Aabb>,
}

#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
pub enum VisibilitySystems {
CalculateBounds,
UpdateBounds,
UpdateOrthographicFrusta,
UpdatePerspectiveFrusta,
Expand All @@ -236,11 +193,7 @@ 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),
)
app.init_resource::<MeshAabbMap>()
.add_system_to_stage(CoreStage::PostUpdate, update_bounds.label(UpdateBounds))
.add_system_to_stage(
CoreStage::PostUpdate,
Expand Down Expand Up @@ -268,7 +221,6 @@ impl Plugin for VisibilityPlugin {
CoreStage::PostUpdate,
check_visibility
.label(CheckVisibility)
.after(CalculateBounds)
.after(UpdateBounds)
.after(UpdateOrthographicFrusta)
.after(UpdatePerspectiveFrusta)
Expand All @@ -279,74 +231,97 @@ impl Plugin for VisibilityPlugin {
}
}

/// 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 queries: ParamSet<(
Query<(&Handle<Mesh>, &Aabb), Changed<Aabb>>,
Query<
(
Entity,
&Handle<Mesh>,
Changed<Handle<Mesh>>,
Option<&mut Aabb>,
),
(Without<NoFrustumCulling>, Without<NoAabbUpdate>),
>,
)>,
mut mesh_aabb_map: ResMut<MeshAabbMap>,
mut mesh_events: EventReader<AssetEvent<Mesh>>,
entities_lost_mesh: RemovedComponents<Handle<Mesh>>,
mut changed_aabbs: Local<HashSet<Handle<Mesh>>>,
mut inserted_aabb_components: Local<HashSet<Handle<Mesh>>>,
) {
for entity in entities_lost_mesh.iter() {
entity_mesh_map.deregister(entity);
// Identify Aabbs added to / updated on entities with Handle<Mesh> outside of this system. The
// goal here is to avoid recomputing Aabbs for Meshes that have already been computed
// elsewhere.
for (mesh_handle, aabb) in queries.p0().iter() {
// If the Aabb component was inserted using Commands on the previous run of this system,
// do not consider it as Changed for this run.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this miss updates on consecutive frames?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the question. Could you elaborate a bit on what you think could be a problem case that this misses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I clear inserted_aabb_components after executing this loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if, on frame N, the Mesh is updated, and then on frame N+1, the Aabb is updated by another system? It seems the update to the Aabb would be ignored, and so there could be two entities with the same Handle<Mesh> and yet different Aabbs.

It's not clear what this system is trying to accomplish - it seems either it should be entirely responsible for updating the Aabb when the NoAabbUpdate component is missing, and hence no need to check if the Aabb has changed, or it should not update the Aabb at all when NoAabbUpdate is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that NoAabbUpdate is a component, and I have made an assumption that an Aabb belongs to a Mesh asset but is also a component. I wanted to at least try to respect and indeed avoid calculating Aabbs for meshes if they have been manually added. For example, gltf stores min/max values of things including vertex positions which allows for very fast Aabb creation. I don’t want to ignore that and overwrite that Aabb for a glTF Mesh when it is already correct. Also, later I expect we will store Aabbs in metadata and will want to load them and not overwrite them. This should be the common case - either it has an Aabb at spawn time, or it needs one calculating, and it isn’t modified. But we also need to support runtime-modified Meshes and then we have to recompute unless the modification process has a faster way of updating the Aabb.

I recognise the corner case you point out, and maybe there is a way to address it by moving some of this logic to another system. I’ll have to think through it carefully.

if inserted_aabb_components.contains(mesh_handle) {
continue;
}
// NOTE: If two entities with the same Handle<Mesh> have different Aabbs then the last one
// will win here
mesh_aabb_map.insert(mesh_handle.clone_weak(), aabb.clone());
changed_aabbs.insert(mesh_handle.clone_weak());
}

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;
inserted_aabb_components.clear();

// Check AssetEvents for Meshes to maintain the Handle<Mesh> -> Aabb map. If a Created or
// Modified event is observed for a Handle<Mesh> that is in the changed_aabbs set, then skip
// recomputing the Aabb.
let mut updated_aabbs = HashSet::new();
for event in mesh_events.iter() {
match event {
AssetEvent::Created { handle } | AssetEvent::Modified { handle } => {
if changed_aabbs.contains(handle) {
// If the Aabb was manually added for this Handle<Mesh>, do not recompute it
changed_aabbs.remove(handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case does not (always) represent "manually adding". It also covers multiple created/modified events happening in the same frame for the same mesh. This would result in skipping changes.

continue;
}
if let Some(mesh) = meshes.get(handle) {
if let Some(updated_aabb) = mesh.compute_aabb() {
mesh_aabb_map.insert(handle.clone_weak(), updated_aabb);
updated_aabbs.insert(handle.clone_weak());
}
}
}
AssetEvent::Removed { handle } => {
mesh_aabb_map.remove(handle);
}
}
}

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());
// Check and update Aabbs on entities with Handle<Mesh>. This handles the following cases:
// - The Handle<Mesh> on the entity is modified.
// - The Aabb corresponding to the Handle<Mesh> was updated. This will happen if a Mesh itself
// is modified.
// - An Aabb was just added to or updated on the entity outside of this system.
for (entity, mesh_handle, mesh_changed, aabb) in queries.p1().iter_mut() {
// If an Aabb was just added/modified outside of this system, skip updating the component
if changed_aabbs.contains(mesh_handle) {
continue;
}
// If the Handle<Mesh> on the entity was modified, or the Aabb was updated due to the Mesh
// itself being modified, then update the entity's Aabb component
if mesh_changed || updated_aabbs.contains(mesh_handle) {
if let Some(updated_aabb) = mesh_aabb_map.get(mesh_handle) {
if let Some(mut aabb) = aabb {
// If there is an existing Aabb component, we can update it in-place
*aabb = updated_aabb.clone();
} else {
commands.entity(entity).insert(updated_aabb.clone());
// NOTE: Inserting a component via Commands causes a Changed<Component> query
// in this system to be hit next time the system is run. As such, track the
// Handle<Mesh> for which Aabbs were inserted on this run in order to avoid
// detecting them as Changed on the next run.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to put this in a different system and stage? Using change detection on something modified in the same system should be avoided, in my experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how that would help as the insertion would cause Changed to be triggered there too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the system that inserts missing Aabb components runs at an earlier stage, Changed<Components> should be triggered on the same frame as the first case on line 315. Or, the Aabb would only be updated in place, and would definitely be changed on the frame that it is first calculated.

Although I prefer avoiding component insertion at arbitrary updates, and would prefer adding the Aabb in some bundle (MaterialMeshBundle?). This means there's no need to check if the component needs to be added, because if it is missing, that means the user didn't want that component for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it were part of the bundle, we’d have to add something else to Aabb to indicate that it is up to date or something. If Aabbs are added with a default bound then we would have to compute it to update it to be correct. How would we know that the Aabb is incorrect without calculating it and comparing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That brings up something else: an Aabb as currently defined always has a center, but the bounds a Mesh with no vertices has no center and zero half extents, while a Mesh with a single vertex does have a center (the location of the vertex) and zero half extents.

I use:

pub enum Bounds<A> {
    Zero,
    Bounds { lo: A, hi: A },
}

so that I can use Bounds::Zero as the default (the bounds of nothing), which allows me to properly join any two bounds, because joining Zero to another bounds should be a no-op. (Zero with joining forms a monoid, if you're familiar with the concept.)

Then I have:

pub struct DefinedBounds<A> {
    pub lo: A,
    pub hi: A,
}

when I know there is at least one point inside the bounds.

I suggest modifying Aabb to also have a zero point case that can serve as the default. Then you can detect that it needs updating if the Mesh has positions, and the default is correct until the mesh is loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of a Mesh with no vertices? Also, it sounds like you're addressing the 'uninitialised' case, but what I was suggesting was the case of a Mesh that has been updated and maybe or maybe not the Aabb was updated this frame too. That is, the Aabb has been initialised but is perhaps no longer correct. I suppose your earlier argument is - should manual Aabb definition be allowed outside the bounds of NoAabbUpdate. If not, that would simplify things for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to use an event system to make updates unambiguous?

  • Newtype the Aabb over a private AabbInner type
  • Add a public getter that returns the inner AabbInner data
  • Add an event UpdateAabb, which is now the only public way to update an Aabb
  • When a mesh has updated, you can check if an event was sent to update the Aabb. If not, go ahead and compute it and send your own event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That or a generational marker. Every time a mesh is updated, bump the generation on the mesh. Check if the Aabb has the same generation as the mesh. If not, update it. External updates would need to bump the generation to match the mesh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of a Mesh with no vertices?

A Mesh with no vertices may not be used intentionally, but generated procedurally. For example, subtracting a shape from another that the first shape encloses results in an empty shape. I could remove the Mesh component or something, but the most natural representation is an empty Mesh. Also, adding and removing components can play havoc with other systems.

Furthermore, an empty mesh is a reasonable thing that has reasonable behavior, so handling it properly makes Bevy more robust, and avoids special cases that must be handled by the user. It's also usually worth implementing no-op behavior e.g. if you want to join several Aabbs, what do you start with? When summing integers, you can start with 0 (the identity element for addition). This extends to, say, filtering based on visibility. Option<Aabb> would just be hacking on an identity element without clear semantics - is None the identity, or an error, or maybe an infinite Aabb?

I do think that there needs to be a clear indication of which system should be updating the Aabb, because otherwise it could be a race.

inserted_aabb_components.insert(mesh_handle.clone_weak());
}
}
}
}
}
Expand Down