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

[Merged by Bors] - Frustum Culling (for Sprites) #1492

Closed
wants to merge 6 commits into from

Conversation

Byteron
Copy link
Contributor

@Byteron Byteron commented Feb 21, 2021

This PR adds two systems to the sprite module that culls Sprites and AtlasSprites that are not within the camera's view.
This is achieved by removing / adding a new Viewable Component dynamically.

Some of the render queries now use a With<Viewable> filter to only process the sprites that are actually on screen, which improves performance drastically for scene swith a large amount of sprites off-screen.

https://streamable.com/vvzh2u

This scene shows a map with a 320x320 tiles, with a grid size of 64p.
This is exactly 102400 Sprites in the entire scene.

Without this PR, this scene runs with 1 to 4 FPS.

With this PR..
.. at 720p, there are around 600 visible sprites and runs at ~215 FPS
.. at 1440p there are around 2000 visible sprites and runs at ~135 FPS

The Systems this PR adds take around 1.2ms (with 100K+ sprites in the scene)

Note:
This is only implemented for Sprites and AtlasTextureSprites.
There is no culling for 3D in this PR.

@Byteron Byteron marked this pull request as draft February 21, 2021 14:08
@Ratysz Ratysz added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Feb 21, 2021
@TheRawMeatball TheRawMeatball added the C-Performance A change motivated by improving speed, memory usage or compile times label Feb 21, 2021
@Byteron Byteron force-pushed the frustum_culling branch 3 times, most recently from b539af9 to e588275 Compare February 21, 2021 19:53
@Byteron Byteron marked this pull request as ready for review February 21, 2021 20:25
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

It would be nice to have a benchmark to spot regressions and toss in the patch notes, but I think this PR is good to go and would be a really nice easy win for 0.5.

