-
Notifications
You must be signed in to change notification settings - Fork 158
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
cube: Support runtime selection of WSI platform #1029
cube: Support runtime selection of WSI platform #1029
Conversation
CI Vulkan-Tools build queued with queue ID 250359. |
CI Vulkan-Tools build # 1516 running. |
CI Vulkan-Tools build # 1516 failed. |
9c86393
to
8927612
Compare
CI Vulkan-Tools build queued with queue ID 250481. |
CI Vulkan-Tools build # 1517 running. |
CI Vulkan-Tools build # 1517 failed. |
8927612
to
615c292
Compare
CI Vulkan-Tools build queued with queue ID 250527. |
CI Vulkan-Tools build # 1518 running. |
CI Vulkan-Tools build # 1518 failed. |
615c292
to
7dcae5f
Compare
CI Vulkan-Tools build queued with queue ID 251139. |
CI Vulkan-Tools build # 1519 running. |
CI Vulkan-Tools build # 1519 failed. |
7dcae5f
to
02cb94e
Compare
CI Vulkan-Tools build queued with queue ID 251170. |
CI Vulkan-Tools build # 1520 running. |
CI Vulkan-Tools build # 1520 failed. |
02cb94e
to
b64c441
Compare
CI Vulkan-Tools build queued with queue ID 251259. |
CI Vulkan-Tools build # 1521 running. |
CI Vulkan-Tools build # 1521 failed. |
b64c441
to
1237a4c
Compare
CI Vulkan-Tools build queued with queue ID 252901. |
CI Vulkan-Tools build # 1522 running. |
CI Vulkan-Tools build # 1522 passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on x86_64 and aarch64 xlib, xcb, wayland and display, and this seems like a great improvement to me. Added a few comments for review. Thanks
1237a4c
to
d2c9a47
Compare
CI Vulkan-Tools build queued with queue ID 254274. |
CI Vulkan-Tools build # 1524 running. |
CI Vulkan-Tools build # 1524 passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
d2c9a47
to
9a4564e
Compare
CI Vulkan-Tools build queued with queue ID 279060. |
CI Vulkan-Tools build queued with queue ID 279076. |
By changing the selection of a WSI platform from a build time choice to a runtime choice, this allows the removal of vkcube-wayland as a separate binary. Use `--wsi <platform>` to choose the WSI platform that vkcube will render with. The chosen platform must be one that vkcube was compiled with support for, so directfb will be unavailable without changing the build parameters to enable it. These changes have been made to both vkcube and vkcubepp. This includes enabling VK_KHR_display where applicable, which is supported on linux and windows at the current time.
The docs lists 3.10 as a minimum required but only used 3.7 in github actions.
92e119e
to
9cb3214
Compare
CI Vulkan-Tools build queued with queue ID 279078. |
CI Vulkan-Tools build # 1547 running. |
CI Vulkan-Tools build # 1547 passed. |
This:
is an incorrect assumption. Windows does have direct to display, as pointed out in the description of #307 . From a quick skim of the changes that were committed, it appears that the changes to CMakeLists.txt preclude selecting direct display on Windows. @charles-lunarg , do you need me to file a new github issue for fixing that, or can a fix be included within the context of the existing issues? |
Was such a big assumption that I found that out during development of the PR. Unfortunately, the commit message wasn't corrected after I changed the code to reflect the reality of DISPLAY being available on windows. Instead I will double check that the option is present on windows, and if not, rectify that with a subsequent PR. |
I just ran vkcube locally with the changes incorporated, both win32 and display were available as runtime options. The display option didn't work because I was running inside a VM with lavapipe as the sole driver, so if changes are needed to make display work on windows, let me know. Otherwise I don't think there is any reasonable way to fix the git commit message without force pushing to main. That occurred because the initial version of the PR did preclude windows, but when it was addressed the commit message wasn't altered. |
By changing the selection of a WSI platform from a build time choice to a runtime choice, this allows the removal of vkcube-wayland as a separate binary.
Use
--wsi <xlib|xcb|wayland|display|directfb>
to choose the WSI platform that vkcube will render with. The chosen platform must be one that vkcube was compiled with support for, so directfb will be unavailable without changing the build parameters to enable it.These changes have been made to both vkcube and vkcubepp.
Platforms which do not have multiple WSI platforms, such as windows, android, and apple (metal is the only non-deprecated platform) do not have this option as there aren't any platforms to choose from.