-
Notifications
You must be signed in to change notification settings - Fork 141
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
Implement VK_KHR_depth_stencil_resolve #1066
Implement VK_KHR_depth_stencil_resolve #1066
Conversation
gapis/api/vulkan/state_rebuilder.go
Outdated
dsrAttachmentRef := NewVkAttachmentReference2ᶜᵖ(sb.MustAllocReadData( | ||
NewVkAttachmentReference2( | ||
VkStructureType_VK_STRUCTURE_TYPE_ATTACHMENT_REFERENCE_2, | ||
NewVoidᶜᵖ(memory.Nullptr), |
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.
AttachmentReference2 here can originally have pNext that we are missing currently.
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.
Done, please check this. Test appears to confirm it is correct.
StencilResolveMode: ext.stencilResolveMode, | ||
) | ||
attachment := ext.pDepthStencilResolveAttachment[0] | ||
description.DepthStencilResolve.DepthStencilResolveAttachment = AttachmentReference( |
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.
We are missing pNext handling here.
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.
@yalcinmelihyasin, just to be clear, the other AttachmentReference()
usages on lines 442, 453, 465 and 476 also need 'pNext' handling, yes?
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.
Yes. with the new PR merging, we are able to do this now.
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.
Done.
There are some minor comments about adding KHR at the end but I believe that this PR showed a fundamental flaw in our API files in regards to pNext handling. We handle pNext inline in the code without proper modularization. Therefore, the new sub structures in Vulkan e.g VkAttachmentReference2 that can be included in many places now is pain to handle. I would like to hold this PR a bit and talk about how we can restructure this. |
dc6a016
to
18daeb7
Compare
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.
LGTM
* Implement VK_KHR_depth_stencil_resolve * Add extension api file * Address review feedback
Tested with
vulkan_test_applications::depth_stencil_resolve.apk
.