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

Use vulkan descriptor arrays instead of flattening them #319

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

apazylbe
Copy link
Collaborator

In order to fully support resource arrays, also require and enable shader*ArrayDynamicIndexing device features.
This allows one to use push constants to select texture within an array of textures like in 24_push_constants

Does not add support for using push descriptors with arrays.

Dynamic indexing features are not required for just using shader resource arrays with constant indices.
Currently, from what I can tell, only 24_push_constants requires enabling shaderSampledImageArrayDynamicIndexing and there are some small number of gpus that don't support those features (software lavapipe).

We could handle this in a few different ways:

  1. Enable dynamic indexing features across the board, fail on the devices that don't support them. This would mean that BigWheels won't run on some devices (lavapipe)
  2. Let applications make sure the used features are supported, and provide a way for applications to enable those device features.
  3. Only flatten resource arrays on devices that support dynamic indexing features. This would require compiling/loading shaders conditionally, and might get messy.

@apazylbe apazylbe marked this pull request as draft October 12, 2023 19:53
@apazylbe
Copy link
Collaborator Author

Ah, looks like the features aren't supported for our github CI. Maybe more of an argument for option #2?

@chaoticbob
Copy link
Contributor

Ah, looks like the features aren't supported for our github CI. Maybe more of an argument for option #2?

It might be a somewhat painful transition, but we should just assume those are supported. The argument for this is that it better aligns with D3D12 - in which these features are implicit. For the devices we'll care about those options will be present.

Copy link
Contributor

@chaoticbob chaoticbob left a comment

Choose a reason for hiding this comment

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

LGTM so far

@chaoticbob
Copy link
Contributor

Tested this against my local branch and it's working as expected for Vulkan.

@apazylbe
Copy link
Collaborator Author

the CI is now good after upgrading to the new mesa vulkan drivers. I will finish testing this on a few devices/projects and then set to ready to review

@apazylbe apazylbe force-pushed the remove_flatten branch 3 times, most recently from 9f48da8 to e82d63a Compare October 26, 2023 18:05
@apazylbe apazylbe marked this pull request as ready for review October 26, 2023 19:26
@apazylbe apazylbe requested a review from Keenuts October 26, 2023 19:26
@apazylbe apazylbe force-pushed the remove_flatten branch 2 times, most recently from 6c95b2b to b9e24b9 Compare November 17, 2023 19:44
In order to fully support resource arrays, also require and enable
shader*ArrayDynamicIndexing device features.
This allows one to use push constants to select texture within an
array of textures like in 24_push_constants

Does not add support for using push descriptors with arrays
@apazylbe apazylbe merged commit 899e05d into google:main Nov 22, 2023
5 checks passed
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.

3 participants