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

Recompute Aabbs for updated Meshes #7971

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Mar 8, 2023

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 james7132 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Mar 8, 2023
@james7132 james7132 requested review from superdump and cart March 8, 2023 11:04
@james7132 james7132 added this to the 0.11 milestone Mar 8, 2023
@IceSentry
Copy link
Contributor

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.

if let Some(aabb) = changed.get(&mesh_handle.id()) {
*entity_aabb = *aabb;
} else if removed.contains(&mesh_handle.id()) {
commands.entity(entity).remove::<Aabb>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to get both a Created and Removed asset event for the same handle?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain, but I think we should probably code assuming this is possible, even if the current implementation happens to not make this possible (due to system ordering / deferred ops / etc).

@cart
Copy link
Member

cart commented Jun 22, 2023

I still think all of this state-syncing is a nightmare and we should try removing it entirely:
#5423 (comment)

Needs more benchmarking though

@cart
Copy link
Member

cart commented Jun 23, 2023

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)

@Elabajaba
Copy link
Contributor

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 (10f5c92), 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

@cart
Copy link
Member

cart commented Jun 27, 2023

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

@cart
Copy link
Member

cart commented Jul 9, 2023

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

@cart cart modified the milestones: 0.11, 0.12 Jul 9, 2023
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 29, 2023
@alice-i-cecile
Copy link
Member

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

@superdump
Copy link
Contributor

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.

@alice-i-cecile alice-i-cecile modified the milestones: 0.12, 0.13 Oct 24, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024
@james7132 james7132 added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Mar 17, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants