From 09c2090c15e19f700f5687097634b883e74e3dfc Mon Sep 17 00:00:00 2001 From: Rob Parrett Date: Thu, 2 Nov 2023 15:06:38 -0700 Subject: [PATCH] Combine visibility queries in check_visibility_system (#10196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Alternative to #7310 ## Solution Implemented the suggestion from https://github.com/bevyengine/bevy/pull/7310#discussion_r1083356655 I am guessing that these were originally split as an optimization, but I am not sure since I believe the original author of the code is the one speculating about combining them up there. ## Benchmarks I ran three benchmarks to compare main, this PR, and the approach from #7310 ([updated](https://github.com/rparrett/bevy/commits/rebased-parallel-check-visibility) to the same commit on main). This seems to perform slightly better than main in scenarios where most entities have AABBs, and a bit worse when they don't (`many_lights`). That seems to make sense to me. Either way, the difference is ~-20 microseconds in the more common scenarios or ~+100 microseconds in the less common scenario. I would speculate that this might perform **very slightly** worse in single-threaded scenarios. Benches were run in release mode for 2000 frames while capturing a trace with tracy. | bench | commit | check_visibility_system mean μs | | -- | -- | -- | | many_cubes | main | 929.5 | | many_cubes | this | 914.0 | | many_cubes | 7310 | 1003.5 | | | | | many_foxes | main | 191.6 | | many_foxes | this | 173.2 | | many_foxes | 7310 | 167.9 | | | | | many_lights | main | 619.3 | | many_lights | this | 703.7 | | many_lights | 7310 | 842.5 | ## Notes Technically this behaves slightly differently -- prior to this PR, view visibility was determined even for entities without `GlobalTransform`. I don't think this has any practical impact though. IMO, I don't think we need to do this. But I opened a PR because it seemed like the handiest way to share the code / benchmarks. ## TODO I have done some rudimentary testing with the examples above, but I can do some screenshot diffing if it seems like we want to do this. --- crates/bevy_render/src/view/visibility/mod.rs | 62 +++++-------------- 1 file changed, 17 insertions(+), 45 deletions(-) diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 2a465c55273e5..8b2a98e7e7537 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -390,19 +390,10 @@ pub fn check_visibility( &InheritedVisibility, &mut ViewVisibility, Option<&RenderLayers>, - &Aabb, + Option<&Aabb>, &GlobalTransform, Has, )>, - mut visible_no_aabb_query: Query< - ( - Entity, - &InheritedVisibility, - &mut ViewVisibility, - Option<&RenderLayers>, - ), - Without, - >, ) { for (mut visible_entities, frustum, maybe_view_mask) in &mut view_query { let view_mask = maybe_view_mask.copied().unwrap_or_default(); @@ -414,7 +405,7 @@ pub fn check_visibility( inherited_visibility, mut view_visibility, maybe_entity_mask, - model_aabb, + maybe_model_aabb, transform, no_frustum_culling, ) = query_item; @@ -430,20 +421,22 @@ pub fn check_visibility( return; } - // If we have an aabb and transform, do frustum culling + // If we have an aabb, do frustum culling if !no_frustum_culling { - let model = transform.affine(); - let model_sphere = Sphere { - center: model.transform_point3a(model_aabb.center), - radius: transform.radius_vec3a(model_aabb.half_extents), - }; - // Do quick sphere-based frustum culling - if !frustum.intersects_sphere(&model_sphere, false) { - return; - } - // If we have an aabb, do aabb-based frustum culling - if !frustum.intersects_obb(model_aabb, &model, true, false) { - return; + if let Some(model_aabb) = maybe_model_aabb { + let model = transform.affine(); + let model_sphere = Sphere { + center: model.transform_point3a(model_aabb.center), + radius: transform.radius_vec3a(model_aabb.half_extents), + }; + // Do quick sphere-based frustum culling + if !frustum.intersects_sphere(&model_sphere, false) { + return; + } + // Do aabb-based frustum culling + if !frustum.intersects_obb(model_aabb, &model, true, false) { + return; + } } } @@ -454,27 +447,6 @@ pub fn check_visibility( cell.set(queue); }); - visible_no_aabb_query.par_iter_mut().for_each(|query_item| { - let (entity, inherited_visibility, mut view_visibility, maybe_entity_mask) = query_item; - - // Skip computing visibility for entities that are configured to be hidden. - // `ViewVisibility` has already been reset in `reset_view_visibility`. - if !inherited_visibility.get() { - return; - } - - let entity_mask = maybe_entity_mask.copied().unwrap_or_default(); - if !view_mask.intersects(&entity_mask) { - return; - } - - view_visibility.set(); - let cell = thread_queues.get_or_default(); - let mut queue = cell.take(); - queue.push(entity); - cell.set(queue); - }); - for cell in &mut thread_queues { visible_entities.entities.append(cell.get_mut()); }