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

[0.5] Add Texture Descriptor Array Feature #3269

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Jun 9, 2020

This adds Feature::TEXTURE_DESCRIPTOR_ARRAY which is unconditionally present on DX12, DX11, and Vulkan, > MTL 2.0 on Metal, and unavailable on OpenGL. Strictly speaking that feature might be supported on opengl, but I'd want to make sure the backend supported it first.

A slightly concerning discovery is that Limits can't be extended without a breaking change, so descriptor indexing might have some issues. Thankfully this feature doesn't change the available limits.

TODO:

  • PR against master

@cwfitzgerald cwfitzgerald changed the title Add Texture Descriptor Array Feature [0.5] Add Texture Descriptor Array Feature Jun 9, 2020
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.

If you need these functions published, please bump the patch versions of the backends.
Also, please add a short note to CHANGELONG.

I'm surprised that this is supported on DX11, tbh. Do you know how this is resolved?

@cwfitzgerald
Copy link
Member Author

I'm surprised that this is supported on DX11, tbh. Do you know how this is resolved?

Spriv-cross happily spits out

Texture2D<float4> u_Textures[2] : register(t0);
SamplerState u_Sampler : register(s1);

for

layout(set = 0, binding = 0) uniform texture2D u_Textures[2];
layout(set = 0, binding = 1) uniform sampler u_Sampler;

all the way down to shader model 4.0. Tbh it's not really that surprising, as it has to be compile time indexed, so it's no different for hardware than having two textures in a row.

@kvark
Copy link
Member

kvark commented Jun 10, 2020

I don't think we should be exposing this feature in backends that don't yet support it. I.e. DX11 may be calling spirv_cross correctly, but the binding side will likely break before it gets to the shader. Are you testing this on anything?

@cwfitzgerald
Copy link
Member Author

cwfitzgerald commented Jun 10, 2020

I just finished my example, and it doesn't seem to work on DX11, though without errors, so I have no idea if that's an issue with the DX11 backend, or what. I'll disable it on DX11 as I have no personal need for that backend.

@kvark
Copy link
Member

kvark commented Jun 10, 2020

Thank you!

please bump the patch versions of the backends, and add a short note to CHANGELONG

bors bot added a commit that referenced this pull request Jun 10, 2020
3270: Port #3269 to Master r=kvark a=cwfitzgerald

This forward ports #3269 to master.

Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
@cwfitzgerald
Copy link
Member Author

Should be good to go

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.

Nice, thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented Jun 10, 2020

Build succeeded:

@bors bors bot merged commit ba29414 into gfx-rs:hal-0.5 Jun 10, 2020
bors bot added a commit to gfx-rs/wgpu that referenced this pull request Jun 11, 2020
711: Implement Descriptor Array Extension r=kvark a=cwfitzgerald

## 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_ARRAY` native 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:
 - Types that were able to be shared among wgt, wgc, and wgpu-rs were moved into wgt.
 - Types that had references to other wgpu-rs specific structures were duplicated into wgc with wgpu-rs structures replaced with the appropriate ID. Notes were added to the wgc types that they were duplicated directly from wgpu-rs.

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.


Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
@cwfitzgerald cwfitzgerald deleted the texture-descriptor-array branch June 11, 2020 17:51
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.

2 participants