Skip to content
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

Merged

Conversation

mark-lunarg
Copy link
Collaborator

@mark-lunarg mark-lunarg commented Mar 21, 2022

Tested with vulkan_test_applications::depth_stencil_resolve.apk.

gapis/api/vulkan/api/enums.api Outdated Show resolved Hide resolved
gapis/api/vulkan/api/enums.api Outdated Show resolved Hide resolved
dsrAttachmentRef := NewVkAttachmentReference2ᶜᵖ(sb.MustAllocReadData(
NewVkAttachmentReference2(
VkStructureType_VK_STRUCTURE_TYPE_ATTACHMENT_REFERENCE_2,
NewVoidᶜᵖ(memory.Nullptr),
Copy link
Contributor

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.

Copy link
Collaborator Author

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(
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@yalcinmelihyasin
Copy link
Contributor

yalcinmelihyasin commented Mar 29, 2022

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.

@mark-lunarg mark-lunarg force-pushed the markl-vk-khr-depth-stencil-resolve branch from dc6a016 to 18daeb7 Compare June 23, 2022 17:51
Copy link
Contributor

@yalcinmelihyasin yalcinmelihyasin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mark-lunarg mark-lunarg merged commit 0643399 into google:master Jun 28, 2022
@mark-lunarg mark-lunarg deleted the markl-vk-khr-depth-stencil-resolve branch June 28, 2022 13:58
rosasco-wk pushed a commit to rosasco-wk/agi that referenced this pull request Sep 2, 2022
* Implement VK_KHR_depth_stencil_resolve

* Add extension api file

* Address review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants