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

Implement MultiDrawIndirect Extensions #754

Merged
merged 1 commit into from
Jun 28, 2020

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Jun 27, 2020

Connections

Closes #742.

Description

These extensions, especially when combined with binding indexing, allow the creation of extremely cpu-efficient gpu powered pipelines.

Adds two extensions allowing various types of multi-draw-indirect

  • MULTI_DRAW_INDIRECT (giving multi_draw_indirect and multi_draw_indexed_indirect)
  • MULTI_DRAW_INDIRECT_COUNT (giving multi_draw_indirect_count and multi_draw_indexed_indirect_count)

This adds what I believe to be an extra restriction on the *count family of functions when compared to the underlying api. The buffer must be large enough to draw max_count draws, even if that many draws are never drawn. This makes these operations no more unsafe than indirect would be, which is currently safe.

I did not implement these for renderbundles, but there's no reason they couldn't work, so those branches are marked with unimplemented as opposed to unreachable.

Additional Changes:

  • Added some validation to the normal draw_*_indirect functions to prevent buffer overruns.
  • The DX12 gfx-hal backend requires the strides to always be non-zero, so I modified the normal indirect draws to use explicit strides.
  • Made device limits and features pub(crate) as they need to be checked in random places in the code.

Testing

The change was tested using a modified version of wgpu-rs's texture-array example using a variety of permutations. I have not been able to test regular MULTI_DRAW_INDIRECT on mac, but I see no reason why that wouldn't work.

gfx-rs/wgpu-rs#414

@cwfitzgerald cwfitzgerald requested a review from kvark June 27, 2020 20:09
Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Looks great! I have a couple of suggestions, and one question before we proceed.

@@ -131,6 +131,30 @@ pub enum RenderCommand {
buffer_id: id::BufferId,
Copy link
Member

Choose a reason for hiding this comment

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

looks like we need to have a parameter to switch between indexed and non-indexed for indirect calls (only), since now we have 3 pairs of variants with exactly same contents, pair-wise

Copy link
Member Author

Choose a reason for hiding this comment

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

So at the end of both conversions, we'll just have a DrawIndirect, MultiDrawIndirectCount which covers all 6 indirect cases?

Copy link
Member

Choose a reason for hiding this comment

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

yep, that sounds good

MultiDrawIndirect {
buffer_id: id::BufferId,
offset: BufferAddress,
count: u32,
Copy link
Member

Choose a reason for hiding this comment

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

We should also probably merge this with DrawIndirect by having count: Option<NonZeroU32>

wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
bors bot added a commit to gfx-rs/gfx that referenced this pull request Jun 28, 2020
3282: Allow DX12 Backend to work with strides of 0 r=kvark a=cwfitzgerald

So this fixes the problem gfx-rs/wgpu#754 was working around, but found a much more interesting problem:

- DX12 _must_ have a stride of 16/20, the value of the stride is ignored other than the assert. This isn't a problem from the wgpu perspective (we assume tightly packed), but would have issues for gfx-portability.

Co-authored-by: Connor Fitzgerald <[email protected]>
@cwfitzgerald
Copy link
Member Author

Alrighty, I think this is all sorted now.

@@ -195,8 +195,8 @@ pub struct Device<B: hal::Backend> {
temp_suspected: life::SuspectedResources,
pub(crate) hal_limits: hal::Limits,
pub(crate) private_features: PrivateFeatures,
limits: wgt::Limits,
features: wgt::Features,
pub(crate) limits: wgt::Limits,
Copy link
Member

Choose a reason for hiding this comment

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

are these necessary?

Copy link
Member

Choose a reason for hiding this comment

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

this is the last concern, everything else looks gorgeous. Thank you for addressing everything!

@@ -195,8 +195,8 @@ pub struct Device<B: hal::Backend> {
temp_suspected: life::SuspectedResources,
pub(crate) hal_limits: hal::Limits,
pub(crate) private_features: PrivateFeatures,
limits: wgt::Limits,
features: wgt::Features,
pub(crate) limits: wgt::Limits,
Copy link
Member

Choose a reason for hiding this comment

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

this is the last concern, everything else looks gorgeous. Thank you for addressing everything!

@cwfitzgerald
Copy link
Member Author

Thanks for the reviews!
bors r=kvark

@bors
Copy link
Contributor

bors bot commented Jun 28, 2020

@bors bors bot merged commit 43c67ac into gfx-rs:master Jun 28, 2020
bors bot added a commit to gfx-rs/wgpu-rs that referenced this pull request Jun 28, 2020
414: Implement MultiDrawIndirect Extensions r=kvark a=cwfitzgerald

The wgpu-rs component of gfx-rs/wgpu#754.

I didn't include the modifications to the example as they are particularly awful and ugly. One of my next steps following this is to make an example of both binding indexing and these multi-draw extensions.

Co-authored-by: Connor Fitzgerald <[email protected]>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
414: Implement MultiDrawIndirect Extensions r=kvark a=cwfitzgerald

The wgpu-rs component of gfx-rs#754.

I didn't include the modifications to the example as they are particularly awful and ugly. One of my next steps following this is to make an example of both binding indexing and these multi-draw extensions.

Co-authored-by: Connor Fitzgerald <[email protected]>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
754: Update naga to gfx-10 r=kvark a=kvark

See gfx-rs#1205

Co-authored-by: Dzmitry Malyshau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

draw_indirect_*_count Extension
2 participants