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

Extend Vulkan Depth Test Overlay #3132

Conversation

Zorro666
Copy link
Collaborator

Description

Vulkan Replay support for Issue #2858

This is a PR in a sequence of PRs which all together complete the implementation of the feature.
The next PR will extend the automated overlay tests GL_Overlay_Test, D3D11_Overlay_Test, D3D12_Overlay_Test, VK_Overlay_Test.

The feature enables supporting shader exported depth for the Depth Test overlay by replaying the draw using the pixel shader from the capture instead of a replacement shader. Depth test passing pixels are identified by using the stencil buffer to generate a stencil mask.

If the depth buffer used for the draw does not have a stencil buffer then a new depth buffer is created with a depth+stencil format and a fullscreen pass is used to copy depth from the original depth buffer to the newly created depth+stencil buffer.

Tested on Windows using changes to Vk_Overlay_Test

The change makes the pixel shader write out depth 0.0 for a small rectangle just bottom-left of the centre

Before (using v1.29) : the rectangle of output depth 0.0 is shown as failing the depth test

Depth Buffer format D24_S8
image

Depth Buffer format D32F_S8
image

Depth Buffer format D16 MSAA
image

Depth Buffer format D24 MSAA
image

Depth Buffer format D32F MSAA
image

After : the rectangle of output depth 0.0 is shown as passing the depth test

Depth Buffer format D24_S8
image

Depth Buffer format D32F_S8
image

Depth Buffer format D16 MSAA
image

Depth Buffer format D24 MSAA
image

Buffer format D32F MSAA
image

@@ -225,7 +225,8 @@ struct ConciseGraphicsPipeline
};

static void create(WrappedVulkan *driver, const char *objName, const int line, VkPipeline *pipe,
const ConciseGraphicsPipeline &info)
const ConciseGraphicsPipeline &info,
const VkPipelineDepthStencilStateCreateInfo &depthStencil)
Copy link
Owner

Choose a reason for hiding this comment

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

I think seeing as we don't have that many ConciseGraphicsPipelines that we create for internal pipelines I would rather update the struct to handle setting the new stencil elements that you need to change here. Otherwise this could quickly become unwieldy and annoying to work with.

Since stencil state is one of those things with many theoretical combinations but few in practice, you could replace the current VkStencilOp stencilOp with a StencilMode stencilOperations where StencilMode is an enum with the different configurations we want - I think currently just 'keep' and 'overwrite with 0'. You could add 'test equal to 1 and keep'. If that becomes difficult to sum up clearly in one enum you could split it into two enums for the test mode and the update mode.

renderdoc/driver/vulkan/vk_debug.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_overlay.cpp Outdated Show resolved Hide resolved
@Zorro666 Zorro666 force-pushed the support-shader-exported-depth-in-depth-test-overlay-vk branch from 2a055c3 to 6d3ace5 Compare November 17, 2023 08:18
}
case StencilMode::WRITE_ZERO:
{
stencilOp = VK_STENCIL_OP_ZERO;
Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the changes here now I misspoke before, not every pipeline is explicitly writing zero as e.g. the FillWithDiscardPattern has a dynamic stencil ref, and so it's replacing with an arbitrary value (which it uses to fill the discard pattern in stencil)

I think it's better to keep the stencil op that was used here, REPLACE, rather than specialising to zero unnecessarily. It has the same effect but is more general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you - good spot. I have kept WRITE_ZERO also even though it can be achieved with REPLACE. It felt more descriptive in its intent.

Support shader exported depth by replaying using the capture pixel shader to determine passing pixels
@Zorro666 Zorro666 force-pushed the support-shader-exported-depth-in-depth-test-overlay-vk branch from 6d3ace5 to 3edecaf Compare November 18, 2023 11:35
@Zorro666 Zorro666 merged commit d72d790 into baldurk:v1.x Nov 27, 2023
16 checks passed
@Zorro666 Zorro666 deleted the support-shader-exported-depth-in-depth-test-overlay-vk branch November 27, 2023 16:33
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