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

Resolve depth buffer in mobile renderer when required #78598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BastiaanOlij
Copy link
Contributor

This PR fixes a long standing issue with the mobile renderer when using MSAA. The subpass logic never had an option for resolving the MSAA depth buffer to a normal depth buffer.
This meant that:

  • reading from depth texture broke as the depth texture being copied to the back buffer was empty
  • when using an external depth texture (XR) the depth texture wasn't populated

This PR does rely on the VK_KHR_depth_stencil_resolve extension. This has been core since Vulkan 1.2 and was a KHR extension in Vulkan 1.0/1.1 and may not be supported on all GPUs.

Todos:

  • consider whether we want a fallback solution when the resolve extension is not available or adjust the back buffer copy to read from the MSAA buffers
  • add check that the MSAA depth buffer and normal depth buffer have compatible formats to enable the resolve
  • implement fallback if the XR solution can't provide a compatible buffer

@BastiaanOlij BastiaanOlij added this to the 4.2 milestone Jun 23, 2023
@BastiaanOlij BastiaanOlij self-assigned this Jun 23, 2023
@BastiaanOlij BastiaanOlij force-pushed the resolve_depth_buffer_mobile branch from d4b4f44 to db2e8d9 Compare June 23, 2023 04:15
@BastiaanOlij
Copy link
Contributor Author

I need to do some more testing but I think this also solves #78544

@Saul2022
Copy link

I think there should be 100% a fallback as the mobile renderer works better on older pc and it also has to support mobile, which nowadays it vulkan support is not the best.

@BastiaanOlij
Copy link
Contributor Author

@Saul2022 agreed though I'd like to mark all the "todos" for future PRs. This is an important fix I'd like to get into 4.1 and the fix window closes Monday. As currently resolving doesn't work plus we have a number of validation errors that are solved by this, this is a step forward and shouldn't break anything that isn't already broken.

@clayjohn @akien-mga I'm marking this for review due to the above reason if you guys are ok with that.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review June 24, 2023 01:30
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner June 24, 2023 01:30
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 24, 2023
@akien-mga
Copy link
Member

As discussed on chat, let's not rush it now, but this is a good candidate for 4.1.1 once finalized and merged for 4.2.

@Calinou
Copy link
Member

Calinou commented Jun 28, 2023

Does this resolve #78785?

@BastiaanOlij
Copy link
Contributor Author

@Calinou I don't think so, but that is one I need to find time for

@BastiaanOlij BastiaanOlij force-pushed the resolve_depth_buffer_mobile branch from b3c9081 to f12e564 Compare September 14, 2023 07:38
@clayjohn clayjohn modified the milestones: 4.2, 4.x Oct 25, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@LeandroSQ
Copy link

Is this still getting merged? I also got the same issue on my XR project so I wonder if there are any workarounds too.

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

Successfully merging this pull request may close these issues.

7 participants