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

Change default vertex layout to seperate buffers #592

Closed
julhe opened this issue Sep 27, 2020 · 9 comments
Closed

Change default vertex layout to seperate buffers #592

julhe opened this issue Sep 27, 2020 · 9 comments
Labels
A-Rendering Drawing game state to the screen C-Enhancement A new feature

Comments

@julhe
Copy link
Contributor

julhe commented Sep 27, 2020

Right now the vertex layout is interleave, meaning data is stored like (VVVNNNCCC). It would make sense to use seperate buffers, like in (VVV) (NNN) (CCC).

Pro:

  • Easier adding of new attributes / more modular approach
  • More controlled binding, like only bind position buffer for shadow rendering
  • Faster modification of certain buffers, like changing the vertex color on the CPU without uploading the entire mesh again

Cons:

@karroffel karroffel added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Sep 27, 2020
@fu5ha
Copy link
Contributor

fu5ha commented Sep 27, 2020

I think this is a good idea. When we were looking into this for Amethyst, benchmarks basically said performance was a wash or slightly better for the separate version, and that's without taking into account stuff like shadow map where you're actually using less of the data. The flexibility gains on top of that makes it very worth it.

@julhe
Copy link
Contributor Author

julhe commented Sep 28, 2020

I'm not sure if we should keep the possiblity to have multiple attributes per buffer, as it just takes up code and is likely to be never used (?). Also the buffers are set up multiple times per frame now. I will fix that once I got more feedback from you people. 🙂

@fu5ha
Copy link
Contributor

fu5ha commented Sep 28, 2020

I don't think it's particularly useful to have multiple attributes per buffer tbh

@nickbryan
Copy link

Hi, I am curious if this solves/affects the issue #155 and if so would it be possible to get an example added as part of the PR that demonstrates how to use separate buffers to add colour data to a mesh or similar?

Forgive me if I am way off on the intentions for this one.

@fu5ha
Copy link
Contributor

fu5ha commented Sep 29, 2020

@nickbryan Yes this would affect #155... I think the scope of this PR isn't necessarily to fully solve that but it will make solving it much easier for sure. Once this lands, #155 and an associated example should be fairly easy.

@nickbryan
Copy link

That's awesome news! Thanks for the information.

@julhe
Copy link
Contributor Author

julhe commented Oct 1, 2020

Okay, I have some additional Info:

Prefere Seperate:

Prefere Interleaved:

Would say we still make this PR and then at some point provide functions that converts seperate buffers into interleaved ones for mobile if the user/platform demands.

@aloucks
Copy link
Contributor

aloucks commented Oct 7, 2020

Something also to consider is that WGPU has limits to the max number of vertex buffers that can be bound. Interleaving is one way to work around that limitation. Ideally the vertex format would be fully customizable as there are use cases for both separate and interleaved.

See also:
gpuweb/gpuweb#693
gfx-rs/wgpu#558

cart pushed a commit that referenced this issue Oct 31, 2020
Mesh overhaul with custom vertex attributes
@cart
Copy link
Member

cart commented Oct 31, 2020

I think I'm going to call this resolved by #592. Midway through the implementation it changed from "separate buffers" back to "interleaved" (but still bound as separate buffers). But it lays solid foundations for both approaches. I think we should open new issues for specific changes to the new approach.

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

No branches or pull requests

6 participants