Mesh AABBs are never updated

[nicopap]

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.

[mlodato517]

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]

@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]

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]

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]

@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]

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]

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.