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

[superdump]

Objective

• Despawning many entities with Handle<Mesh> would cause O(n^2) worst case behaviour in update_bounds after [Merged by Bors] - Recalculate entity aabbs when meshes change #4944 . Improve performance here.

Background and desired behaviours

The use cases to be supported are:

• Entities being spawn with a Handle<Mesh>, maybe an Aabb component, and maybe a NoAabbUpdate component
• The Handle<Mesh> component being modified
• The Mesh asset being modified

In addition to this, I decided to consider the cases of many unique meshes (i.e. one Mesh asset per entity for many entities) and instanced meshes (i.e. one Mesh asset for many entities).

The resulting behaviours in the solution are:

• If unique meshes are spawned, Aabbs are computed and Aabb components are inserted
• If unique meshes with aabbs are spawned, aabbs are not recomputed, nor are they modified/inserted
• If unique meshes with aabbs are spawned and no aabb update is configured, aabbs are not recomputed, nor are they modified/inserted
• If an instanced mesh is spawned with aabbs and no aabb update, aabbs are not recomputed, nor are they modified/inserted
• If an instanced mesh is spawned with aabbs, aabbs are not recomputed, nor are they modified/inserted
• If an instanced mesh is spawned, one aabb is computed, Aabb components are inserted
• If an instanced mesh is modified, one aabb is computed and the entity Aabb components are updated in-place
• If an instanced mesh has its mesh handle re-assigned, the entity Aabb components are updated in-place

Solution

• Remove EntityMeshMap
• Remove the calculate_bounds system
• Add MeshAabbMap containing HashMap<Handle<Mesh>, Aabb> to track the Aabb calculated for each Mesh asset for fast lookup
• Modify update_bounds and add comments explaining how it works and why each portion of code or non-obvious logic is there.

---

Changelog

• Removed: The calculate_bounds system and its label were removed. Now all Aabb updates happen in the update_bounds system.
• Fixed: Performance regression when despawning many mesh entities that was introduced in [Merged by Bors] - Recalculate entity aabbs when meshes change #4944

Migration Guide

If your system previously depended on the CalculateBounds label, it should now depend on the UpdateBounds label.

[superdump]

It's possible this can be simplified. I was aiming for improving the asymptotic performance from worst case O(n^2) to O(n) and making sure to address all the important use cases so there are no bad performance spikes, except for creating a large number of new Mesh assets for which there are no Aabbs. There is not too much that can be done about that by bevy. When we have an asset pipeline we can store Aabbs in metadata. And for procedural Meshes, one could keep track of/create one's own Aabbs as part of the Mesh generation process.

I had to add some system-local HashSets to track things. In particular the mut inserted_aabb_components: Local<HashSet<Handle<Mesh>>>, is to work around the fact that using Commands to insert a component from within a system that also queries for whether that component Changed, will trigger a hit for that query on its next execution because application of system commands has no knowledge of which system queued the command to be able to track whether the component changed within that system or separately.

For the instancing cases I was testing on top of an updated version of my improved shader_instancing example PR that uses one Entity per instance.

[HackerFoo]

Couldn't this miss updates on consecutive frames?

[superdump]

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

[superdump]

Note that I clear inserted_aabb_components after executing this loop.

[HackerFoo]

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.

[superdump]

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.

[HackerFoo]

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.

[superdump]

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

[HackerFoo]

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.

[superdump]

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?

[HackerFoo]

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.

[superdump]

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.

[aevyrie]

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.

[aevyrie]

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.

[HackerFoo]

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.

[cart]

This approach (and our previous approach) still feels off to me:

1. We're duplicating the same AABB potentially hundreds of thousands of times across entities where the mesh data is the source of truth. We've broken ourselves into "state sync" jail, largely because we don't want to look up the Aabb for each mesh during visibility calculations. Is this memory + complexity cost worth it? We should quantify this (if we haven't already). Note that AABBs will absolutely need to be pre-computed for meshes as part of asset preprocessing.
2. We currently use Commands to insert AABBs for entities when they are available. This adds unnecessary overhead: Command allocation and execution, Archetype changes can be costly.
3. In this PR an entity's AABB component value is the product of complicated change detection logic on the AABB itself, the "uniqueness" of the mesh, whether or not NoAabbUpdate is present, etc. At a given moment in time, it is very unclear how data flows. As a user, I would have no clue what interactions with AABB are appropriate / when my changes will be overwritten / etc. Eight lines of cases is too hard to reason about.

