Recompute AABBs

[aevyrie]

Objective

• Recompute AABBs when meshes change.
• Optimize sprite AABB re-computation to avoid component insertion where mutation is possible.
• Fixes Mesh AABBs are never updated #4294 and closes Recompute Aabbs for updated Meshes #7971

Solution

• Implement the things.

Testing

• CI

[tychedelia]

Ooohh, this is such a nice longstanding issue to fix, really confused me the first time I hit it. So glad to see AssetChanged helping here. There's probably a bit of perf hit for the table scan but I think this is such a sharp edge and PostUpdate has a lot of noise anyway that I think it's totally worth it.

[aevyrie]

There's probably a bit of perf hit for the table scan

Agreed, but, I would expect entities with meshes is going to be a pretty manageable number, and would likely be render bottlenecked first. (could be wrong!)

[aevyrie]

That reminds me, let's make the mutation queries go brrrrr

[aevyrie]

Looks like this I open, need to review why: #7971

The notable bit:

Note: this only addresses when the Mesh is updated itself, not when the Handle has changed. I attempted to include a Ref<Handle>::is_changed check, but that easily added 1+ms on many_cubes to do what is basically zero work in that common case. I attempted to parallelize it with QueryParIter, which shrunk it down to 360us, but it still was an excessive amount of work for basically nothing. This current implementation only incurs a cost when a Mesh is created or modified.

I view this as a stop gap until we have a form of asset preprocessing that can be used to compute the Aabb offline, or force meshes to be immutable after construction and precompute it's Aabb ahead of time.

[IceSentry]

What does it mean to be ambiguous with itself?

[alice-i-cecile]

Ignore all ambiguities between systems in the same set.

[IceSentry]

Code looks good, might need an opt-out just to be on the safe side.

The previous versions of this seemed to suffer from various performance issues but they also didn't have access to AssetChanged that allows for such a simple implementation.

[alice-i-cecile]

We should bench this before merging, but I'm inclined to merge even with a modest regression. This is a really nasty bug, and "fast but incorrect" really isn't very useful.

[rparrett]

As far as I can tell, with this setup, meshes spawned with Aabbs will have that Aabb immediately overwritten. This might just result in a double calculation for something like persisted scenes, or a pretty nasty situation in cases where the user actually needs a custom Aabb. (For example, with animated meshes)

[aevyrie]

Yep. There are a few ways we could solve this, I don't know if we have precedent for any. The closest I can think of is something like an opt-out ZST like NoAutoAabb.

There is no way to tell if a newly added Aabb is there as a default initialized value, or as a precomputed value. From what I can tell, it seems like you need to have some way of specifying this, and doing it via archetype (marker) is the most efficient.

[atlv24]

What if we initialized Aabb to NaNs

[alice-i-cecile]

What if we initialized Aabb to NaNs

We could even use special NaNs with cursed marker bits!! I don't know if we should do this, but it's an exciting idea because that feature of floats is so useless normally.

[greeble-dev]

My unasked for take on NaNs: please don't! NaNs can easily go viral, and tracking them back to the source can be hard. This makes them expensive in terms of developer time, particularly for beginners.

Glam has some asserts that can help, but they have limited coverage and are disabled by default.

[kfc35]

I’m curious what’s blocking this pull request from moving forward (besides the conflicts)? Just asking since it seems like this pull request has been waiting for a while, and it seems like something that should be resolved!

RE an opt-out: having an opt-out Component for the new functionality sounds reasonable enough to me. Is that what’s being suggested? Or the opt-out applies to any calls to compute_aabb basically?

[alice-i-cecile]

@aevyrie I'm inclined to merge this as-is, following discussion on Discord. If you have a bit of time to resolve merge conflicts I'd appreciate it; otherwise I'll try and find the time to do it myself.