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

extensions/khr: Implement additional Swapchain functions since Vulkan 1.1 #629

Merged
merged 2 commits into from
Jun 5, 2022

Conversation

MarijnS95
Copy link
Collaborator

These are also made available by VK_KHR_device_group when VK_KHR_swapchain (and for certain functions only the underlying VK_KHR_surface extension) is enabled.

@MarijnS95 MarijnS95 mentioned this pull request Jun 4, 2022
55 tasks
@MarijnS95 MarijnS95 force-pushed the khr-swapchain-vulkan1.1 branch 3 times, most recently from 260ef50 to 2a81efc Compare June 4, 2022 10:29
@MarijnS95 MarijnS95 mentioned this pull request Jun 4, 2022
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

I feel like conditionally-available function pointers are undermining the value of having separate extension structs. More weight in favor of globbing everything together?

@MarijnS95
Copy link
Collaborator Author

@Ralith You want to:

  1. move these functions specifically to a separate extension struct (but that's not how the generator loads them)?
  2. Combine all the high-level wrappers?
  3. And/or combine all the loaders created by the generator?

MarijnS95 added 2 commits June 5, 2022 12:34
…an 1.1

These are also made available by `VK_KHR_device_group` when
`VK_KHR_swapchain` (and for certain functions only the underlying
`VK_KHR_surface` extension) is enabled.
@Ralith
Copy link
Collaborator

Ralith commented Jun 5, 2022

Out of scope of this particular PR, but broadly:

move these functions specifically to a separate extension struct?

I'm not sure this would be maintainable long term, especially in the absence of clear computer-readable grouping of conditionally available functions for dependent cases like this.

Combine all the high-level wrappers? And/or combine all the loaders created by the generator?

Right, for a long time I've been going back and forth on whether a single monolithic Device function table with every single function on it would be more usable. The big appeal to preserving separate tables is, IMO, promoting correctness-by-construction of code that optionally employs certain extensions. If we cannot consistently guarantee that functions provided in the function table of an enabled extension actually exist, then this is weakened.

@MarijnS95
Copy link
Collaborator Author

Out of scope of this particular PR, but broadly:

move these functions specifically to a separate extension struct?

I'm not sure this would be maintainable long term, especially in the absence of clear computer-readable grouping of conditionally available functions for dependent cases like this.

There is, apparently. Taking vkGetDeviceGroupPresentCapabilitiesKHR for example:

Inside <extension name="VK_KHR_swapchain" ...> there is:

<require feature="VK_VERSION_1_1">
    ...
    <command name="vkGetDeviceGroupPresentCapabilitiesKHR"/>
    ...
</require>

Clearly stating the 1.1 requirement. Same for access through <extension name="VK_KHR_device_group" ...>:

<require extension="VK_KHR_surface">
    <enum offset="7" extends="VkStructureType"                      name="VK_STRUCTURE_TYPE_DEVICE_GROUP_PRESENT_CAPABILITIES_KHR"/>
    ...
    <command name="vkGetDeviceGroupPresentCapabilitiesKHR"/>
    ...
</require>

We could possibly support this by smacking the two names together for a separate loader struct, stating that these functions are only available in the combined case? Seems like a thing for a future API break if we feel strongly about it.

The big appeal to preserving separate tables is, IMO, promoting correctness-by-construction of code that optionally employs certain extensions. If we cannot consistently guarantee that functions provided in the function table of an enabled extension actually exist, then this is weakened.

And I have enjoyed that feature quite often in my codebases, cleanly wrapping optional extension structures inside Option and simply not having access to them. You are correct that this implementation muddies the waters quite a bit here, but not sure if we can/should address it right now or leave this extension helper unimplemented.

@Ralith
Copy link
Collaborator

Ralith commented Jun 5, 2022

I think it's fine to move forwards with this PR for now; it's just food for thought about future directions.

@Ralith
Copy link
Collaborator

Ralith commented Jun 5, 2022

There is, apparently.

Also, nice! I have mixed feelings about proliferating fine-grained tables--we'll have to see how big the impact would be--but it's good that we have options to explore.

@MarijnS95
Copy link
Collaborator Author

Also, nice! I have mixed feelings about proliferating fine-grained tables--we'll have to see how big the impact would be--but it's good that we have options to explore.

Exactly, I haven't really thought out how to best represent these. A struct including both names seems like the only obvious and clear way. Let's create a followup issue to discuss and track this.

@MarijnS95 MarijnS95 merged commit 1459da4 into master Jun 5, 2022
@MarijnS95 MarijnS95 deleted the khr-swapchain-vulkan1.1 branch June 5, 2022 19:47
@MarijnS95
Copy link
Collaborator Author

Gobbled a short write-up together in #635, hope it's clear as I'm beyond tired 😩

MarijnS95 added a commit that referenced this pull request Jun 5, 2022
…an 1.1 (#629)

* extensions/khr: Reorder `Swapchain` functions to match `KhrSwapchainFn`

* extensions/khr: Implement additional `Swapchain` functions since Vulkan 1.1

These are also made available by `VK_KHR_device_group` when
`VK_KHR_swapchain` (and for certain functions only the underlying
`VK_KHR_surface` extension) is enabled.
@Ralith
Copy link
Collaborator

Ralith commented Jun 5, 2022

Looks good, thanks!

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