feat(vr): Screen Space Shadows VR optimization#2004
Draft
vrnord wants to merge 3 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive VR stereo optimization system combining stencil-based culling and reprojection with modifications to screen-space shadows, extended materials, and various shader color-space conversions. The system classifies per-pixel stereo modes, writes stencil patterns for hardware culling, and performs bilateral depth-weighted reprojection to optimize stereo rendering performance. Changes
Sequence DiagramsequenceDiagram
participant CPU as CPU/Main Thread
participant Deferred as Deferred.cpp
participant Stencil as StencilCS<br/>(GPU)
participant StencilWrite as StencilWrite<br/>VS/PS (GPU)
participant Composite as CompositeCS<br/>(GPU)
participant StereoBlend as StereoBlendCS<br/>(GPU)
participant Reprojection as ReprojectionCS<br/>(GPU)
participant DSV as Hardware<br/>Depth-Stencil
CPU->>Deferred: StartDeferred()
Deferred->>Stencil: DispatchStencil()
Stencil->>Stencil: Read depth from DepthTexture
Stencil->>Stencil: Classify pixels into modes<br/>(DISOCCLUDED, EDGE, MAIN, etc.)
Stencil->>Stencil: Write ModeTexture output
Stencil->>StencilWrite: Trigger stencil write pass
StencilWrite->>StencilWrite: Read mode & depth
StencilWrite->>StencilWrite: Discard/allow per pixel
StencilWrite->>DSV: Write stencil ref=1 pattern
Deferred->>Composite: DeferredCompositeCS dispatch
Composite->>Composite: Check StereoOptModeTexture
Composite->>Composite: Early-return if mode==1 or 2
Composite->>Composite: Otherwise compose deferred lighting
Deferred->>StereoBlend: DrawStereoBlend()
note over StereoBlend: VR Stereo Optimization Active
StereoBlend->>StereoBlend: Bilateral blend (Eye 0/1 samples)
StereoBlend->>StereoBlend: Depth-weighted color interpolation
StereoBlend->>StereoBlend: Apply edge tinting & debug modes
Deferred->>Reprojection: DispatchReprojection()
Reprojection->>Reprojection: Reproject Eye 0→Eye 1<br/>for stencil-skipped pixels
Reprojection->>Reprojection: Read mode & validate
Reprojection->>Reprojection: Copy/blend Eye 0 color to Eye 1
Reprojection->>DSV: Stencil-culled writes
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)
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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/VR/SettingsUI.cpp (1)
326-345:⚠️ Potential issue | 🟡 MinorMissing tooltip descriptions for new debug modes.
The
debugModesarray includes "Overwrite" and "Overwrite Eye1", but the tooltip text (lines 329-344) doesn't document what these modes visualize. Users will see the options but have no guidance on their purpose.Consider adding descriptions similar to the existing modes, e.g.:
"Edge Detection: Highlights pixels excluded by depth discontinuity checks.\n" " Yellow = source edge (discontinuity at this pixel).\n" " Orange = destination edge (discontinuity at reprojected pixel).\n" - " Scene = all other pixels shown with normal blending."); + " Scene = all other pixels shown with normal blending.\n\n" + "Overwrite: Visualizes VR stereo optimization overwrite mode.\n" + "Overwrite Eye1: Same as Overwrite, Eye 1 only.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VR/SettingsUI.cpp` around lines 326 - 345, Add missing tooltip descriptions for the "Overwrite" and "Overwrite Eye1" entries in the debugModes array so users understand their behavior: update the ImGui::Text block inside the Util::HoverTooltipWrapper() (the same place that documents "Off", "Back-Check", "Blend Weight", and "Edge Detection") to append concise lines explaining "Overwrite" (e.g., forcefully displays the corrected/reprojected result, showing where full overwrite occurs) and "Overwrite Eye1" (e.g., same as Overwrite but only applies overwrite to eye 1 for comparison), ensuring the text references settings.StereoBlendDebugMode context so the tooltip covers all modes shown by debugModes.
🧹 Nitpick comments (9)
package/Shaders/DistantTree.hlsl (1)
206-214: LGTM! VR per-eye alpha threshold adjustment for stereo convergence.The pattern correctly reduces
alphaRefby 0.1 for the right eye (eyeIndex 1), enabling different alpha test outcomes between eyes for improved stereo edge handling. BothRENDER_DEPTHand non-RENDER_DEPTHpaths are consistently updated.The magic number
0.1appears in multiple shaders (DistantTree, RunGrass). Consider defining it as a shared constant for maintainability:💡 Optional: Use shared VR alpha adjustment constant
+// In Common/VR.hlsli or SharedData.hlsli +static const float kVRAlphaEyeAdjustment = 0.1; // Then in each shader: - alphaRef -= eyeIndex * 0.1; + alphaRef -= eyeIndex * kVRAlphaEyeAdjustment;Also applies to: 222-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DistantTree.hlsl` around lines 206 - 214, Replace the hardcoded per-eye adjustment value 0.1 with a named shared constant (e.g., VR_ALPHA_EYE_ADJUST) and use that constant wherever the shader currently does "alphaRef -= eyeIndex * 0.1" (in the DistantTree shader blocks around AlphaTestRefRS/eyeIndex/alphaRef and the analogous block used for RENDER_DEPTH and the other occurrence similar to RunGrass). Add the constant to the common shader include/shared header used by these shaders so both DistantTree and RunGrass reference the same symbol, then update the subtraction sites to "alphaRef -= eyeIndex * VR_ALPHA_EYE_ADJUST" to ensure maintainability and consistent behavior across shaders.package/Shaders/Common/VR.hlsli (1)
630-638: LGTM! VR Eye 1 sub-pixel jitter for stereo edge supersampling.The jitter implementation correctly applies an NDC offset scaled by
wfor proper clip-space transformation. The magic numbers (0.53, 1.06) are documented as resolution-relative values for a reference SBS resolution.Consider extracting these to named constants for clarity:
💡 Optional: Extract jitter constants
+ // Stereo edge supersampling jitter constants (pixels at reference resolution) + static const float kJitterHorizPx = 0.53; // ~0.75px diagonal, horizontal component + static const float kJitterVertPx = 1.06; // ~0.75px diagonal, vertical component 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); + float2 kJitterNDC = float2(kJitterHorizPx / (FrameBuffer::BufferDim.x * 0.5), -kJitterVertPx / FrameBuffer::BufferDim.y); vsout.VRPosition.xy += kJitterNDC * vsout.VRPosition.w; }🤖 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, Extract the hardcoded jitter magic numbers into named constants and use them when computing kJitterNDC: define descriptive constants (e.g., kJitterHalfWidthFactor = 0.53 and kJitterHeightFactor = 1.06) and replace the literals in the calculation inside the a_eyeIndex == 1 block so kJitterNDC = float2(kJitterHalfWidthFactor / (FrameBuffer::BufferDim.x * 0.5), -kJitterHeightFactor / FrameBuffer::BufferDim.y); keep the subsequent vsout.VRPosition.xy += kJitterNDC * vsout.VRPosition.w and leave a_eyeIndex, kJitterNDC, VRPosition, and FrameBuffer::BufferDim usage unchanged.src/Features/ScreenSpaceShadows.h (1)
65-65: Default behavior change:enableStereoSyncnow defaults tofalse.This changes the out-of-box experience for VR users. Previously, stereo sync was enabled by default; now it must be explicitly enabled. Ensure this is intentional and documented in release notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceShadows.h` at line 65, The default for enableStereoSync in ScreenSpaceShadows.h was changed to false which alters VR behavior; either revert the default to true by restoring the initializer for enableStereoSync in the ScreenSpaceShadows class/struct so stereo sync remains enabled out-of-the-box, or if the new default is intentional, add a clear comment next to the enableStereoSync declaration explaining the change and update the release notes/CHANGELOG to call out that enableStereoSync now defaults to false and must be explicitly enabled for VR users.src/Features/VRStereoOptimizations.cpp (1)
83-86: CAS disabled by forcing strength to 0.The code comment indicates CAS is "disabled for now" and the saved value is ignored. This is acceptable for a work-in-progress feature, but consider adding a TODO or tracking issue reference:
// CAS disabled for now — ignore saved value + // TODO: Re-enable CAS when stability issues are resolved (see issue `#XXXX`) // if (o_json.contains("CASStrength")) // settings.casStrength = o_json["CASStrength"].get<float>(); settings.casStrength = 0.0f;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 83 - 86, Replace the hardcoded disabling of CAS (settings.casStrength = 0.0f) with a short TODO comment referencing a tracking issue or task ID and why the saved "CASStrength" is intentionally ignored; for example, add a TODO/TICKET tag above the assignment mentioning the issue number and expected re-enable condition, and keep the current behavior (assignment to 0.0f) until that issue is resolved so reviewers can see this is deliberate (referencing CASStrength and settings.casStrength in VRStereoOptimizations.cpp).package/Shaders/VR/VRPostProcessCS.hlsl (2)
29-33: Mode constants duplicated - same issue as StereoBlendCS.This is the third location defining these mode constants. As suggested for StereoBlendCS, consider extracting to a shared header.
See previous comment on StereoBlendCS.hlsl for the proposed solution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/VRPostProcessCS.hlsl` around lines 29 - 33, The mode constants (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here (also in StereoBlendCS), so extract these defines into a single shared header (e.g., a common Modes.hlsl) and include that header from VRPostProcessCS.hlsl and StereoBlendCS.hlsl; replace the local `#define` block in VRPostProcessCS.hlsl with an `#include` of the shared file to remove duplication and keep symbols consistent across shaders.
79-110: Bilateral blend logic duplicated from StereoBlendCS.The depth-weighted bilateral blend implementation (lines 80-110) is nearly identical to the MODE_FULL_BLEND path in StereoBlendCS.hlsl (lines 151-198). Consider extracting to a shared utility function:
♻️ Extract shared bilateral blend function
Create a helper in
Common/VR.hlslior a new shared header:float4 BilateralBlendCrossEye( float4 centerColor, float centerDepth, float4 otherColor, float otherDepth, float debugEdgeTint) { float maxDepth = max(max(centerDepth, otherDepth), 1e-5); float depthAgreement = 1.0 - saturate(abs(centerDepth - otherDepth) / maxDepth / 0.02); float blendWeight = 0.5 * depthAgreement; float4 result = lerp(centerColor, otherColor, blendWeight); if (debugEdgeTint > 0) result.rgb = lerp(result.rgb, float3(0, 1, 1), debugEdgeTint); return result; }This reduces code duplication and ensures both shaders use identical blending behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/VRPostProcessCS.hlsl` around lines 79 - 110, Duplicate depth-weighted bilateral blend logic in VRPostProcessCS.hlsl (inside the MODE_FULL_BLEND block using variables center/result, centerDepth, otherColor, otherDepth, DebugEdgeTint) should be extracted into a shared helper (e.g., BilateralBlendCrossEye) in a common header (Common/VR.hlsli) and both shaders should call that helper instead of duplicating the math; create a function that accepts centerColor (float4), centerDepth (float), otherColor (float4), otherDepth (float), debugEdgeTint (float) and returns the blended float4, then replace the blending code in VRPostProcessCS.hlsl and the MODE_FULL_BLEND path in StereoBlendCS.hlsl with a call to BilateralBlendCrossEye and preserve the existing DebugEdgeTint-based tinting behavior and result assignment.package/Shaders/VR/StereoBlendCS.hlsl (1)
28-34: Mode constants duplicated - risk of desync.Mode constants are duplicated here because the cbuffers conflict. Consider extracting just the mode constants to a separate header that both shaders can include:
♻️ Extract mode constants to shared header
Create
VRStereoOptimizations/modes.hlsli:`#ifndef` VR_STEREO_OPT_MODES_HLSLI `#define` VR_STEREO_OPT_MODES_HLSLI `#define` MODE_DISOCCLUDED 0 `#define` MODE_EDGE 1 `#define` MODE_MAIN 2 `#define` MODE_EDGE_NEIGHBOUR 3 `#define` MODE_FULL_BLEND 4 `#endif`Then include it in both files instead of duplicating the defines.
This reduces the risk of the constants diverging between files.
🤖 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 mode defines (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated and risk diverging; extract these constants into a shared header (e.g., VRStereoOptimizations/modes.hlsli) and replace the local `#define` block in StereoBlendCS.hlsl (and the other shader that currently defines them) with an `#include` of that shared header so both shaders reference the same canonical constants.src/Features/VR/StereoBlend.cpp (1)
110-120: RTV references released prematurely before restore.The RTVs retrieved at line 114 are immediately released in lines 116-119, but they're only used to keep the same RTVs bound (with
nullptrDSV). The issue is that after dispatch, the restore logic at lines 174-180 fetches new RTVs rather than restoring the originally saved ones. This works because the RTVs don't actually change during the CS dispatch, but the pattern is fragile and unnecessarily releases COM references.Consider simplifying by only saving/restoring the DSV since the RTVs aren't being modified:
♻️ Simplified DSV-only save/restore
// Save and unbind DSV to avoid SRV/DSV conflict on depth buffer in overwrite mode - ID3D11RenderTargetView* savedRTVs[D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT] = {}; ID3D11DepthStencilView* savedDSV = nullptr; if (isOverwriteMode) { - context->OMGetRenderTargets(D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, savedRTVs, &savedDSV); - context->OMSetRenderTargets(D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, savedRTVs, nullptr); - for (auto& rtv : savedRTVs) { - if (rtv) - rtv->Release(); - } + context->OMGetRenderTargets(0, nullptr, &savedDSV); + if (savedDSV) + context->OMSetDepthStencilView(nullptr, savedDSV); // Unbind DSV only }Note: The current approach does work correctly at runtime since RTVs remain unchanged, but the code could be clearer about its intent.
🤖 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 110 - 120, The RTV array is unnecessary and the code prematurely Releases RTV references; instead only save/restore the DSV. Replace the savedRTVs logic (savedRTVs, OMGetRenderTargets with D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, the OMSetRenderTargets call that passes savedRTVs, and the loop that Releases RTVs) with a DSV-only sequence: call context->OMGetRenderTargets(0, nullptr, &savedDSV) when isOverwriteMode is true, call context->OMSetRenderTargets(0, nullptr, nullptr) before the compute dispatch, and on restore call context->OMSetRenderTargets(0, nullptr, savedDSV) and Release savedDSV; remove any code that Releases or manages savedRTVs and adjust the restore logic to expect only savedDSV.package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
96-124: Edge detection performance note.The nested loop at lines 106-124 runs for each of the 4 directions up to
maxWidth(2) distance. The[loop]attribute on the outer loop prevents unrolling, which is appropriate for the variable iteration count. The inner[unroll]on the 4-direction loop is correct.However, consider that
nearestEdgeDistis checked inside the inner loop but the outer loop continues even after finding an edge at distance 1. Since distance 1 edges immediately classify asMODE_EDGE, you could add an early break:♻️ Optional: Early break when immediate edge found
[loop] for (uint d = 1; d <= maxWidth; d++) { [unroll] for (int i = 0; i < 4; i++) { // ... existing code ... if (relDepthDiff > EdgeDepthThreshold && d < nearestEdgeDist) { nearestEdgeDist = d; nearestWeAreOuter = (linNeighbor < linCenter); } } + // Early exit if we found an immediate neighbor discontinuity + if (nearestEdgeDist == 1) + break; }This is a minor optimization since the loop only runs 2 iterations max, but it could save a few texture fetches per edge pixel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl` around lines 96 - 124, The edge-detection loops should exit early once an immediate neighbor edge is found: inside the outer loop that iterates d from 1..maxWidth (when skipEdgeDetection is false), after the inner unrolled loop that checks all offsets[], check if nearestEdgeDist == d (or specifically == 1) and break the outer loop to avoid extra texture fetches; update the loop surrounding logic that uses nearestEdgeDist/nearestWeAreOuter (and references DepthTexture, linCenter, EdgeDepthThreshold) so behavior is unchanged except for the early exit.
🤖 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/SharedData.hlsli`:
- Around line 55-56: The HLSL field pad0 was changed to bool which can mismatch
C++ packing; update the HLSL cbuffer field pad0 to use uint (matching the C++
ExtendedMaterials.h struct's uint pad0) to ensure consistent 4-byte packing and
cross-language alignment between the SharedData.hlsli cbuffer and the
ExtendedMaterials.h struct.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-105: The comparison against StereoOptModeTexture should mask
off packed upper bits like StereoSyncCS does: when checking for MODE_MAIN for
eyeIndex == 1, read the value from StereoOptModeTexture[uint2(dispatchID.xy)]
and apply a low-nibble mask (e.g. & 0xF) before comparing to MODE_MAIN so any
flag bits in the upper bits don't prevent the early return; update the branch
that currently tests uint mode = StereoOptModeTexture[...] to mask the mode
prior to the if (mode == MODE_MAIN) check.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3200-3205: psout.Reflectance.w is only set for PARALLAX/LANDSCAPE
permutations causing pixelOffset to be dropped for other paths; always forward
pixelOffset into psout.Reflectance.w so VR overwrite reprojection gets the
depth/parallax correction. Change the assignment that sets psout.Reflectance
(used with indirectLobeWeights.specular) to always write the same w component
(e.g., use the existing (pixelOffset>0.0)?saturate(pixelOffset):0.0 expression)
instead of only inside the PARALLAX/LANDSCAPE branch so psout.Reflectance and
pixelOffset are consistent across EMAT/TRUE_PBR, PARALLAX, LANDSCAPE, and
complex-material parallax/displacement paths.
In `@package/Shaders/RunGrass.hlsl`:
- Around line 483-489: The shader references four missing SharedData members
(VRMipBias, VRMipBiasMode, VRMipBiasNearDist, VRMipBiasFarDist) from
RunGrass.hlsl; add these fields to the SharedData cbuffer in
package/Shaders/Common/SharedData.hlsli with appropriate types (e.g., float
VRMipBias, int VRMipBiasMode, float VRMipBiasNearDist, float VRMipBiasFarDist),
ensure proper packing/alignment within the cbuffer, give sensible defaults, and
update any matching CPU-side uniform/constant-buffer struct so the GPU and CPU
layouts stay in sync.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 142-154: CreateSamplerState can fail and leave
stereoBlendLinearSampler null, but the code always calls context->CSSetSamplers
with stereoBlendLinearSampler; check the HRESULT returned by
globals::d3d::device->CreateSamplerState when creating stereoBlendLinearSampler
inside the isOverwriteMode branch, and only call context->CSSetSamplers if
creation succeeded and stereoBlendLinearSampler.get() is non-null; on failure
log or fallback (e.g., skip binding or use an existing default sampler) so you
never pass a null sampler to CSSetSamplers.
In `@src/Features/VRStereoOptimizations.h`:
- Around line 94-95: The comment for the int variable reprojectionMode is
misleading because values 4 and 5 do not map to distinct shader behavior; update
the header to clarify that 4 and 5 collapse to the same overwrite path (used by
stereoBlendOverwriteCS) or replace the numeric magic with a named enum/consts
and a note that both overwrite debug modes share the same shader path in
StereoBlend.cpp (see stereoBlendOverwriteCS) so they are not distinct at
runtime; ensure the comment references reprojectionMode and
StereoBlend.cpp/stereoBlendOverwriteCS for maintainers.
- Around line 78-114: LoadSettings currently assigns JSON fields directly into
the Settings struct (see Settings members like foveatedRegionRadius,
foveatedRegionCenterX/Y, fullBlendDistance, minEdgeDistance, mipBiasNearDist,
mipBiasFarDist, alphaTestThreshold) and then passes them to the GPU via
UpdateParams; add input validation using std::clamp or explicit checks inside
LoadSettings immediately after reading values: clamp foveatedRegionRadius to
[0.0,1.0], foveatedRegionCenterX/Y to [0.0,1.0], ensure fullBlendDistance and
minEdgeDistance are >= 0 and that minEdgeDistance <= fullBlendDistance (or
swap/adjust consistently), enforce mipBiasNearDist <= mipBiasFarDist (swap or
clamp), and clamp alphaTestThreshold to a safe positive range (e.g., [0.0,
1.0e-2]); validate/mutate the Settings instance before any call to UpdateParams
so only sanitized values reach the GPU.
---
Outside diff comments:
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 326-345: Add missing tooltip descriptions for the "Overwrite" and
"Overwrite Eye1" entries in the debugModes array so users understand their
behavior: update the ImGui::Text block inside the Util::HoverTooltipWrapper()
(the same place that documents "Off", "Back-Check", "Blend Weight", and "Edge
Detection") to append concise lines explaining "Overwrite" (e.g., forcefully
displays the corrected/reprojected result, showing where full overwrite occurs)
and "Overwrite Eye1" (e.g., same as Overwrite but only applies overwrite to eye
1 for comparison), ensuring the text references settings.StereoBlendDebugMode
context so the tooltip covers all modes shown by debugModes.
---
Nitpick comments:
In `@package/Shaders/Common/VR.hlsli`:
- Around line 630-638: Extract the hardcoded jitter magic numbers into named
constants and use them when computing kJitterNDC: define descriptive constants
(e.g., kJitterHalfWidthFactor = 0.53 and kJitterHeightFactor = 1.06) and replace
the literals in the calculation inside the a_eyeIndex == 1 block so kJitterNDC =
float2(kJitterHalfWidthFactor / (FrameBuffer::BufferDim.x * 0.5),
-kJitterHeightFactor / FrameBuffer::BufferDim.y); keep the subsequent
vsout.VRPosition.xy += kJitterNDC * vsout.VRPosition.w and leave a_eyeIndex,
kJitterNDC, VRPosition, and FrameBuffer::BufferDim usage unchanged.
In `@package/Shaders/DistantTree.hlsl`:
- Around line 206-214: Replace the hardcoded per-eye adjustment value 0.1 with a
named shared constant (e.g., VR_ALPHA_EYE_ADJUST) and use that constant wherever
the shader currently does "alphaRef -= eyeIndex * 0.1" (in the DistantTree
shader blocks around AlphaTestRefRS/eyeIndex/alphaRef and the analogous block
used for RENDER_DEPTH and the other occurrence similar to RunGrass). Add the
constant to the common shader include/shared header used by these shaders so
both DistantTree and RunGrass reference the same symbol, then update the
subtraction sites to "alphaRef -= eyeIndex * VR_ALPHA_EYE_ADJUST" to ensure
maintainability and consistent behavior across shaders.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 28-34: The mode defines (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN,
MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated and risk diverging; extract
these constants into a shared header (e.g., VRStereoOptimizations/modes.hlsli)
and replace the local `#define` block in StereoBlendCS.hlsl (and the other shader
that currently defines them) with an `#include` of that shared header so both
shaders reference the same canonical constants.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 29-33: The mode constants (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN,
MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here (also in
StereoBlendCS), so extract these defines into a single shared header (e.g., a
common Modes.hlsl) and include that header from VRPostProcessCS.hlsl and
StereoBlendCS.hlsl; replace the local `#define` block in VRPostProcessCS.hlsl with
an `#include` of the shared file to remove duplication and keep symbols consistent
across shaders.
- Around line 79-110: Duplicate depth-weighted bilateral blend logic in
VRPostProcessCS.hlsl (inside the MODE_FULL_BLEND block using variables
center/result, centerDepth, otherColor, otherDepth, DebugEdgeTint) should be
extracted into a shared helper (e.g., BilateralBlendCrossEye) in a common header
(Common/VR.hlsli) and both shaders should call that helper instead of
duplicating the math; create a function that accepts centerColor (float4),
centerDepth (float), otherColor (float4), otherDepth (float), debugEdgeTint
(float) and returns the blended float4, then replace the blending code in
VRPostProcessCS.hlsl and the MODE_FULL_BLEND path in StereoBlendCS.hlsl with a
call to BilateralBlendCrossEye and preserve the existing DebugEdgeTint-based
tinting behavior and result assignment.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 96-124: The edge-detection loops should exit early once an
immediate neighbor edge is found: inside the outer loop that iterates d from
1..maxWidth (when skipEdgeDetection is false), after the inner unrolled loop
that checks all offsets[], check if nearestEdgeDist == d (or specifically == 1)
and break the outer loop to avoid extra texture fetches; update the loop
surrounding logic that uses nearestEdgeDist/nearestWeAreOuter (and references
DepthTexture, linCenter, EdgeDepthThreshold) so behavior is unchanged except for
the early exit.
In `@src/Features/ScreenSpaceShadows.h`:
- Line 65: The default for enableStereoSync in ScreenSpaceShadows.h was changed
to false which alters VR behavior; either revert the default to true by
restoring the initializer for enableStereoSync in the ScreenSpaceShadows
class/struct so stereo sync remains enabled out-of-the-box, or if the new
default is intentional, add a clear comment next to the enableStereoSync
declaration explaining the change and update the release notes/CHANGELOG to call
out that enableStereoSync now defaults to false and must be explicitly enabled
for VR users.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 110-120: The RTV array is unnecessary and the code prematurely
Releases RTV references; instead only save/restore the DSV. Replace the
savedRTVs logic (savedRTVs, OMGetRenderTargets with
D3D11_SIMULTANEOUS_RENDER_TARGET_COUNT, the OMSetRenderTargets call that passes
savedRTVs, and the loop that Releases RTVs) with a DSV-only sequence: call
context->OMGetRenderTargets(0, nullptr, &savedDSV) when isOverwriteMode is true,
call context->OMSetRenderTargets(0, nullptr, nullptr) before the compute
dispatch, and on restore call context->OMSetRenderTargets(0, nullptr, savedDSV)
and Release savedDSV; remove any code that Releases or manages savedRTVs and
adjust the restore logic to expect only savedDSV.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 83-86: Replace the hardcoded disabling of CAS
(settings.casStrength = 0.0f) with a short TODO comment referencing a tracking
issue or task ID and why the saved "CASStrength" is intentionally ignored; for
example, add a TODO/TICKET tag above the assignment mentioning the issue number
and expected re-enable condition, and keep the current behavior (assignment to
0.0f) until that issue is resolved so reviewers can see this is deliberate
(referencing CASStrength and settings.casStrength in VRStereoOptimizations.cpp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04210b5e-1e52-4111-a587-7b97614639d0
📒 Files selected for processing (33)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.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/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/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.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.cpp
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
from
ce5fa6a to
5c60c53
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
package/Shaders/Lighting.hlsl (1)
3180-3185:⚠️ Potential issue | 🟠 Major
EMAT_ENVMAPstill dropspixelOffsetinReflectance.w.The guard on Line 3180 still excludes ENVMAP-only
EMATpermutations, even thoughcomplexMaterialParallaxwritespixelOffsetin that path. In VR overwrite mode those materials will still reproject without the parallax depth correction.🛠️ Suggested fix
-# if (defined(EMAT) || defined(TRUE_PBR)) && (defined(PARALLAX) || defined(LANDSCAPE) || defined(TRUE_PBR)) - psout.Reflectance = float4(indirectLobeWeights.specular, - (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0); -# else - psout.Reflectance = float4(indirectLobeWeights.specular, 0.0); -# endif + psout.Reflectance = float4( + indirectLobeWeights.specular, + (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 3180 - 3185, The Reflectance.w assignment currently only includes pixelOffset for permutations matching (EMAT || TRUE_PBR) && (PARALLAX || LANDSCAPE || TRUE_PBR), which misses ENVMAP-only EMAT permutations where complexMaterialParallax writes pixelOffset; update the preprocessor condition around the psout.Reflectance assignment to also include the EMAT_ENVMAP/ENVMAP-only EMAT permutation (e.g., change the guard to check defined(EMAT) || defined(EMAT_ENVMAP) || defined(TRUE_PBR)) so that when complexMaterialParallax produces pixelOffset you assign psout.Reflectance = float4(indirectLobeWeights.specular, (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0); otherwise keep the existing zero fallback.
🧹 Nitpick comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)
100-103: Use the shared stereo-opt mode constants here too.
package/Shaders/VRStereoOptimizations/cbuffers.hlsliis already the single source of truth forMODE_EDGE/MODE_MAIN. Duplicating1and2here makes this skip path easy to drift from the classification shaders if those values ever move.🤖 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 - 103, The if-block in DeferredCompositeCS.hlsl uses magic numbers (1 and 2) to check StereoOptModeTexture for stereo-opt modes; replace those literals with the shared constants MODE_MAIN and MODE_EDGE from package/Shaders/VRStereoOptimizations/cbuffers.hlsli so the check uses the canonical enums; ensure you `#include` or import that hlsli at the top of DeferredCompositeCS.hlsl and change the condition "if (mode == 2 || mode == 1)" to "if (mode == MODE_MAIN || mode == MODE_EDGE)" (using the existing variables eyeIndex, StereoOptModeTexture, dispatchID.xy) so the logic stays in sync with the classification shaders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Deferred.cpp`:
- Around line 373-379: The code currently only binds t16 when
vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off, but
the VR_STEREO_OPT shader is compiled whenever vrStereoOptimizations.loaded and
may sample t16 even when disabled; update the block that references
globals::features::vrStereoOptimizations/vrStereoOpt so that when
vrStereoOpt.loaded you always call context->CSSetShaderResources(16, 1,
&modeSRV) — passing either vrStereoOpt.GetModeTextureSRV() or nullptr depending
on stereoMode — and also expand the cleanup that clears SRV slots (the array
that currently clears 0..15) to include slot 16 so t16 is deterministically
cleared after dispatch.
In `@src/FeatureBuffer.cpp`:
- Around line 57-58: The CPU-side GetFeatureBufferData() now appends
globals::features::vrTerrainBlend.settings but the HLSL FeatureData cbuffer in
package/Shaders/Common/SharedData.hlsli was not updated; add a matching
VRTerrainBlendSettings field/struct definition to the end of cbuffer FeatureData
(after ExponentialHeightFogSettings) with the same member layout, types and
packing as the C++ vrTerrainBlend.settings so the GPU buffer layout stays in
sync with GetFeatureBufferData(); ensure any padding/alignment matches the C++
struct to avoid misaligned data.
In `@src/Features/ScreenSpaceShadows.h`:
- Line 74: Clamp the deserialized value for the StereoOptRightEyeReduction
setting when loading JSON so only 0 or 1 are accepted: in the JSON load path
that assigns to the stereoOptRightEyeReduction variable (the code handling the
"StereoOptRightEyeReduction" key in ScreenSpaceShadows.cpp) replace the direct
get<int>() assignment with a clamped assignment (e.g., use std::clamp on the
parsed int to the range [0,1]) to prevent malformed configs from propagating
into the reduced-shader selection logic.
In `@src/Features/TerrainHelper.cpp`:
- Around line 186-188: Before returning when settings.enableParallax is false,
clear the terrain-parallax GPU state so stale permutation bits/SRVs aren't left
bound: reset the ExtraFeatureDescriptor used for terrain-parallax (clear its
flags/fields) and unbind/clear pixel shader resource slots 92–97 that the
parallax path uses; place this logic immediately before the existing if
(!settings.enableParallax) return; check in TerrainHelper.cpp so the function
exits with parallax state cleared.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-36: SaveSettings/LoadSettings currently omit fields edited by
DrawSettings (pomDepthScale, debugFullBlendDepth, debugPOMDepth), so those UI
edits never persist and UpdateConstantBuffer uses defaults; update both
VRStereoOptimizations::SaveSettings and VRStereoOptimizations::LoadSettings to
serialize and deserialize these three members using clear JSON keys (e.g.,
"PomDepthScale", "DebugFullBlendDepth", "DebugPOMDepth"), and ensure
LoadSettings sets the class fields before UpdateConstantBuffer is used; also
mirror the same additions for the other Save/Load pair referenced around the
same block so the round-trip is complete.
---
Duplicate comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 3180-3185: The Reflectance.w assignment currently only includes
pixelOffset for permutations matching (EMAT || TRUE_PBR) && (PARALLAX ||
LANDSCAPE || TRUE_PBR), which misses ENVMAP-only EMAT permutations where
complexMaterialParallax writes pixelOffset; update the preprocessor condition
around the psout.Reflectance assignment to also include the
EMAT_ENVMAP/ENVMAP-only EMAT permutation (e.g., change the guard to check
defined(EMAT) || defined(EMAT_ENVMAP) || defined(TRUE_PBR)) so that when
complexMaterialParallax produces pixelOffset you assign psout.Reflectance =
float4(indirectLobeWeights.specular, (pixelOffset > 0.0) ? saturate(pixelOffset)
: 0.0); otherwise keep the existing zero fallback.
---
Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-103: The if-block in DeferredCompositeCS.hlsl uses magic
numbers (1 and 2) to check StereoOptModeTexture for stereo-opt modes; replace
those literals with the shared constants MODE_MAIN and MODE_EDGE from
package/Shaders/VRStereoOptimizations/cbuffers.hlsli so the check uses the
canonical enums; ensure you `#include` or import that hlsli at the top of
DeferredCompositeCS.hlsl and change the condition "if (mode == 2 || mode == 1)"
to "if (mode == MODE_MAIN || mode == MODE_EDGE)" (using the existing variables
eyeIndex, StereoOptModeTexture, dispatchID.xy) so the logic stays in sync with
the classification shaders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b82c8397-65ef-41cd-b5ca-9e10624bf67b
📒 Files selected for processing (39)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlslifeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.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/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/FeatureBuffer.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.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.cpp
✅ Files skipped from review due to trivial changes (13)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- src/Features/ExtendedMaterials.h
- src/Globals.h
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- .claude/CLAUDE.md
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- src/Features/VR/SettingsUI.cpp
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- src/Features/ScreenSpaceShadows.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl
- package/Shaders/DistantTree.hlsl
- src/Features/VR.cpp
- package/Shaders/RunGrass.hlsl
- src/Features/VR.h
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
- src/Features/VR/StereoBlend.cpp
- src/Globals.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
6 tasks
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
from
a45a9da to
8fa918e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/Deferred.cpp (1)
373-393:⚠️ Potential issue | 🟠 MajorAlways initialize
t16for theVR_STEREO_OPTcomposite variants.
GetComputeMainComposite*()compiles the shader withVR_STEREO_OPTwhenevervrStereoOptimizations.loadedis true, but slott16is only bound and cleared whenstereoMode != Off. If the feature is loaded but disabled, the shader can still see whatever SRV was left in slot 16 from an earlier dispatch.Suggested fix
- if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { - ID3D11ShaderResourceView* modeSRV = vrStereoOpt.GetModeTextureSRV(); - if (modeSRV) - context->CSSetShaderResources(16, 1, &modeSRV); - } + if (REL::Module::IsVR() && vrStereoOpt.loaded) { + ID3D11ShaderResourceView* modeSRV = + vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off ? + vrStereoOpt.GetModeTextureSRV() : + nullptr; + context->CSSetShaderResources(16, 1, &modeSRV); + } @@ - if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { + if (REL::Module::IsVR() && vrStereoOpt.loaded) { ID3D11ShaderResourceView* nullSRV = nullptr; context->CSSetShaderResources(16, 1, &nullSRV); } @@ - ID3D11ShaderResourceView* views[16]{ nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr }; + ID3D11ShaderResourceView* views[17]{}; context->CSSetShaderResources(0, ARRAYSIZE(views), views);Also applies to: 421-422, 596-625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 373 - 393, The VR stereo-mode SRV slot (t16) can remain stale because GetComputeMainComposite()/GetComputeMainCompositeInterior() compiles with VR_STEREO_OPT whenever globals::features::vrStereoOptimizations.loaded is true, but the code only binds/clears slot 16 when vrStereoOptimizations.settings.stereoMode != Off; ensure t16 is always initialized when vrStereoOptimizations.loaded is true by binding a null SRV to slot 16 before Dispatch if stereoMode == Off (and still bind the real modeSRV when stereoMode != Off), and always clear slot 16 after Dispatch; update the same logic around both GetComputeMainComposite and GetComputeMainCompositeInterior call sites (references: vrStereoOptimizations, GetModeTextureSRV(), GetComputeMainComposite(), GetComputeMainCompositeInterior(), context->CSSetShaderResources(16,...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 567-576: The "Lessons Learned" markdown section has formatting
errors: the heading lacks a space after "##" and several bolded items use
escaped asterisks ("\*\*") instead of Markdown bold ("**"), causing them to
render literally; update the heading to "## Lessons Learned" and replace
occurrences of "\*\*VR.hlsli", "\*\*Build/packaging:", "\*\*VS Code Edit tool",
and "\*\* Reprojection investigation" with proper "**VR.hlsli**",
"**Build/packaging:**", "**VS Code Edit tool**", and "**Reprojection
investigation:**" (and similar escaped asterisks elsewhere) so the bullets and
bold text render correctly while preserving the existing content mentioning
VR.hlsli, BuildRelease.bat ALL / AIO, StereoBlendCS, and DeferredCompositeCS.
- Around line 17-38: Replace the incorrect preset names in the documentation:
change all occurrences of "ALL-VS18" to "ALL" and "Dev-VS18" to "Dev" in
.claude/CLAUDE.md so the configure/build commands read "cmake -S . --preset=ALL"
and "cmake --build --preset=Dev"; also keep the note that Visual Studio 2026
generator configuration must live in CMakeUserPresets.json (do not edit
CMakePresets.json) and ensure the text no longer implies renaming presets in the
upstream CMakePresets.json.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 19-27: The VRPostProcessCB cbuffer layout at register(b1) is
missing the initial StereoBlendCB fields, causing DebugEdgeTint, DebugMode, and
FullBlendDistance to map to the wrong bytes; update VRPostProcessCB to mirror
the full 48-byte StereoBlendCB structure (the same field names/order/types as
defined by StereoBlendCB in src/Features/VR.h) before the existing
DebugEdgeTint/DebugMode/FullBlendDistance/_pad so its memory layout matches what
StereoBlend.cpp uploads; ensure correct types and any required padding so the
total cbuffer size and field offsets match StereoBlendCB exactly.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 97-143: The classification currently hardcodes maxWidth to
kInnerWidth and never uses edgeWidth nor emits MODE_EDGE_NEIGHBOUR, so
outer/background bands are ignored; change maxWidth to use the larger of
kInnerWidth and the configured edgeWidth (e.g., maxWidth = max(kInnerWidth,
edgeWidth)) so the search covers outer-band distances, and after the search
update the classification logic: if nearestEdgeDist == 1 set ModeTextureRW[dtid]
= MODE_EDGE; else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) set
MODE_EDGE as now; else if (nearestWeAreOuter && nearestEdgeDist <= edgeWidth)
set ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR (so background-side hits within
edgeWidth become neighbour mode) before falling back to MODE_MAIN. Ensure you
reference kInnerWidth, edgeWidth, maxWidth, nearestEdgeDist, nearestWeAreOuter,
ModeTextureRW, MODE_EDGE, and MODE_EDGE_NEIGHBOUR when making the changes.
In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 84-93: Clamp bendSettings.SampleCount to the valid range (1–4)
before it is used to compute the compile-time SAMPLE_COUNT; specifically, modify
GetScaledSampleCount() to validate/bound bendSettings.SampleCount to [1,4] (or
call a helper that does this) and then compute scaledSampleCount = clampedValue
* 64, and also ensure LoadSettings() performs the same validation when loading
JSON so malformed persisted values cannot produce zero or pathological
SAMPLE_COUNTs; update any other places that derive SAMPLE_COUNT (the same logic
around GetScaledSampleCount / SAMPLE_COUNT usage) to use the clamped value.
---
Duplicate comments:
In `@src/Deferred.cpp`:
- Around line 373-393: The VR stereo-mode SRV slot (t16) can remain stale
because GetComputeMainComposite()/GetComputeMainCompositeInterior() compiles
with VR_STEREO_OPT whenever globals::features::vrStereoOptimizations.loaded is
true, but the code only binds/clears slot 16 when
vrStereoOptimizations.settings.stereoMode != Off; ensure t16 is always
initialized when vrStereoOptimizations.loaded is true by binding a null SRV to
slot 16 before Dispatch if stereoMode == Off (and still bind the real modeSRV
when stereoMode != Off), and always clear slot 16 after Dispatch; update the
same logic around both GetComputeMainComposite and
GetComputeMainCompositeInterior call sites (references: vrStereoOptimizations,
GetModeTextureSRV(), GetComputeMainComposite(),
GetComputeMainCompositeInterior(), context->CSSetShaderResources(16,...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02927ee3-8f3c-40fe-a6d5-d5c286b83a6e
📒 Files selected for processing (40)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlslifeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.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/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.hlslipackage/Shaders/VRStereoOptimizations/modes.hlslisrc/Deferred.cppsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.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.cpp
✅ Files skipped from review due to trivial changes (15)
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- package/Shaders/Common/VR.hlsli
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- src/Features/ExtendedMaterials.h
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/RunGrass.hlsl
- src/Globals.h
- src/Features/VR/SettingsUI.cpp
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- src/State.cpp
- package/Shaders/Lighting.hlsl
- src/Features/VR/StereoBlend.cpp
🚧 Files skipped from review as they are similar to previous changes (16)
- src/FeatureBuffer.cpp
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl
- package/Shaders/DistantTree.hlsl
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- package/Shaders/Common/SharedData.hlsli
- src/Features/TerrainHelper.cpp
- src/Features/TerrainHelper.h
- src/Features/VR.cpp
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- src/Features/ScreenSpaceShadows.h
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlsli
- src/Features/TerrainBlending.cpp
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
- src/Globals.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Features/VRStereoOptimizations.cpp
Comment on lines
+17
to
+38
| This machine runs Visual Studio 18 2026 Community. The upstream CMakePresets.json | ||
| hardcodes "Visual Studio 17 2022" which will fail. | ||
|
|
||
| **Do NOT modify CMakePresets.json.** A gitignored CMakeUserPresets.json already exists | ||
| in the project root with the correct configuration. Always use these presets: | ||
|
|
||
| ```bash | ||
| cmake -S . --preset=ALL-VS18 | ||
| cmake --build --preset=Dev-VS18 | ||
| ``` | ||
|
|
||
| This builds CommunityShaders and the AIO target in Release mode. | ||
| Always build with the AIO target — never build CommunityShaders alone. | ||
|
|
||
| Key details: | ||
|
|
||
| - Configure preset is `ALL-VS18` (not `ALL`) — inherits from `skyrim` base, overrides generator to "Visual Studio 18 2026" | ||
| - Build preset is `Dev-VS18` — targets CommunityShaders and AIO in Release configuration | ||
| - Cannot inherit from `ALL` because it sets the VS 2022 generator and you cannot override an inherited generator | ||
| - Cannot reuse preset names that exist in CMakePresets.json — CMake forbids duplicates | ||
| - If CMakeUserPresets.json is missing or cmake fails with a generator error, recreate it — do not edit CMakePresets.json | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List available CMake presets from CMakePresets.json
echo "=== Configure Presets ==="
jq -r '.configurePresets[].name' CMakePresets.json
echo ""
echo "=== Build Presets ==="
jq -r '.buildPresets[].name' CMakePresets.jsonRepository: doodlum/skyrim-community-shaders
Length of output: 189
🏁 Script executed:
cat -n .claude/CLAUDE.md | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 2250
Incorrect CMake preset names will cause build failures.
The documentation (lines 24-25, 33-34) references ALL-VS18 and Dev-VS18, but these presets do not exist in CMakePresets.json. The actual presets are ALL (configure) and Dev (build).
Users following these instructions will fail immediately at the configure/build step. Update the commands and explanations to use the correct preset names:
cmake -S . --preset=ALL(notALL-VS18)cmake --build --preset=Dev(notDev-VS18)
If Visual Studio 2026 generator configuration is needed, this should be handled in CMakeUserPresets.json as the documentation mentions, not by renaming presets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md around lines 17 - 38, Replace the incorrect preset names
in the documentation: change all occurrences of "ALL-VS18" to "ALL" and
"Dev-VS18" to "Dev" in .claude/CLAUDE.md so the configure/build commands read
"cmake -S . --preset=ALL" and "cmake --build --preset=Dev"; also keep the note
that Visual Studio 2026 generator configuration must live in
CMakeUserPresets.json (do not edit CMakePresets.json) and ensure the text no
longer implies renaming presets in the upstream CMakePresets.json.
Comment on lines
+567
to
+576
| ##Lessons Learned | ||
|
|
||
| - \*\*VR.hlsli has nested #ifdef gates — reprojection functions need both VR and FRAMEBUFFER defines, not COMPUTESHADER. Shaders compile "successfully" even when functions are gated out; the error only appears at runtime. | ||
|
|
||
| - \*\*Build/packaging: BuildRelease.bat ALL and the AIO directory are separate — must run cmake --install to sync the new DLL. We hit this today (packaged stale DLL). | ||
| Tooling: | ||
|
|
||
| - \*\*VS Code Edit tool fails on paths with spaces — use Python scripts as workaround. | ||
|
|
||
| - \*\* Reprojection investigation: The double image persisted even with a near-exact copy of StereoBlendCS's proven shader + dispatch pattern. This strongly suggests the problem is NOT in the reprojection shader itself but in something else writing to eye 1 in the pipeline (likely DeferredCompositeCS compositing stale G-buffer data). Worth considering integrating reprojection directly into DeferredCompositeCS next time. |
Contributor
There was a problem hiding this comment.
Fix markdown formatting in Lessons Learned section.
The section heading (line 567) is missing a space after ##, and the bullet points (lines 569-576) use escaped asterisks \*\* instead of proper markdown bold **, causing them to render as literal text rather than bold formatting.
📝 Proposed formatting fixes
-##Lessons Learned
+## Lessons Learned
-- \*\*VR.hlsli has nested `#ifdef` gates — reprojection functions need both VR and FRAMEBUFFER defines, not COMPUTESHADER. Shaders compile "successfully" even when functions are gated out; the error only appears at runtime.
+- **VR.hlsli has nested `#ifdef` gates** — reprojection functions need both VR and FRAMEBUFFER defines, not COMPUTESHADER. Shaders compile "successfully" even when functions are gated out; the error only appears at runtime.
-- \*\*Build/packaging: BuildRelease.bat ALL and the AIO directory are separate — must run cmake --install to sync the new DLL. We hit this today (packaged stale DLL).
+- **Build/packaging** — BuildRelease.bat ALL and the AIO directory are separate — must run cmake --install to sync the new DLL. We hit this today (packaged stale DLL).
+
Tooling:
-- \*\*VS Code Edit tool fails on paths with spaces — use Python scripts as workaround.
+- **VS Code Edit tool fails on paths with spaces** — use Python scripts as workaround.
-- \*\* Reprojection investigation: The double image persisted even with a near-exact copy of StereoBlendCS's proven shader + dispatch pattern. This strongly suggests the problem is NOT in the reprojection shader itself but in something else writing to eye 1 in the pipeline (likely DeferredCompositeCS compositing stale G-buffer data). Worth considering integrating reprojection directly into DeferredCompositeCS next time.
+- **Reprojection investigation** — The double image persisted even with a near-exact copy of StereoBlendCS's proven shader + dispatch pattern. This strongly suggests the problem is NOT in the reprojection shader itself but in something else writing to eye 1 in the pipeline (likely DeferredCompositeCS compositing stale G-buffer data). Worth considering integrating reprojection directly into DeferredCompositeCS next time.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ##Lessons Learned | |
| - \*\*VR.hlsli has nested #ifdef gates — reprojection functions need both VR and FRAMEBUFFER defines, not COMPUTESHADER. Shaders compile "successfully" even when functions are gated out; the error only appears at runtime. | |
| - \*\*Build/packaging: BuildRelease.bat ALL and the AIO directory are separate — must run cmake --install to sync the new DLL. We hit this today (packaged stale DLL). | |
| Tooling: | |
| - \*\*VS Code Edit tool fails on paths with spaces — use Python scripts as workaround. | |
| - \*\* Reprojection investigation: The double image persisted even with a near-exact copy of StereoBlendCS's proven shader + dispatch pattern. This strongly suggests the problem is NOT in the reprojection shader itself but in something else writing to eye 1 in the pipeline (likely DeferredCompositeCS compositing stale G-buffer data). Worth considering integrating reprojection directly into DeferredCompositeCS next time. | |
| ## Lessons Learned | |
| - **VR.hlsli has nested `#ifdef` gates** — reprojection functions need both VR and FRAMEBUFFER defines, not COMPUTESHADER. Shaders compile "successfully" even when functions are gated out; the error only appears at runtime. | |
| - **Build/packaging** — BuildRelease.bat ALL and the AIO directory are separate — must run cmake --install to sync the new DLL. We hit this today (packaged stale DLL). | |
| Tooling: | |
| - **VS Code Edit tool fails on paths with spaces** — use Python scripts as workaround. | |
| - **Reprojection investigation** — The double image persisted even with a near-exact copy of StereoBlendCS's proven shader + dispatch pattern. This strongly suggests the problem is NOT in the reprojection shader itself but in something else writing to eye 1 in the pipeline (likely DeferredCompositeCS compositing stale G-buffer data). Worth considering integrating reprojection directly into DeferredCompositeCS next time. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/CLAUDE.md around lines 567 - 576, The "Lessons Learned" markdown
section has formatting errors: the heading lacks a space after "##" and several
bolded items use escaped asterisks ("\*\*") instead of Markdown bold ("**"),
causing them to render literally; update the heading to "## Lessons Learned" and
replace occurrences of "\*\*VR.hlsli", "\*\*Build/packaging:", "\*\*VS Code Edit
tool", and "\*\* Reprojection investigation" with proper "**VR.hlsli**",
"**Build/packaging:**", "**VS Code Edit tool**", and "**Reprojection
investigation:**" (and similar escaped asterisks elsewhere) so the bullets and
bold text render correctly while preserving the existing content mentioning
VR.hlsli, BuildRelease.bat ALL / AIO, StereoBlendCS, and DeferredCompositeCS.
Comment on lines
+97
to
+143
| // --- Edge detection with two-tier classification --- | ||
| // MODE_EDGE: immediate neighbor (distance 1) has depth discontinuity, OR | ||
| // inner/foreground band (distance <= kInnerWidth). | ||
| static const uint kInnerWidth = 2; | ||
| int2 offsets[4] = { int2(-1, 0), int2(1, 0), int2(0, -1), int2(0, 1) }; | ||
|
|
||
| uint nearestEdgeDist = 0xFFFFFFFF; // nearest distance at which a discontinuity was found | ||
| bool nearestWeAreOuter = false; // whether we are on the background side at that nearest hit | ||
|
|
||
| // Use the larger of inner/outer widths for the search | ||
| uint maxWidth = kInnerWidth; | ||
|
|
||
| if (!skipEdgeDetection) { | ||
| [loop] for (uint d = 1; d <= maxWidth; d++) | ||
| { | ||
| [unroll] for (int i = 0; i < 4; i++) | ||
| { | ||
| int2 rawNeighbor = int2(dtid) + offsets[i] * (int)d; | ||
| uint2 neighborCoord = Stereo::ClampToEyeBounds(rawNeighbor, eyeIndex, FrameDim); | ||
|
|
||
| float neighborDepth = DepthTexture[neighborCoord]; | ||
| bool neighborIsSky = (neighborDepth < 1e-5) || (neighborDepth >= 1.0); | ||
| float linNeighbor = neighborIsSky ? 999999.0 : SharedData::GetScreenDepth(neighborDepth); | ||
| float maxLin = max(max(linCenter, linNeighbor), 1e-5); | ||
| float relDepthDiff = abs(linCenter - linNeighbor) / maxLin; | ||
|
|
||
| if (relDepthDiff > EdgeDepthThreshold && d < nearestEdgeDist) { | ||
| nearestEdgeDist = d; | ||
| nearestWeAreOuter = (linNeighbor < linCenter); // neighbor closer to camera = we are background | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } // !skipEdgeDetection | ||
|
|
||
| if (nearestEdgeDist != 0xFFFFFFFF) { | ||
| // Classify based on distance and side | ||
| if (nearestEdgeDist == 1) { | ||
| // Immediate neighbor discontinuity: always MODE_EDGE regardless of side | ||
| ModeTextureRW[dtid] = MODE_EDGE; | ||
| return; | ||
| } else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) { | ||
| // Inner/foreground band beyond distance 1 | ||
| ModeTextureRW[dtid] = MODE_EDGE; | ||
| return; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
edgeWidth is a no-op in the classification pass.
The normal path hardcodes maxWidth to kInnerWidth and never emits MODE_EDGE_NEIGHBOUR, so any configured outer edge band above 2 pixels is ignored and background-side edge pixels fall back to MODE_MAIN.
Suggested fix
static const uint kInnerWidth = 2;
int2 offsets[4] = { int2(-1, 0), int2(1, 0), int2(0, -1), int2(0, 1) };
- // Use the larger of inner/outer widths for the search
- uint maxWidth = kInnerWidth;
+ // Search far enough to classify both the fixed inner band and the configurable outer band.
+ uint maxWidth = max(kInnerWidth, EdgeWidth);
@@
if (nearestEdgeDist != 0xFFFFFFFF) {
// Classify based on distance and side
if (nearestEdgeDist == 1) {
// Immediate neighbor discontinuity: always MODE_EDGE regardless of side
ModeTextureRW[dtid] = MODE_EDGE;
return;
} else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) {
// Inner/foreground band beyond distance 1
ModeTextureRW[dtid] = MODE_EDGE;
return;
+ } else if (nearestWeAreOuter && nearestEdgeDist <= EdgeWidth) {
+ // Outer/background band
+ ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR;
+ return;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // --- Edge detection with two-tier classification --- | |
| // MODE_EDGE: immediate neighbor (distance 1) has depth discontinuity, OR | |
| // inner/foreground band (distance <= kInnerWidth). | |
| static const uint kInnerWidth = 2; | |
| int2 offsets[4] = { int2(-1, 0), int2(1, 0), int2(0, -1), int2(0, 1) }; | |
| uint nearestEdgeDist = 0xFFFFFFFF; // nearest distance at which a discontinuity was found | |
| bool nearestWeAreOuter = false; // whether we are on the background side at that nearest hit | |
| // Use the larger of inner/outer widths for the search | |
| uint maxWidth = kInnerWidth; | |
| if (!skipEdgeDetection) { | |
| [loop] for (uint d = 1; d <= maxWidth; d++) | |
| { | |
| [unroll] for (int i = 0; i < 4; i++) | |
| { | |
| int2 rawNeighbor = int2(dtid) + offsets[i] * (int)d; | |
| uint2 neighborCoord = Stereo::ClampToEyeBounds(rawNeighbor, eyeIndex, FrameDim); | |
| float neighborDepth = DepthTexture[neighborCoord]; | |
| bool neighborIsSky = (neighborDepth < 1e-5) || (neighborDepth >= 1.0); | |
| float linNeighbor = neighborIsSky ? 999999.0 : SharedData::GetScreenDepth(neighborDepth); | |
| float maxLin = max(max(linCenter, linNeighbor), 1e-5); | |
| float relDepthDiff = abs(linCenter - linNeighbor) / maxLin; | |
| if (relDepthDiff > EdgeDepthThreshold && d < nearestEdgeDist) { | |
| nearestEdgeDist = d; | |
| nearestWeAreOuter = (linNeighbor < linCenter); // neighbor closer to camera = we are background | |
| } | |
| } | |
| } | |
| } // !skipEdgeDetection | |
| if (nearestEdgeDist != 0xFFFFFFFF) { | |
| // Classify based on distance and side | |
| if (nearestEdgeDist == 1) { | |
| // Immediate neighbor discontinuity: always MODE_EDGE regardless of side | |
| ModeTextureRW[dtid] = MODE_EDGE; | |
| return; | |
| } else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) { | |
| // Inner/foreground band beyond distance 1 | |
| ModeTextureRW[dtid] = MODE_EDGE; | |
| return; | |
| } | |
| } | |
| // --- Edge detection with two-tier classification --- | |
| // MODE_EDGE: immediate neighbor (distance 1) has depth discontinuity, OR | |
| // inner/foreground band (distance <= kInnerWidth). | |
| static const uint kInnerWidth = 2; | |
| int2 offsets[4] = { int2(-1, 0), int2(1, 0), int2(0, -1), int2(0, 1) }; | |
| uint nearestEdgeDist = 0xFFFFFFFF; // nearest distance at which a discontinuity was found | |
| bool nearestWeAreOuter = false; // whether we are on the background side at that nearest hit | |
| // Search far enough to classify both the fixed inner band and the configurable outer band. | |
| uint maxWidth = max(kInnerWidth, EdgeWidth); | |
| if (!skipEdgeDetection) { | |
| [loop] for (uint d = 1; d <= maxWidth; d++) | |
| { | |
| [unroll] for (int i = 0; i < 4; i++) | |
| { | |
| int2 rawNeighbor = int2(dtid) + offsets[i] * (int)d; | |
| uint2 neighborCoord = Stereo::ClampToEyeBounds(rawNeighbor, eyeIndex, FrameDim); | |
| float neighborDepth = DepthTexture[neighborCoord]; | |
| bool neighborIsSky = (neighborDepth < 1e-5) || (neighborDepth >= 1.0); | |
| float linNeighbor = neighborIsSky ? 999999.0 : SharedData::GetScreenDepth(neighborDepth); | |
| float maxLin = max(max(linCenter, linNeighbor), 1e-5); | |
| float relDepthDiff = abs(linCenter - linNeighbor) / maxLin; | |
| if (relDepthDiff > EdgeDepthThreshold && d < nearestEdgeDist) { | |
| nearestEdgeDist = d; | |
| nearestWeAreOuter = (linNeighbor < linCenter); // neighbor closer to camera = we are background | |
| } | |
| } | |
| } | |
| } // !skipEdgeDetection | |
| if (nearestEdgeDist != 0xFFFFFFFF) { | |
| // Classify based on distance and side | |
| if (nearestEdgeDist == 1) { | |
| // Immediate neighbor discontinuity: always MODE_EDGE regardless of side | |
| ModeTextureRW[dtid] = MODE_EDGE; | |
| return; | |
| } else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) { | |
| // Inner/foreground band beyond distance 1 | |
| ModeTextureRW[dtid] = MODE_EDGE; | |
| return; | |
| } else if (nearestWeAreOuter && nearestEdgeDist <= EdgeWidth) { | |
| // Outer/background band | |
| ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR; | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl` around lines 97 - 143,
The classification currently hardcodes maxWidth to kInnerWidth and never uses
edgeWidth nor emits MODE_EDGE_NEIGHBOUR, so outer/background bands are ignored;
change maxWidth to use the larger of kInnerWidth and the configured edgeWidth
(e.g., maxWidth = max(kInnerWidth, edgeWidth)) so the search covers outer-band
distances, and after the search update the classification logic: if
nearestEdgeDist == 1 set ModeTextureRW[dtid] = MODE_EDGE; else if
(!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) set MODE_EDGE as now;
else if (nearestWeAreOuter && nearestEdgeDist <= edgeWidth) set
ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR (so background-side hits within
edgeWidth become neighbour mode) before falling back to MODE_MAIN. Ensure you
reference kInnerWidth, edgeWidth, maxWidth, nearestEdgeDist, nearestWeAreOuter,
ModeTextureRW, MODE_EDGE, and MODE_EDGE_NEIGHBOUR when making the changes.
Comment on lines
84
to
93
| uint ScreenSpaceShadows::GetScaledSampleCount() | ||
| { | ||
| float2 renderSize = Util::ConvertToDynamic(globals::state->screenSize); | ||
|
|
||
| // In VR, renderSize covers both eyes side-by-side; raymarch dispatches per-eye | ||
| if (globals::game::isVR) | ||
| renderSize.x /= 2.0f; | ||
|
|
||
| // Scale sample count based on both dimensions relative to 1920x1080 reference | ||
| float2 referenceRes = { 1920.0f, 1080.0f }; | ||
| float referenceArea = referenceRes.x * referenceRes.y; | ||
| float currentArea = renderSize.x * renderSize.y; | ||
| float areaScale = std::sqrt(currentArea / referenceArea); | ||
| uint scaledSampleCount = static_cast<uint>(std::round(bendSettings.SampleCount * 60 * areaScale)); | ||
|
|
||
| // Quantize to steps of 8 to prevent frequent recompilation from small DRS oscillations | ||
| scaledSampleCount = ((scaledSampleCount + 7u) / 8u) * 8u; | ||
| scaledSampleCount = std::max(scaledSampleCount, 8u); | ||
|
|
||
| // Shadow reach in pixels is resolution-independent: a tree branch casts | ||
| // the same pixel-length shadow at 1080p and 3000p. Sample count controls | ||
| // reach, not quality-per-pixel. The old formula (multiplier * 64) was | ||
| // correct; the area-based scaling produced 2-8x more samples at VR | ||
| // resolution with no quality benefit, only GPU cost. | ||
| // Always produces WAVE_SIZE-aligned counts for correct Bend READ_COUNT. | ||
| uint scaledSampleCount = bendSettings.SampleCount * 64; | ||
| return scaledSampleCount; |
Contributor
There was a problem hiding this comment.
Clamp the JSON-backed sample settings before deriving SAMPLE_COUNT.
The UI constrains SampleCount to 1–4, but LoadSettings() still accepts any persisted value and GetScaledSampleCount() now turns it directly into the compile-time SAMPLE_COUNT define. A bad config can silently compile a zero-sample or pathological raymarch variant.
Suggested fix
void ScreenSpaceShadows::LoadSettings(json& o_json)
{
bendSettings = o_json;
+ bendSettings.SampleCount = std::clamp(bendSettings.SampleCount, 1u, 4u);
if (o_json.contains("StereoOptRightEyeReduction"))
- stereoOptRightEyeReduction = o_json["StereoOptRightEyeReduction"];
+ stereoOptRightEyeReduction = std::clamp(o_json["StereoOptRightEyeReduction"].get<int>(), 0, 1);
if (o_json.contains("EnableStereoSync"))
enableStereoSync = o_json["EnableStereoSync"].get<bool>();
}As per coding guidelines, “Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations.”
Also applies to: 367-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/ScreenSpaceShadows.cpp` around lines 84 - 93, Clamp
bendSettings.SampleCount to the valid range (1–4) before it is used to compute
the compile-time SAMPLE_COUNT; specifically, modify GetScaledSampleCount() to
validate/bound bendSettings.SampleCount to [1,4] (or call a helper that does
this) and then compute scaledSampleCount = clampedValue * 64, and also ensure
LoadSettings() performs the same validation when loading JSON so malformed
persisted values cannot produce zero or pathological SAMPLE_COUNTs; update any
other places that derive SAMPLE_COUNT (the same logic around
GetScaledSampleCount / SAMPLE_COUNT usage) to use the clamped value.
Contributor
Author
|
CodeRabbit feedback reviewed — most findings are inherited from the PR #2002 base and already addressed there: 🔴 RunGrass VRMipBias symbols: fixed in PR #2002 (VR MIP bias fully removed) ✓ |
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
from
c97e03f to
8f25230
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Lighting.hlsl (1)
2255-2279:⚠️ Potential issue | 🟠 MajorKeep legacy envmap decode on the Skyrim curve.
These two paths were previously decoding cubemap/F0 data with the Skyrim helpers as well. Moving them to the generic gamma curve changes reflection tint and intensity for existing assets.
Suggested fix
- material.F0 = Color::GammaToLinear(envColorBase.rgb); + material.F0 = Color::SkyrimGammaToLinear(envColorBase.rgb); ... - float3 envColorBase = Color::GammaToLinear(TexEnvSampler.Sample(SampEnvSampler, envSamplingPoint).xyz); + float3 envColorBase = Color::SkyrimGammaToLinear(TexEnvSampler.Sample(SampEnvSampler, envSamplingPoint).xyz);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 2255 - 2279, The cubemap/F0 decode needs to use the Skyrim gamma curve helper instead of the generic Color::GammaToLinear in two places: replace the use of Color::GammaToLinear when assigning material.F0/material.Roughness inside the CREATOR block (the assignments using SharedData::cubemapCreatorSettings.CubemapColor) and replace Color::GammaToLinear around the TexEnvSampler.Sample result in the non-dynamic-cubemap path (the float3 envColorBase = ... line); apply the same Skyrim-specific decode helper that other cubemap/F0 code uses so material.F0 and material.Roughness are decoded with the Skyrim curve.
♻️ Duplicate comments (4)
package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
106-143:⚠️ Potential issue | 🟠 Major
EdgeWidthstill has no effect on the outer/background band.
maxWidthis hardcoded tokInnerWidth, and the post-search classification never emitsMODE_EDGE_NEIGHBOUR. That makes the configurable outer band dead code and drops background-side hits back toMODE_MAIN.Suggested fix
- // Use the larger of inner/outer widths for the search - uint maxWidth = kInnerWidth; + // Search far enough to classify both the fixed inner band and the configurable outer band. + uint maxWidth = max(kInnerWidth, EdgeWidth); @@ if (nearestEdgeDist != 0xFFFFFFFF) { // Classify based on distance and side if (nearestEdgeDist == 1) { // Immediate neighbor discontinuity: always MODE_EDGE regardless of side ModeTextureRW[dtid] = MODE_EDGE; return; } else if (!nearestWeAreOuter && nearestEdgeDist <= kInnerWidth) { // Inner/foreground band beyond distance 1 ModeTextureRW[dtid] = MODE_EDGE; return; + } else if (nearestWeAreOuter && nearestEdgeDist <= EdgeWidth) { + // Outer/background band + ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR; + return; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl` around lines 106 - 143, The search loop currently uses maxWidth = kInnerWidth so outer-band hits never get detected; change maxWidth to include the outer band (e.g., maxWidth = max(kInnerWidth, kOuterWidth) or use the configured EdgeWidth) and keep the same neighbor detection logic (uses skipEdgeDetection, nearestEdgeDist, nearestWeAreOuter). After the loop, update the classification: if nearestEdgeDist == 1 set ModeTextureRW[dtid] = MODE_EDGE; else if nearestWeAreOuter and nearestEdgeDist <= outerWidth then set ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR; else if !nearestWeAreOuter and nearestEdgeDist <= kInnerWidth set MODE_EDGE; ensure you reference nearestEdgeDist, nearestWeAreOuter, ModeTextureRW, MODE_EDGE_NEIGHBOUR and the chosen outer width symbol so background-side hits are emitted correctly.package/Shaders/VR/VRPostProcessCS.hlsl (1)
19-27:⚠️ Potential issue | 🟠 MajorMirror the full
StereoBlendCBlayout atb1.
StereoBlend.cppuploadsDepthSigma,MaxBlendFactor,ColorDiffThreshold,DebugEdgeTint,DebugMode,FullBlendDistance, andPOMDepthScale, but this cbuffer only declares the tail fields. Every overwrite/debug parameter afterRcpFrameDimis therefore read from the wrong bytes.Suggested fix
cbuffer VRPostProcessCB : register(b1) { float2 FrameDim; float2 RcpFrameDim; + float DepthSigma; + float MaxBlendFactor; + float ColorDiffThreshold; float DebugEdgeTint; // 0 = off, >0 = debug visualization strength uint DebugMode; // 0 = normal, 1 = depth map diagnostic, 2 = full blend depth visualizer float FullBlendDistance; // Linearized depth threshold for full blend zone visualization + float POMDepthScale; float _pad; // Pad to 16-byte alignment };Expected result: the C++
StereoBlendCBdefinition and the HLSLVRPostProcessCBshould have the same field order and compatible types.#!/bin/bash set -euo pipefail echo "=== C++ StereoBlendCB ===" rg -n -A20 -B3 'StereoBlendCB' src/Features/VR.h echo echo "=== Fields written before stereoBlendCB->Update ===" nl -ba src/Features/VR/StereoBlend.cpp | sed -n '62,90p' echo echo "=== HLSL VRPostProcessCB ===" nl -ba package/Shaders/VR/VRPostProcessCS.hlsl | sed -n '19,27p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/VRPostProcessCS.hlsl` around lines 19 - 27, The VRPostProcessCB cbuffer in VRPostProcessCS.hlsl is missing the leading StereoBlendCB fields written by StereoBlend.cpp (DepthSigma, MaxBlendFactor, ColorDiffThreshold, etc.), causing subsequent fields to be misread; update VRPostProcessCB to mirror the exact field order and types of the C++ StereoBlendCB (include DepthSigma, MaxBlendFactor, ColorDiffThreshold, DebugEdgeTint, DebugMode, FullBlendDistance, POMDepthScale, and any padding) so the HLSL layout matches StereoBlendCB used by StereoBlend.cpp and remains 16-byte aligned and bound to register(b1).src/Deferred.cpp (1)
376-396:⚠️ Potential issue | 🟠 MajorAlways bind and clear
t16whenVR_STEREO_OPTis compiled.
GetComputeMainComposite*()still enablesVR_STEREO_OPTwhenever the feature is loaded, but slot 16 is only set/cleared whenstereoMode != Off. In the loaded-but-disabled case, Eye 1 can read an unbound or stale SRV, and the generic clear block still leaves slot 16 dirty.Suggested fix
- if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { - ID3D11ShaderResourceView* modeSRV = vrStereoOpt.GetModeTextureSRV(); - if (modeSRV) - context->CSSetShaderResources(16, 1, &modeSRV); - } + if (REL::Module::IsVR() && vrStereoOpt.loaded) { + ID3D11ShaderResourceView* modeSRV = + vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off ? + vrStereoOpt.GetModeTextureSRV() : + nullptr; + context->CSSetShaderResources(16, 1, &modeSRV); + } @@ - if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { + if (REL::Module::IsVR() && vrStereoOpt.loaded) { ID3D11ShaderResourceView* nullSRV = nullptr; context->CSSetShaderResources(16, 1, &nullSRV); } @@ - ID3D11ShaderResourceView* views[16]{ nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr }; + ID3D11ShaderResourceView* views[17]{}; context->CSSetShaderResources(0, ARRAYSIZE(views), views);Also applies to: 414-417, 591-592, 619-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 376 - 396, The VR stereo mode SRV binding/clearing is conditional on vrStereoOpt.settings.stereoMode, leaving slot t16 dirty when the feature is loaded but disabled; update the code paths around GetComputeMainComposite() / GetComputeMainCompositeInterior() to always bind a valid SRV (either vrStereoOpt.GetModeTextureSRV() when stereoMode != Off or a null SRV when stereoMode == Off) whenever vrStereoOpt.loaded is true, and always clear slot 16 after Dispatch using context->CSSetShaderResources(16, 1, &nullSRV); apply the same change to the other occurrences noted (around the other Dispatch/unbind blocks) so t16 is never left uninitialized or stale.src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorPersist the new debug toggles.
DrawSettings()editsdebugFullBlendDepthanddebugPOMDepth, butSaveSettings()/LoadSettings()still ignore both fields. Those checkboxes reset every restart.Suggested fix
void VRStereoOptimizations::SaveSettings(json& o_json) { o_json["StereoMode"] = settings.stereoMode; o_json["DisocclusionDepthThreshold"] = settings.disocclusionDepthThreshold; o_json["FullBlendDistance"] = settings.fullBlendDistance; o_json["QualityJitterOffset"] = settings.qualityJitterOffset; o_json["FoveatedRegionRadius"] = settings.foveatedRegionRadius; o_json["FoveatedRegionCenterX"] = settings.foveatedRegionCenterX; o_json["FoveatedRegionCenterY"] = settings.foveatedRegionCenterY; o_json["UseEyeTracking"] = settings.useEyeTracking; o_json["DebugVisualization"] = settings.debugVisualization; o_json["DebugSkipMerge"] = settings.debugSkipMerge; o_json["DebugForceAllStencil"] = settings.debugForceAllStencil; o_json["DebugForceAllReprojectCS"] = settings.debugForceAllReprojectCS; o_json["DebugDepthMap"] = settings.debugDepthMap; + o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth; + o_json["DebugPOMDepth"] = settings.debugPOMDepth; o_json["POMDepthScale"] = settings.pomDepthScale; } @@ if (auto it = o_json.find("DebugDepthMap"); it != o_json.end() && it->is_boolean()) settings.debugDepthMap = it->get<bool>(); + if (auto it = o_json.find("DebugFullBlendDepth"); it != o_json.end() && it->is_boolean()) + settings.debugFullBlendDepth = it->get<bool>(); + if (auto it = o_json.find("DebugPOMDepth"); it != o_json.end() && it->is_boolean()) + settings.debugPOMDepth = it->get<bool>(); if (auto it = o_json.find("FullBlendDistance"); it != o_json.end() && it->is_number()) settings.fullBlendDistance = std::clamp(it->get<float>(), 0.0f, 50000.0f); if (auto it = o_json.find("POMDepthScale"); it != o_json.end() && it->is_number()) settings.pomDepthScale = std::clamp(it->get<float>(), 0.0f, 500.0f); }Also applies to: 39-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 21 - 37, SaveSettings/LoadSettings currently ignore the two new debug toggles edited by DrawSettings: settings.debugFullBlendDepth and settings.debugPOMDepth; add persistence for these fields by writing them into SaveSettings (e.g., add JSON entries for DebugFullBlendDepth and DebugPOMDepth) and reading them back in LoadSettings so the boolean values are restored on startup, making sure the JSON keys match between save/load and the read uses the same types as other debug flags (refer to functions VRStereoOptimizations::SaveSettings, VRStereoOptimizations::LoadSettings and the fields debugFullBlendDepth/debugPOMDepth).
🤖 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/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl:
- Around line 18-20: StereoSyncCS.hlsl's StereoOptModeTexture and its
early-return optimization are guarded by VR_STEREO_OPT but
ScreenSpaceShadows.cpp compiles the shader with VR and FRAMEBUFFER without
defining VR_STEREO_OPT and doesn't bind an SRV at t16, so the VR optimization
never runs; fix by either (A) moving the declaration/early-return out of the
VR_STEREO_OPT guard so StereoOptModeTexture (Texture2D<uint>
StereoOptModeTexture) and the early-exit logic are always present when VR is
enabled, or (B) ensure the build path that compiles StereoSyncCS.hlsl for
VR+FRAMEBUFFER defines VR_STEREO_OPT and that ScreenSpaceShadows.cpp binds the
mode SRV to register t16 when compiling for VR (update the code that sets shader
macros or the SRV binding code to attach the mode texture at t16); reference
symbols: StereoSyncCS.hlsl, StereoOptModeTexture, VR_STEREO_OPT, and the binding
logic in ScreenSpaceShadows.cpp that should bind the SRV to t16.
In `@package/Shaders/Lighting.hlsl`:
- Around line 1867-1873: The current FACEGEN_RGB_TINT branch uses the generic
Color::LinearToGamma/Color::GammaToLinear round-trip which shifts skin tones
when linear lighting is enabled; change the round-trip to use the Skyrim facegen
helpers from Common/Color.hlsli (the facegen-specific gamma converters) around
GetFacegenRGBTintBaseColor so that when
SharedData::linearLightingSettings.enableLinearLighting is true you call the
facegen LinearToGamma before GetFacegenRGBTintBaseColor and then the facegen
GammaToLinear after, keeping the existing non-linear branch unchanged.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 13-15: Shader lacks the reflectance SRV and the DebugMode==3
branch so the "Debug POM Depth" toggle is ignored; add a Texture2D<float4>
ReflectanceTexture : register(t3) declaration (matching how StereoBlend binds
t3) and update the pixel/compute path that reads DebugMode (symbol: DebugMode)
to handle value 3 by sampling ReflectanceTexture and using that value in the
overwrite/blend path (the same overwrite behavior used for other debug modes),
ensuring you read from the same UV/coords used for ColorTexture and DepthTexture
so the debugPOMDepth toggle actually shows reflectance output.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The condition for shouldShow currently only checks
globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME), which excludes the
Tween/Pause menu; update the logic in the shouldShow assignment so the
onboarding overlay appears for both menus: either call IsMenuOpen for both menu
names (e.g., globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) ||
globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME)) or, if the UI has a
generic query (e.g., IsAnyMenuOpen or a no-arg IsMenuOpen), use that instead;
keep the other guards (settings.kAutoHideSeconds, globals::game::ui,
globals::menu && !globals::menu->IsEnabled) unchanged and reference the existing
symbols (shouldShow, settings.kAutoHideSeconds, globals::game::ui,
globals::menu) when making the change.
---
Outside diff comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 2255-2279: The cubemap/F0 decode needs to use the Skyrim gamma
curve helper instead of the generic Color::GammaToLinear in two places: replace
the use of Color::GammaToLinear when assigning material.F0/material.Roughness
inside the CREATOR block (the assignments using
SharedData::cubemapCreatorSettings.CubemapColor) and replace
Color::GammaToLinear around the TexEnvSampler.Sample result in the
non-dynamic-cubemap path (the float3 envColorBase = ... line); apply the same
Skyrim-specific decode helper that other cubemap/F0 code uses so material.F0 and
material.Roughness are decoded with the Skyrim curve.
---
Duplicate comments:
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 19-27: The VRPostProcessCB cbuffer in VRPostProcessCS.hlsl is
missing the leading StereoBlendCB fields written by StereoBlend.cpp (DepthSigma,
MaxBlendFactor, ColorDiffThreshold, etc.), causing subsequent fields to be
misread; update VRPostProcessCB to mirror the exact field order and types of the
C++ StereoBlendCB (include DepthSigma, MaxBlendFactor, ColorDiffThreshold,
DebugEdgeTint, DebugMode, FullBlendDistance, POMDepthScale, and any padding) so
the HLSL layout matches StereoBlendCB used by StereoBlend.cpp and remains
16-byte aligned and bound to register(b1).
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 106-143: The search loop currently uses maxWidth = kInnerWidth so
outer-band hits never get detected; change maxWidth to include the outer band
(e.g., maxWidth = max(kInnerWidth, kOuterWidth) or use the configured EdgeWidth)
and keep the same neighbor detection logic (uses skipEdgeDetection,
nearestEdgeDist, nearestWeAreOuter). After the loop, update the classification:
if nearestEdgeDist == 1 set ModeTextureRW[dtid] = MODE_EDGE; else if
nearestWeAreOuter and nearestEdgeDist <= outerWidth then set ModeTextureRW[dtid]
= MODE_EDGE_NEIGHBOUR; else if !nearestWeAreOuter and nearestEdgeDist <=
kInnerWidth set MODE_EDGE; ensure you reference nearestEdgeDist,
nearestWeAreOuter, ModeTextureRW, MODE_EDGE_NEIGHBOUR and the chosen outer width
symbol so background-side hits are emitted correctly.
In `@src/Deferred.cpp`:
- Around line 376-396: The VR stereo mode SRV binding/clearing is conditional on
vrStereoOpt.settings.stereoMode, leaving slot t16 dirty when the feature is
loaded but disabled; update the code paths around GetComputeMainComposite() /
GetComputeMainCompositeInterior() to always bind a valid SRV (either
vrStereoOpt.GetModeTextureSRV() when stereoMode != Off or a null SRV when
stereoMode == Off) whenever vrStereoOpt.loaded is true, and always clear slot 16
after Dispatch using context->CSSetShaderResources(16, 1, &nullSRV); apply the
same change to the other occurrences noted (around the other Dispatch/unbind
blocks) so t16 is never left uninitialized or stale.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: SaveSettings/LoadSettings currently ignore the two new
debug toggles edited by DrawSettings: settings.debugFullBlendDepth and
settings.debugPOMDepth; add persistence for these fields by writing them into
SaveSettings (e.g., add JSON entries for DebugFullBlendDepth and DebugPOMDepth)
and reading them back in LoadSettings so the boolean values are restored on
startup, making sure the JSON keys match between save/load and the read uses the
same types as other debug flags (refer to functions
VRStereoOptimizations::SaveSettings, VRStereoOptimizations::LoadSettings and the
fields debugFullBlendDepth/debugPOMDepth).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cc670b8-8c1d-455f-b62c-bba027de4dd9
📒 Files selected for processing (34)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.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/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.hlslipackage/Shaders/VRStereoOptimizations/modes.hlslisrc/Deferred.cppsrc/Feature.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.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.cpp
✅ Files skipped from review due to trivial changes (13)
- package/Shaders/Common/VR.hlsli
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- src/State.cpp
- src/Features/ExtendedMaterials.h
- src/Globals.h
- src/Feature.cpp
- package/Shaders/RunGrass.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlsli
- src/Features/ScreenSpaceShadows.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl
- package/Shaders/DeferredCompositeCS.hlsl
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- src/Features/ScreenSpaceShadows.h
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Globals.cpp
Comment on lines
+1867
to
+1873
| baseColor.xyz = Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz), uv)); | ||
| } | ||
| # elif defined(FACEGEN_RGB_TINT) | ||
| if (!SharedData::linearLightingSettings.enableLinearLighting) { | ||
| baseColor.xyz = GetFacegenRGBTintBaseColor(baseColor.xyz, uv); | ||
| } else { | ||
| baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv)); | ||
| baseColor.xyz = Color::GammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToGamma(baseColor.xyz), uv)); |
Contributor
There was a problem hiding this comment.
Keep facegen on the Skyrim gamma helpers.
package/Shaders/Common/Color.hlsli still separates the Skyrim 1.6 curve from the generic 2.2 helpers. Replacing the facegen round-trip here will visibly shift skin tone and tint whenever linear lighting is enabled.
Suggested fix
- baseColor.xyz = Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz), uv));
+ baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv));
...
- baseColor.xyz = Color::GammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToGamma(baseColor.xyz), uv));
+ baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| baseColor.xyz = Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz), uv)); | |
| } | |
| # elif defined(FACEGEN_RGB_TINT) | |
| if (!SharedData::linearLightingSettings.enableLinearLighting) { | |
| baseColor.xyz = GetFacegenRGBTintBaseColor(baseColor.xyz, uv); | |
| } else { | |
| baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv)); | |
| baseColor.xyz = Color::GammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToGamma(baseColor.xyz), uv)); | |
| baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv)); | |
| } | |
| # elif defined(FACEGEN_RGB_TINT) | |
| if (!SharedData::linearLightingSettings.enableLinearLighting) { | |
| baseColor.xyz = GetFacegenRGBTintBaseColor(baseColor.xyz, uv); | |
| } else { | |
| baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Lighting.hlsl` around lines 1867 - 1873, The current
FACEGEN_RGB_TINT branch uses the generic
Color::LinearToGamma/Color::GammaToLinear round-trip which shifts skin tones
when linear lighting is enabled; change the round-trip to use the Skyrim facegen
helpers from Common/Color.hlsli (the facegen-specific gamma converters) around
GetFacegenRGBTintBaseColor so that when
SharedData::linearLightingSettings.enableLinearLighting is true you call the
facegen LinearToGamma before GetFacegenRGBTintBaseColor and then the facegen
GammaToLinear after, keeping the existing non-linear branch unchanged.
Comment on lines
+13
to
+15
| Texture2D<float4> ColorTexture : register(t0); // Copy of final composited image | ||
| Texture2D<uint> ModeTexture : register(t1); | ||
| Texture2D<float> DepthTexture : register(t2); |
Contributor
There was a problem hiding this comment.
DebugMode == 3 currently falls through to normal blending.
StereoBlend.cpp sets DebugMode = 3u for debugPOMDepth and binds the reflectance SRV in overwrite mode, but this shader only implements debug branches 1 and 2 and never declares/reads the t3 reflectance input. The "Debug POM Depth" toggle therefore does nothing.
Also applies to: 37-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/VR/VRPostProcessCS.hlsl` around lines 13 - 15, Shader lacks
the reflectance SRV and the DebugMode==3 branch so the "Debug POM Depth" toggle
is ignored; add a Texture2D<float4> ReflectanceTexture : register(t3)
declaration (matching how StereoBlend binds t3) and update the pixel/compute
path that reads DebugMode (symbol: DebugMode) to handle value 3 by sampling
ReflectanceTexture and using that value in the overwrite/blend path (the same
overwrite behavior used for other debug modes), ensuring you read from the same
UV/coords used for ColorTexture and DepthTexture so the debugPOMDepth toggle
actually shows reflectance output.
| static LARGE_INTEGER freq = { 0 }; | ||
|
|
||
| bool shouldShow = settings.kAutoHideSeconds > 0 && globals::state->isMainMenuOpen && globals::menu && !globals::menu->IsEnabled; | ||
| bool shouldShow = settings.kAutoHideSeconds > 0 && globals::game::ui && globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) && globals::menu && !globals::menu->IsEnabled; |
Contributor
There was a problem hiding this comment.
Don't narrow the onboarding overlay to MainMenu only.
This gate now excludes the Tween Menu, even though the overlay copy and controller instructions still say both menus are supported. Users who open only the pause/tween menu won't see the help overlay there.
Suggested fix
- bool shouldShow = settings.kAutoHideSeconds > 0 && globals::game::ui && globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) && globals::menu && !globals::menu->IsEnabled;
+ const bool instructionMenuOpen = globals::game::ui &&
+ (globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) ||
+ globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME));
+ bool shouldShow = settings.kAutoHideSeconds > 0 && instructionMenuOpen && globals::menu && !globals::menu->IsEnabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VR/SettingsUI.cpp` at line 76, The condition for shouldShow
currently only checks globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME),
which excludes the Tween/Pause menu; update the logic in the shouldShow
assignment so the onboarding overlay appears for both menus: either call
IsMenuOpen for both menu names (e.g.,
globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) ||
globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME)) or, if the UI has a
generic query (e.g., IsAnyMenuOpen or a no-arg IsMenuOpen), use that instead;
keep the other guards (settings.kAutoHideSeconds, globals::game::ui,
globals::menu && !globals::menu->IsEnabled) unchanged and reference the existing
symbols (shouldShow, settings.kAutoHideSeconds, globals::game::ui,
globals::menu) when making the change.
267d768 to
7d7d66e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorPersist the debug toggles that
DrawSettings()edits.
debugFullBlendDepthanddebugPOMDepthare user-editable here, butSaveSettings()/LoadSettings()never round-trip them. Both flags reset on restart, and the matchingVR::DrawStereoBlend()debug modes stop being reproducible from config.🛠️ Proposed fix
void VRStereoOptimizations::SaveSettings(json& o_json) { o_json["StereoMode"] = settings.stereoMode; o_json["DisocclusionDepthThreshold"] = settings.disocclusionDepthThreshold; o_json["FullBlendDistance"] = settings.fullBlendDistance; o_json["QualityJitterOffset"] = settings.qualityJitterOffset; o_json["FoveatedRegionRadius"] = settings.foveatedRegionRadius; o_json["FoveatedRegionCenterX"] = settings.foveatedRegionCenterX; o_json["FoveatedRegionCenterY"] = settings.foveatedRegionCenterY; o_json["UseEyeTracking"] = settings.useEyeTracking; o_json["DebugVisualization"] = settings.debugVisualization; o_json["DebugSkipMerge"] = settings.debugSkipMerge; o_json["DebugForceAllStencil"] = settings.debugForceAllStencil; o_json["DebugForceAllReprojectCS"] = settings.debugForceAllReprojectCS; o_json["DebugDepthMap"] = settings.debugDepthMap; + o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth; + o_json["DebugPOMDepth"] = settings.debugPOMDepth; o_json["POMDepthScale"] = settings.pomDepthScale; } … if (auto it = o_json.find("DebugDepthMap"); it != o_json.end() && it->is_boolean()) settings.debugDepthMap = it->get<bool>(); + if (auto it = o_json.find("DebugFullBlendDepth"); it != o_json.end() && it->is_boolean()) + settings.debugFullBlendDepth = it->get<bool>(); + if (auto it = o_json.find("DebugPOMDepth"); it != o_json.end() && it->is_boolean()) + settings.debugPOMDepth = it->get<bool>(); if (auto it = o_json.find("FullBlendDistance"); it != o_json.end() && it->is_number()) settings.fullBlendDistance = std::clamp(it->get<float>(), 0.0f, 50000.0f);Also applies to: 39-69, 252-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 21 - 37, SaveSettings/LoadSettings currently do not persist the user-editable debug toggles debugFullBlendDepth and debugPOMDepth edited by DrawSettings/VR::DrawStereoBlend, so they reset on restart; update SaveSettings (function VRStereoOptimizations::SaveSettings) to write settings.debugFullBlendDepth and settings.debugPOMDepth into the JSON (use keys like "DebugFullBlendDepth" and "DebugPOMDepth") and update the corresponding LoadSettings logic to read those keys back into settings.debugFullBlendDepth and settings.debugPOMDepth so the flags round-trip with the rest of settings.src/Deferred.cpp (1)
375-395:⚠️ Potential issue | 🟠 MajorBind and clear
t16on the same condition used to compileVR_STEREO_OPT.
Deferred::GetComputeMainComposite*()enablesVR_STEREO_OPTwhenevervrStereoOpt.loaded, butDeferred::DeferredPasses()only touches slot 16 whenstereoMode != Off. In the loaded-but-disabled case the composite shader can still read stale or unbound data fromt16, and the generic cleanup block never clears that slot.🛠️ Proposed fix
- if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { - ID3D11ShaderResourceView* modeSRV = vrStereoOpt.GetModeTextureSRV(); - if (modeSRV) - context->CSSetShaderResources(16, 1, &modeSRV); - } + if (REL::Module::IsVR() && vrStereoOpt.loaded) { + ID3D11ShaderResourceView* modeSRV = + vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off ? + vrStereoOpt.GetModeTextureSRV() : + nullptr; + context->CSSetShaderResources(16, 1, &modeSRV); + } … - if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { + if (REL::Module::IsVR() && vrStereoOpt.loaded) { ID3D11ShaderResourceView* nullSRV = nullptr; context->CSSetShaderResources(16, 1, &nullSRV); } … - ID3D11ShaderResourceView* views[16]{ nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr }; + ID3D11ShaderResourceView* views[17]{}; context->CSSetShaderResources(0, ARRAYSIZE(views), views);Also applies to: 415-416, 590-591, 618-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 375 - 395, DeferredPasses binds t16 only when vrStereoOpt.settings.stereoMode != Off but the composite shaders (Deferred::GetComputeMainComposite*()) enable VR_STEREO_OPT whenever vrStereoOpt.loaded, which can leave t16 stale; change the SRV bind and unbind checks to the same condition used at compile time (i.e., REL::Module::IsVR() && vrStereoOpt.loaded) so slot 16 is always set to a valid SRV or explicitly cleared when vrStereoOpt.loaded is true, using vrStereoOpt.GetModeTextureSRV() for bind and a null SRV for unbind (affects the CSSetShaderResources calls that touch slot 16 and the cleanup paths at the other occurrences you noted).
🧹 Nitpick comments (4)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
1-1: Optional PR polish: commit title casing + issue keyword.Consider:
feat(vr): optimize screen-space shadows for vr(lowercase description), and if tracked, add an issue keyword likeImplements #<id>in the PR body.As per coding guidelines, PRs should include Conventional Commit formatting suggestions and Issue References where applicable.
🤖 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 1, Update the PR title to follow Conventional Commits (e.g., "feat(vr): optimize screen-space shadows for vr" with a lowercase description) and add an issue reference line in the PR body like "Implements #<id>" so the change in ExtendedMaterials.hlsli (the Extended Materials shader file) is tracked; ensure the commit message and PR body are updated accordingly before merging.src/Features/ScreenSpaceShadows.h (1)
65-65: DefaultenableStereoSync = falseis a behavior change for existing users.Existing VR users with saved
enableStereoSync=truewill retain that setting (per LoadSettings behavior in the context snippet), but new installations will default to disabled. Consider whether this warrants a note in release documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceShadows.h` at line 65, The field enableStereoSync in class/struct ScreenSpaceShadows currently defaults to false which changes behavior for new installs; to preserve prior VR behavior, change the default initializer of enableStereoSync to true in ScreenSpaceShadows.h (symbol: enableStereoSync) and also add a short note to the release/upgrade documentation indicating the default for enableStereoSync for clarity; update any related LoadSettings/SaveSettings handling if it assumes the old default.package/Shaders/DeferredCompositeCS.hlsl (1)
100-107: Consider using named constants instead of magic numbers.The mode values
1and2correspond toMODE_EDGEandMODE_MAINfrommodes.hlsli. Using the named constants would improve readability and prevent errors if mode values change.💡 Suggested improvement
+#include "VRStereoOptimizations/modes.hlsli" ... - if (mode == 2 || mode == 1) { // MODE_MAIN or MODE_EDGE — stencil-culled, reprojected by StereoBlend + if (mode == MODE_MAIN || mode == MODE_EDGE) { // stencil-culled, reprojected by StereoBlend🤖 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 - 107, Replace the magic numeric literals 1 and 2 in the VR_STEREO_OPT block with the corresponding named constants from modes.hlsli (MODE_EDGE and MODE_MAIN) to improve readability and maintainability; update the condition in the DeferredCompositeCS.hlsl code that checks StereoOptModeTexture[uint2(dispatchID.xy)] to use MODE_MAIN and MODE_EDGE (and include or reference modes.hlsli if necessary) so the comparison becomes explicit and robust if values change.src/Features/VRStereoOptimizations.h (1)
194-195: Consider documenting cache invalidation strategy.The
dssCachemaps original DSS pointers to modified versions. If the game recreates DSS objects with the same parameters but different addresses, stale entries will accumulate. This is likely fine for typical usage patterns, but a brief comment about expected lifecycle would help maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.h` around lines 194 - 195, dssCache currently maps ID3D11DepthStencilState* to modified com_ptrs but has no comment about when entries are invalidated; add a short comment next to dssCache explaining the expected lifecycle and chosen invalidation strategy (e.g., "entries are valid while the original ID3D11DepthStencilState* lives; caller must clear or rebuild this cache on device reset or when the game recreates DSS objects", or document that alternative strategies like keying by state descriptor or pruning on Release/creation hooks should be used if reuse-by-address is not guaranteed). Mention the symbol dssCache and the involved type ID3D11DepthStencilState* and, if applicable, reference the code paths that should trigger cache clearing (device reset, depth-stencil creation hook, or explicit ClearDssCache/OnDssDestroyed helper).
🤖 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/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli:
- Around line 259-276: The right-eye UV checks
(coord_out_of_eye/coord_offset_out_of_eye) only test the left seam and miss
samples that step past the right-side active region when DynamicRes.x < 1;
update the boolean guards in the ScreenSpaceShadows shader to also reject
coordinates >= inParameters.DynamicRes.x for the right-eye path (and
symmetrically <= 0.0 for the left-eye branch if applicable) so that depths.x and
depths.y use 1.0 when coord.x or coord_with_offset.x lie outside the full active
interval [0.5*inParameters.DynamicRes.x, inParameters.DynamicRes.x]; adjust the
expressions referenced (coord_out_of_eye, coord_offset_out_of_eye) used before
calling inParameters.DepthTexture.SampleLevel with
inParameters.PointBorderSampler so cross-eye reads clamp to FarDepthValue.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 29-31: The vrStereoOptActive predicate should also verify that the
mode texture SRV exists before choosing the overwrite shader: update the
condition that defines vrStereoOptActive to include a non-null check of
VRStereoOptimizations::GetModeTextureSRV() (the same additional check must be
applied to the analogous condition around lines 126-128), so when
GetModeTextureSRV() returns nullptr the code falls back to the
legacy/non-overwrite path instead of selecting stereoBlendOverwriteCS and
reading from t2.
---
Duplicate comments:
In `@src/Deferred.cpp`:
- Around line 375-395: DeferredPasses binds t16 only when
vrStereoOpt.settings.stereoMode != Off but the composite shaders
(Deferred::GetComputeMainComposite*()) enable VR_STEREO_OPT whenever
vrStereoOpt.loaded, which can leave t16 stale; change the SRV bind and unbind
checks to the same condition used at compile time (i.e., REL::Module::IsVR() &&
vrStereoOpt.loaded) so slot 16 is always set to a valid SRV or explicitly
cleared when vrStereoOpt.loaded is true, using vrStereoOpt.GetModeTextureSRV()
for bind and a null SRV for unbind (affects the CSSetShaderResources calls that
touch slot 16 and the cleanup paths at the other occurrences you noted).
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: SaveSettings/LoadSettings currently do not persist the
user-editable debug toggles debugFullBlendDepth and debugPOMDepth edited by
DrawSettings/VR::DrawStereoBlend, so they reset on restart; update SaveSettings
(function VRStereoOptimizations::SaveSettings) to write
settings.debugFullBlendDepth and settings.debugPOMDepth into the JSON (use keys
like "DebugFullBlendDepth" and "DebugPOMDepth") and update the corresponding
LoadSettings logic to read those keys back into settings.debugFullBlendDepth and
settings.debugPOMDepth so the flags round-trip with the rest of settings.
---
Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 1: Update the PR title to follow Conventional Commits (e.g., "feat(vr):
optimize screen-space shadows for vr" with a lowercase description) and add an
issue reference line in the PR body like "Implements #<id>" so the change in
ExtendedMaterials.hlsli (the Extended Materials shader file) is tracked; ensure
the commit message and PR body are updated accordingly before merging.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: Replace the magic numeric literals 1 and 2 in the
VR_STEREO_OPT block with the corresponding named constants from modes.hlsli
(MODE_EDGE and MODE_MAIN) to improve readability and maintainability; update the
condition in the DeferredCompositeCS.hlsl code that checks
StereoOptModeTexture[uint2(dispatchID.xy)] to use MODE_MAIN and MODE_EDGE (and
include or reference modes.hlsli if necessary) so the comparison becomes
explicit and robust if values change.
In `@src/Features/ScreenSpaceShadows.h`:
- Line 65: The field enableStereoSync in class/struct ScreenSpaceShadows
currently defaults to false which changes behavior for new installs; to preserve
prior VR behavior, change the default initializer of enableStereoSync to true in
ScreenSpaceShadows.h (symbol: enableStereoSync) and also add a short note to the
release/upgrade documentation indicating the default for enableStereoSync for
clarity; update any related LoadSettings/SaveSettings handling if it assumes the
old default.
In `@src/Features/VRStereoOptimizations.h`:
- Around line 194-195: dssCache currently maps ID3D11DepthStencilState* to
modified com_ptrs but has no comment about when entries are invalidated; add a
short comment next to dssCache explaining the expected lifecycle and chosen
invalidation strategy (e.g., "entries are valid while the original
ID3D11DepthStencilState* lives; caller must clear or rebuild this cache on
device reset or when the game recreates DSS objects", or document that
alternative strategies like keying by state descriptor or pruning on
Release/creation hooks should be used if reuse-by-address is not guaranteed).
Mention the symbol dssCache and the involved type ID3D11DepthStencilState* and,
if applicable, reference the code paths that should trigger cache clearing
(device reset, depth-stencil creation hook, or explicit
ClearDssCache/OnDssDestroyed helper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fcec754-e7ef-43cd-95a5-d26548440ab3
📒 Files selected for processing (32)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlslifeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.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.hlslipackage/Shaders/VRStereoOptimizations/modes.hlslisrc/Deferred.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceShadows.cppsrc/Features/ScreenSpaceShadows.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.cpp
💤 Files with no reviewable changes (1)
- src/Globals.h
✅ Files skipped from review due to trivial changes (12)
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl
- src/State.cpp
- package/Shaders/RunGrass.hlsl
- package/Shaders/Common/VR.hlsli
- src/Features/ExtendedMaterials.h
- package/Shaders/DistantTree.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlsli
- src/Features/ScreenSpaceShadows.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- package/Shaders/Common/SharedData.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- package/Shaders/Lighting.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
- package/Shaders/VR/StereoBlendCS.hlsl
Comment on lines
+29
to
+31
| bool vrStereoOptActive = globals::features::vr.stereoOpt.loaded && | ||
| globals::features::vr.stereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off && | ||
| stereoBlendOverwriteCS; |
Contributor
There was a problem hiding this comment.
Fall back to the legacy blend path when the mode texture is unavailable.
vrStereoOptActive only checks loaded, stereoMode, and stereoBlendOverwriteCS, but VRStereoOptimizations::GetModeTextureSRV() can still return nullptr. In that state the overwrite shader is selected anyway and reads missing classification data from t2 instead of cleanly staying on the non-overwrite path.
🛠️ Proposed fix
- bool vrStereoOptActive = globals::features::vr.stereoOpt.loaded &&
- globals::features::vr.stereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off &&
- stereoBlendOverwriteCS;
+ auto* stereoModeSRV = globals::features::vr.stereoOpt.GetModeTextureSRV();
+ bool vrStereoOptActive = globals::features::vr.stereoOpt.loaded &&
+ globals::features::vr.stereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off &&
+ stereoBlendOverwriteCS &&
+ stereoModeSRV;
…
if (isOverwriteMode) {
- ID3D11ShaderResourceView* modeSRV = globals::features::vr.stereoOpt.GetModeTextureSRV();
- context->CSSetShaderResources(2, 1, &modeSRV);
+ context->CSSetShaderResources(2, 1, &stereoModeSRV);Also applies to: 126-128
🤖 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 29 - 31, The vrStereoOptActive
predicate should also verify that the mode texture SRV exists before choosing
the overwrite shader: update the condition that defines vrStereoOptActive to
include a non-null check of VRStereoOptimizations::GetModeTextureSRV() (the same
additional check must be applied to the analogous condition around lines
126-128), so when GetModeTextureSRV() returns nullptr the code falls back to the
legacy/non-overwrite path instead of selecting stereoBlendOverwriteCS and
reading from t2.
5bf82e1 to
5536a01
Compare
|
✅ A pre-release build is available for this PR: |
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
5 times, most recently
from
adfbc3f to
3ada805
Compare
Collaborator
|
Taking to draft until dependency merged. |
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
from
8eb5f0d to
3701ae1
Compare
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>
Reduced sample count for right eye when VRStereoOptimizations active (StereoBlend overwrites most right-eye pixels anyway). StereoSync toggle exposed in VR settings UI. Configurable right-eye reduction level (half or quarter samples). Depends on: feat(vr): VR stereo reprojection optimizations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vrnord
force-pushed
the
pr/vr-sss-optimization
branch
2 times, most recently
from
3ebb498 to
932ebda
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reduces right-eye SSS cost when VR stereo optimizations are active.
Depends on: #2002 (VR stereo reprojection optimizations)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements