Skip to content

Commit

Permalink
Combine visibility queries in check_visibility_system (#10196)
Browse files Browse the repository at this point in the history
# Objective

Alternative to #7310

## Solution

Implemented the suggestion from
#7310 (comment)

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:/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.
  • Loading branch information
rparrett authored Nov 2, 2023
1 parent 0dfb6cf commit 09c2090
Showing 1 changed file with 17 additions and 45 deletions.
62 changes: 17 additions & 45 deletions crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,19 +390,10 @@ pub fn check_visibility(
&InheritedVisibility,
&mut ViewVisibility,
Option<&RenderLayers>,
&Aabb,
Option<&Aabb>,
&GlobalTransform,
Has<NoFrustumCulling>,
)>,
mut visible_no_aabb_query: Query<
(
Entity,
&InheritedVisibility,
&mut ViewVisibility,
Option<&RenderLayers>,
),
Without<Aabb>,
>,
) {
for (mut visible_entities, frustum, maybe_view_mask) in &mut view_query {
let view_mask = maybe_view_mask.copied().unwrap_or_default();
Expand All @@ -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;
Expand All @@ -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;
}
}
}

Expand All @@ -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());
}
Expand Down

0 comments on commit 09c2090

Please sign in to comment.