I think we should seriously consider alternatives in the vein of:

• AABBs are directly tied to Mesh assets by default. Short term, in a separate HashMap<Handle<Mesh>, AABB>. We fill this using asset events. Long term, we sort out the best way to do this with asset preprocessing.
• Frustum culling looks up the AABB for the Mesh
• Custom per-entity AABBs are accomplished in one of two ways:
  1. Config Component (added to every mesh bundle)

     // downside is that this still takes up extra space on each entity. the pro is that its easily discoverable.
     #[derive(Component)]
     enum AabbConfig { FromMesh, Custom(Aabb)  }

  2. Implicitly assume mesh-provided unless an AabbOverride(Aabb) component is added. This has the benefit of saving space.

I did a quick and dirty port to test many_cubes perf (camera rotation disabled for stability) with the extra hashing:

(red is this pr, yellow is "no AABB components and per-entity hashing to look up AABB using handles")

There is more variance, and the mean time does increase by 0.27 milliseconds. But the median time is almost completely unaffected.

To account for this change, I inverted the NoFrustumCulling marker component to FrustumCulling to make it "opt-in", which it effectively is in practice now anyway (now that we have a unified visibility system ... we were just using AABB as the "opt in"). I then added that component to the MaterialMeshBundle.

This has the benefit of being much simpler to reason about / review (in addition to the other benefits mentioned above).

[cart]

Another ~24 second sample of the "per entity aabb removal in favor of hash sets":

I think the difference might be completely in the noise.

[cart]

That being said, im proposing changes that are enough changes to the api that im not super comfortable merging them last minute. I think our only two short term options are:

1. Merge this PR (after thoroughly reviewing each case)
2. Wait until after 0.8 to make a call. Eat the bad despawn behavior in the short term. Maybe do a patch release.

[alice-i-cecile]

I'm okay to eat the poor behavior for right now rather than rushing through a complex fix or delaying the release. @aevyrie, how bad is this going to hurt for y'all?

[aevyrie]

We'll be fine. We aren't altering AABBs, that I'm aware of, so I don't think it would be an issue. Plus, we can just wait to upgrade until a point release. 🙂

[cart]

As a note: we couldn't do the changes I'm proposing in a point release as they are very breaking.

[cart]

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.

[aevyrie]

I agree that the AABBs should be bound to the Mesh directly, but with the caveat that we need to be sure we support non-mesh AABBs, which you've addressed in the above comment.

I think I'd prefer we make the Aabb component purely user-driven; an input for the culling system. For culling we would:

1. (With<Handle<Mesh>>, Without<Aabb>): use the system-internal mesh-aabb hashmap lookup.
2. (With<Aabb>): use Aabb as the source of truth. If users need to opt-in to a custom AABB, they just add an Aabb. We would never use the hashmap for these cases.

In other words, we split this into two disjoint sets, so there is never possibility for a conflict between the auto-update and the user update. It also optimizes memory usage for the happy/simple path where a user just has a mesh and no need to customize AABB behavior.

[cart]

I lied we have one more alternative for 0.8: revert #4944.

People who really need this feature can re-implement it in user-space, as these are all public types. Thats my personal favorite path at this point.

[alice-i-cecile]

Yeah, I'm on team "let's just revert and reopen the issue".

[cart]

Coolio. Doing that now.

[cart]

Closing in favor of #4944 (and ideally reworking this as described)

[superdump]

Just a note: when profiling, I’d appreciate to see a comparison between execution times of the affected system as well as frame times. Frame times are affected by vsync in confusing ways that are not so reliable. Most system execution times are much more reliable. @cart - for historic reasons could you please post a comparison of check_visibility and I guess also check_light_mesh_visibility?

I agree that this PR is super complicated to reason about. I agree with reverting. I agree that Aabbs being looked up for a Mesh makes sense as long as performance isn’t hurt significantly as it’s much simpler. As for memory usage - an Aabb is 6 floats iirc which is 24 bytes, so even for millions of entities in an entire scene it’s still only 10s of MB which for a scene that size is perhaps not unreasonable. Looking up Aabbs would definitely be more efficient in terms of memory usage though.