-
Notifications
You must be signed in to change notification settings - Fork 28
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
build: Update to header 1.3.295 #231
Conversation
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.
Seems like the code gen removed a bunch of structs from the safe structs.
src/vulkan/vk_safe_struct_utils.cpp
Outdated
case VK_STRUCTURE_TYPE_COMPUTE_PIPELINE_CREATE_INFO: | ||
safe_pNext = new safe_VkComputePipelineCreateInfo(reinterpret_cast<const VkComputePipelineCreateInfo *>(pNext), copy_state, false); | ||
break; | ||
case VK_STRUCTURE_TYPE_GRAPHICS_PIPELINE_CREATE_INFO: | ||
safe_pNext = new safe_VkGraphicsPipelineCreateInfo(reinterpret_cast<const VkGraphicsPipelineCreateInfo *>(pNext), copy_state, false); | ||
break; |
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.
Why are these removed?
src/vulkan/vk_safe_struct_utils.cpp
Outdated
case VK_STRUCTURE_TYPE_EXECUTION_GRAPH_PIPELINE_CREATE_INFO_AMDX: | ||
safe_pNext = new safe_VkExecutionGraphPipelineCreateInfoAMDX(reinterpret_cast<const VkExecutionGraphPipelineCreateInfoAMDX *>(pNext), copy_state, false); | ||
break; |
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.
Also this.
case VK_STRUCTURE_TYPE_RAY_TRACING_PIPELINE_CREATE_INFO_NV: | ||
safe_pNext = new safe_VkRayTracingPipelineCreateInfoNV(reinterpret_cast<const VkRayTracingPipelineCreateInfoNV *>(pNext), copy_state, false); | ||
break; |
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.
And this.
src/vulkan/vk_safe_struct_utils.cpp
Outdated
case VK_STRUCTURE_TYPE_RAY_TRACING_PIPELINE_CREATE_INFO_KHR: | ||
safe_pNext = new safe_VkRayTracingPipelineCreateInfoKHR(reinterpret_cast<const VkRayTracingPipelineCreateInfoKHR *>(pNext), copy_state, false); | ||
break; |
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.
This too.
src/vulkan/vk_safe_struct_utils.cpp
Outdated
case VK_STRUCTURE_TYPE_COMPUTE_PIPELINE_CREATE_INFO: | ||
delete reinterpret_cast<safe_VkComputePipelineCreateInfo *>(header); | ||
break; | ||
case VK_STRUCTURE_TYPE_GRAPHICS_PIPELINE_CREATE_INFO: | ||
delete reinterpret_cast<safe_VkGraphicsPipelineCreateInfo *>(header); | ||
break; |
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.
As well as this - And I'm getting tired of commenting on each change so I wont.
It is a bit odd, I'm looking at it now. When safe struct was part of VVL I know it only made deep copies for structs with embedded handle ids that needed to be remapped. If the header update changed the "structextends" dependency tree to remove some possible pNext structs with handles, that might have caused the codegen to no longer care about them. |
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'll go ahead and approve so you can move forward with the header update. Seems that something in the xml changed, and while it may not be a big deal it definitely was something unexpected.
Pipeline create info structs can appear in the VkPipelineCreatInfoKHR pNext chain even though they don't extend that structure in the traditional sense.
@charles-lunarg Turns out this was a good catch! The reason those were removed was because the various pipeline create info structs no longer "structextend" VkPipelineCreateInfoKHR in the xml. It turns out that they don't behave like true extension structs and the adding them as such screws up other parts of the spec build (like determining the base struct of pNext chain) but... Since they are still valid in the pNext chain, I added a manual workaround to include them back |
Sounds like we understand the root cause then! I already reviewed it so I can't approve it again. So consider this a second approval |
No description provided.