Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d` #7885

opstic · 2023-03-03T17:22:05Z

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 2d frustum culling #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 Anchors 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)

james7132 · 2023-03-16T09:12:34Z

Wondering if we should have a Aabb2D to reduce the memory usage and computation cost of running this. Not blocking, but probably a good idea as an optimization.

Elabajaba · 2023-04-17T16:19:31Z

This gave me a huge fps boost on many_sprites with the default settings (102400 sprites), going from ~60fps to ~1100fps.

Bevymark performance seemed slightly faster with 10 batches of 10000 sprites (100k total), though that could've just been the normal run to run variation since I didn't bother closing everything first to reduce noise.

edit: Code looks good to me.

irate-devil

I'm surprised this doesn't negatively impact performance when all sprites are on screen like in bevymark, but that's good :)

crates/bevy_sprite/src/lib.rs

Weasy666 · 2023-04-21T11:52:54Z

Oh... i absolutely forgot that my PR stil existed, if would just have pinged me, i would have invested the time to bring it up to date 🙃.
But I have absolutely nothing against the fact that someone else has invested the time to get this done 😆. I would just be happy to also get some form of attribution 🙂🙈

opstic · 2023-04-21T13:48:36Z

I would just be happy to also get some form of attribution 🙂🙈

I have this line in the main section of this pull request that references the original one:
"Derived from @Weasy666's 2d frustum culling #3944 pull request, which had no activity since multiple months."
Shouldn't this be included in the commit if this was merged? Because I've seen this behavior for other pull requests.
If that's not enough, please tell me how to add more attribution. I'm pretty new to contributing, after all 😅.

Co-authored-by: ira <JustTheCoolDude@gmail.com>

# Objective Frustum culling for 2D components has been enabled since #7885, Fixes #8490 ## Solution Re-introduced the comments about frustum culling in the many_animated_sprites.rs and many_sprites.rs examples. --------- Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com> Co-authored-by: François <mockersf@gmail.com>

github-actions · 2023-07-12T23:35:45Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

# Objective This PR's first aim is to fix a mistake in `HalfSpace`'s documentation. When defining a `Frustum` myself in bevy_basic_portals, I realised that the "distance" of the `HalfSpace` is not, as the current doc defines, the "distance from the origin along the normal", but actually the opposite of that. See the example I gave in this PR. This means one of two things: 1. The documentation about `HalfSpace` is wrong (it is either way because of the `n.p + d > 0` formula given later anyway, which is how it behaves, but in that formula `d` is indeed the opposite of the "distance from the origin along the normal", otherwise it should be `n.p > d`) 2. The distance is supposed to be the "distance from the origin along the normal" but when used in a Frustum it's used as the opposite, and it is a mistake 3. Same as 2, but it is somehow intended Since I think `HalfSpace` is only used for `Frustum`, and it's easier to fix documentation than code, I assumed for this PR we're in case number 1. If we're in case number 3, the documentation of `Frustum` needs to change, and in case number 2, the code needs to be fixed. While I was at it, I also : - Tried to improve the documentation for `Frustum`, `Aabb`, and `VisibilitySystems`, among others, since they're all related to `Frustum`. - Fixed documentation about frustum culling not applying to 2d objects, which is not true since #7885 ## Remarks and questions - What about a `HalfSpace` with an infinite distance, is it allowed and does it represents the whole space? If so it should probably be mentioned. - I referenced the `update_frusta` system in `bevy_render::view::visibility` directly instead of referencing its system set, should I reference the system set instead? It's a bit annoying since it's in 3 sets. - `visibility_propagate` is not public for some reason, I think it probably should be, but for now I only documented its system set, should I make it public? I don't think that would count as a breaking change? - Why is `Aabb` inserted by a system, with `NoFrustumCulling` as an opt-out, instead of having it inserted by default in `PbrBundle` for example and then the system calculating it when it's added? Is it because there is still no way to have an optional component inside a bundle? --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

…ne#9136) # Objective This PR's first aim is to fix a mistake in `HalfSpace`'s documentation. When defining a `Frustum` myself in bevy_basic_portals, I realised that the "distance" of the `HalfSpace` is not, as the current doc defines, the "distance from the origin along the normal", but actually the opposite of that. See the example I gave in this PR. This means one of two things: 1. The documentation about `HalfSpace` is wrong (it is either way because of the `n.p + d > 0` formula given later anyway, which is how it behaves, but in that formula `d` is indeed the opposite of the "distance from the origin along the normal", otherwise it should be `n.p > d`) 2. The distance is supposed to be the "distance from the origin along the normal" but when used in a Frustum it's used as the opposite, and it is a mistake 3. Same as 2, but it is somehow intended Since I think `HalfSpace` is only used for `Frustum`, and it's easier to fix documentation than code, I assumed for this PR we're in case number 1. If we're in case number 3, the documentation of `Frustum` needs to change, and in case number 2, the code needs to be fixed. While I was at it, I also : - Tried to improve the documentation for `Frustum`, `Aabb`, and `VisibilitySystems`, among others, since they're all related to `Frustum`. - Fixed documentation about frustum culling not applying to 2d objects, which is not true since bevyengine#7885 ## Remarks and questions - What about a `HalfSpace` with an infinite distance, is it allowed and does it represents the whole space? If so it should probably be mentioned. - I referenced the `update_frusta` system in `bevy_render::view::visibility` directly instead of referencing its system set, should I reference the system set instead? It's a bit annoying since it's in 3 sets. - `visibility_propagate` is not public for some reason, I think it probably should be, but for now I only documented its system set, should I make it public? I don't think that would count as a breaking change? - Why is `Aabb` inserted by a system, with `NoFrustumCulling` as an opt-out, instead of having it inserted by default in `PbrBundle` for example and then the system calculating it when it's added? Is it because there is still no way to have an optional component inside a bundle? --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Add Aabb calculation for Sprite, TextureAtlasSprite and Mesh2d

d0204fd

james7132 added this to the 0.11 milestone Mar 3, 2023

james7132 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Mar 3, 2023

Merge branch 'bevyengine:main' into 2d-frustum-culling

4d5b762

opstic added 4 commits March 19, 2023 19:50

Merge branch 'main' into 2d-frustum-culling

3a70da9

Fix compile

70de2c9

Make center scale with size

caba635

Run cargo fmt

ef384f0

Elabajaba approved these changes Apr 17, 2023

View reviewed changes

irate-devil reviewed Apr 21, 2023

View reviewed changes

opstic and others added 2 commits April 21, 2023 09:55

Apply suggestions from @devil-ira

dd12281

Co-authored-by: ira <JustTheCoolDude@gmail.com>

Fix compile

2d50da9

irate-devil approved these changes Apr 21, 2023

View reviewed changes

Weasy666 mentioned this pull request Apr 21, 2023

2d frustum culling #3944

Closed

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 21, 2023

Add the proper system

d428491

alice-i-cecile added this pull request to the merge queue Apr 24, 2023

Merged via the queue into bevyengine:main with commit 9a3225d Apr 24, 2023

opstic deleted the 2d-frustum-culling branch April 25, 2023 00:12

nicopap mentioned this pull request Apr 25, 2023

Document frustum culling in 2d examples #8490

Closed

hate mentioned this pull request May 9, 2023

Re-introduce comments about frustum culling #8579

Merged

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

Selene-Amanita mentioned this pull request Jul 12, 2023

Sprites not being rendered with translation.z < 0.0 #9138

Closed

Selene-Amanita added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 12, 2023

Selene-Amanita mentioned this pull request Jul 13, 2023

Improve documentation relating to Frustum and HalfSpace #9136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d` #7885

Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d` #7885

opstic commented Mar 3, 2023 •

edited

Loading

james7132 commented Mar 16, 2023

Elabajaba commented Apr 17, 2023 •

edited

Loading

irate-devil left a comment

Weasy666 commented Apr 21, 2023 •

edited

Loading

opstic commented Apr 21, 2023 •

edited

Loading

github-actions bot commented Jul 12, 2023

Add Aabb calculation for Sprite, TextureAtlasSprite and Mesh2d #7885

Add Aabb calculation for Sprite, TextureAtlasSprite and Mesh2d #7885

Conversation

opstic commented Mar 3, 2023 • edited Loading

Objective

Solution

Note

james7132 commented Mar 16, 2023

Elabajaba commented Apr 17, 2023 • edited Loading

irate-devil left a comment

Choose a reason for hiding this comment

Weasy666 commented Apr 21, 2023 • edited Loading

opstic commented Apr 21, 2023 • edited Loading

github-actions bot commented Jul 12, 2023

Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d` #7885

Add `Aabb` calculation for `Sprite`, `TextureAtlasSprite` and `Mesh2d` #7885

opstic commented Mar 3, 2023 •

edited

Loading

Elabajaba commented Apr 17, 2023 •

edited

Loading

Weasy666 commented Apr 21, 2023 •

edited

Loading

opstic commented Apr 21, 2023 •

edited

Loading