-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[3PP] Uplifted gslang 16, SPIR-V Tools, Vulkan-Headers and VMA #2329
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
base: master
Are you sure you want to change the base?
Conversation
| }) | ||
|
|
||
| defines({ | ||
| "VULKAN_HPP_NO_TO_STRING", |
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.
vk::to_string is used in Xenia, primarily for VkResult reporting. Currently it's only used in the ui/vulkan part, but it will be used frequently in gpu/vulkan as well in the future. What exactly does defining this change in Xenia?
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.
Oh, that definition doesn't prevent vk::to_string from being used completely, rather it removes std::to_string usage. Yeah, that's okay, though it needs some comment like -- Use vk::to_string rather than std::to_string. to prevent confusion. Also, gpu/vulkan and ui/vulkan would probably be the better place for this, unless it affects building of multiple third-party libraries.
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 think it'd be better to eliminate the actual reason of why our code doesn't work without defining this. It's likely that it's something trivial, like we need to change vk::to_string to std::to_string, or to specify what namespace Vulkan-Hpp should be using, or something like that.
| } | ||
|
|
||
| SpirvBuilder::IfBuilder::IfBuilder(spv::Id condition, unsigned int control, | ||
| spv::Id SpirvBuilder::createAccessChain(spv::StorageClass storage_class, |
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.
It's kind of scary to override a non-virtual method this way, but since we don't seem to be passing instances of this class to anything that uses spv::Builder directly that may call this probably, should be okay.
|
This pull request seems quite complete aside from code formatting and the questionable part about |
bab072b to
07d11b3
Compare
- Removed usage of spirv_remap from xenia-build. It was removed in 2022 in favor of spirv-opt Co-authored-by: Herman S. <[email protected]>
07d11b3 to
8fb5444
Compare
No description provided.