fix(VR): depthbuffer culling when upscaling#1889
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds VR depth-buffer upscaling functionality and refines shadowmap cascade initialization. It introduces a new depth upscaling shader with associated constant buffers and rendering paths, clamps shadowmap cascade values during initialization, removes obsolete VR depth-culling constraint logic, and updates display strings in settings. Changes
Sequence DiagramsequenceDiagram
participant Main as Rendering Pipeline
participant Upscale as Upscaling Module
participant GPU as GPU/DirectX
participant Shader as DepthUpscalePS
participant DepthBuf as Depth Buffer
Main->>Upscale: PerformUpscaling(isVR, scale)
alt VR and enableVRDepthUpscale and scale != 1.0
Upscale->>Upscale: UpscaleDepth()
Upscale->>Upscale: GetDepthUpscalePS() [lazy compile]
Upscale->>GPU: Setup DepthUpscaleCB (SourceDim, Scale)
Upscale->>GPU: Bind DepthLowRes SRV
Upscale->>GPU: Set render target & UAV
Upscale->>GPU: Invoke depth-upscale pass
GPU->>Shader: Execute DepthUpscalePS main()
Shader->>DepthBuf: Sample 2x2 neighborhood
Shader->>Shader: Compute minDepth (conservative)
Shader->>GPU: Output upscaled depth (SV_Depth)
GPU->>DepthBuf: Write upscaled depth
end
Upscale->>Main: Continue main upscaling pass
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ A pre-release build is available for this PR: |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Ok, looks like issues when submerged exist. |
15ac966 to
5286627
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/Features/Upscaling.h (1)
112-126: VR depth upscale scaffolding looks well-structured.The
DepthUpscaleCBlayout matches the HLSL cbuffer declaration, and the new members follow the existing patterns in this class (e.g.,jitterCB,upscalingDataCB).One minor note:
enableVRDepthUpscaledefaults totruebut doesn't appear to be exposed inDrawSettings()or serialized inSaveSettings/LoadSettings. Consider adding a UI toggle and persistence so users can disable this if it causes issues.src/Features/Upscaling.cpp (1)
1609-1671: Missing render-state cleanup after VR depth-only pass.The VR depth pass sets
OMSetDepthStencilState,RSSetState,VSSetShader,PSSetShader,PSSetSamplers, andPSSetConstantBuffersbut only unbinds SRVs at lines 1667-1668. The subsequent code block at line 1673 re-sets most of these, so it likely works in practice, but ifresolutionScale.x == 1.0fsomehow evaluates differently between the two blocks (or future code changes the flow), stale state could leak.At minimum, consider unbinding the render target / DSV:
ID3D11ShaderResourceView* nullSRVs[1] = { nullptr }; context->PSSetShaderResources(0, 1, nullSRVs); + context->OMSetRenderTargets(0, nullptr, nullptr);features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl (2)
59-63: SBS UV clamping is a good defense but doesn't fully protectGatherRedwith a non-CLAMP sampler.The
clamp()on lines 62-63 correctly constrains the center sample point, butGatherRedfetches a 2×2 footprint that can extend one texel beyond the clamped coordinate when the sampler addressing is WRAP. This is complementary to the C++ fix (switching to a CLAMP sampler) — with CLAMP addressing, this UV clamping becomes redundant but harmless as a defense-in-depth measure.
65-77: Consider filtering out zero-depth samples from theminDepthcomputation instead of using a binary branch.Currently, if any sample in the quad is 0 (HMD mask), the shader falls back entirely to point sampling. A more robust approach would filter zero-depth samples from the
min()so valid depth neighbors still contribute to conservative culling near mask boundaries:Suggested improvement
float4 depthQuad = DepthLowRes.GatherRed(LinearSampler, uv); - float minDepth = min(min(depthQuad.x, depthQuad.y), min(depthQuad.z, depthQuad.w)); - - // HMD hidden area mask: depth == 0 in reversed-Z. - // If ANY sample in the 2x2 quad is 0, we're at or near the mask boundary. - // Use point sampling only to avoid bilinear blending with mask pixels. - if (minDepth == 0.0) { - psout.Depth = DepthLowRes.Load(int3(texel, 0)); - return psout; - } - - // All four neighbors are valid depth. Blend point-sampled depth toward - // the conservative minimum for safe culling. - float pointDepth = DepthLowRes.Load(int3(texel, 0)); + // Filter out HMD mask (depth == 0) from min computation + float minDepth = 1.0; // Start at far plane (reversed-Z) + [unroll] for (int i = 0; i < 4; i++) { + if (depthQuad[i] > 0.0) + minDepth = min(minDepth, depthQuad[i]); + } + + float pointDepth = DepthLowRes.Load(int3(texel, 0)); + + // If point sample itself is mask, output 0 (mask) + if (pointDepth == 0.0) { + psout.Depth = 0.0; + return psout; + }This was also recommended in the PR discussion as a complementary defense-in-depth measure.
|
I suspect this was actually unnecessary as we do upscale the depth buffer. The issue underwater is caused by the depthbuffer being upscaled twice. I'll likely keep this open for a few more days and close it out. |
|
Closing in favor of #1858 |


Depends on #1888
Fix by @vrnord from
https://github.com/vrnord/skyrim-community-shaders-VR-DLSS/tree/vr-depth-upscale-taa-reorder
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Style