Implement Descriptor Array Extension #711
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
kvark
requested changes
Jun 10, 2020
There was a problem hiding this comment.
This is some impressive work!
I was worried that you mixed the binding model struct refactor into this extension PR, but the code appears to be much nicer now, and I'll be happy to have this extension supported.
Below is a few concerns and questions. Sorry about the annoying "don't match on a reference" notes! :)
wgpu-core/src/binding_model.rs
Outdated
| @@ -135,32 +64,30 @@ pub struct BufferBinding { | |||
| pub size: wgt::BufferSize, | |||
| } | |||
|
|
|||
| #[repr(C)] | |||
| /// Duplicated in wgpu-rs | |||
There was a problem hiding this comment.
It's great that the doc comment is used, but it doesn't really explain what the structure is :)
Maybe lets turn it into a regular comment, or a //Note:?
wgpu-core/src/binding_model.rs
Outdated
| pub layout: BindGroupLayoutId, | ||
| pub entries: *const BindGroupEntry, | ||
| pub entries_length: usize, | ||
| pub bindings: Vec<BindGroupEntry>, |
There was a problem hiding this comment.
can this be using a slice &'a []?
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
4425bf2
to
2a27434
Compare
|
Everything should be fixed now. Still waiting on the publishing of the backends so I can fix up cargo deps and make CI pass, but other than that, we're all set. wgpu-rs pr coming very shortly |
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
2a27434
to
98a5a40
Compare
kvark
approved these changes
Jun 10, 2020
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
98a5a40
to
3105394
Compare
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
3105394
to
501548a
Compare
|
Alright, we should be all ready. |
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
501548a
to
44eb7ca
Compare
kvark
requested changes
Jun 11, 2020
kvark
approved these changes
Jun 11, 2020
cwfitzgerald
force-pushed
the
descriptor-arrays
branch
from
44eb7ca
to
57b3b72
Compare
kvark
approved these changes
Jun 11, 2020
|
Yay! |
bors bot
added a commit
to gfx-rs/wgpu-rs
that referenced
this pull request
Jun 11, 2020
360: Implement TEXTURE_BINDING_ARRAY extension r=kvark a=cwfitzgerald This extends gfx-rs/wgpu#711 into wgpu-rs. Notable changes: - Added an example showing off both this extension and the future descriptor indexing extension. - Changed the framework so there is a static function showing what extensions you need. This is provided by the trait,, which defaults to no extensions, so existing examples and new ones don't have to care about extensions. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
kvark
pushed a commit
to kvark/wgpu
that referenced
this pull request
Jun 3, 2021
360: Implement TEXTURE_BINDING_ARRAY extension r=kvark a=cwfitzgerald This extends gfx-rs#711 into wgpu-rs. Notable changes: - Added an example showing off both this extension and the future descriptor indexing extension. - Changed the framework so there is a static function showing what extensions you need. This is provided by the trait,, which defaults to no extensions, so existing examples and new ones don't have to care about extensions. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Blocked on gfx-rs/gfx#3269 and does some funky git overrides to get CI to return meaningful results what will be removed once that PR lands and is published.
Description
This PR implements the
TEXTURE_BINDING_ARRAYnative extension. This allows users to specify a uniform sized array of textures for use in shaders.As a corollary, this PR rustifies the Bind Group and Bind Group Layout interface. Two main actions were taken when doing this:
From what I can tell, this resulted in a significant reduction in code complexity when dealing with bind groups, favoring strong types over runtime assertions.
Naga validation of arrays of textures was not implemented.
Documentation was added to extensions to help users understand what underlying tech was being relied on as well as the platforms it should be expected to work on. I think this pattern should be implemented across the board for extensions as it makes them much more user friendly to use.
Testing
There is an example included in the wgpu-rs PR which was used for testing this feature. It worked on DX12, Vulkan, and Metal (MSL > 2.0), as was expected.
Additionally the other examples were run and are still verified to run.