Skip to content
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.

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

Mesh AABBs are never updated #4294

Open
nicopap opened this issue Mar 22, 2022 · 20 comments
Open

Mesh AABBs are never updated #4294

nicopap opened this issue Mar 22, 2022 · 20 comments
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help!

Comments

@nicopap
Copy link
Contributor

nicopap commented Mar 22, 2022

Bevy version: 0.6
OS & graphic stack: ArchLinux kernel 5.16.2

Problem

When using https://github.com/aevyrie/bevy_mod_raycast which relies on bevy's Aabb, and I update an entity's mesh, either by (1) changing it's Handle<Mesh> or (2) updating the mesh itself in the Assets<Mesh>, the plugin doesn't account for the change in mesh value.

This is because bevy never updates the Aabb of an entity after the initial spawn of a mesh.

Solution

It is legitimate to expect the Aabbs would be updated with the meshes or mesh handles of entities (certainly the documentation doesn't mention that). Either this should be documented as expected behavior or the code amended to update Aabbs based on the current mesh.

Technical solution

It seems the problematic system is bevy_render::view::visibility::calculate_bounds . ( crates/bevy_render/src/view/visibility/mod.rs line 112 on 024d984).

#5489 has a few discussions around potential fixes.

Current Workaround

Manually update the Aabb after updating the mesh.

@nicopap nicopap added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 22, 2022
@alice-i-cecile alice-i-cecile added A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Mar 23, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone May 2, 2022
@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label May 2, 2022
@mlodato517
Copy link
Contributor

Hi! I'm fairly new to bevy and would love to try and help here (if I could get some guidance!). These are a few new concepts for me here so I'm just going to parrot things back to see if my understanding is correct:

  1. calculate_bounds is a system responsible for finding all entities with meshes without aabbs and calculating aabbs for them
  2. if an entity has one mesh and the handle is changed so that it has a different mesh, the aabb isn't recalculated for that entity
  3. if a mesh is mutated to be different, the aabb isn't recalculated for entities with that mesh

One proposal for solving 2 is to add Changed<Handle<Mesh>> to the query in the system so that it operates not just on entities with meshes without aabbs but also entities whose mesh handle has changed.

One proposal for solving 3 is to add a listener for mesh mutation events to recalculate aabbs for meshes that have changed (though this event reading approach may introduce some performance issues).

Is that roughly correct? If so, I'm happy to go ahead and try to do 2 since it sounds straightforward/non-contentious. I'm also happy to attempt 3 as directed unless we want to discuss other ways to handle it!

@nicopap
Copy link
Contributor Author

nicopap commented May 28, 2022

@mlodato517 Your whole speculation is not just "roughly" correct, it's dead on! I said that the event reading approach might introduce perf issues. For posterity, I think it's going to be a perf issue because, even if you can listen for AssetEvent<Mesh>::Modified, you would need to iterate over all entities with a Handle<Mesh> and update the AABB of the ones that has changed. There might be no way around it. I think it would require keeping a registry of which entities have which Handle<Mesh> (something like HashMap<Handle<Mesh>, Vec<Entity>>).

@nicopap
Copy link
Contributor Author

nicopap commented May 28, 2022

Note that solving only 2 is nice, but is not going to be a huge gain. I would expect that most mesh mutations would occur through mutating the Mesh in Assets<Mesh> rather than swapping the Handle<Mesh>.

@mlodato517
Copy link
Contributor

For posterity, I think it's going to be a perf issue because, even if you can listen for AssetEvent<Mesh>::Modified, you would need to iterate over all entities with a Handle<Mesh> and update the AABB of the ones that has changed.

Oh I'm glad you said this! I had assumed (silly me!) that the performance hit was something inherent to EventReader or something.

There might be no way around it. I think it would require keeping a registry of which entities have which Handle<Mesh> (something like HashMap<Handle<Mesh>, Vec<Entity>>).

I think this makes sense! I'm guessing the lookup would be undesirable in the hot path of collision detection but to avoid the O(n) update of entity mesh AABBs when a mesh is mutated we could:

  1. not store mesh AABBs with the entity but instead in a separate lookup by mesh
  2. whenever we want to do collision with an entity we look up its AABB in some HashMap<Handle<Mesh>, AABB>

This would mean that updating the AABB after a mutation might be O(1) but you'd need a HashMap lookup every time you want an entity's bounding box which is suspicious at best (even if we did use something like ahash because we wouldn't need siphash for these I assume?). And I'm guessing mesh handle IDs aren't so well behaved that we could get away with Vec indexing?

@nicopap
Copy link
Contributor Author

nicopap commented May 31, 2022

@mlodato517 I was rather thinking the following: The HashMap can be used locally "to keep track" (bevy let you keep local system state with the Local<Foo> system parameter). The HashMap lookup would only happen when you read an update event, you'd then use the list of entities from the HashMap for the provided handle to find which Aabbs to update with a Query::get_mut(entity). Consumers of the Aabb components (such as a potential collision detection system) will access it through the ECS with a Query<&Aabb> or similar. So the performance hit is restricted to the calculate_bound system in any case.

@mlodato517
Copy link
Contributor

Hmmm maybe I am a bit confused. I thought originally you were suggesting roughly this pseudocode:

pub fn calculate_bounds(
    mut commands: Commands,
    meshes: Res<Assets<Mesh>>,
    without_aabb: Query<(Entity, &Handle<Mesh>), (Without<Aabb>, Without<NoFrustumCulling>)>,
    mesh_events: EventReader<AssetEvent<Mesh>>,              // new
    entities_with_mesh: &HashMap<Handle<Mesh>, Vec<Entity>>, // new
) {
    // same old code plus...

    let updated_meshes = mesh_events.iter().filter_map(|&event| match event {
        Modified { handle } => Some(handle),
        _ => None,
    });
    for mesh_handle in updated_meshes {
        if let Some(mesh) = meshes.get(mesh_handle) {
            if let Some(aabb) = mesh.compute_aabb() {
                if let Some(entities) = entities_with_mesh(mesh_handle) {
                    commands.entity(entity).insert(aabb); // or do we need to remove + insert?
                // lots of closing curlies...

Was I way off there? (I also wonder if this should be a different system from calculate_bounds since that system would be getting cluttered 🤔 But it's still under the idea of "calculating bounds" so ⚖️)

If not

I think this makes perfect sense. I was just saying that, if we wanted to, instead of storing HashMap<Handle<Mesh>, Vec<Entity>> we could store HashMap<Handle<Mesh>, Aabb> and we mutate that when there's an event and then instead of having Query<&Aabb> we have like Query<Handle<Mesh>> and then they do a lookup in the HashMap for the Aabb.

I think that's undesirable because it impacts the performance of everyone checking for Aabb (and it leaks the abstraction for how these Aabbs are managed) but, if we expect frequent mesh updates (which I doubt) then this would avoid the big loop in calculate_bounds.

Not that I'm saying we should go this way, just wanted to make sure my suggestion was clear :-)

@nicopap
Copy link
Contributor Author

nicopap commented May 31, 2022

I had in mind pretty much what you wrote down yeah. I was thinking of the entities_with_mesh argument as follow though:

mut entities_with_mesh: Local<HashMap<Handle<Mesh>, Vec<Entity>>>

(and updating the hashmap when necessary).

A hint: you could maybe chain the filter_map in the iterator to reduce rightward drift, or even use the ? operator in a closure that returns an option.

The idea described in the if not section seems indeed not the best.

@mlodato517
Copy link
Contributor

Oh, sorry, last thing, did we want this to be a separate system (e.g. "update_mesh_bounds()") or keep it in calculate_bounds? It seems like you've more or less said keep it in the same system but just double checking because it wasn't crazy explicit and apparently I need my hand held the whole way 😅

@nicopap
Copy link
Contributor Author

nicopap commented May 31, 2022

@mlodato517 If we want to use a Local<HashMap<_>>, you'll need to keep the initialization and update in the same system.

@mlodato517
Copy link
Contributor

Oh, yeah, because we're going to add items to the map when we calculate bounds for them the first time?

@nicopap
Copy link
Contributor Author

nicopap commented May 31, 2022

@mlodato517 this is correct. I think you have everything you need now.

@mockersf
Copy link
Member

Another way would be to consider that the aabb of a mesh should be an asset rather than a component. That way each entity would have the handle to the same aabb linked to their mesh. Not sure how much that would add when querying though...

@mlodato517
Copy link
Contributor

That's an interesting idea! If it were an Asset would it essentially require a HashMap lookup to get the Aabb? If so, is that sort of similar to the suggestion at the bottom of this comment or is it different in a way I don't see yet?

Side Question

What is "an Asset"? Kind of from like a semantic/best practice point of view? It seems like there may be situations where you can make something an asset for some benefit but I wonder if there are any docs for when you should (or maybe more importantly shouldn't) make something an asset. It's totally tangential to this thread so we can take the conversation elsewhere but I'd be happy to either update the Assets documentation or cheatbook docs (or maybe it's already clear enough from there but my take from that description is that Aabbs don't "make sense" as assets).

@bors bors bot closed this as completed in c2b332f Jul 20, 2022
@cart
Copy link
Member

cart commented Jul 29, 2022

Reopening this to account for #5489

@cart cart reopened this Jul 29, 2022
@cart cart removed this from the Bevy 0.8 milestone Jul 29, 2022
@nicopap nicopap added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Jul 30, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
# Objective

Update the `calculate_bounds` system to update `Aabb`s
for entities who've either:
- gotten a new mesh
- had their mesh mutated

Fixes bevyengine#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::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();
    }
}
```

</details>

## Changelog

### Fixed
Entity `Aabb`s not updating when meshes are mutated or re-assigned.
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# Objective

Update the `calculate_bounds` system to update `Aabb`s
for entities who've either:
- gotten a new mesh
- had their mesh mutated

Fixes bevyengine#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::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();
    }
}
```

</details>

## Changelog

### Fixed
Entity `Aabb`s not updating when meshes are mutated or re-assigned.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Update the `calculate_bounds` system to update `Aabb`s
for entities who've either:
- gotten a new mesh
- had their mesh mutated

Fixes bevyengine#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::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();
    }
}
```

</details>

## Changelog

### Fixed
Entity `Aabb`s not updating when meshes are mutated or re-assigned.
@alice-i-cecile
Copy link
Member

Reproduced in Bevy 0.9: great video of the problem here.

@nerdachse
Copy link

This is super annoying. One can easily circumvent this, but it's still a trap. Can we get this merged in 0.11?

@splashdust
Copy link

splashdust commented Jun 25, 2023

If anyone is here looking for a quick workaround in the meantime, manually removing the AABB from the entity, after the mesh has been changed, seem to do the trick:

commands
    .entity(entity_id)
    .remove::<bevy::render::primitives::Aabb>();

@jonathandw743
Copy link

jonathandw743 commented Jun 9, 2024

If anyone is here looking for a quick workaround in the meantime, manually removing the AABB from the entity, after the mesh has been changed, seem to do the trick:

commands
    .entity(entity_id)
    .remove::<bevy::render::primitives::Aabb>();

I don't know if this just removes the aabb and disables culling or not.
This is the solution I've been using:

if let Some(aabb) = mesh.compute_aabb() {
    commands.entity(entity).try_insert(aabb);
}

(from #10801)

@Affinator
Copy link

This is documented for Aabb as "It won’t be updated automatically if the space occupied by the entity changes, for example if the vertex positions of a Mesh inside a Handle are updated."

It is also documented for Mesh under "Common points of confusion" as "Bevy performs frustum culling based on the Aabb of meshes, which is calculated and added automatically for new meshes only. If a mesh is modified, the entity’s Aabb needs to be updated manually or deleted so that it is re-calculated."

I also stumbled over this while implementing terrain generation/modification, but I think the documentation is clear enough. It is subjective if there should already be a system in place to update the Aabbs for modified meshes. But IMHO if someone is modifying vertices by hand, then it can be expected that this someone reads the documentation anyway.

I propose to close this issue.

@alice-i-cecile
Copy link
Member

It's good that this is documented but I don't see a fundamental reason why we need to keep this footgun in place. Is there an unavoidable performance cost?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants