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

The maximum vertex buffer input bindings is set too low #558

Closed
aloucks opened this issue Apr 7, 2020 · 3 comments · Fixed by #562
Closed

The maximum vertex buffer input bindings is set too low #558

aloucks opened this issue Apr 7, 2020 · 3 comments · Fixed by #562
Labels
area: api Issues related to API surface type: enhancement New feature or request

Comments

@aloucks
Copy link
Contributor

aloucks commented Apr 7, 2020

pub const MAX_VERTEX_BUFFERS: usize = 8;

A super conservative value would be 16, but modern hardware supports 32 vertex input bindings.

https://vulkan.gpuinfo.org/displaydevicelimit.php?name=maxVertexInputBindings

The error I ran into manifested as a slice index out of bounds. I think this should really be a SmallVec or something similar that can handle more than the bare minimum.

for vbs in state.vertex.inputs[pipeline.vertex_strides.len()..].iter_mut() {

@kvark
Copy link
Member

kvark commented Apr 7, 2020

Thank you for filing!

A super conservative value would be 16, but modern hardware supports 32 vertex input bindings.

What use case do you have to need more than 4? :)

You are right that we need to support at least 16, according to Vulkan capabilities and our code in gfx-rs for other backends. Although, it's a bit more tricky than it seems on Metal: the vertex bindings are shared between descriptor resources and vertex attributes. And it can have up to 31 vertex buffer in total... So there must be a balance between how many vertex buffers we allow coming for the attributes, versus ones coming in a bind group. And I'd much prefer to have the higher limit for the latter, since I don't find multiple vertex buffers particularly useful.

The error I ran into manifested as a slice index out of bounds. I think this should really be a SmallVec or something similar that can handle more than the bare minimum.

Yes, I think SmallVec is a proper primitive in general for anything limit-related, since we know the baseline (minimum requirements), we can specify it as a fixed portion. In the near future, the limit on the vertex buffer count will be exposed in Limits, and you'll be able to request more than the standard amount.

@kvark kvark added area: api Issues related to API surface type: enhancement New feature or request labels Apr 7, 2020
@aloucks
Copy link
Contributor Author

aloucks commented Apr 7, 2020

What use case do you have to need more than 4? :)

Loading glTF files using their buffers as-is without converting the vertex input.

The attributes aren't necessarily interleaved and the offset into the view sometimes exceeds the stride. It makes it difficult or impossible to put the attributes on a single buffer binding and you either need to (1) create an input binding per attribute or (2) pre-process the input into a custom/known vertex format. I'm trying to solve for (1) because (2) prevents you from loading the data directly.

Although most assets aren't going to have all of these attributes, once you run into models with animation, you can go past the limit of 8 pretty easily (which is what happened to me).

  1. Position
  2. Normal
  3. Tangent
  4. UV1
  5. UV2
  6. Color
  7. Joint1
  8. Weight1
  9. Joint2
  10. Weight2
  11. MorphTarget0_Position
  12. MorphTarget0_Normal
  13. MorphTarget0_Tangent
  14. MorphTarget1_Position
  15. MorphTarget1_Normal
  16. MorphTarget1_Tangent
  17. MorphTarget2_Position **
  18. MorphTarget2_Normal
  19. MorphTarget2_Tangent
  20. MorphTarget3_Position
  21. MorphTarget3_Normal
  22. MorphTarget3_Tangent

** I have a flag in the shader that disables morph targets 2 and 3 during compilation if only 16 input bindings are supported. This is why I opened gfx-rs/wgpu-rs#239.

@kvark
Copy link
Member

kvark commented Apr 7, 2020

I want to say that glTF is just not doing its best here - KhronosGroup/glTF#21
But I agree - that's the reality we have to deal with. In any case, even 16 isn't enough here, so we'd need to let you opt into more than standard count.

aloucks added a commit to aloucks/wgpu that referenced this issue Apr 8, 2020
Additionally, the input states are now stored in a `SmallVec` to
enable higher limits.

Fixes gfx-rs#558
aloucks added a commit to aloucks/wgpu that referenced this issue Apr 8, 2020
Additionally, the input states are now stored in a `SmallVec` to
enable higher limits.

Fixes gfx-rs#558
bors bot added a commit that referenced this issue Apr 8, 2020
562: Increase default maximum vertex buffer inputs from 8 to 16 r=kvark a=aloucks

Additionally, the input states are now stored in a `SmallVec` to enable higher limits.

Fixes #558

Co-authored-by: Aaron Loucks <aloucks@cofront.net>
@bors bors bot closed this as completed in b6ec844 Apr 8, 2020
@bors bors bot closed this as completed in #562 Apr 8, 2020
Wumpf pushed a commit to Wumpf/wgpu that referenced this issue Apr 11, 2020
Additionally, the input states are now stored in a `SmallVec` to
enable higher limits.

Fixes gfx-rs#558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants