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

Index buffer specialization #568

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Index buffer specialization #568

merged 3 commits into from
Sep 25, 2020

Conversation

fu5ha
Copy link
Contributor

@fu5ha fu5ha commented Sep 25, 2020

Allows the use of U32 indices in Mesh index buffers in addition to the usual U16 indices.

Note: I use U32 indices for Icosphere here both as a test but also because there could be a reasonable case where you surpass the U16 space with a highly tesselated icosphere.

@memoryruins memoryruins added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Sep 25, 2020
@cart
Copy link
Member

cart commented Sep 25, 2020

Awesome this looks good to me.

At some point (not in this pr) I think we should make a decision on "u16 vs u32" as the default (and where that default is applied).

Ex: if half of the meshes with a given material use u16 and the other half use u32, will the memory reduction savings outweigh the state change cost (because specialization results in different pipelines for u16 vs u32). would it be cheaper to just use u32 everywhere to allow increased batching/instancing? Should all shapes use u32 for that reason?

@cart cart merged commit f7c8882 into bevyengine:master Sep 25, 2020
@cart
Copy link
Member

cart commented Sep 25, 2020

(i dont expect you to come up with all of the answers. im just thinking out loud)

@fu5ha
Copy link
Contributor Author

fu5ha commented Sep 25, 2020

Yeah so I've been pondering that stuff as well. I can ask around in a games-tech slack I'm in what people do in production these days -- I'm not sure how much benefit there is to using u16 for indices. Seems to me we should definitely make everything in the engine use one type by default to limit state changes between pipelines for sure, and at this point I'm leaning towards u16 standard for all the standard stuff provided by the engine and just having u32 there supported if people want to do things with it.

@cart
Copy link
Member

cart commented Sep 25, 2020

That would be much appreciated! Im thinking if theres no significant difference, then we might as well use u32 as the default so things "just work" in basically every case. My main issue with u16 as the default is that when it breaks down, it can be silently / subtly broken. And there will be a "class of thing" where u16 can't (always) be the default (like icospheres). I'd want some protections against the "overflow" case to prevent silent breakage like we saw on discord.

@fu5ha fu5ha deleted the index_buffer_specialization branch September 25, 2020 19:42
@fu5ha
Copy link
Contributor Author

fu5ha commented Sep 25, 2020

Alright so basically what I've gathered is that it's possible you could have a small perf impact in situations where you're not bound by pixel shader compute -- i.e. shadow map passes and other simple shaders. But even in that case it would be single digit percentage improvement. It can be helpful for memory savings as well especially on mobile platforms or places where you're very memory starved, but I don't think we're really going to be experiencing that at the moment. So, I'd propose to move forward with U32 as default for the time being.

@cart
Copy link
Member

cart commented Sep 25, 2020

Cool that makes sense to me. I like that as its fool-proof in general and we can optimize later if we need to.

@julhe
Copy link
Contributor

julhe commented Sep 25, 2020

I agree with the u32 as default part, because after all functionally > performance.

But it still should be easy to switch back to u16, since asking arround on twitter with this issue revealed that there are indeed performance implications on mobile and u16 still is sorta standard. (see: https://twitter.com/IRCSS/status/1309453536045027328) And just because its very cheap doesnt mean its free, and I'm kinda worried about taking a lot of small perf hits stacking up to "hm that code just slow".

@fu5ha
Copy link
Contributor Author

fu5ha commented Sep 25, 2020

Agreed @julhe ... it should be fairly easy, at this point changing between the two is a matter of just a few lines of code from saying U16 -> U32 basically, as seen in the PR I just opened/mentioned :D

mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants