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

Support separate depth and stencil attachments during dynamic rendering #1878

Merged

Conversation

billhollings
Copy link
Contributor

With the introduction of VK_KHR_dynamic_rendering, Vulkan permitted separate depth and stencil attachments, which Metal always supported, although in Vulkan, the two attachments are, for the time being, restricted to the same pixel format and image view.

This PR implements depth and stencil attachments as separate attachments, and fixes attachment Metal validation errors in the process. This PR is more than strictly necessary to fix the reported Metal validation issues, but better aligns MoltenVK with Metal depth and stencil handling, and is positioned for a potential future enhancement to Vulkan, where separate depth and stencil formats might be supported in the future.

  • MVKRenderSubpass add separate getDepthFormat() & getStencilFormat(), and isDepthAttachmentUsed() & isStencilAttachmentUsed() and use instead of testing pixel format for depth and stencil components.
  • Add MVKRenderingAttachmentIterator class to consistently iterate, and take actions, on the attachments in VkRenderingInfo to create synthetic MVKRenderPass and extract image views and clear colors.
  • Remove mvkCreateRenderPass() and mvkCreateFramebuffer() in favor of additional constructors, and remove mvkGetDepthStencilFormat() in favor of retrieving formats for separate depth and stencil attachments.
  • MVKRenderpass constructors reorganize order of adding attachments and subpasses, and connecting the two.
  • Renmame MVKRenderPassAttachment to MVKAttachmentDescription.
  • MVKPipeline reorganize member variables to minimize gaps in content and remove unnecessary _isRasterizingDepthStencil member var (unrelated).
  • Fix Metal validation errors with dynamic rendering.
  • MVKPipeline validate depth & stencil formats before setting frag shader depth and stencil builtins, and before setting formats in MTLRenderPipelineDescriptor.
  • MVKCmdClearAttachments always set depth/stencil format if subpass includes a depth or stencil attachment, even if they are not being cleared.
  • MVKRenderingAttachmentIterator add synthetic attachment if depth or stencil attachment is not provided, but image format of underlying MVKImage supports both depth and stencil.

This PR fixes issue #1579.

- MVKRenderSubpass add separate getDepthFormat() & getStencilFormat(),
  and isDepthAttachmentUsed() & isStencilAttachmentUsed() and use
  instead of testing pixel format for depth and stencil components.
- Add MVKRenderingAttachmentIterator class to consistently iterate,
  and take actions, on the attachments in VkRenderingInfo to create
  synthetic MVKRenderPass and extract image views and clear colors.
- Remove mvkCreateRenderPass() and mvkCreateFramebuffer() in favor
  of additional constructors, and remove mvkGetDepthStencilFormat() in
  favor of retrieving formats for separate depth and stencil attachments.
- MVKRenderpass constructors reorganize order of adding attachments and
  subpasses, and connecting the two.
- Renmame MVKRenderPassAttachment to MVKAttachmentDescription.
- MVKPipeline reorganize member variables to minimize gaps in content
  and remove unnecessary _isRasterizingDepthStencil member var (unrelated).
- MVKPipeline validate depth & stencil formats before setting frag shader depth
  and stencil builtins, and before setting formats in MTLRenderPipelineDescriptor.
- MVKCmdClearAttachments always set depth/stencil format if subpass includes
  a depth or stencil attachment, even if they are not being cleared.
- MVKRenderingAttachmentIterator add synthetic attachment if depth or stencil
  attachment is not provided, but image format supports both depth and stencil.
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Just one question.

MoltenVK/MoltenVK/Commands/MVKCmdTransfer.mm Outdated Show resolved Hide resolved
- Also rename kMVKCachedColorAttachmentCount to kMVKMaxColorAttachmentCount and
  kMVKCachedViewportScissorCount to kMVKMaxViewportScissorCount and (unrelated)
@billhollings billhollings merged commit e50cb32 into KhronosGroup:main Apr 28, 2023
@billhollings billhollings deleted the dyn-rend-separate-depth-stencil branch April 28, 2023 16:39
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.

None yet

2 participants