-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
2d frustum culling #3944
2d frustum culling #3944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The math checks out and the code quality is good. The benchmarks are encouraging too! I really like reducing performance footguns for new users.
However, this needs a lot of tests: this functionality is notoriously fragile. We should test with:
- scaled sprites
- scaled cameras
- orthographic zoomed cameras
- rotated cameras and sprites
- sprites that are on the border of the screen
- sprites that are larger than the camera's view
- sprites that are moving onto the screen
- sprites that are just spawned
@@ -178,6 +181,45 @@ impl SpecializedPipeline for SpritePipeline { | |||
} | |||
} | |||
|
|||
pub fn calculate_bounds( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs doc strings: it's a public system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups...that's an oversight, the system doesn't need to be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...well maybe not exactly an oversight, because all the other functions in that file are pub
too. But i can make it pub(crate) calculate_bounds
if you want.
Uff...well...do such tests exist for 3d frustum culling? I am mostly using what is already used for 3d, so it should work equally well for 2d as it does for 3d. But i have tested all your points
|
There aren't, but there should be :p I didn't review that PR though. It should be fine to create these tests for either 2D or 3D.
It's great to hear that these all work, and it makes me much more confident in this PR. But tests aren't just for verifying that the code works at time of creation; they're essential for ensuring that code isn't accidentally broken in the future <3 I want contributors to feel confident when trying to further optimize or refactor this code, so we should add automated tests for this to the code base. |
Sure, i can understand that and think it is useful. But such tests also don't exist for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor suggestions that are probably inconsequential but worth trying anyway, and please try batch insertion as for many entities it should be faster.
let size = sprite.custom_size.unwrap_or_else(|| image.size()); | ||
let aabb = Aabb { | ||
center: Vec3::ZERO, | ||
half_extents: size.extend(0.0) * 0.5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe faster?
half_extents: size.extend(0.0) * 0.5, | |
half_extents: (0.5 * size).extend(0.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try that. Now that you mention this, i noticed that in some other places divisions are used instead of multiplications, but as far as i know or as far as i was taught, multiplications are preferred because they are (in most cases) faster than divisions. Is this something we want to change? Not in this PR, but in a new one.
.unwrap_or_else(|| (rect.min - rect.max).abs()); | ||
let aabb = Aabb { | ||
center: Vec3::ZERO, | ||
half_extents: size.extend(0.0) * 0.5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
half_extents: size.extend(0.0) * 0.5, | |
half_extents: (0.5 * size).extend(0.0), |
for (entity, mesh_handle) in without_aabb.iter() { | ||
if let Some(mesh) = meshes.get(&mesh_handle.0) { | ||
if let Some(aabb) = mesh.compute_aabb() { | ||
commands.entity(entity).insert(aabb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Batch insertion may be faster. Please try it.
Sorry for the late reply! Can you reproduce this bug with cubes and EDIT: Tried it myself. 3D seems to not have the same problem. That will be fun to debug 🙃. The 3D code i used use bevy::prelude::*;
fn main() {
App::new()
.insert_resource(Msaa { samples: 4 })
.add_plugins(DefaultPlugins)
.add_startup_system(setup)
.run();
}
const CUBE_WIDTH: f32 = 0.11;
const CUBE_HEIGHT: f32 = 0.11;
fn setup(
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
) {
let mesh = meshes.add(Mesh::from(shape::Cube { size: 0.1 }));
let mut camera = OrthographicCameraBundle::new_3d();
camera.transform.scale.z = 1000.0;
commands.spawn_bundle(camera);
for i in 0..10 {
for j in 0..10 {
let (x, y) = (i as f32 * CUBE_WIDTH, j as f32 * CUBE_HEIGHT);
commands.spawn_bundle(PbrBundle {
mesh: mesh.clone(),
transform: Transform::from_translation(Vec3::new(x, y, -y)),
..default()
});
}
}
} |
Ok...it boils down to how the two orthographic cameras are set up. |
In the context of bevy we use a reverse-z, so I would expect negative z to show since it should be in front of the camera. Other than that, I would indeed expect sprites behind the camera to be culled, it doesn't really make sense to have sprites behind the camera. |
Closing in favor of #7885 |
#7885) # Objective - Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d`. - Enable frustum culling for 2D entities since frustum culling requires a `Aabb` component in the entity to function. - Improve 2D performance massively when there are many sprites out of view. (ex: `many_sprites`) ## Solution - Derived from @Weasy666's #3944 pull request, which had no activity since multiple months. - Adapted the code to the latest version of Bevy. - Added support for sprites with non-center `Anchor`s to avoid culling prematurely when part of the sprite is still in view or not culling when sprite is already out of view. ### Note - Gives 15.8x performance boosts in some scenarios. (5 fps vs 79 fps with 409600 sprites in `many_sprites`) --------- Co-authored-by: ira <[email protected]>
Objective
Enable frustum culling for 2D, to be more precise, for
Mesh2d
,Sprite
andTextureAtlasSprite
.Solution
Mesh2d
is essentially only a regularMesh
with its handle wrapped byMesh2dHandle
, which enables it to be drawn by the 2d pipeline. As such it already has thecompute_aabb()
function, which i used for frustum culling the same way it is done for the regularMesh
.Sprite
has an image/texture, which provides a size, or a user specifiedcustom_size
. I've addedComputedVisibility
to theSpriteBundle
and use the size to create anAabb
and insert it as a component. The visibility is computed by the already existingcheck_visibility
system.TextureAtlasSprite
i use the user specifiedcustom_size
, or a size computed from the sprite'sRect
as specified in theTextureAtlas
. I've addedComputedVisibility
to theSpriteSheetBundle
too and the rest is the same as with what i have done for theSprite
.Benchmarks
System: Windows 10, AMD Ryzen 5 5600XT, AMD Radeon RX 570
Commands
cargo run --release --example bevymark -- 13 10000
cargo run --release --example many_sprites
Additional stuff
I have run
tracy-trace
too, here are two captures ofmany_sprites
, one with and one without culling.tracy_traces.zip
The traces show, that a lot of the additional fps come from the time drop in
queue_sprites
, from 15.5ms to 43us. That should come from the reduced number of extracted sprites that need to be queued.