-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
[skia] Fix vulkan feature #32740
[skia] Fix vulkan feature #32740
Conversation
Use vendored copies of headers and tools, as already done by feature 'dawn'. Guard vulkan inclusion in vulkanmemoryallocator as would be done in vendored copy.
CC @jimwang118 |
I have an idea how to devendor vulkan-headers. |
vcpkg_replace_string("${SOURCE_PATH}/third_party/externals/vulkanmemoryallocator/include/vk_mem_alloc.h" | ||
"#include <vulkan/vulkan.h>" | ||
"#ifndef VULKAN_H_\n #include <vulkan/vulkan.h>\n#endif" | ||
) |
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.
Check third_party\vulkanmemoryallocator\BUILD.gn
, the #include <vulkan/vulkan.h>
used in vk_mem_alloc.h
should come from the /include/third_party/vulkan
path, but it is strange that the header file inclusion path is set in .gn
, but it does not work during the compilation process.
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.
Note the link in the initial post why this is needed.
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.
Note the link in the initial post why this is needed.
Actually not that link, but the comment in front of these lines:
Cf. third_party/vulkanmemoryallocator/GrVulkanMemoryAllocator.h:25
which is
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.
Ok, then I have no problem.
Testing |
@dg0yt, thanks for your contribution, when I test the usage of
usageTest.cpp
Config:
build:
|
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags. |
Test feature
|
@JonLiu1993 Error or successful? AFAIU you need to use C++17 - this is what the error message says, and this is what skia is using. This usage requirement isn't exposed by the CMake config, but this is not a regression. |
Ok, I successfully tested usage when I added
|
Thanks for debugging tricky cases for us @dg0yt @jimwang118 and @JonLiu1993 ! |
Use vcpkg port vulkan-headers instead of vendored copy.
Use vendored copies of
headers andtools for featurevulkan
, as already done by featuredawn
.Fix in-source location of vulkanmemoryallocator.
Guard vulkan inclusion in vulkanmemoryallocator as would be done in the vendored copy:
https://chromium.googlesource.com/external/github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/+/7de5cc00de50e71a3aab22dea52fbb7ff4efceb6/include/vk_mem_alloc.h#129
Fixes #32706. Amends #29448. Alternative to #32715.
./vcpkg x-add-version --all
and committing the result.