crates/bevy_render/src/draw.rs Outdated Show resolved Hide resolved
@@ -48,6 +49,14 @@ impl Plugin for SpritePlugin {
.add_asset::<TextureAtlas>()
.register_type::<Sprite>()
.add_system_to_stage(CoreStage::PostUpdate, sprite_system.system())
.add_system_to_stage(
Copy link
Member

Choose a reason for hiding this comment

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

These are probably going to need an ordering dependency on hierarchy propagation systems, but maybe that's better left out of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they also need to be ordered with the visible_entities system in camera I think.
IIRC they are both called on Update and that might lead to sprites popping up on the camera's borders a frame later. It's barely noticable but it is noticable.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately ordering won't be enough because the component insert commands won't be applied until after the PostUpdate stage finishes. There are a number of systems in PostUpdate that filter on that component's existence.

I think we should add the relevant dependencies to be as correct as possible (and to ensure transform updates are applied), but we'll still have a one frame lag to cull / uncull sprites unless we either:

  1. Ensure all systems that rely on the frustum component live in a stage after PostUpdate. This isn't ideal.
  2. Do frustum culling later, remove With<WithinFrustum> and accept that we'll do unnecessary work for those systems (ex: we only filter out unnecessary draw calls, not the cpu work like shader management). This also isn't ideal.
  3. Move all of the steps out of the ECS (something like the "pipelined rendering" we've been discussing lately). This is a lot of work.

I'm hesitant to do (1) as it feels short sighted. Given how much cpu work is a bottleneck right now (2) doesn't feel like its "worth it".

So imo our only real options are "don't include frustum culling in 0.5 and wait for pipelined rendering" or "accept one frame of cull lag"

@alice-i-cecile 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 Feb 21, 2021
@alice-i-cecile
Copy link
Member

This will partially resolve #1333.

@cart
Copy link
Member

cart commented Feb 22, 2021

On paper I'm sold, although short lived marker components shouldn't be allowed until after we add support for SparseSet component storage (which will happen before 0.5. the draft pr should be out in the next couple of days). I'll give this a full review after I wrap that up. The only change required to adapt to that will be flagging the Visible component as "sparse" when we add the RenderPlugin.

@Byteron
Copy link
Contributor Author

Byteron commented Feb 22, 2021

^I broke something when fixing clippy things. Fixed that and squashed commits.
(Hence the force push)

@Byteron
Copy link
Contributor Author

Byteron commented Feb 22, 2021

It would be nice to have a benchmark to spot regressions and toss in the patch notes, but I think this PR is good to go and would be a really nice easy win for 0.5.

Where would I put such a bench? Into benches//bench/bevy_sprite/?

@Byteron
Copy link
Contributor Author

Byteron commented Feb 22, 2021

It would be nice to have a benchmark to spot regressions and toss in the patch notes, but I think this PR is good to go and would be a really nice easy win for 0.5.

#1503 shall serve as bench / example project for this working.

@Byteron Byteron changed the title Frustum Culling (for Sprites) (WIP) Frustum Culling (for Sprites) Feb 22, 2021
@mockersf
Copy link
Member

mockersf commented Feb 22, 2021

With Visible and Transparent as unit struct that are parts of the bundle, how can one spawn an invisible or non-transparent SpriteBundle?

@rparrett
Copy link
Contributor

rparrett commented Feb 23, 2021

That's a great question. I also asked in discord about manually managing visibility (but in a post-spawning scenario).

https://discord.com/channels/691052431525675048/743663924229963868/813505768069398529

It sounds like Byteron's going to be looking into using a separate Culled marker struct instead, leaving the old Visible struct as-is.

@rparrett
Copy link
Contributor

Sprite rotation is also currently unaccounted for.

@alice-i-cecile alice-i-cecile removed 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 Feb 23, 2021
@aevyrie aevyrie mentioned this pull request Feb 23, 2021
@Byteron
Copy link
Contributor Author

Byteron commented Feb 23, 2021

That's a great question. I also asked in discord about manually managing visibility (but in a post-spawning scenario).

https://discord.com/channels/691052431525675048/743663924229963868/813505768069398529

It sounds like Byteron's going to be looking into using a separate Culled marker struct instead, leaving the old Visible struct as-is.

With Visible and Transparent as unit struct that are parts of the bundle, how can one spawn an invisible or non-transparent SpriteBundle?

Helloooo.
I reimplemented this with using a separate Frustum marker component.
Which means the Visible component stays untouched for this PR. There is also no need to manually add this Frustum Component yourself, that's done entirely by bevy.

Sprite rotation is also currently unaccounted for.

This is still uncounted for. I need to figure out the math first in order to do this.

@aevyrie
Copy link
Member

aevyrie commented Feb 23, 2021

Could we use something like WithinFrustum that makes it more clear what the marker does? The name Frustum doesn't make it immediately obvious whether entities with the struct are inside or outside the camera frustum.

@Byteron
Copy link
Contributor Author

Byteron commented Feb 23, 2021

Could we use something like WithinFrustum that makes it more clear what the marker does? The name Frustum doesn't make it immediately obvious whether entities with the struct are inside or outside the camera frustum.

As I understand the Frustum is the visible part. So it makes sense to me that entities with Frustum are visible, while entities without Frustum are not.
Though if you think this is not descriptive enough I can rename it for sure.

@aevyrie
Copy link
Member

aevyrie commented Feb 23, 2021

A frustum is a shape, it doesn't really make much sense when we look at other component names:

  • entities are Visible
  • entities are Transparent
  • entities are Frustum (?)

The first two make it clear what functionality the components add. Hence the suggestion for WithinFrustum. Alternatives might be InsideFrustum, InFrustum, NotFrustumCulled, etc.

@Byteron Byteron force-pushed the frustum_culling branch 2 times, most recently from 2d8692c to 549b5f6 Compare March 11, 2021 08:51
@alice-i-cecile alice-i-cecile added this to the Bevy 0.5 milestone Mar 16, 2021
@alice-i-cecile 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 Mar 16, 2021
@rparrett
Copy link
Contributor

I updated the example locally to non-uniformly scale the sprites

let scale = Vec3::new(0.2 + rng.gen::<f32>() * 4.8, 1.0, 1.0);

And I am seeing some popping-in and popping-out around the edges that I didn't notice before making that change.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Having thought about this a bit, I don't think we should use marker components here:

  1. This won't work for multi-camera situations (ex: render to texture, split screen, editors). Each camera needs its own visibility set, but the marker component effectively merges those sets together.
  2. As much as possible I'd like Bevy to treat components as "user defined state", which it derives internal state from. That feels "cleaner" to me. I'm not sure we'll always be able to do it, but so far we've been able to.

Instead I think it makes sense for each camera to store a list of entities that are within the frustum (or for the renderer to populate that on the fly). However this makes the "query filter optimizations" you added to the various system (ex: shader def and draw system) harder to do (which would in turn make the wins here smaller).

I think all of those issues go away if we adopt the "pipelined rendering" model we've been discussing in various places, where we extract world state each frame and construct draw state on the fly. That would lend itself nicely to "dynamic per camera visibilty lists"

Buuut that work hasn't even started yet and won't land until well after 0.5. The implementation in this pr (while limited) works for the common case and gives us a nice win. And it doesn't outright break multiple cameras/views (it just makes them draw stuff they don't see).

I think if we can sort out a good solution to the broken 3d (and custom) rendering, we should merge this.

crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
This allows rendering to work as expected without hacks
@cart
Copy link
Member

cart commented Mar 24, 2021

Just pushed the changes mentioned here: #1492 (comment)

This "inverts" the queries so that they search for Without<OutsideFrustum>. This allows non-sprite rendering to work by default without hacks (and notably without requiring SpritePlugin).

@cart
Copy link
Member

cart commented Mar 24, 2021

I think there might be a bug with insertion commands, which @Byteron may have already encountered given that they included checks that shouldn't be necessary, which i tried removing and it resulted in a bundle insertion panic. Re-adding the check resolved the panic, but this feels like a nasty bevy_ecs bug that needs solving before 0.5. Investigating now.

@cart
Copy link
Member

cart commented Mar 24, 2021

Found the issue. It was a bevy_ecs bug introduced in the "reliable change detection" pr. Simple fix.

Comment on lines 69 to 70
/// Viewable is used for frustum culling.
/// Any Sprite or AtlasTextureSprite will have this removed if they are outside the camera frustum and thus not be rendered.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs updating with the recent name change (and inversion of logic).

visible_query: Query<(Entity, &Visible, Option<&RenderLayers>)>,
visible_transform_query: Query<&GlobalTransform, With<Visible>>,
visible_query: Query<(Entity, &Visible, Option<&RenderLayers>), Without<OutsideFrustum>>,
visible_transform_query: Query<&GlobalTransform, Without<OutsideFrustum>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Double negatives 🤯

(nothing we can do here, but always causes me to pause for a moment when reading logic like this)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like to avoid double negatives in apis whenever possible. But yeah the tradeoff here for the inverse is too much :)

@cart
Copy link
Member

cart commented Mar 24, 2021

An issue with using OutsideFrustum instead of InsideFrustum is that it won't support multiple views/cameras correctly. The old impl "under culled" objects viewed by multiple cameras. The new impl will "over cull" objects, which is much worse. We don't support that sort of thing officially, but I think we should provide an escape hatch in case someone hits this issue in a custom render pipeline. Something like SpriteSettings { frustum_culling_enabled: false }.

@cart
Copy link
Member

cart commented Mar 24, 2021

We'll also want to switch to only culling using the "active camera" to support multiple cameras of the same "type" (ex: switching back and forth between two "2d" cameras from different perspectives).

@cart
Copy link
Member

cart commented Mar 24, 2021

(working on these changes now)

@alice-i-cecile
Copy link
Member

We don't support that sort of thing officially, but I think we should provide an escape hatch in case someone hits this issue in a custom render pipeline. Something like SpriteSettings { frustum_culling_enabled: false }.

#1389 is quite mature FWIW. I'd prefer to see a real integration here, but that can wait.

@cart
Copy link
Member

cart commented Mar 24, 2021

#1389 is quite mature FWIW. I'd prefer to see a real integration here, but that can wait.

Integration with multiple views would involve a complete rewrite / moving away from marker components (as discussed above). That definitely needs to wait.

@cart
Copy link
Member

cart commented Mar 24, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 24, 2021
This PR adds two systems to the sprite module that culls Sprites and AtlasSprites that are not within the camera's view.
This is achieved by removing / adding a new  `Viewable` Component dynamically.

Some of the render queries now use a `With<Viewable>` filter to only process the sprites that are actually on screen, which improves performance drastically for scene swith a large amount of sprites off-screen.

https://streamable.com/vvzh2u

This scene shows a map with a 320x320 tiles, with a grid size of 64p.
This is exactly 102400 Sprites in the entire scene.

Without this PR, this scene runs with 1 to 4 FPS.

With this PR..
.. at 720p, there are around 600 visible sprites and runs at ~215 FPS
.. at 1440p there are around 2000 visible sprites and runs at ~135 FPS

The Systems this PR adds take around 1.2ms (with 100K+ sprites in the scene)

Note:
This is only implemented for Sprites and AtlasTextureSprites.
There is no culling for 3D in this PR.

Co-authored-by: Carter Anderson <[email protected]>
@bors bors bot changed the title Frustum Culling (for Sprites) [Merged by Bors] - Frustum Culling (for Sprites) Mar 24, 2021
@bors bors bot closed this Mar 24, 2021
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-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.