bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Recompute Aabbs for updated Meshes

Open james7132 opened this issue 2 years ago • 1 comments

Objective

Partially address #4294. This was attempted in #4944 and #5423., but reverted in #5489. Aabb components are not updated when the underlying Mesh has changed in some way.

Solution

Recompute their Aabb when it's been updated or newly created.

Note: this only addresses when the Mesh is updated itself, not when the Handle<Mesh> has changed. I attempted to include a Ref<Handle<Mesh>>::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.


Changelog

TODO

Migration Guide

TODO

james7132 avatar Mar 08 '23 11:03 james7132

I'm not sure I understand your comment about asset preprocessing. Isn't the point of this to be able to recompute AABBs on runtime meshes? Spawning a mesh will already create the correct AABB. Asset preprocessing will help of course, but this will still need to exist for people making dynamic runtime meshes.

IceSentry avatar Mar 08 '23 21:03 IceSentry

I still think all of this state-syncing is a nightmare and we should try removing it entirely: https://github.com/bevyengine/bevy/pull/5423#issuecomment-1199995825

Needs more benchmarking though

cart avatar Jun 22 '23 21:06 cart

I just put together a "store aabb directly on meshes" impl: https://github.com/cart/bevy/commit/a465c1da355fa337f5b8659ae3789cf5b8e58d76

(still needs benchmarking ... haven't been able to compile Tracy on my new debian install yet)

cart avatar Jun 23 '23 01:06 cart

I just put together a "store aabb directly on meshes" impl: cart@a465c1d

(still needs benchmarking ... haven't been able to compile Tracy on my new debian install yet)

Tested a few different stress tests.

On many_cubes with --sphere, this was faster for overall frametimes but the par_for_each{query="(bevy_ecs::entity::Entity, &mut bevy_render::view::visibility::ComputedVisibility, ... query took longer.

(this is cart's "store aabb directly on meshes" cherrypicked onto bevy main (10f5c9206847ae01b8dc833c2680562e7bd46664), while ext is bevy main)

image

image

I tried testing many_lights, but it doesn't render any of the lights on cart's PR, and many_foxes panics with:

thread 'main' panicked at 'scene contains the unregistered type bevy_render::primitives::AabbSource. consider registering the type using app.register_type::<T>()', crates\bevy_scene\src\scene_spawner.rs:358:35

Elabajaba avatar Jun 27 '23 04:06 Elabajaba

Hmm thats certainly a big perf difference (likely prohibitively so). Worth trying to see what we can optimize though.

cart avatar Jun 27 '23 18:06 cart

Sadly punting this to 0.12. We've run out of time and this needs more of it :)

cart avatar Jul 09 '23 03:07 cart

This is a pretty high impact bug: I'm in favor of getting any fix in rather than delaying another cycle.

alice-i-cecile avatar Sep 29 '23 15:09 alice-i-cecile

I think this could be updated to use the new AssetId. And there was some feedback. It’s a shame it has to do so many lookups but if the benchmarking for this looks ok then it seems reasonable.

superdump avatar Oct 13 '23 19:10 superdump