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

Optional batch size for bevy_render #5025

Closed

Conversation

ManevilleF
Copy link
Contributor

@ManevilleF ManevilleF commented Jun 16, 2022

Objective

Following #4489 by @aevyrie I think we can improve the batching:

  • Making it optional* but enabled by default
  • Making the batch size customizable
  • Propagating the batching to other bevy_render systems

Solution

I'm opening this Draft showing how I think we can apply the target improvements:

  • The batch size can be enabled or disabled directly through the RenderPlugin where I added an optional field, set by default to 1024 like in [Merged by Bors] - Parallel Frustum Culling #4489 .
  • The systems retrieve the batch size through an optional resource, meaning we can add, edit or remove this resource at any time, making it customizable and dynamic.

Notes

  • The code is not very pretty but I think there are ways to improve it or maybe make the whole conditional parallelization generic and therefore easily appliable to other systems in bevy_render or maybe other crates.
  • I didn't test or benchmark the new code yet, since I don't think this MR should be merged before the design is discussed

@ManevilleF ManevilleF marked this pull request as ready for review June 16, 2022 09:41
@ManevilleF
Copy link
Contributor Author

An other design choice could be that RenderBatchSize is always present and set through a App::init_resource() but storing an Option<usize> for the batch_size.
this would mean that it could be retrieved through Res<> instead of Option<Res<>> and maybe add clarity

@james7132
Copy link
Member

Wouldn't #4777 largely remove the need for this?

@ManevilleF
Copy link
Contributor Author

ManevilleF commented Jun 16, 2022

Wouldn't #4777 largely remove the need for this?

Absolutely, I didn't know it existed but if the batch size can be dynamically estimated I don't think there is need for a config resource. Or maybe the resource can have like a Auto value, using the estimate, and an Override value allowing more customization

Edit: Maybe I should base this PR off yours
Edit 2: Maybe the batch size estimation could be overriden using a global resource, defining the global paralelization behaviour ?

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 20, 2022
@ManevilleF
Copy link
Contributor Author

#4777 and #4663 make this obsolete

@ManevilleF ManevilleF closed this Jun 22, 2022
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-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants