feat(vr): VR stereo reprojection quality improvements#1982
Conversation
…g, and DLSS fix - Hardware stencil-based Eye 1 pixel culling for forward passes - StereoBlend lateral reprojection from Eye 0 to Eye 1 - AMD CAS post-TAA sharpening with strength slider - RGB fringe suppression for tree alpha test artifacts - Fix UpscaleDepth to use upstream dev version (prevents DLSS crash) - Mip LOD bias control for VR foliage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Streamline proxy wrapping disrupts VR compositor frame pacing. DLSS works without wrapped interfaces; only frame generation needs them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace nearest-neighbor integer Load() with hardware bilinear SampleLevel() for color reads during Eye 1 reprojection. Fixes visible "distortion ridges" at iso-depth contours where floor(offset) jumps by 1 pixel. Motion vectors remain integer-sampled via Load() — DLSS requires discrete motion data. The decoupled approach eliminates visual artifacts without affecting temporal accumulation. - Add SampleReprojectedColor() helper using GetDimensions() + UV math - Bind linear sampler at s0 in overwrite mode (lazy-created) - Clamp UVs to active DRS viewport bounds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store parallax occlusion mapping (POM) depth offset in Reflectance.w so the stereo reprojection shader can account for parallax displacement when reprojecting Eye 0 → Eye 1. Without this, POM surfaces show incorrect stereo depth. - Enable alpha writes on RT[5] (REFLECTANCE) in deferred blend states - Output pixelOffset to Reflectance.w in Lighting.hlsl (EMAT+PARALLAX) - Initialize pixelOffset=0 in ExtendedMaterials GetParallaxCoords - Add pomDepthScale setting and debugPOMDepth visualization - Disable CAS (force 0) pending proper integration; hide UI slider Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix HLSL/C++ struct mismatch in CPMSettings.pad0: change float1 → bool in HLSL and float → uint in C++ to match the bool fields preceding it. Required after merging VR MIP bias fields with upstream SH ambient fields in SharedDataCB. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reset CommonLibSSE-NG submodule to upstream 4.7.1 (23bf5512). Fix SharedDataCB struct padding: float pad0 → float4 pad0 to match HLSL cbuffer layout (float2 + implicit 8-byte gap before float4 AmbientSHR) and ensure sizeof is a multiple of 16. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new VR stereo optimization feature implementing per-pixel stencil classification, reprojection of Eye 0 color into culled Eye 1 pixels, and CAS sharpening; extends SharedData and State for VR MIP-bias and alpha thresholds; integrates new shaders and pipeline hooks into deferred rendering and VR feature surfaces. Changes
Sequence DiagramsequenceDiagram
participant CPU as CPU/Main
participant StencilCS as StencilCS<br/>(Classification)
participant ModeTex as ModeTexture<br/>(RW)
participant DepthTex as DepthTexture
participant StencilWP as StencilWrite<br/>VS+PS
participant ReprojCS as ReprojectionCS
participant CAS as CASCS
participant Output as OutputTexture
CPU->>StencilCS: Dispatch classification (full SBS)
StencilCS->>DepthTex: Sample depth per-pixel
Note over StencilCS: Depth reprojection check\nEdge detection\nNear-field check
StencilCS->>ModeTex: Write pixel mode (DISOCCLUDED/EDGE/MAIN/FULL_BLEND)
CPU->>StencilWP: Draw fullscreen stencil pass (Eye 1)
StencilWP->>ModeTex: Read mode
Note over StencilWP: Discard non-reprojectable\nWrite stencil ref for MAIN/EDGE
CPU->>ReprojCS: Dispatch reprojection (Eye 1 region)
ReprojCS->>DepthTex: Read depth for reproj validation
ReprojCS->>Output: Sample Eye 0 color and write into Eye 1 pixels
CPU->>CAS: Dispatch CAS sharpening (post-TAA)
CAS->>Output: Read neighborhood, apply adaptive sharpen, write back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Deferred.cpp (1)
404-421:⚠️ Potential issue | 🟠 MajorUnbind the deferred-composite UAVs before
DrawStereoBlend().At line 386, the deferred composite shader writes to
main.UAV,normals.UAV, andmotionVectors.UAV(slots 0–2). These bindings persist through the subsequent cleanup block (lines 410–422). WhenDrawStereoBlend()executes at line 407, it immediately callsCopyResource()onmain.texture(StereoBlend.cpp:58), which is the same resource asmain.UAV. This violates D3D11's constraint against reading from a resource bound as a compute shader output and can produce validation layer errors or undefined copy results.Move the cleanup block (lines 410–422) above line 407, or explicitly unbind UAV slots 0–2 before calling
DrawStereoBlend().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 404 - 421, The deferred composite UAVs (main.UAV, normals.UAV, motionVectors.UAV) remain bound when globals::features::vr.DrawStereoBlend() is called; unbind those UAVs (compute shader UAV slots 0–2) before calling DrawStereoBlend() to avoid reading a resource that is still bound as a UAV. Move the existing cleanup/unbind block so it runs before globals::features::vr.DrawStereoBlend(), or explicitly call the CS unbind for slots 0–2 (via context->CSSetUnorderedAccessViews for slots 0–2 with null UAVs and clear any related CS state like CSSetShader/CSSetShaderResources/CSSetConstantBuffers) immediately prior to invoking DrawStereoBlend().
🧹 Nitpick comments (4)
features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini (1)
1-2: Consider adding a linked issue in the PR description.Since this PR implements a substantial VR feature set, add a tracking reference (for example,
Implements #<id>orAddresses #<id>).As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/VR` Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini around lines 1 - 2, Add a linked GitHub issue reference to the PR description for this VR feature set (e.g., use "Implements #<id>" or "Addresses #<id>") so the change in Features/VR Stereo Optimizations (see Info section and Version = 1-0-0) is tracked; update the PR description to include that keyword and the issue number.package/Shaders/VR/StereoBlendCS.hlsl (1)
28-34: Avoid duplicatedMODE_*definitions across shader files.These constants now exist in multiple places; splitting shared mode defines into a no-cbuffer include would prevent drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/StereoBlendCS.hlsl` around lines 28 - 34, The duplicated MODE_* defines (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) should be moved to a single shared header and included rather than redefined in StereoBlendCS.hlsl: create a new header (e.g., VRStereoModes.hlsli) that contains only these mode defines (no cbuffers), update StereoBlendCS.hlsl to remove the local `#define` block and `#include` the new header, and update any other shader files that currently duplicate these constants to include the same shared header; keep StereoBlendCB and other cbuffers untouched to avoid conflicts.package/Shaders/Common/VR.hlsli (1)
630-636: Make Eye 1 jitter runtime-configurable (or resolution-derived).The hardcoded NDC values make the effective pixel offset vary by output resolution, which can create inconsistent stereo behavior across headsets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/VR.hlsli` around lines 630 - 636, Replace the hardcoded NDC jitter (static const float2 kJitterNDC) applied when a_eyeIndex == 1 with a runtime-configurable or resolution-derived value: expose a shader constant/uniform (e.g., g_VRJitterNDC or a constant buffer field) and use that instead of kJitterNDC, or compute the NDC from a desired subpixel diagonal offset divided by the render-target diagonal in pixels (using available render-target size/viewport inputs) so the applied offset uses vsout.VRPosition.w * computedNDC; update any initialization code or C++/render setup to populate the new uniform when creating the view. Ensure references to a_eyeIndex and vsout.VRPosition.xy / VRPosition.w are preserved and only the jitter source is replaced.package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl (1)
49-49: Consider tightening the clamp to Eye 0 bounds only.The current clamp allows coordinates up to
FrameDim - 1(full SBS width), but since we're sampling from Eye 0, clamping toeyeWidth - 1would be more precise:-int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(FrameDim) - 1); +int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(eyeWidth - 1, eyeHeight - 1));This is a minor robustness improvement—the stereo conversion should already produce valid Eye 0 coordinates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl` at line 49, Clamp for eye0Px should use Eye 0's width instead of full FrameDim: when computing eye0Px from eye0StereoUV and FrameDim, change the clamp upper bound to int2(eyeWidth, FrameDim) - 1 (keep the lower bound int2(0,0)). Update the clamp call that constructs eye0Px so it uses eyeWidth for the X limit while leaving Y limited by FrameDim to ensure we only sample within Eye 0 bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 500: Clamp pixelOffset after computing it with lerp to ensure numerical
overshoot from parallaxAmount cannot produce exaggerated values; locate the
assignment to pixelOffset (pixelOffset = lerp(parallaxAmount, 0.5,
nearBlendToFar)) and replace it so pixelOffset is constrained to a safe range
(e.g., using clamp(pixelOffset, minValue, maxValue)) before any export/write.
Use the existing variable names pixelOffset, parallaxAmount, and nearBlendToFar
to find the line, and pick sensible bounds for your pipeline (e.g., 0..0.5 or
another agreed stereo-correction range) when clamping.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-106: The fast-path early-return in DeferredCompositeCS.hlsl is
too broad: inside the VR_STEREO_OPT block the check on StereoOptModeTexture
currently returns for mode == 1 (MODE_EDGE) as well as mode == 2 (MODE_MAIN),
but ExecuteStencilWritePass() only writes stencil for MODE_MAIN so skipping
MODE_EDGE drops valid G-buffer shading; change the condition to only
early-return for MODE_MAIN (i.e., restrict the check to mode == MODE_MAIN) or
update ExecuteStencilWritePass() to also stencil-cull MODE_EDGE—modify the
branch that reads StereoOptModeTexture[uint2(dispatchID.xy)] and the comparison
against MODE_MAIN/MODE_EDGE accordingly to ensure MODE_EDGE is processed by
deferred composite.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3089-3095: In the TREE_ANIM branch the alpha test is hardcoded and
ignores the new SharedData::VRAlphaTestThreshold; update the two checks so they
use the shared threshold instead of 0.1 and AlphaTestRefRS — e.g. replace "if
(alpha < 0.1)" and "if (alpha - AlphaTestRefRS < 0)" with a single consistent
comparison against SharedData.VRAlphaTestThreshold (or use if (alpha <
max(AlphaTestRefRS, SharedData.VRAlphaTestThreshold)) to preserve the existing
AlphaTestRefRS floor), referencing the SharedData::VRAlphaTestThreshold symbol
so foliage rejection honors the new setting.
- Around line 3200-3205: The code only exports pixelOffset into
psout.Reflectance.w when EMAT is defined, so TRUE_PBR parallax paths never write
pixelOffset; update the preprocessor condition around the psout.Reflectance
assignment so that TRUE_PBR parallax also sets Reflectance.w (e.g. change the
guard to include TRUE_PBR as well), ensuring pixelOffset (the variable named
pixelOffset used earlier) is written into psout.Reflectance.w whenever PARALLAX
or LANDSCAPE is active for either EMAT or TRUE_PBR; modify the conditional using
the existing symbols EMAT, TRUE_PBR, PARALLAX, and LANDSCAPE to accomplish this.
In `@package/Shaders/VR/CASCS.hlsl`:
- Around line 60-62: The expression computing ampRGB uses rcp(mxRGB) which can
produce Inf/NaN when any component of mxRGB is zero; fix by clamping mxRGB to a
small positive epsilon before inverting (e.g., use max(mxRGB, epsilon) or
rcp(max(mxRGB, epsilon))) so ampRGB = saturate(min(mnRGB, 2.0 - mxRGB) *
rcp(clamped_mxRGB)); keep the subsequent rsqrt(ampRGB) unchanged and choose an
epsilon like 1e-6 to avoid altering valid values.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 162-167: The POM correction can produce zero/negative linear depth
before converting back to reprojection space; clamp the corrected linear depth
to a small positive epsilon before using it. In the block referencing
pomOffsetFB, POMDepthScale, SharedData::GetScreenDepth(centerDepth) and
reprojDepthFB compute depthCorrectionFB and newLinDepthFB as before but then
ensure newLinDepthFB = max(newLinDepthFB, epsilon) (use a small value like 1e-4)
before the reprojDepthFB calculation; apply the same clamp to the other
POM-corrected branch (the block around the other pomOffset/FB variables at the
later lines) so both conversions never divide by zero or negative depth.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 19-20: The shader currently uses the hardcoded constant
kDisocclusionThreshold (0.015) when classifying pixels even though the CPU
populates the DisocclusionThreshold parameter; update the classification
comparisons in the StencilCS compute shader to use the DisocclusionThreshold
variable instead of kDisocclusionThreshold (replace uses of
kDisocclusionThreshold in the classification logic and the other occurrences
around the same block) so the CPU-driven slider actually affects stencil
classification.
In `@src/Features/Upscaling.cpp`:
- Around line 1233-1240: The Map/Unmap manual pattern around vrClearHMDMaskCB
(and the other occurrence) must be replaced with the ScopedD3DMap RAII wrapper
to check HRESULT and guarantee Unmap; inside the scope use ScopedD3DMap
scopedMap(context.get(), vrClearHMDMaskCB.get(), D3D11_MAP_WRITE_DISCARD) and
copy the uint32_t cbData[8] into scopedMap.pData (or use scopedMap.mapped.pData)
via memcpy, removing the explicit context->Map/Unmap calls; follow the same
replacement for the second occurrence (see VRStereoOptimizations.cpp usage) so
error handling and cleanup are consistent.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 98-109: The selector for compute shaders ignores the new debug
modes 4/5 and also bypasses settings when vrStereoOptActive is true; update the
logic around activeCS (using stereoBlendCS, stereoBlendOverwriteCS,
stereoBlendDebugBackCheckCS, stereoBlendDebugBlendWeightCS,
stereoBlendDebugEdgeDetectionCS and any new shader variants you have for
"Overwrite" and "Overwrite Eye1") so that settings.StereoBlendDebugMode values 4
and 5 map to the correct shader variants instead of falling back to defaults,
and ensure that vrStereoOptActive doesn’t unconditionally override those debug
selections (or explicitly handle the debug modes when vrStereoOptActive is
true). If you don’t have shader variants for 4/5, remove those UI options
instead.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 658-665: The early-return paths in VRStereoOptimizations.cpp
(inside the function that calls DispatchStencil()) sometimes return without
disarming stencil hooks, leaving stencilActive true and hooks rewriting later
passes; ensure that every early exit first disables/unarms the stencil hooks by
calling the existing disarm routine (e.g., the function that clears
stencilActive and removes the context hooks — reference DispatchStencil(),
stencilActive, and the disarm/unarm function name used elsewhere) or convert the
hook lifetime to an RAII guard so hooks are always disarmed on scope exit; apply
the same fix for the similar block around lines ~707-720.
- Around line 406-407: The code path enables a stencil write pass even when the
write DSV/state is missing; change the early-exit so the function fails closed:
if any of the write-pass resources (stencilCS, stencilWriteVS, stencilWritePS,
texPerPixelMode, paramsCB) or the write DSV/state are unavailable, return early
and ensure stencilActive is set to false (do not enable the write-pass), and
emit a clear warning; update SetupResources()/the entrypoint that checks
stencilCS/stencilWriteVS/stencilWritePS/texPerPixelMode/paramsCB to also
validate the write DSV/state, set stencilActive = false before returning, and
avoid scheduling the write pass or setting hooks that depend on it.
In `@src/Globals.cpp`:
- Around line 314-323: The hook for ClearDepthStencilView currently strips
D3D11_CLEAR_STENCIL whenever globals::features::vrStereoOptimizations reports
stencil active, which affects all DSVs; change the logic in the
ClearDepthStencilView hook to only remove D3D11_CLEAR_STENCIL when the cleared
view matches the main scene DSV (or the specific stereo-culling DSV) instead of
unconditionally. Concretely, before mutating ClearFlags check pDepthStencilView
is non-null and compare its underlying ID3D11DepthStencilView (or its resource
via GetResource) to the known main scene DSV/stereo target stored in your
globals (e.g., the same object referenced by your main scene DSV or
vrStereoOptimizations target) and only then clear D3D11_CLEAR_STENCIL; otherwise
leave ClearFlags intact and proceed. Ensure null checks and safe resource
queries (graceful degradation) so malformed or missing DSVs don’t cause crashes.
---
Outside diff comments:
In `@src/Deferred.cpp`:
- Around line 404-421: The deferred composite UAVs (main.UAV, normals.UAV,
motionVectors.UAV) remain bound when globals::features::vr.DrawStereoBlend() is
called; unbind those UAVs (compute shader UAV slots 0–2) before calling
DrawStereoBlend() to avoid reading a resource that is still bound as a UAV. Move
the existing cleanup/unbind block so it runs before
globals::features::vr.DrawStereoBlend(), or explicitly call the CS unbind for
slots 0–2 (via context->CSSetUnorderedAccessViews for slots 0–2 with null UAVs
and clear any related CS state like
CSSetShader/CSSetShaderResources/CSSetConstantBuffers) immediately prior to
invoking DrawStereoBlend().
---
Nitpick comments:
In `@features/VR` Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini:
- Around line 1-2: Add a linked GitHub issue reference to the PR description for
this VR feature set (e.g., use "Implements #<id>" or "Addresses #<id>") so the
change in Features/VR Stereo Optimizations (see Info section and Version =
1-0-0) is tracked; update the PR description to include that keyword and the
issue number.
In `@package/Shaders/Common/VR.hlsli`:
- Around line 630-636: Replace the hardcoded NDC jitter (static const float2
kJitterNDC) applied when a_eyeIndex == 1 with a runtime-configurable or
resolution-derived value: expose a shader constant/uniform (e.g., g_VRJitterNDC
or a constant buffer field) and use that instead of kJitterNDC, or compute the
NDC from a desired subpixel diagonal offset divided by the render-target
diagonal in pixels (using available render-target size/viewport inputs) so the
applied offset uses vsout.VRPosition.w * computedNDC; update any initialization
code or C++/render setup to populate the new uniform when creating the view.
Ensure references to a_eyeIndex and vsout.VRPosition.xy / VRPosition.w are
preserved and only the jitter source is replaced.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 28-34: The duplicated MODE_* defines (MODE_DISOCCLUDED, MODE_EDGE,
MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) should be moved to a single
shared header and included rather than redefined in StereoBlendCS.hlsl: create a
new header (e.g., VRStereoModes.hlsli) that contains only these mode defines (no
cbuffers), update StereoBlendCS.hlsl to remove the local `#define` block and
`#include` the new header, and update any other shader files that currently
duplicate these constants to include the same shared header; keep StereoBlendCB
and other cbuffers untouched to avoid conflicts.
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Line 49: Clamp for eye0Px should use Eye 0's width instead of full FrameDim:
when computing eye0Px from eye0StereoUV and FrameDim, change the clamp upper
bound to int2(eyeWidth, FrameDim) - 1 (keep the lower bound int2(0,0)). Update
the clamp call that constructs eye0Px so it uses eyeWidth for the X limit while
leaving Y limited by FrameDim to ensure we only sample within Eye 0 bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e9997e5-d18f-4a6a-a9fd-30e6a48782e3
📒 Files selected for processing (31)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/VR/CASCS.hlslpackage/Shaders/VR/StereoBlendCS.hlslpackage/Shaders/VR/VRPostProcessCS.hlslpackage/Shaders/VRStereoOptimizations/ReprojectionCS.hlslpackage/Shaders/VRStereoOptimizations/StencilCS.hlslpackage/Shaders/VRStereoOptimizations/StencilWritePS.hlslpackage/Shaders/VRStereoOptimizations/StencilWriteVS.hlslpackage/Shaders/VRStereoOptimizations/cbuffers.hlslisrc/Deferred.cppsrc/Feature.cppsrc/Features/ExtendedMaterials.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cppsrc/Globals.hsrc/State.cppsrc/State.h
| # if defined(TREE_ANIM) | ||
| // Fixed alpha floor — catch zombie texels with near-zero alpha | ||
| if (alpha < 0.1) { | ||
| discard; | ||
| } | ||
| if (alpha - AlphaTestRefRS < 0) { | ||
| discard; |
There was a problem hiding this comment.
VRAlphaTestThreshold is still a no-op for TREE_ANIM.
This branch hardcodes 0.1 and AlphaTestRefRS, so the new SharedData::VRAlphaTestThreshold plumbing never participates in foliage rejection. The setting added by this PR will not affect tree alpha testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Lighting.hlsl` around lines 3089 - 3095, In the TREE_ANIM
branch the alpha test is hardcoded and ignores the new
SharedData::VRAlphaTestThreshold; update the two checks so they use the shared
threshold instead of 0.1 and AlphaTestRefRS — e.g. replace "if (alpha < 0.1)"
and "if (alpha - AlphaTestRefRS < 0)" with a single consistent comparison
against SharedData.VRAlphaTestThreshold (or use if (alpha < max(AlphaTestRefRS,
SharedData.VRAlphaTestThreshold)) to preserve the existing AlphaTestRefRS
floor), referencing the SharedData::VRAlphaTestThreshold symbol so foliage
rejection honors the new setting.
There was a problem hiding this comment.
This is correct — VRAlphaTestThreshold is used in DistantTree.hlsl and RunGrass.hlsl but not in the Lighting.hlsl TREE_ANIM path. The Lighting TREE_ANIM path uses a different hardcoded approach (0.1 floor + fringe suppression). This will be addressed when we split the VR foliage changes to a separate PR — the threshold wiring in Lighting.hlsl will be part of that work.
There was a problem hiding this comment.
@vrnord, understood — thanks for the clear explanation. The hardcoded 0.1 floor + fringe suppression in the TREE_ANIM path is intentionally decoupled from VRAlphaTestThreshold for now, with the wiring planned for a dedicated VR foliage PR.
Would you like me to open a follow-up GitHub issue to track the VRAlphaTestThreshold wiring into Lighting.hlsl's TREE_ANIM path so it doesn't get lost?
✏️ Learnings added
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:06.980Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:37.608Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:34:58.304Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1858
File: src/FrameAnnotations.cpp:1022-1024
Timestamp: 2026-02-21T22:01:15.534Z
Learning: In the skyrim-community-shaders repository, for the `REL::Relocate` function, when the VR offset is not explicitly provided as a third argument, it defaults to using the first parameter (SE offset) as the VR offset. This means `REL::Relocate(0x37f, 0)` will use `0x37f` for both SE and VR, with AE getting `0`.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
| ID3D11ComputeShader* activeCS = stereoBlendCS.get(); | ||
| if (settings.StereoBlendDebugMode == 1 && stereoBlendDebugBackCheckCS) | ||
| activeCS = stereoBlendDebugBackCheckCS.get(); | ||
| else if (settings.StereoBlendDebugMode == 2 && stereoBlendDebugBlendWeightCS) | ||
| activeCS = stereoBlendDebugBlendWeightCS.get(); | ||
| else if (settings.StereoBlendDebugMode == 3 && stereoBlendDebugEdgeDetectionCS) | ||
| activeCS = stereoBlendDebugEdgeDetectionCS.get(); | ||
| if (vrStereoOptActive) { | ||
| activeCS = stereoBlendOverwriteCS.get(); | ||
| } else { | ||
| int effectiveMode = settings.StereoBlendDebugMode; | ||
| if (effectiveMode == 1 && stereoBlendDebugBackCheckCS) | ||
| activeCS = stereoBlendDebugBackCheckCS.get(); | ||
| else if (effectiveMode == 2 && stereoBlendDebugBlendWeightCS) | ||
| activeCS = stereoBlendDebugBlendWeightCS.get(); | ||
| else if (effectiveMode == 3 && stereoBlendDebugEdgeDetectionCS) | ||
| activeCS = stereoBlendDebugEdgeDetectionCS.get(); | ||
| } |
There was a problem hiding this comment.
Wire the new debug mode values through this selector.
StereoBlendDebugMode now exposes "Overwrite" and "Overwrite Eye1", but this branch only handles modes 1-3. Selecting 4/5 currently falls back to the normal path, and when vrStereoOptActive is true the setting is ignored entirely. Either map those values to concrete shader variants here or remove the extra entries from the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VR/StereoBlend.cpp` around lines 98 - 109, The selector for
compute shaders ignores the new debug modes 4/5 and also bypasses settings when
vrStereoOptActive is true; update the logic around activeCS (using
stereoBlendCS, stereoBlendOverwriteCS, stereoBlendDebugBackCheckCS,
stereoBlendDebugBlendWeightCS, stereoBlendDebugEdgeDetectionCS and any new
shader variants you have for "Overwrite" and "Overwrite Eye1") so that
settings.StereoBlendDebugMode values 4 and 5 map to the correct shader variants
instead of falling back to defaults, and ensure that vrStereoOptActive doesn’t
unconditionally override those debug selections (or explicitly handle the debug
modes when vrStereoOptActive is true). If you don’t have shader variants for
4/5, remove those UI options instead.
There was a problem hiding this comment.
Minor issue. Debug modes 4/5 (Overwrite, Overwrite Eye1) are not separate shader variants — they use the same stereoBlendOverwriteCS shader with different cbuffer parameters. When vrStereoOptActive is true, the overwrite CS is always used and the debug mode selector doesn't apply. Will clarify the UI labels or remove the unused entries.
There was a problem hiding this comment.
@vrnord, thanks for the clarification! That makes sense — since modes 4/5 share stereoBlendOverwriteCS and are differentiated only via cbuffer state, the StereoBlendDebugMode selector is effectively a no-op when vrStereoOptActive is true.
Removing the UI entries (or gating them so they only appear when vrStereoOptActive is false) would be the cleanest resolution to avoid user confusion. Either way, appreciate the context!
ʕ •ᴥ•ʔ ✏️
✏️ Learnings added
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:25.118Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:37.608Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Learnt from: zndxcvbn
Repo: doodlum/skyrim-community-shaders PR: 1929
File: src/Utils/UI.cpp:55-57
Timestamp: 2026-02-22T14:39:02.543Z
Learning: In the Skyrim community shaders project, initialize display-related scaling values once at startup because the resolution cannot be changed at runtime. Do not recompute or refresh these values during a session; store them in a static or application-wide initialization path (e.g., at startup or in a startup singleton) to avoid stale scaling. This applies to UI-related initialization in source files like src/Utils/UI.cpp.
| if (!REL::Module::IsVR()) | ||
| return; | ||
| if (settings.stereoMode == StereoMode::Off) | ||
| return; | ||
| if (!reprojectionCS || !texPerPixelMode || !paramsCB) | ||
| return; | ||
| if (settings.debugSkipMerge) | ||
| return; |
There was a problem hiding this comment.
Always disarm the stencil hooks on early exits.
If DispatchStencil() has already armed the hooks and this function exits on StereoMode::Off, missing reprojection resources, or debugSkipMerge, stencilActive stays true and both context hooks keep rewriting later passes. That makes a debug path or shader compile failure capable of corrupting the rest of the frame.
Suggested fix
void VRStereoOptimizations::DispatchReprojection()
{
if (!REL::Module::IsVR())
return;
- if (settings.stereoMode == StereoMode::Off)
- return;
- if (!reprojectionCS || !texPerPixelMode || !paramsCB)
- return;
- if (settings.debugSkipMerge)
- return;
+ if (settings.stereoMode == StereoMode::Off) {
+ DeactivateStencil();
+ return;
+ }
+ if (!reprojectionCS || !texPerPixelMode || !paramsCB) {
+ DeactivateStencil();
+ return;
+ }
+ if (settings.debugSkipMerge) {
+ DeactivateStencil();
+ return;
+ }
@@
- stencilActive = false;
+ DeactivateStencil();Also applies to: 707-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VRStereoOptimizations.cpp` around lines 658 - 665, The
early-return paths in VRStereoOptimizations.cpp (inside the function that calls
DispatchStencil()) sometimes return without disarming stencil hooks, leaving
stencilActive true and hooks rewriting later passes; ensure that every early
exit first disables/unarms the stencil hooks by calling the existing disarm
routine (e.g., the function that clears stencilActive and removes the context
hooks — reference DispatchStencil(), stencilActive, and the disarm/unarm
function name used elsewhere) or convert the hook lifetime to an RAII guard so
hooks are always disarmed on scope exit; apply the same fix for the similar
block around lines ~707-720.
There was a problem hiding this comment.
Valid — this is a real bug. If DispatchReprojection() early-exits after DispatchStencil() has armed the hooks, stencilActive stays true and corrupts subsequent passes. Will add DeactivateStencil() to all early-exit paths.
There was a problem hiding this comment.
Rate Limit Exceeded
@vrnord have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 5 seconds before sending another message.
alandtse
left a comment
There was a problem hiding this comment.
Thanks.
- Consider why it's a separate feature. It's disfavored if we expect everyone to use it which something like this looks like.
- I see 2 and possibly 3 prs mixed together here. Break them up. I see general stereo optimization, some DLSS/TAA improvements, and foliage?
- Minimize churn and make sure you read every change you have when you submit a PR. There's a lot of review comments because you made this a huge PR.
- Please consider comments generally across the code base instead of where we tag it. We don't always catch repeat patterns.
| nearBlendToFar *= nearBlendToFar; | ||
| float offset = (1.0 - parallaxAmount) * -maxHeight + minHeight; | ||
| pixelOffset = lerp(parallaxAmount * scale, 0, nearBlendToFar); | ||
| pixelOffset = lerp(parallaxAmount, 0.5, nearBlendToFar); |
There was a problem hiding this comment.
This refers to all changes in this file how will this impact flatrim? We have to be very careful making changes without considering the impact if the item is shared. Was this intentional? Why? What does it do?
There was a problem hiding this comment.
Good catch — this change is for VR stereo reprojection depth correction (StereoBlendCS reads Reflectance.w to account for POM displacement). On flatrim, nothing consumes this channel for that purpose so it's functionally a no-op data change. However, you're right that it shouldn't modify shared behavior unnecessarily.
Action: Will gate the pixelOffset export behind #ifdef VR so flatrim codepaths are untouched. Will also add saturate() clamp per CodeRabbit's suggestion.
There was a problem hiding this comment.
Is there any situation where this wouldn't need to be installed for VR or disabled separately? Please note any CS features are likely to be demonetized by Nexus so there's minimal value to create a new feature to maintain a whole new Nexus page. If it will be part of the core, add it to the VR feature but you can have its files live in appropriate separate locations under VR to keep vr.cpp from becoming unwieldy.
There was a problem hiding this comment.
Agreed — this should not be a separate feature. It will always be needed for VR and there's no reason to maintain a separate Nexus page.
Action: Will fold VRStereoOptimizations into the existing VR feature. Files will live under features/VR/ and src/Features/VR/ to keep vr.cpp manageable, but settings and lifecycle will be part of the VR feature.
| float VRMipBias; // Additional negative MIP bias for VR foliage sharpening (depth-scaled) | ||
| float VRMipBiasNearDist; // Game units: no VR MIP bias closer than this | ||
| float VRMipBiasFarDist; // Game units: full VR MIP bias beyond this | ||
| uint VRMipBiasMode; // 0=Off, 1=All Textures, 2=Distant Trees (TREE_ANIM) only | ||
| float VRAlphaTestThreshold; // Alpha test threshold for VR TREE_ANIM (0 = disabled) |
There was a problem hiding this comment.
VR settings should not be part of a flat cbuffer if possible. I forgot if we have our own cbuffer to pass through but we should make it conditional on being in VR.
There was a problem hiding this comment.
Agreed. The VR MIP bias fields (VRMipBias, VRMipBiasNearDist, VRMipBiasFarDist, VRMipBiasMode, VRAlphaTestThreshold) waste cbuffer space on flatrim.
Action: Will move these to a VR-specific cbuffer or gate them so they don't occupy space in the shared SharedData cbuffer for SE/AE builds.
| // Larger offset increases chance of different alpha test outcomes between eyes | ||
| // (tree branches vs sky). NDC for 6304x3088 SBS reference; scales with resolution. | ||
| if (a_eyeIndex == 1) { | ||
| static const float2 kJitterNDC = float2(1.68e-4, -3.44e-4); |
There was a problem hiding this comment.
Given different HMDs, this is suspect. If there's no way to avoid this, then fine.
There was a problem hiding this comment.
This is a hardcoded ~0.75px diagonal jitter for Eye 1 to increase the chance of different alpha test outcomes between eyes (tree branches vs sky) for stereo edge supersampling. It's referenced to a 6304x3088 SBS resolution.
You're right that it's HMD-dependent. The NDC jitter should ideally scale with actual resolution rather than being a fixed constant. Will investigate making this resolution-adaptive or parameterizing it. If there's no clean way, we'll document the limitation.
| // Converts integer pixel coords to proper full-texture UV for SampleLevel, | ||
| // clamped to the active DRS viewport to prevent sampling stale data. | ||
| // Motion vectors stay as integer Load() — filtering them breaks DLSS. | ||
| float4 SampleReprojectedColor(int2 reprojPx, float2 frameDim) |
There was a problem hiding this comment.
For any inline function, consider whether it's best in a shared library for others to use too.
There was a problem hiding this comment.
Good suggestion. The bilinear sampling helper (SampleReprojectedColor) does DRS viewport clamping + bilinear interpolation which could be useful for other VR passes. Will consider moving it to VR.hlsli or a shared VR utility header.
| if (upscaleMethod != UpscaleMethod::kNONE && upscaleMethod != UpscaleMethod::kTAA) | ||
| upscaling.PerformUpscaling(); | ||
| // Increment diagnostic counter (rate-limits TAAReorder logging) | ||
| if (TAAReorder::g_initialized) { |
There was a problem hiding this comment.
Is this VR gated? Is this something that can be put into a VR specific function?
There was a problem hiding this comment.
This section (EncodeTextures in Main_PostProcessing) runs for both flat and VR — it encodes TAA mask, normals, motion vectors, and depth for the upscaler. The VR-specific additions (per-eye intermediate texture setup, viewport scaling) are gated behind `if (globals::game::isVR)` checks within the function. Will verify all VR-specific logic has proper guards and evaluate moving VR-specific code to a helper.
| uint useGatherWideKernel = 1; // 0=Legacy 3x3, 1=Gather wide-kernel | ||
| uint presetDLSS = 0; // 0=Default, 1=J, 2=K, 3=L, 4=M | ||
| uint useGatherWideKernel = 1; // 0=Legacy 3x3, 1=Gather wide-kernel | ||
| float vrDlssViewportScale = 1.0f; // 0.5 to 1.0, fraction of each eye that DLSS processes (VR only) |
There was a problem hiding this comment.
Same question on whether it should be passed through during flat. may not be an issue here though.
There was a problem hiding this comment.
The VR settings (`vrDlssViewportScale`, `vrPeripheryTAA`, etc.) are serialized in the settings JSON but only used when `isVR` is true at runtime. On flatrim they're inert data — no GPU resources created, no UI shown, no runtime cost. Could add compile-time `#ifdef ENABLE_SKYRIM_VR` guards but given this is a multi-runtime (ALL) build, runtime gating is the standard pattern.
| virtual void Load() override; | ||
| virtual void PostPostLoad() override; | ||
| virtual void SetupResources() override; | ||
| virtual std::vector<FeatureConstraints::Constraint> GetActiveConstraints() const override; |
There was a problem hiding this comment.
The `PostPostLoad()` override was added as a lifecycle hook for late initialization that needs to run after all features have loaded. If it's empty or unnecessary, will remove it.
| eastl::unique_ptr<Texture2D> vrCropColorIn[2]; // crop-sized DLSS color input (VR viewport scaling only) | ||
|
|
||
| // Periphery TAA (conductor approach) — used by two-call func() flow | ||
| winrt::com_ptr<ID3D11Texture2D> vrPreTAACopy; // full stereo kMAIN copy (Phase 1 PP, pre-TAA) |
There was a problem hiding this comment.
I realize we may have already started by why the mix of different pointer types?
There was a problem hiding this comment.
The mix is inherited from the existing codebase pattern:
- `eastl::unique_ptr` — for textures that need SRV/UAV wrappers (Texture2D is the CS helper class)
- `winrt::com_ptr` — for raw D3D resources that only need CopyResource/CopySubresourceRegion (no SRV/UAV needed)
We followed the existing conventions but agree it's inconsistent. Can standardize on `Texture2D` wrappers if preferred.
| data.MipBias = 0; | ||
| } | ||
|
|
||
| // VR MIP bias modes: 1 = All (global), 2 = Distant Trees (per-pixel in TREE_ANIM) |
There was a problem hiding this comment.
We should not be modifying core files unless absolutely unavoidable through the feature boundary. also VR gate it so it doesn't impact flat.
There was a problem hiding this comment.
Agreed — State.cpp is a core file. The VR MIP bias population (lines 854-868) is already gated behind `if (globals::game::isVR)`, but ideally VR-specific settings shouldn't be set in State::UpdateSharedData at all.
Action: Will evaluate moving VR constant buffer population to the VR feature's own update path that writes to a VR-specific cbuffer, avoiding modification of core State.cpp. This ties into the SharedData.hlsli comment about moving VR fields out of the shared cbuffer.
…hanges Review fixes: - DeferredCompositeCS: only early-out MODE_MAIN (mode==2), not MODE_EDGE - CASCS: add max(mxRGB, 1e-4) guard against division by zero - StereoBlendCS: clamp POM-corrected depth to prevent div-by-zero - StencilCS: use cbuffer DisocclusionThreshold instead of hardcoded constant - VR.hlsli: make Eye 1 jitter resolution-adaptive via FrameBuffer::BufferDim - Lighting.hlsl: include TRUE_PBR in pixelOffset export guard - ExtendedMaterials.hlsli: saturate() clamp on pixelOffset output - VRStereoOptimizations.cpp: add DeactivateStencil() to early-exit paths - VRStereoOptimizations.cpp: add null check for depthSRV - Globals.cpp: scope ClearDepthStencilView hook to main scene DSV only - StereoBlend.cpp: use terrain-aware Util::GetCurrentSceneDepthSRV() Revert Upscaling.cpp/h to upstream dev — all DLSS viewport scaling, TAAReorder, and periphery TAA code belongs exclusively in PR2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
500-500:⚠️ Potential issue | 🟠 MajorGate the new
pixelOffsetencoding to VR-only paths.Line 500 applies a VR-style midpoint encoding globally. Since
Reflectance.wis consumed in non-VR paths too (package/Shaders/Lighting.hlslLines 3199-3205), this risks flatrim behavior drift. Keep the new mapping for VR and preserve legacy mapping for non-VR.🔧 Proposed fix
- pixelOffset = saturate(lerp(parallaxAmount, 0.5, nearBlendToFar)); +#if defined(VR) + // VR reprojection depth encoding: 0.5 is neutral + pixelOffset = saturate(lerp(parallaxAmount, 0.5, nearBlendToFar)); +#else + // Preserve legacy non-VR behavior + pixelOffset = saturate(lerp(parallaxAmount * scale, 0.0, nearBlendToFar)); +#endifBased on learnings: "Flag potential problems proactively including performance impact on rendering, SE/AE/VR runtime compatibility issues, GPU register conflicts, and security risks from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli at line 500, Gate the new pixelOffset encoding so it only applies to VR paths: around the assignment to pixelOffset (using parallaxAmount and nearBlendToFar) add a VR-only conditional (compile-time macro like XR_ENABLED/VR_RENDERING or a runtime isVR flag used by your engine) and leave the legacy pixelOffset mapping for non-VR consumers (so Reflectance.w used by Lighting.hlsl remains unchanged). Update ExtendedMaterials.hlsli to set pixelOffset = saturate(lerp(parallaxAmount, 0.5, nearBlendToFar)) only when the VR flag is true and otherwise restore the original/legacy pixelOffset assignment; also add a short comment referencing Reflectance.w to make the intent explicit.package/Shaders/Lighting.hlsl (1)
3200-3205:⚠️ Potential issue | 🟠 MajorAdd TRUE_PBR to the pixelOffset export guard.
The guard at lines 3200-3205 currently requires
PARALLAXorLANDSCAPE, but the TRUE_PBR parallax code block (lines 1113–1142) is guarded only by#if defined(TRUE_PBR) && !defined(LANDSCAPE)without requiringPARALLAX. This means TRUE_PBR materials can computepixelOffsetviaGetParallaxCoords(line 1139) without thePARALLAXdefine, but the export guard will fail andpixelOffsetwon't be written to the output.The export condition should be updated to:
-# if (defined(EMAT) || defined(TRUE_PBR)) && (defined(PARALLAX) || defined(LANDSCAPE)) +# if (defined(EMAT) || defined(TRUE_PBR)) && (defined(PARALLAX) || defined(LANDSCAPE) || defined(TRUE_PBR))Or equivalently:
-# if (defined(EMAT) || defined(TRUE_PBR)) && (defined(PARALLAX) || defined(LANDSCAPE)) +# if defined(TRUE_PBR) || (defined(EMAT) && (defined(PARALLAX) || defined(LANDSCAPE)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 3200 - 3205, The pixelOffset export guard around psout.Reflectance needs TRUE_PBR included so TRUE_PBR-only paths that call GetParallaxCoords can write pixelOffset; update the preprocessor condition that controls writing psout.Reflectance (the `#if` wrapping psout.Reflectance and using pixelOffset and indirectLobeWeights.specular) to include TRUE_PBR in the second clause (the (PARALLAX || LANDSCAPE) part) so that the condition allows export when TRUE_PBR is defined as well.src/Features/VRStereoOptimizations.cpp (2)
400-427:⚠️ Potential issue | 🟡 MinorDispatchStencil early-exit is incomplete — missing write-pass resource checks.
The guard at line 406 checks
stencilCS,stencilWriteVS,stencilWritePS,texPerPixelMode,paramsCB, butExecuteStencilWritePass()at line 457 also requires:
stencilWriteReadOnlyDSVstencilWriteDSSstencilWriteRSIf any of these are null (e.g., DSV creation failed at line 180-182), the write pass will fail.
🛡️ Proposed fix to add missing resource checks
void VRStereoOptimizations::DispatchStencil() { if (!REL::Module::IsVR()) return; if (settings.stereoMode == StereoMode::Off) return; - if (!stencilCS || !stencilWriteVS || !stencilWritePS || !texPerPixelMode || !paramsCB) + if (!stencilCS || !stencilWriteVS || !stencilWritePS || !texPerPixelMode || !paramsCB || + !stencilWriteReadOnlyDSV || !stencilWriteDSS || !stencilWriteRS) { + DeactivateStencil(); return; + }As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 400 - 427, The DispatchStencil early-exit is missing checks for resources needed by ExecuteStencilWritePass; update VRStereoOptimizations::DispatchStencil to also verify stencilWriteReadOnlyDSV, stencilWriteDSS, and stencilWriteRS are non-null before proceeding (in addition to existing checks for stencilCS, stencilWriteVS, stencilWritePS, texPerPixelMode, paramsCB), and if any are null log a warning and return to avoid executing ExecuteStencilWritePass with incomplete D3D11 state.
664-665:⚠️ Potential issue | 🟡 MinorMissing DeactivateStencil on StereoMode::Off early exit.
If
DispatchStencil()armed the hooks and the user toggles the mode toOffvia ImGui beforeDispatchReprojection()runs, this exit path leavesstencilActive = true. The DSS hooks would remain armed until the next frame's reset.🛡️ Proposed fix to disarm on mode change
if (settings.stereoMode == StereoMode::Off) + { + DeactivateStencil(); return; + }This is an edge case (mid-frame settings change) but ensures consistent hook state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 664 - 665, The early return when settings.stereoMode == StereoMode::Off leaves the DSS hooks armed; before returning from the function containing that check (in VRStereoOptimizations::DispatchReprojection or the function shown), call DeactivateStencil() to disarm the hooks and clear stencilActive so hooks aren't left armed mid-frame; ensure you call DeactivateStencil() (and any related state resets) immediately before the early return that currently checks settings.stereoMode == StereoMode::Off, preserving existing control flow otherwise.
🧹 Nitpick comments (3)
package/Shaders/DeferredCompositeCS.hlsl (1)
100-106: Minor cleanup suggestions for maintainability.The logic is correct — only
MODE_MAIN(2) Eye 1 pixels should early-exit since they have no valid G-buffer.Two optional refinements:
- Magic number: Consider including
VRStereoOptimizations/cbuffers.hlslior defining a local constant to referenceMODE_MAINsymbolically instead of the raw literal2.- Redundant cast:
dispatchID.xyis alreadyuint2(fromuint3), so the explicituint2()wrapper is unnecessary.♻️ Proposed cleanup
`#if` defined(VR_STEREO_OPT) if (eyeIndex == 1) { - uint mode = StereoOptModeTexture[uint2(dispatchID.xy)]; - if (mode == 2) { // MODE_MAIN — stencil-culled, no valid G-buffer + uint mode = StereoOptModeTexture[dispatchID.xy]; + if (mode == 2) { // MODE_MAIN — stencil-culled, no valid G-buffer return; } } `#endif`If you want to use the symbolic constant:
`#if` defined(VR_STEREO_OPT) # include "VRStereoOptimizations/cbuffers.hlsli" `#endif` // ... if (mode == MODE_MAIN) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 100 - 106, The early-exit uses a magic literal and an unnecessary cast: inside the VR_STEREO_OPT block, replace the numeric literal 2 with the symbolic constant MODE_MAIN (import it by including VRStereoOptimizations/cbuffers.hlsli when VR_STEREO_OPT is defined) and remove the redundant uint2(...) wrapper around dispatchID.xy when sampling StereoOptModeTexture; update the conditional to compare mode against MODE_MAIN and sample StereoOptModeTexture with dispatchID.xy directly to improve readability and maintainability (references: VR_STEREO_OPT, StereoOptModeTexture, dispatchID, MODE_MAIN).package/Shaders/VR/StereoBlendCS.hlsl (1)
226-233: Skip Eye 1 POM re-reprojection when depth scaling is disabledLine 226 triggers a second reprojection for POM-tagged pixels even when
POMDepthScaleis effectively off. Gate this branch to avoid unnecessary VR compute work.♻️ Proposed fix
- if (pomOffset > 1e-2) { + if (pomOffset > 1e-2 && abs(POMDepthScale) > 1e-5) {Based on learnings: Flag potential problems proactively including performance impact on rendering, SE/AE/VR runtime compatibility issues, GPU register conflicts, and security risks from malformed configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/StereoBlendCS.hlsl` around lines 226 - 233, The POM re-reprojection branch currently runs whenever pomOffset > 1e-2 even if POMDepthScale is effectively disabled, wasting VR compute; update the condition guarding the block that computes linearDepth, depthCorrection, newLinearDepth, reprojDepth and calls Stereo::ReprojectToOtherEye (the r = Stereo::ReprojectToOtherEye(...) / if (!r.valid branch) to also check that POMDepthScale is non-zero (or above a tiny epsilon) so the code skips reprojecting when depth scaling is disabled; locate the pomOffset check and add a guard using POMDepthScale (or fabs(POMDepthScale) > 1e-6) to avoid unnecessary work for eyeIndex 1 POM pixels.src/Features/VR/StereoBlend.cpp (1)
172-181: DSV restore pattern is correct but could be cleaner.The restore logic acquires RTVs a second time to preserve whatever may have changed during dispatch. This works but adds a subtle asymmetry with the save pattern at lines 110-120.
♻️ Optional: Simplify by reusing saved RTVs if dispatch doesn't modify them
If the CS dispatch never modifies RTVs (which compute shaders don't), you could skip the second
OMGetRenderTargetsand just passnullptrfor RTVs since only the DSV needs restoring:// Restore DSV after CS dispatch in overwrite mode if (isOverwriteMode && savedDSV) { - context->OMGetRenderTargets(D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, savedRTVs, nullptr); - context->OMSetRenderTargets(D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, savedRTVs, savedDSV); - for (auto& rtv : savedRTVs) { - if (rtv) - rtv->Release(); - } + // RTVs unchanged by CS; just rebind the saved DSV + ID3D11RenderTargetView* nullRTVs[D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT] = {}; + context->OMSetRenderTargets(0, nullptr, savedDSV); savedDSV->Release(); }However, the current approach is safer if future changes bind RTVs during dispatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VR/StereoBlend.cpp` around lines 172 - 181, The restore block for DSV in StereoBlend.cpp redundantly calls OMGetRenderTargets a second time; update the logic to reuse the previously saved RTV pointers (savedRTVs) instead of re-querying them or, if you guarantee CS never touches RTVs, call OMSetRenderTargets with nullptr for RTVs and only restore savedDSV; modify the branch guarded by isOverwriteMode to call context->OMSetRenderTargets with the existing savedRTVs (or nullptr) and then Release the savedRTVs and savedDSV, ensuring you still Release each non-null savedRTV and savedDSV after restoring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/Common/VR.hlsli`:
- Around line 630-638: The Eye 1 screen-space jitter (kJitterNDC applied to
vsout.VRPosition in the projection path) must also be applied to the
reprojection helpers so reprojection math uses the same per-eye projection;
update the stereo reprojection to consume the same jitter rather than only
adjusting post-projection. Concretely, move or mirror the jitter into the shared
projection stage (or compute a per-eye projection offset) and pass that jitter
or adjusted projection into ConvertMonoUVToOtherEye and FinalizeStereoBlend (or
apply the equivalent NDC offset inside those functions) so UV round-trips and
bilateral depth checks use the identical Eye 1 offset as used for
vsout.VRPosition. Ensure the symbol names referenced are kJitterNDC,
vsout.VRPosition, ConvertMonoUVToOtherEye, and FinalizeStereoBlend.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 124-127: The proximity normalization for DebugMode 2 currently
uses max(FullBlendDistance, 1.0) which warps results when FullBlendDistance <
1.0; update the proximity calculation in the block referencing linDepth and
FullBlendDistance (the line computing proximity = saturate(1.0 - linDepth /
max(FullBlendDistance, 1.0))) to clamp the denominator with a small epsilon
(e.g., max(FullBlendDistance, 1e-6) or similar) instead of 1.0 so the tint
scales correctly for sub-1.0 distances while preserving stability against
division-by-zero.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 617-654: The dssCache currently keys on raw
ID3D11DepthStencilState* and is never cleared, risking stale matches when the
game releases/reallocates DSS objects; update VRStereoOptimizations to either
(A) key the cache by a stable D3D11_DEPTH_STENCIL_DESC hash instead of the raw
pointer (compute a size_t hash of the desc struct and map size_t ->
winrt::com_ptr<ID3D11DepthStencilState>), or (B) at minimum clear dssCache from
ClearShaderCache() and whenever device/context resources are recreated (ensure
ClearShaderCache() now clears the dssCache unordered_map), and replace
raw-pointer keys with winrt::com_ptr or description-based keys so
GetOrCreateModifiedDSS no longer returns modified DSS for a different real
object.
---
Duplicate comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 500: Gate the new pixelOffset encoding so it only applies to VR paths:
around the assignment to pixelOffset (using parallaxAmount and nearBlendToFar)
add a VR-only conditional (compile-time macro like XR_ENABLED/VR_RENDERING or a
runtime isVR flag used by your engine) and leave the legacy pixelOffset mapping
for non-VR consumers (so Reflectance.w used by Lighting.hlsl remains unchanged).
Update ExtendedMaterials.hlsli to set pixelOffset =
saturate(lerp(parallaxAmount, 0.5, nearBlendToFar)) only when the VR flag is
true and otherwise restore the original/legacy pixelOffset assignment; also add
a short comment referencing Reflectance.w to make the intent explicit.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3200-3205: The pixelOffset export guard around psout.Reflectance
needs TRUE_PBR included so TRUE_PBR-only paths that call GetParallaxCoords can
write pixelOffset; update the preprocessor condition that controls writing
psout.Reflectance (the `#if` wrapping psout.Reflectance and using pixelOffset and
indirectLobeWeights.specular) to include TRUE_PBR in the second clause (the
(PARALLAX || LANDSCAPE) part) so that the condition allows export when TRUE_PBR
is defined as well.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 400-427: The DispatchStencil early-exit is missing checks for
resources needed by ExecuteStencilWritePass; update
VRStereoOptimizations::DispatchStencil to also verify stencilWriteReadOnlyDSV,
stencilWriteDSS, and stencilWriteRS are non-null before proceeding (in addition
to existing checks for stencilCS, stencilWriteVS, stencilWritePS,
texPerPixelMode, paramsCB), and if any are null log a warning and return to
avoid executing ExecuteStencilWritePass with incomplete D3D11 state.
- Around line 664-665: The early return when settings.stereoMode ==
StereoMode::Off leaves the DSS hooks armed; before returning from the function
containing that check (in VRStereoOptimizations::DispatchReprojection or the
function shown), call DeactivateStencil() to disarm the hooks and clear
stencilActive so hooks aren't left armed mid-frame; ensure you call
DeactivateStencil() (and any related state resets) immediately before the early
return that currently checks settings.stereoMode == StereoMode::Off, preserving
existing control flow otherwise.
---
Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-106: The early-exit uses a magic literal and an unnecessary
cast: inside the VR_STEREO_OPT block, replace the numeric literal 2 with the
symbolic constant MODE_MAIN (import it by including
VRStereoOptimizations/cbuffers.hlsli when VR_STEREO_OPT is defined) and remove
the redundant uint2(...) wrapper around dispatchID.xy when sampling
StereoOptModeTexture; update the conditional to compare mode against MODE_MAIN
and sample StereoOptModeTexture with dispatchID.xy directly to improve
readability and maintainability (references: VR_STEREO_OPT,
StereoOptModeTexture, dispatchID, MODE_MAIN).
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 226-233: The POM re-reprojection branch currently runs whenever
pomOffset > 1e-2 even if POMDepthScale is effectively disabled, wasting VR
compute; update the condition guarding the block that computes linearDepth,
depthCorrection, newLinearDepth, reprojDepth and calls
Stereo::ReprojectToOtherEye (the r = Stereo::ReprojectToOtherEye(...) / if
(!r.valid branch) to also check that POMDepthScale is non-zero (or above a tiny
epsilon) so the code skips reprojecting when depth scaling is disabled; locate
the pomOffset check and add a guard using POMDepthScale (or fabs(POMDepthScale)
> 1e-6) to avoid unnecessary work for eyeIndex 1 POM pixels.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 172-181: The restore block for DSV in StereoBlend.cpp redundantly
calls OMGetRenderTargets a second time; update the logic to reuse the previously
saved RTV pointers (savedRTVs) instead of re-querying them or, if you guarantee
CS never touches RTVs, call OMSetRenderTargets with nullptr for RTVs and only
restore savedDSV; modify the branch guarded by isOverwriteMode to call
context->OMSetRenderTargets with the existing savedRTVs (or nullptr) and then
Release the savedRTVs and savedDSV, ensuring you still Release each non-null
savedRTV and savedDSV after restoring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78f2fc63-859e-445e-908f-e254550cd9ac
📒 Files selected for processing (10)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/VR/CASCS.hlslpackage/Shaders/VR/StereoBlendCS.hlslpackage/Shaders/VRStereoOptimizations/StencilCS.hlslsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- package/Shaders/VR/CASCS.hlsl
| // Hardcoded ~0.75px diagonal jitter for Eye 1 stereo edge supersampling. | ||
| // Larger offset increases chance of different alpha test outcomes between eyes | ||
| // (tree branches vs sky). NDC for 6304x3088 SBS reference; scales with resolution. | ||
| if (a_eyeIndex == 1) { | ||
| // ~0.75px diagonal jitter for Eye 1 stereo edge supersampling. | ||
| // Scales with resolution: 0.53/halfWidth horizontal, 1.06/height vertical. | ||
| float2 kJitterNDC = float2(0.53 / (FrameBuffer::BufferDim.x * 0.5), -1.06 / FrameBuffer::BufferDim.y); | ||
| vsout.VRPosition.xy += kJitterNDC * vsout.VRPosition.w; | ||
| } |
There was a problem hiding this comment.
Propagate the Eye 1 jitter into the reprojection math too.
Line 637 adds a post-projection screen-space offset only on Eye 1, but ConvertMonoUVToOtherEye and the round-trip in FinalizeStereoBlend still assume the unjittered per-eye projection. That creates a fixed subpixel bias whenever Eye 1 participates, which can perturb depth taps and back-checks in the stereo bilateral path. Please make this jitter a shared projection term, or reuse the same adjustment in the UV reprojection helpers as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Common/VR.hlsli` around lines 630 - 638, The Eye 1
screen-space jitter (kJitterNDC applied to vsout.VRPosition in the projection
path) must also be applied to the reprojection helpers so reprojection math uses
the same per-eye projection; update the stereo reprojection to consume the same
jitter rather than only adjusting post-projection. Concretely, move or mirror
the jitter into the shared projection stage (or compute a per-eye projection
offset) and pass that jitter or adjusted projection into ConvertMonoUVToOtherEye
and FinalizeStereoBlend (or apply the equivalent NDC offset inside those
functions) so UV round-trips and bilateral depth checks use the identical Eye 1
offset as used for vsout.VRPosition. Ensure the symbol names referenced are
kJitterNDC, vsout.VRPosition, ConvertMonoUVToOtherEye, and FinalizeStereoBlend.
| if (linDepth < FullBlendDistance) { | ||
| float4 c = ColorTexture[dtid]; | ||
| float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0)); | ||
| OutputRW[dtid] = float4(lerp(c.rgb, float3(0, 1, 1), proximity * 0.4), c.a); |
There was a problem hiding this comment.
Fix DebugMode 2 proximity normalization for sub-1.0 distances
Line 126 clamps the denominator with max(FullBlendDistance, 1.0), which distorts the visualizer when FullBlendDistance < 1.0. Use an epsilon clamp instead so the tint reflects the configured distance.
🔧 Proposed fix
- float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0));
+ float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1e-5));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/VR/StereoBlendCS.hlsl` around lines 124 - 127, The proximity
normalization for DebugMode 2 currently uses max(FullBlendDistance, 1.0) which
warps results when FullBlendDistance < 1.0; update the proximity calculation in
the block referencing linDepth and FullBlendDistance (the line computing
proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0))) to clamp the
denominator with a small epsilon (e.g., max(FullBlendDistance, 1e-6) or similar)
instead of 1.0 so the tint scales correctly for sub-1.0 distances while
preserving stability against division-by-zero.
| ID3D11DepthStencilState* VRStereoOptimizations::GetOrCreateModifiedDSS(ID3D11DepthStencilState* originalDSS) | ||
| { | ||
| if (!originalDSS || !stencilActive) | ||
| return originalDSS; | ||
|
|
||
| stencilSwapCount++; | ||
|
|
||
| auto it = dssCache.find(originalDSS); | ||
| if (it != dssCache.end()) | ||
| return it->second.get(); | ||
|
|
||
| // Clone original desc and add read-only stencil NOT_EQUAL test | ||
| D3D11_DEPTH_STENCIL_DESC desc{}; | ||
| originalDSS->GetDesc(&desc); | ||
|
|
||
| desc.StencilEnable = TRUE; | ||
| desc.StencilReadMask = 0xFF; | ||
| desc.StencilWriteMask = 0x00; // Read-only: game rendering must not modify our marks | ||
|
|
||
| // NOT_EQUAL with ref=1: skip pixels where stencil == 1 (MODE_MAIN) | ||
| desc.FrontFace.StencilFunc = D3D11_COMPARISON_NOT_EQUAL; | ||
| desc.FrontFace.StencilFailOp = D3D11_STENCIL_OP_KEEP; | ||
| desc.FrontFace.StencilDepthFailOp = D3D11_STENCIL_OP_KEEP; | ||
| desc.FrontFace.StencilPassOp = D3D11_STENCIL_OP_KEEP; | ||
| desc.BackFace = desc.FrontFace; | ||
|
|
||
| winrt::com_ptr<ID3D11DepthStencilState> modifiedDSS; | ||
| HRESULT hr = globals::d3d::device->CreateDepthStencilState(&desc, modifiedDSS.put()); | ||
| if (FAILED(hr)) { | ||
| logger::warn("[VRStereoOptimizations] Failed to create modified DSS (HRESULT: {:#x})", static_cast<uint32_t>(hr)); | ||
| return originalDSS; | ||
| } | ||
|
|
||
| auto* result = modifiedDSS.get(); | ||
| dssCache[originalDSS] = std::move(modifiedDSS); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other examples of DSS caching in the codebase
rg -n "unordered_map.*DepthStencilState" --type cppRepository: doodlum/skyrim-community-shaders
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Check if ClearShaderCache() actually clears dssCache
rg -n "ClearShaderCache" --type cpp -A 10 | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Look for Reset() or cleanup methods in VRStereoOptimizations
rg -n "void.*Reset|void.*Clear" src/Features/VRStereoOptimizations.cpp --type cpp -A 5Repository: doodlum/skyrim-community-shaders
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Check how GetOrCreateModifiedDSS is called and frequency
rg -n "GetOrCreateModifiedDSS" --type cpp -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 1341
🏁 Script executed:
#!/bin/bash
# Look for other pointer-based caches in the codebase to see patterns
rg -n "unordered_map.*\*.*com_ptr|unordered_map.*\*" src/Features --type cpp | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 336
DSS cache uses raw pointers as keys with no eviction — stale reference risk is real.
The cache maps original ID3D11DepthStencilState* to modified versions. If the game releases and reallocates a DSS object at the same address with different settings, the cache would return an incorrectly modified DSS.
The mitigation mentioned in previous analysis is incomplete: ClearShaderCache() only clears shader pointers, not dssCache. The cache persists across shader reloads and the entire session, making stale pointer reuse a practical concern rather than a theoretical edge case.
♻️ Consider adding cache validation or using description-based keys
Option 1: Clear cache per-frame (simple but adds overhead):
void VRStereoOptimizations::Reset()
{
stencilActive = false;
stencilSwapCount = 0;
+ dssCache.clear(); // Prevent stale entries across frames
}Option 2: Use DSS description as key instead of pointer (more robust):
// Hash D3D11_DEPTH_STENCIL_DESC and use as key
std::unordered_map<size_t, winrt::com_ptr<ID3D11DepthStencilState>> dssCache;Option 3: Clear cache in ClearShaderCache():
void VRStereoOptimizations::ClearShaderCache()
{
stencilCS = nullptr;
stencilDebugDepthMapCS = nullptr;
stencilWriteVS = nullptr;
stencilWritePS = nullptr;
+ dssCache.clear();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VRStereoOptimizations.cpp` around lines 617 - 654, The dssCache
currently keys on raw ID3D11DepthStencilState* and is never cleared, risking
stale matches when the game releases/reallocates DSS objects; update
VRStereoOptimizations to either (A) key the cache by a stable
D3D11_DEPTH_STENCIL_DESC hash instead of the raw pointer (compute a size_t hash
of the desc struct and map size_t -> winrt::com_ptr<ID3D11DepthStencilState>),
or (B) at minimum clear dssCache from ClearShaderCache() and whenever
device/context resources are recreated (ensure ClearShaderCache() now clears the
dssCache unordered_map), and replace raw-pointer keys with winrt::com_ptr or
description-based keys so GetOrCreateModifiedDSS no longer returns modified DSS
for a different real object.
The resolution-adaptive jitter used FrameBuffer::BufferDim which is defined in SharedData.hlsli, not FrameBuffer.hlsli. The VSHADER section of VR.hlsli only includes FrameBuffer.hlsli, causing every vertex shader to fail with "undeclared identifier 'FrameBuffer::BufferDim'". Revert to hardcoded NDC values (1.68e-4, -3.44e-4) which give ~0.75px offset at the 6304x3088 SBS reference resolution. Since NDC space is inherently resolution-relative, these constants scale proportionally at other resolutions without needing an explicit BufferDim lookup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✅ A pre-release build is available for this PR: |
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Superseded by split PRs per maintainer feedback:
All review feedback from this PR has been addressed in #2002. |
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stencil-based Eye 1 culling with compute shader reprojection, folded into VR feature per maintainer request (not standalone). Core features: - Hardware stencil classification + NOT_EQUAL enforcement - Bilinear color sampling for stereo reprojection - POM depth-aware reprojection via Reflectance.w - StereoBlend overwrite mode for culled pixels - DeactivateStencil() on all early-exit paths Cleanup (PR community-shaders#1982 review feedback addressed): - SharedData cbuffer: VR mip bias fields removed, pad adjusted - Eye 1 sub-pixel jitter removed - Foliage fringe suppression removed - CAS feature fully removed - pixelOffset TRUE_PBR export guard added - pixelOffset parallax fade-out discontinuity fixed - JSON settings loading hardened - Stencil dispatch guards for missing DSV resources - Dead code removed (BILINEAR_UPSCALE, vrTAAdPerEye, vrPreTAACopy) Replaces PR community-shaders#1982. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Details
Stencil Culling & CAS (83769b0)
Core VR stereo optimizations feature adding hardware stencil-based culling to skip Eye 1 pixels during deferred composite (saving ~30% GPU on that pass), CAS contrast-adaptive sharpening for reprojected pixels, and a DLSS viewport fix that was causing crashes in VR.
Bilinear Color Sampling (bd1c480)
Upgrades StereoBlendCS from point sampling to bilinear interpolation when fetching reprojected color. This smooths subpixel-accuracy reprojection instead of snapping to nearest texel.
POM Depth-Aware Reprojection (b130bf5)
Encodes parallax occlusion mapping depth offset into an unused channel (Reflectance.w) during Lighting.hlsl, then reads it back in StereoBlendCS to adjust the reprojection disparity. This prevents ghosting/smearing on surfaces with strong parallax displacement.
Struct Alignment Fixes (19edb70, 39d26c6, f58e0c6)
Merge conflict resolution for SharedData cbuffer after upstream added SH ambient fields — corrects CPMSettings alignment, SharedDataCB padding (
float4 pad0to match HLSL layout), and a missing semicolon.Test plan
REL::Module::IsVR())🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
UI