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 texturing from depth buffers (Vulkan only so far) #13262

Merged
merged 3 commits into from
Aug 10, 2020

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Aug 9, 2020

See #13256 , this should fix some of them.

Uses the old depal path, meaning we make a copy of the depth buffer but converted through a CLUT, then texture using that. Can be optimized more later to use direct CLUT lookups during texturing - but can be tricky if that depth buffer is currently bound!

Verified that it fixes the fog in Me and My Kamari, so helps #6411 (but just in Vulkan, so not closing):

Before:
ULES00339_00003

After:
ULES00339_00004

@hrydgard hrydgard added the Vulkan label Aug 9, 2020
@hrydgard hrydgard added this to the v1.11.0 milestone Aug 9, 2020
@hrydgard
Copy link
Owner Author

hrydgard commented Aug 9, 2020

There seems to be some weird new asserts that I need to debug before merge (Unexpected layout 6). I think I know why though.

@hrydgard hrydgard marked this pull request as draft August 9, 2020 13:09
@iota97
Copy link
Contributor

iota97 commented Aug 9, 2020

While totally out of the scope of this PR, can this path be used to get depth buffer in post processing shader eventually?

@hrydgard
Copy link
Owner Author

hrydgard commented Aug 9, 2020

@iota That can be done entirely separately, at least for games where the depth buffer is easily possible to associate with the buffer being presented (which excludes GTA for example where the present buffer isn't the main one, but a different stretched one). Please create an issue and I might get around to it one day...

(EDIT: Actually some this stuff will be handy for that).

@hrydgard
Copy link
Owner Author

hrydgard commented Aug 9, 2020

Note to self, it's the second commit: "Vulkan/generic: Initial prep for depth texturing" (87b41cf) that breaks some games.

I think I'll actually try to get the first two commits in first separately, testing carefully, before bringing in the rest.

@hrydgard hrydgard marked this pull request as ready for review August 9, 2020 18:31
@hrydgard
Copy link
Owner Author

hrydgard commented Aug 9, 2020

Merged that and rebased on master. Should be working fine now, at least for Katamari.

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

Cool. We do have code on GL to download the depth to a temp buffer, in case games do CPU copies which I think we saw an upload. We may think this isn't working for some games when it's actually a download, so we should watch out for that...

-[Unknown]

GPU/Common/DepalettizeShaderCommon.cpp Outdated Show resolved Hide resolved
textureCache_->NotifyFramebuffer(v->fb_address, v, NOTIFY_FB_DESTROYED);
// Notify the texture cache of both the color and depth buffers.
textureCache_->NotifyFramebuffer(v->fb_address, v, NOTIFY_FB_DESTROYED, NOTIFY_FB_COLOR);
textureCache_->NotifyFramebuffer(v->z_address, v, NOTIFY_FB_DESTROYED, NOTIFY_FB_DEPTH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. What if two framebufs use the same z address? Do/should we destroy?

It's definitely going to be common that we decimate a temp buf that uses the same depth buf as a normal color buf.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah that's a good question... Although it'll soon repair itself anyway if that happens. This just notifies the texture cache that it should forget about a framebuffer/texture association, so as soon as it's needed again, it'll be back.

GPU/Common/FramebufferManagerCommon.cpp Outdated Show resolved Hide resolved
GPU/Common/TextureCacheCommon.cpp Outdated Show resolved Hide resolved
GPU/Common/TextureCacheCommon.cpp Outdated Show resolved Hide resolved
VkImageView fbo = (VkImageView)draw_->GetNativeObject(Draw::NativeObject::BOUND_TEXTURE0_IMAGEVIEW);

VkDescriptorSet descSet = vulkan2D_->GetDescriptorSet(fbo, samplerNearest_, clutTexture->GetImageView(), samplerNearest_);
VulkanRenderManager *renderManager = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
renderManager->BindPipeline(depalShader->pipeline);

if (depth) {
DepthScaleFactors scaleFactors = GetDepthScaleFactors();
Copy link
Collaborator

Choose a reason for hiding this comment

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

With accurate depth off, we actually deform the stored depth (it incorrectly depends on the depth viewport parameters when drawing - much like the XY viewport problem), which might cause problems for depth texturing. I think accurate depth is always on for Vulkan, though.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah we really need to get away from the stored depth being dependent on the viewport.. If we distort the viewport Z and the projection matrix together we should be able to get consistent results in the depth buffer while choosing how the clip happens, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, accurate depth already does it "right". It basically does as you describe, but also tries to buffer things to make depth clamp scenarios fail less often. If we didn't distort depth, we'd have the same cases of depth going wrong that accurate depth has.

For example, that game where it uses a crazy depth scale (like a million), without incorrectly deforming the depth and without clamping, won't draw. That's why it seems like accurate depth is "wrong" there, only because clamping isn't working and the deformed depth path is accidentally working.

-[Unknown]

ext/native/thin3d/VulkanQueueRunner.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants