[Merged by Bors] - Recalculate entity aabbs when meshes change

[mlodato517]

Objective

Update the calculate_bounds system to update Aabbs
for entities who've either:

• gotten a new mesh
• had their mesh mutated

Fixes #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 Aabbs 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 Aabbs 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
and added some systems to mutate/assign meshes:

Test Script

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<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();
    }
}

Changelog

Fixed

Entity Aabbs not updating when meshes are mutated or re-assigned.

[alice-i-cecile]

This is a fantastic contribution; thank you!

This type is getting pretty gnarly - should I extract some types?

I don't think so; it's not reused, and it'll result in indirections. You get good at reading query signatures with time. My main gripe with it is the double negative Without<NoFrustumCulling>, but that's not your fault.

This system is public - should I add some quick docs while I'm here?

Yes please!

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?

bevy_utils::HashMap does exactly this; use it instead.

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)?

I would expect this edge case to be very rare, and the costs of using the HashSet non-trivial when we're modifying large numbers of mesh handles at once. IMO we should just repeat the work.

Please leave a comment to this effect :)

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.

The Changed information won't cache the old data. I'll need to chew on what should be done here.

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?

I have no good intuition for this particular question.

[nicopap]

Regarding the question on the edge case. I think you are right. We'd need to keep a HashMap<Entity, Handle<Mesh>>. So that it's possible to remove the Entity from right entry when things gets updated or removed (I think it might be less complex and more performant this way) (also check RemovedComponents for the case when an entity is despawned or have their Handle<Mesh> removed).

You can also have a update_bounds systems that update in-place the Aabbs with a Query<&mut Aabb>, this way, the update logic is separated from the create logic and the Aabbs are updated as soon as the update_bounds system is complete. but I don't think it's a big deal.

[superdump]

As the original author of the frustum culling code, I’d like to review this when I have time.

[mlodato517]

Updated some of the easy stuff. Will tackle the more complicated stuff later today hopefully!

I'll want to squash this before merging and add co-authors but for now hopefully this makes review easier.

[superdump]

Pretty much looks good to me, just a comment about what I perceive to be unnecessary documentation. Maybe @alice-i-cecile disagrees?

[mlodato517]

We'd need to keep a HashMap<Entity, Handle<Mesh>>. So that it's possible to remove the Entity from right entry when things gets updated or removed (I think it might be less complex and more performant this way) (also check RemovedComponents for the case when an entity is despawned or have their Handle<Mesh> removed).

You can also have a update_bounds systems that update in-place the Aabbs with a Query<&mut Aabb>, this way, the update logic is separated from the create logic and the Aabbs are updated as soon as the update_bounds system is complete. but I don't think it's a big deal.

I'm a little confused here. I like the idea of the separate update_bounds system because it sounds much cleaner but it won't have access to calculate_bound's Local<HashMap>. If we want to move the entire Local<HashMap> to update_bounds then I think it'll be hard to tell when to add an entity to a mesh handle's bucket.

Here's the pseudocode I was imagining - lemme know if that was the wrong read:

pseudocode

fn update_bounds(
    meshes: Res<Assets<Mesh>>,
    mesh_events: EventReader<AssetEvent<Mesh>>,
    mut to_update: Query<
        (Entity, &mut Aabb, &Handle<Mesh>),
        With<Changed<Handle<Mesh>>>,
    >,
    mut entities_with_mesh: Local<HashMap<Mesh, Entities>>,
    mut mesh_for_entity: Local<HashMap<Entity, Mesh>>,
    mut entities_with_removed_mesh: RemovedComponents<Handle<Mesh>>,
) {
    // If an entity no longer has a mesh, remove it from the list
    // of entities with that mesh.
    for entity in entities_with_removed_mesh {
        let mesh = &mesh_for_entity[entity];
        let entities_with_mesh = &mut entities_with_mesh[mesh];
        entities_with_mesh.swap_remove(get_idx(entity));
    }

    // If a mesh was mutated, update the Aabb for all entities with that mesh
    for event in mesh_events {
        let entities_with_mesh = &entities_with_mesh[event.mesh];
        let mesh = &meshes[event.mesh];
        let aabb = mesh.aabb();
        entities_with_mesh.iter_mut().for_each(|e| e.aabb = aabb.clone());
    }

    // Update Aabb for entities that got a new mesh
    for (entity, &mut aabb, mesh) in to_update {
        let mesh = meshes[mesh];
        let new_aabb = mesh.aabb();
        *aabb = new_aabb;

        // Does it go here? Does `Changed` work when things are added
        // for the first time? If not we'll probably fail on some mesh mutation
        // events. Maybe we can use `ChangeTrackers` to get changed _and_ added?
        entities_with_mesh.insert(mesh, entity);
    }
}

[nicopap]

@mlodato517 You would need to have the HashMap<Mesh, Entities> and HashMap<Entity, Mesh> in a Res shared between the two systems, probably create a new struct and have it inserted as a resource.

[mlodato517]

Added the maps in a resource in 5ee488a and added an update_bounds system. Hopefully this is somewhere along the lines of what y'all were thinking 🤞

[mlodato517]

Huh the rerequest button doesn’t seem to be working for nicopap 🤷

[james7132]

It's not something I want us to address in this PR, but skinned meshes are an extreme example of this, where the AABB both out of date and inaccessible basically every frame. We probably will need an exception for skinned meshes, and precompute a "extremes" bound for them using asset preprocessing of some kind.

[nicopap]

Looks good to me. Fixes #4294 alright. I think the comments on documentation should be addressed and then it's good for me.

[nicopap]

It's not something I want us to address in this PR, but skinned meshes are an extreme example of this, where the AABB both out of date and inaccessible basically every frame. We probably will need an exception for skinned meshes, and precompute a "extremes" bound for them using asset preprocessing of some kind.

@james7132 I opened an issue so that it's possible to discuss this independently in #4971

[superdump]

Basically LGTM. :)

[superdump]

Still basically looks good to me but spotted another thing. :) Thanks for your patience and persistence. I think we should be able to merge this very soon.

[superdump]

check_visibility must be run after this has run. Please add this to the list of check_visibility.after() systems below.

[mlodato517]

Excellent catch! Added in 9b36e77.

[superdump]

@alice-i-cecile @mockersf you previously reviewed this and left comments. Do you want to review again?

[superdump]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12