feat(vr): reproject stereo#2002
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughIntroduces a comprehensive VR stereo optimization feature comprising stencil-based pixel classification, compute-shader-driven reprojection, and an overwrite-mode stereo-blend pipeline. Adds new compute shaders for per-pixel mode detection and cross-eye color reprojection, integrates stereo state management into D3D11 hooks, extends shader gamma/alpha logic, and modifies the deferred composite to bind mode-texture resources. Changes
Sequence DiagramsequenceDiagram
participant CPU as CPU (Deferred Pass)
participant SC as StencilCS
participant SW as StencilWrite<br/>(VS+PS)
participant RC as ReprojectionCS
participant SBC as StereoBlendCS<br/>(Overwrite)
participant GPU as GPU Output
CPU->>SC: DispatchStencil()<br/>Input: DepthTexture
SC->>SC: Classify each pixel<br/>(MODE_DISOCCLUDED,<br/>MODE_EDGE, MODE_MAIN,<br/>MODE_FULL_BLEND)
SC->>GPU: Write mode → ModeTexture
CPU->>SW: ExecuteStencilWritePass()<br/>Input: ModeTexture
SW->>SW: Discard MODE_DISOCCLUDED/<br/>MODE_FULL_BLEND
SW->>GPU: Write stencil=1<br/>for MODE_EDGE/MAIN
CPU->>RC: DispatchReprojection()<br/>Input: OutputRW (Eye0),<br/>DepthTexture, ModeTexture
RC->>RC: Reproject Eye0→Eye1<br/>for MODE_MAIN pixels
RC->>GPU: Write Eye1 pixels<br/>to OutputRW
CPU->>SBC: DrawStereoBlend()<br/>Overwrite Mode
SBC->>SBC: Read ModeTexture,<br/>reproject to other eye,<br/>depth-weighted blend
SBC->>GPU: Write merged<br/>result to OutputRW
GPU->>GPU: Present stereo<br/>composited output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
This was referenced Mar 22, 2026
|
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: 10
🧹 Nitpick comments (3)
package/Shaders/VR/VRPostProcessCS.hlsl (1)
29-33: Split theMODE_*contract into a shared include.These IDs now exist here and in
VRStereoOptimizations/cbuffers.hlsli. If one side changes, this pass will silently reinterpret the classification texture. Extract the mode constants into a tiny shared header and keep the pass-specific cbuffers separate.🤖 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_* definitions (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated between VRPostProcessCS.hlsl and VRStereoOptimizations/cbuffers.hlsli and must be centralized: create a tiny shared header (e.g., VRModes.hlsli) containing only those MODE_* `#defines` or enum, replace the duplicated defines in VRPostProcessCS.hlsl and VRStereoOptimizations/cbuffers.hlsli with an `#include` of that shared header, and leave pass-specific cbuffers (the existing cbuffer blocks) unchanged so both files reference the single source of truth for the mode IDs.package/Shaders/VR/StereoBlendCS.hlsl (1)
28-34: Consider extracting shared MODE constants to avoid duplication.The MODE constants are duplicated here because
cbuffers.hlslihas a conflicting cbuffer on b1. While the comment explains the rationale, this creates a maintenance burden if mode values change.A cleaner approach would be to extract just the mode constants into a separate header (e.g.,
ModeConstants.hlsli) that both files can include without cbuffer conflicts.🤖 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, Extract the duplicated MODE_* defines (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) into a new shared header (e.g., ModeConstants.hlsli) that contains only these constant definitions (no cbuffers), then replace the local defines in StereoBlendCS.hlsl and any other file (e.g., VRStereoOptimizations/cbuffers.hlsli) with an `#include` of that new header so both use the single source of truth and avoid cbuffer conflicts; ensure the new header has no b1 cbuffer declarations so it can be safely included.src/Features/VRStereoOptimizations.cpp (1)
240-287: Shader compilation failures don't prevent feature activation.When shader compilation fails, the code logs an error but continues. Later,
DispatchStencil()(line 406) checks for null shaders and returns early, which is correct. However, the feature'ssettings.stereoModecould still beEnable, potentially causing repeated failed dispatch attempts each frame.Consider adding a flag to track compilation success and disable the feature gracefully if critical shaders fail to compile.
Suggested approach
void VRStereoOptimizations::CompileShaders() { + bool allShadersCompiled = true; + std::vector<std::pair<const char*, const char*>> csDefines = { { "VR", nullptr }, { "FRAMEBUFFER", nullptr } }; // ... existing compilation code ... if (auto* ptr = Util::CompileShader(L"Data\\Shaders\\VRStereoOptimizations\\StencilCS.hlsl", csDefines, "cs_5_0")) stencilCS.attach(reinterpret_cast<ID3D11ComputeShader*>(ptr)); - else + else { logger::error("[VRStereoOptimizations] Failed to compile StencilCS"); + allShadersCompiled = false; + } // ... repeat for other critical shaders ... + + if (!allShadersCompiled) { + logger::warn("[VRStereoOptimizations] Critical shaders failed to compile, feature disabled"); + settings.stereoMode = StereoMode::Off; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.cpp` around lines 240 - 287, The shader compile path in VRStereoOptimizations::CompileShaders currently logs failures but leaves the feature active; add a member boolean (e.g., shadersCompiledOk) initialized true and set it false if any critical shader compilation (StencilCS/stencilWriteVS/stencilWritePS/ReprojectionCS or CASCS as needed) fails in CompileShaders (check pointers for null and flip the flag), and then use that flag in activation code and at the start of DispatchStencil (and any other dispatch paths) to return early or set settings.stereoMode = Disable so the feature is gracefully disabled when shadersCompiledOk is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 328: The out-parameter pixelOffset is initialized to 0 (neutral should be
0.5), so any early return paths report a non-neutral offset; update the default
initialization of pixelOffset at the top of the routine in
ExtendedMaterials.hlsli (the assignment currently at line with "pixelOffset =
0;") to use the midpoint neutral value (0.5) so paths that skip the intersection
logic return the correct neutral offset consistent with the later logic that
treats 0.5 as no displacement.
In `@package/Shaders/Common/VR.hlsli`:
- Around line 630-638: GetVRVSOutput() currently applies an Eye 1-only jitter by
modifying vsout.VRPosition when a_eyeIndex == 1, which causes mismatched
projection between eyes; remove the conditional block that computes kJitterNDC
and adjusts vsout.VRPosition so the helper returns identical projections for
both eyes, and if eye-specific subpixel sampling is required move that jitter to
an eye-specific post-vertex step (not inside GetVRVSOutput) so symbols to edit
are the GetVRVSOutput function and the vsout.VRPosition modification guarded by
a_eyeIndex == 1.
In `@package/Shaders/Lighting.hlsl`:
- Around line 1783-1795: The TREE_ANIM-specific branch currently only checks
SharedData::VRMipBias < 0 and thus ignores SharedData::VRMipBiasMode; change the
condition used where vrFoliageBias is computed so it also requires
SharedData::VRMipBiasMode == 1 (i.e., make the if that wraps the depth-based
bias calculation include the VRMipBiasMode check like the non-TREE_ANIM path),
ensuring the later use in TexColorSampler.SampleBias and the depth query via
SharedData::GetScreenDepth only run when the mode is enabled.
In `@package/Shaders/RunGrass.hlsl`:
- Around line 483-489: The shader references (SharedData::VRMipBias,
VRMipBiasMode, VRMipBiasNearDist, VRMipBiasFarDist) no longer exist; update the
vrGrassBias calculation (the vrGrassBias local and its use with
SharedData::GetScreenDepth) to read the new VR settings source used by the
project (e.g., the new VRSettings/VRParams accessor or struct) and wire the same
logic to those fields, and make the identical change in the other occurrence
around lines 864-870; specifically, replace SharedData::VRMipBias,
SharedData::VRMipBiasMode, SharedData::VRMipBiasNearDist, and
SharedData::VRMipBiasFarDist with the new symbol names or accessors (or call the
new getter function) so the saturate((linDepth - near)/max(far-near,1.0)) and
vrGrassBias = VRMipBias * t computation compile against the new API.
- Around line 505-513: The alpha-test uses an eye-index bias
(convergenceEyeIndex from Stereo::GetEyeIndexPS) to lower AlphaTestRefRS for the
right eye, causing mismatched grass cutouts between eyes; remove the
right-eye-only adjustment so AlphaTestRefRS is used symmetrically (delete the
subtraction of convergenceEyeIndex * 0.1 or guard it out) wherever it appears
(references: AlphaTestRefRS, Stereo::GetEyeIndexPS, convergenceEyeIndex,
diffuseAlpha) — apply the same removal in both occurrences (the grass
pixel/fragment shader alpha-test blocks that compute alphaRef and compare
diffuseAlpha).
In `@src/Deferred.cpp`:
- Around line 372-378: The slot 16 SRV must be explicitly set even when
GetModeTextureSRV() returns null to avoid leaving stale data; in the block that
checks REL::Module::IsVR() and vrStereoOpt settings, call
context->CSSetShaderResources(16, 1, &modeSRV) unconditionally (using the
modeSRV pointer returned from vrStereoOpt.GetModeTextureSRV()), so the shader
resource view for slot 16 is either bound to the texture or cleared (nullptr) to
ensure defined shader behavior.
In `@src/Features/VR.h`:
- Line 263: The JSON settings round-trip doesn't persist the expanded
StereoBlendDebugMode values because VR::LoadSettings() / VR::SaveSettings()
still only map up to StereoBlendColorThreshold; update those functions to
include the StereoBlendDebugMode field (and any new enum/value entries up to 5)
in both save and load paths so the value is serialized and deserialized (respect
clamping already done by StereoBlendDebugMode = std::clamp(...)); reference
VR::LoadSettings, VR::SaveSettings, StereoBlendDebugMode and
StereoBlendColorThreshold when making the change.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 126-130: The code may leave slot 2 populated with a previous SRV
when globals::features::vrStereoOptimizations.GetModeTextureSRV() returns null;
update the isOverwriteMode branch in StereoBlend.cpp so that after fetching
modeSRV you explicitly bind a null SRV to slot 2 if modeSRV is nullptr (via
context->CSSetShaderResources(2, 1, &nullPtr)) or alternatively disable/bail out
of overwrite mode when the required ModeTexture SRV is missing; target the block
that sets modeSRV and the compute shader binding for stereoBlendOverwriteCS to
ensure slot 2 is never left with stale data.
In `@src/Features/VRStereoOptimizations.h`:
- Around line 36-42: The PixelMode enum in VRStereoOptimizations.h is missing
MODE_FULL_BLEND which leads to a mismatch with shader constants; update the enum
(PixelMode) to include MODE_FULL_BLEND = 4 (named MODE_FULL_BLEND) alongside the
existing entries so it stays synchronized with StencilCS.hlsl and
StereoBlendCS.hlsl references; locate the enum declaration (PixelMode) and
insert MODE_FULL_BLEND = 4 in the sequence (e.g., after MODE_EDGE_NEIGHBOUR)
maintaining the uint8_t type and existing comment style.
In `@src/Globals.cpp`:
- Around line 291-297: The current guard skips stencil-rewrite when
pDepthStencilState is null, so when globals::game::isVR and
globals::features::vrStereoOptimizations (stereoOpt.loaded &&
stereoOpt.IsStencilActive()) are true you must handle the null-bind case:
instead of leaving OMSetDepthStencilState(nullptr, ...) to bind the implicit
default state, obtain or synthesize a modified default DSS via the
vrStereoOptimizations helper and bind that (i.e. treat nullptr the same as a
concrete state by calling the existing GetOrCreateModifiedDSS-like logic or a
new GetOrCreateModifiedDefaultDSS), assign the returned state to
pDepthStencilState, and set StencilRef = 1 so the NOT_EQUAL stencil test still
culls Eye 1 pixels; this change should live alongside the existing branch that
currently handles non-null pDepthStencilState.
---
Nitpick comments:
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 28-34: Extract the duplicated MODE_* defines (MODE_DISOCCLUDED,
MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) into a new shared
header (e.g., ModeConstants.hlsli) that contains only these constant definitions
(no cbuffers), then replace the local defines in StereoBlendCS.hlsl and any
other file (e.g., VRStereoOptimizations/cbuffers.hlsli) with an `#include` of that
new header so both use the single source of truth and avoid cbuffer conflicts;
ensure the new header has no b1 cbuffer declarations so it can be safely
included.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 29-33: The MODE_* definitions (MODE_DISOCCLUDED, MODE_EDGE,
MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated between
VRPostProcessCS.hlsl and VRStereoOptimizations/cbuffers.hlsli and must be
centralized: create a tiny shared header (e.g., VRModes.hlsli) containing only
those MODE_* `#defines` or enum, replace the duplicated defines in
VRPostProcessCS.hlsl and VRStereoOptimizations/cbuffers.hlsli with an `#include`
of that shared header, and leave pass-specific cbuffers (the existing cbuffer
blocks) unchanged so both files reference the single source of truth for the
mode IDs.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 240-287: The shader compile path in
VRStereoOptimizations::CompileShaders currently logs failures but leaves the
feature active; add a member boolean (e.g., shadersCompiledOk) initialized true
and set it false if any critical shader compilation
(StencilCS/stencilWriteVS/stencilWritePS/ReprojectionCS or CASCS as needed)
fails in CompileShaders (check pointers for null and flip the flag), and then
use that flag in activation code and at the start of DispatchStencil (and any
other dispatch paths) to return early or set settings.stereoMode = Disable so
the feature is gracefully disabled when shadersCompiledOk is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 122fd7df-ce93-4f95-8039-6e0d862614c8
📒 Files selected for processing (27)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/VR/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/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-stereo-opt
branch
from
350e9f8 to
ce8584f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/Features/VR/StereoBlend.cpp (1)
126-129:⚠️ Potential issue | 🟠 MajorDon’t leave
ModeTexturestale when the mode SRV is missing.
stereoBlendOverwriteCSreadsModeTextureont2unconditionally. IfGetModeTextureSRV()returns null and this branch skipsCSSetShaderResources(2, …), slot 2 keeps whatever SRV the previous compute pass left there, so overwrite classification becomes nondeterministic. Either clear the slot explicitly or abort overwrite mode before dispatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VR/StereoBlend.cpp` around lines 126 - 129, When isOverwriteMode is true but globals::features::vrStereoOptimizations.GetModeTextureSRV() returns null, slot 2 is left stale causing nondeterministic reads by stereoBlendOverwriteCS which always samples ModeTexture; update the branch handling in the isOverwriteMode path to either explicitly clear slot 2 via context->CSSetShaderResources(2, 1, nullptr) when modeSRV is null, or disable/abort overwrite mode before dispatch so stereoBlendOverwriteCS never runs without a valid ModeTexture; reference isOverwriteMode, globals::features::vrStereoOptimizations.GetModeTextureSRV(), context->CSSetShaderResources, and stereoBlendOverwriteCS when making the change.src/Globals.cpp (1)
293-299:⚠️ Potential issue | 🟠 MajorHandle
OMSetDepthStencilState(nullptr, …)while stencil culling is active.
nullptris a valid D3D11 bind for the implicit default DSS. This guard skips the rewrite in exactly that case, so Eye 1 pixels bypass the NOT_EQUAL test whenever the engine falls back to the default state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.cpp` around lines 293 - 299, The current guard skips stencil rewrite when pDepthStencilState is nullptr, letting Eye 1 pixels bypass NOT_EQUAL; remove or adjust the pDepthStencilState check so that when globals::features::vrStereoOptimizations.IsStencilActive() is true you still call stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState) (handling a nullptr input) and set StencilRef = 1; update the condition around globals::game::isVR and vrStereoOptimizations to trigger the modified DSS creation even for the implicit default (nullptr) case so stencil culling works correctly.
🧹 Nitpick comments (1)
package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
1-14: Consider extracting MODE constants to a shared header without the cbuffer.Per the context snippets,
StereoBlendCS.hlslduplicates the MODE_ constants (with a comment explaining the cbuffer conflict), andDeferredCompositeCS.hlsluses hardcoded numeric valuesmode == 2 || mode == 1. This creates a synchronization risk if mode values change.Consider splitting
cbuffers.hlsliinto two files:
modes.hlsli— only the#define MODE_*constantscbuffers.hlsli— includesmodes.hlsliplus the cbuffer declarationThis would allow
StereoBlendCS.hlslandDeferredCompositeCS.hlslto include only the constants without cbuffer conflicts.🤖 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 1 - 14, Extract the MODE_* definitions out of cbuffers.hlsli into a new header (e.g., modes.hlsli) containing only the `#define` MODE_DISOCCLUDED / MODE_EDGE / MODE_MAIN / MODE_FULL_BLEND constants, then update cbuffers.hlsli to `#include` "modes.hlsli" and retain the cbuffer; update StereoBlendCS.hlsl and DeferredCompositeCS.hlsl to include only "modes.hlsli" (removing duplicated MODE_* definitions and any hardcoded numeric checks like mode == 2 || mode == 1) so all shaders reference the single source of truth for MODE_* values.
🤖 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/VR/StereoBlendCS.hlsl`:
- Around line 40-50: SampleReprojectedColor currently takes an int2 reprojPx
which forces samples to texel centers and prevents bilinear sub-pixel filtering;
change SampleReprojectedColor to accept a float2 reprojPx (or add a float2
fractional parameter) and update Stereo::ReprojectToOtherEye to pass the
fractional reprojection/UV instead of a rounded int2, then compute sampleUV
using that float2 (e.g. sampleUV = (reprojPx + 0.5) / texSize), keep the same
minUV/maxUV clamping and LinearSampler SampleLevel call so the linear sampler
can perform proper bilinear sub-pixel filtering.
In `@src/Features/TerrainHelper.cpp`:
- Around line 34-38: TerrainHelper::LoadSettings currently calls
o_json["EnableParallax"].get<bool>() directly which can throw on malformed
input; change it to validate the JSON value first (e.g., check
o_json.contains("EnableParallax") and o_json["EnableParallax"].is_boolean() or
accept convertible types like strings/numbers), then assign to
settings.enableParallax only when the type is valid; on invalid types, log a
warning and leave settings.enableParallax at its safe default (or coerce a
sensible value) to ensure graceful degradation rather than throwing in
TerrainHelper::LoadSettings.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 305-320: BeginPerfEvent("VR Stereo Opt - Stencil") is called
before the depthSRV null check, so an early return leaves the perf-event stack
unbalanced; update the DispatchStencil flow to balance it by calling
EndPerfEvent (guarded by globals::state->frameAnnotations) before returning when
depthSRV is null, or move the BeginPerfEvent call to after the depthSRV
check—refer to globals::state->BeginPerfEvent / EndPerfEvent, the depthSRV guard
(renderer->GetDepthStencilData().depthStencils[...].depthSRV), and the early
return to implement the fix.
- Around line 21-36: The new overwrite controls exposed by DrawSettings —
POMDepthScale, debugFullBlendDepth, and debugPOMDepth — are not being persisted;
update VRStereoOptimizations::SaveSettings and the corresponding
VRStereoOptimizations::LoadSettings to serialize and deserialize these three
fields (use the same JSON keys as used/displayed in DrawSettings) so the values
in the settings struct are round-tripped across restarts; modify the
SaveSettings to set o_json["POMDepthScale"], o_json["DebugFullBlendDepth"], and
o_json["DebugPOMDepth"] and update LoadSettings to read those keys back into
settings.POMDepthScale, settings.debugFullBlendDepth, and settings.debugPOMDepth
(handle missing keys with sensible defaults).
---
Duplicate comments:
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 126-129: When isOverwriteMode is true but
globals::features::vrStereoOptimizations.GetModeTextureSRV() returns null, slot
2 is left stale causing nondeterministic reads by stereoBlendOverwriteCS which
always samples ModeTexture; update the branch handling in the isOverwriteMode
path to either explicitly clear slot 2 via context->CSSetShaderResources(2, 1,
nullptr) when modeSRV is null, or disable/abort overwrite mode before dispatch
so stereoBlendOverwriteCS never runs without a valid ModeTexture; reference
isOverwriteMode, globals::features::vrStereoOptimizations.GetModeTextureSRV(),
context->CSSetShaderResources, and stereoBlendOverwriteCS when making the
change.
In `@src/Globals.cpp`:
- Around line 293-299: The current guard skips stencil rewrite when
pDepthStencilState is nullptr, letting Eye 1 pixels bypass NOT_EQUAL; remove or
adjust the pDepthStencilState check so that when
globals::features::vrStereoOptimizations.IsStencilActive() is true you still
call stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState) (handling a nullptr
input) and set StencilRef = 1; update the condition around globals::game::isVR
and vrStereoOptimizations to trigger the modified DSS creation even for the
implicit default (nullptr) case so stencil culling works correctly.
---
Nitpick comments:
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 1-14: Extract the MODE_* definitions out of cbuffers.hlsli into a
new header (e.g., modes.hlsli) containing only the `#define` MODE_DISOCCLUDED /
MODE_EDGE / MODE_MAIN / MODE_FULL_BLEND constants, then update cbuffers.hlsli to
`#include` "modes.hlsli" and retain the cbuffer; update StereoBlendCS.hlsl and
DeferredCompositeCS.hlsl to include only "modes.hlsli" (removing duplicated
MODE_* definitions and any hardcoded numeric checks like mode == 2 || mode == 1)
so all shaders reference the single source of truth for MODE_* values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8af15c23-c6ba-4204-862a-21efd714c82b
📒 Files selected for processing (33)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.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/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 (9)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- src/State.cpp
- .claude/CLAUDE.md
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- package/Shaders/Common/VR.hlsli
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- src/Features/VR.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Features/ExtendedMaterials.h
- package/Shaders/DistantTree.hlsl
- package/Shaders/Common/SharedData.hlsli
- src/Globals.h
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- package/Shaders/DeferredCompositeCS.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
- package/Shaders/RunGrass.hlsl
- src/Feature.cpp
- src/Features/VRStereoOptimizations.h
- src/Deferred.cpp
This was referenced Mar 23, 2026
alandtse
requested changes
Mar 23, 2026
Collaborator
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Thanks for breaking apart but I'm confused by the direction here. Why are you adding two new features? Why are you modifying terrain blending? I thought our discussion in your omnibus was that you would make the vr optimization part of VR and not as separate feature.
Also, why is there a need for a VR only terrain blending? That splits the code base and just means you have 5% of possible development on the VR only code. Unless there is an architectural issue driving a separate terrain blending algorithm, we need to stay with flat as much as possible.
I appreciate you breaking things apart to try to make it easier. If they come in a series, then we'll have to handle them one at a time. If it's small enough that it's self-contained, we can handle that in parallel. The foliage one was the only one that looked remotely like that though.
Collaborator
|
Please note, it needs to pass CI for us to even review it. I reviewed it early just to try to help you get this through but it needs to compile before we'll take it in. |
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
from
77931a5 to
d53f9de
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/Features/TerrainHelper.cpp (1)
34-38:⚠️ Potential issue | 🟡 MinorAdd type validation for JSON settings loading.
The PR description mentions "hardened JSON settings loading (type checks + clamping)", but this
LoadSettingsimplementation directly calls.get<bool>()without first validating the value type. If the JSON contains a non-boolean value, this will throw an exception.🛡️ Proposed fix to add type validation
void TerrainHelper::LoadSettings(json& o_json) { - if (o_json.contains("EnableParallax")) - settings.enableParallax = o_json["EnableParallax"].get<bool>(); + if (o_json.contains("EnableParallax") && o_json["EnableParallax"].is_boolean()) + settings.enableParallax = o_json["EnableParallax"].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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/TerrainHelper.cpp` around lines 34 - 38, TerrainHelper::LoadSettings currently calls o_json["EnableParallax"].get<bool>() directly which can throw for non-boolean values; update LoadSettings to validate the JSON type before assignment by checking o_json.contains("EnableParallax") && o_json["EnableParallax"].is_boolean() and only then set settings.enableParallax, otherwise leave the existing default (or fallback) and optionally log/ignore malformed values; reference TerrainHelper::LoadSettings and settings.enableParallax to locate where to add the is_boolean() guard and graceful degradation.src/Globals.cpp (1)
291-303:⚠️ Potential issue | 🟠 MajorHandle null depth-stencil state binds during active stencil culling.
The hook condition at line 293 (
if (globals::game::isVR && pDepthStencilState)) skips the DSS rewrite whenpDepthStencilStateis null. However, the engine does callOMSetDepthStencilState(nullptr, ...)which selects the default depth-stencil state. When stereo stencil optimization is active and a null DSS is bound, subsequent renders bypass the NOT_EQUAL stencil test, causing visual artifacts on marked Eye 1 pixels.🐛 Proposed fix to handle null DSS
static void thunk(ID3D11DeviceContext* This, ID3D11DepthStencilState* pDepthStencilState, UINT StencilRef) { - if (globals::game::isVR && pDepthStencilState) { + if (globals::game::isVR) { auto& stereoOpt = globals::features::vrStereoOptimizations; if (stereoOpt.loaded && stereoOpt.IsStencilActive()) { - pDepthStencilState = stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState); - StencilRef = 1; // Must match the ref written by our stencil pass + // Handle null DSS (D3D11 default state) by creating a modified default DSS + pDepthStencilState = stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState); + if (pDepthStencilState) // Only set ref if we got a valid modified DSS + StencilRef = 1; // Must match the ref written by our stencil pass } } func(This, pDepthStencilState, StencilRef); }And in
VRStereoOptimizations::GetOrCreateModifiedDSS(), handle the null case by creating a modified version of the default DSS:if (!originalDSS) { // Create modified default DSS with stencil NOT_EQUAL test // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.cpp` around lines 291 - 303, The thunk in Globals.cpp currently skips the DSS rewrite when pDepthStencilState is null, causing default-state binds (OMSetDepthStencilState(nullptr,...)) to bypass stereo stencil culling; update the thunk to still call stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState) even when pDepthStencilState is null and set StencilRef = 1 when a modified DSS is returned, and update VRStereoOptimizations::GetOrCreateModifiedDSS to handle a null originalDSS by creating/reusing a modified default depth-stencil state (a copy of the engine default with a NOT_EQUAL stencil test) and returning that modified DSS.src/Deferred.cpp (1)
373-379:⚠️ Potential issue | 🟡 MinorBind slot 16 even when
GetModeTextureSRV()returns null.Line 377 skips the bind when the mode texture is missing, so
t16can keep a stale SRV from an earlier compute pass.DeferredCompositeCSshould see either the current texture or an explicit null binding here.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); + context->CSSetShaderResources(16, 1, &modeSRV); }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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 373 - 379, The code currently skips calling context->CSSetShaderResources(16, 1, ...) when vrStereoOpt.GetModeTextureSRV() returns null, leaving slot t16 potentially stale; change the logic in Deferred.cpp so that you always call context->CSSetShaderResources for slot 16 with either the returned ID3D11ShaderResourceView* from vrStereoOpt.GetModeTextureSRV() or an explicit null pointer, using the vrStereoOpt/GetModeTextureSRV reference and the existing context->CSSetShaderResources call to ensure DeferredCompositeCS sees a cleared binding when the texture is missing.package/Shaders/VR/StereoBlendCS.hlsl (1)
34-45:⚠️ Potential issue | 🟠 Major
SampleReprojectedColor()still collapses to texel-center sampling.Because
reprojPxis already anint2, Line 39 always reconstructs a texel-center UV. At texel centers a linear sampler behaves like a point sample, so the overwrite path at Line 177 and Line 238 still loses the sub-pixel reprojection this feature is aiming for.Stereo::ReprojectToOtherEye()needs to preserve a fractional position here.package/Shaders/VR/VRPostProcessCS.hlsl (1)
81-95:⚠️ Potential issue | 🟠 MajorThis full-blend pass still blends two point samples.
Line 94 uses
ColorTexture[r.otherPx], which is an integer texel fetch. That means this "2x supersampling" branch never gets bilinear/sub-pixel color from the reprojected eye; it just lerps two nearest-neighbor samples. To preserve the detail this path is trying to add, the reprojection result needs to carry fractional UV/pixel coordinates and the color read should go throughSampleLevel.🤖 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 81 - 95, The reprojection branch is using an integer texel fetch (ColorTexture[r.otherPx]) so it only blends nearest-neighbor samples; update the reprojection result (Stereo::StereoBilateralResult returned by Stereo::ReprojectToOtherEye) to carry fractional UV/pixel coordinates (e.g., a float2 otherUV or otherPxFloat) computed in ReprojectToOtherEye, then replace the integer color fetch with a bilinear sample (e.g., ColorTexture.SampleLevel or Sample on the linear sampler using r.otherUV at LOD 0) so the blended color uses sub-pixel values; keep the integer ModeTexture check (ModeTexture[r.otherPx] or texelFetch) as-is or convert to an integer index derived from floor(r.otherUV) if needed so MODE_FULL_BLEND/MODE_DISOCCLUDED logic remains correct.
🧹 Nitpick comments (3)
package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl (1)
22-26: Remove unusedTexCoordfrom PS_INPUT.The
TexCoordmember is declared but never used in the shader—SV_Position.xyis used directly for texture sampling. Removing it would eliminate unnecessary interpolator bandwidth.♻️ Proposed cleanup
struct PS_INPUT { float4 Position: SV_Position; - float2 TexCoord: TEXCOORD0; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl` around lines 22 - 26, Remove the unused TexCoord interpolator from the PS_INPUT struct to reduce interpolator bandwidth: in StencilWritePS.hlsl delete the line declaring "float2 TexCoord: TEXCOORD0;" inside struct PS_INPUT and ensure any references (none expected since SV_Position.xy is used for sampling) are updated or removed; keep the Position: SV_Position member intact.package/Shaders/DeferredCompositeCS.hlsl (1)
100-107: Consider including mode constants instead of magic numbers.The comparison uses raw values
2and1instead of the named constantsMODE_MAINandMODE_EDGEdefined inVRStereoOptimizations/modes.hlsli. While the comment explains the values, including the header would improve maintainability and prevent drift.♻️ Proposed improvement
+#if defined(VR_STEREO_OPT) +#include "VRStereoOptimizations/modes.hlsli" +#endif `#if` defined(VR_STEREO_OPT) if (eyeIndex == 1) { uint mode = StereoOptModeTexture[uint2(dispatchID.xy)] & 0x0F; - 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 return; } } `#endif`🤖 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 with the named constants by including the VRStereoOptimizations/modes.hlsli header and using MODE_MAIN and MODE_EDGE in the comparison: locate the VR_STEREO_OPT block that reads StereoOptModeTexture[uint2(dispatchID.xy)] into `mode` and swap the if (mode == 2 || mode == 1) check to use the symbolic constants (MODE_MAIN, MODE_EDGE) after adding `#include` "VRStereoOptimizations/modes.hlsli" at the top of DeferredCompositeCS.hlsl so the stencil-culled reprojected path uses the defined names rather than raw numbers.package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl (1)
24-29: Consider dispatching only over Eye 1 region for efficiency.The shader dispatches over the full SBS texture (
fullWidth × fullHeight) but immediately returns for half the threads (lines 28-29). This wastes GPU occupancy launching threads that do nothing.♻️ Suggested optimization
Either:
- Dispatch only over Eye 1 dimensions in
VRStereoOptimizations::DispatchReprojection():-context->Dispatch((fullWidth + 7) / 8, (fullHeight + 7) / 8, 1); +uint32_t eyeWidth = fullWidth / 2; +context->Dispatch((eyeWidth + 7) / 8, (fullHeight + 7) / 8, 1);And adjust the shader to interpret
dtidas Eye 1-local coordinates (no offset needed, current code already does this).
- Or keep the current dispatch and adjust the bounds check to skip Eye 0:
-if (any(dtid >= uint2(eyeWidth, eyeHeight))) +if (dtid.x < eyeWidth || any(dtid >= uint2(eyeWidth * 2, eyeHeight))) return; -uint2 stereoCoord = uint2(dtid.x + eyeWidth, dtid.y); +uint2 stereoCoord = dtid;Option 1 is cleaner and halves the dispatch cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl` around lines 24 - 29, The shader currently early-returns half of threads in main(uint2 dtid : SV_DispatchThreadID) by computing eyeWidth/eyeHeight from FrameDim, so change the CPU dispatch in VRStereoOptimizations::DispatchReprojection() to dispatch only (eyeWidth, eyeHeight) threads (i.e., half the X dimension) so dtid already maps to Eye 1 local coordinates and no per-thread bounds check is needed; update any dispatch parameters or computed dispatch sizes to use (FrameDim.x / 2, FrameDim.y) and remove the redundant any(dtid >= uint2(eyeWidth, eyeHeight)) return in the shader.
🤖 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`:
- Line 55: The CPMSettings struct in SharedData.hlsli uses bool members that
must be uint to match the C++ interface; update every member of CPMSettings
(including the pad0 field) to use uint instead of bool so the HLSL struct
matches ExtendedMaterials.h for 4-byte size and 16-byte alignment and avoids
C++/HLSL layout mismatches.
---
Duplicate comments:
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 81-95: The reprojection branch is using an integer texel fetch
(ColorTexture[r.otherPx]) so it only blends nearest-neighbor samples; update the
reprojection result (Stereo::StereoBilateralResult returned by
Stereo::ReprojectToOtherEye) to carry fractional UV/pixel coordinates (e.g., a
float2 otherUV or otherPxFloat) computed in ReprojectToOtherEye, then replace
the integer color fetch with a bilinear sample (e.g., ColorTexture.SampleLevel
or Sample on the linear sampler using r.otherUV at LOD 0) so the blended color
uses sub-pixel values; keep the integer ModeTexture check
(ModeTexture[r.otherPx] or texelFetch) as-is or convert to an integer index
derived from floor(r.otherUV) if needed so MODE_FULL_BLEND/MODE_DISOCCLUDED
logic remains correct.
In `@src/Deferred.cpp`:
- Around line 373-379: The code currently skips calling
context->CSSetShaderResources(16, 1, ...) when vrStereoOpt.GetModeTextureSRV()
returns null, leaving slot t16 potentially stale; change the logic in
Deferred.cpp so that you always call context->CSSetShaderResources for slot 16
with either the returned ID3D11ShaderResourceView* from
vrStereoOpt.GetModeTextureSRV() or an explicit null pointer, using the
vrStereoOpt/GetModeTextureSRV reference and the existing
context->CSSetShaderResources call to ensure DeferredCompositeCS sees a cleared
binding when the texture is missing.
In `@src/Features/TerrainHelper.cpp`:
- Around line 34-38: TerrainHelper::LoadSettings currently calls
o_json["EnableParallax"].get<bool>() directly which can throw for non-boolean
values; update LoadSettings to validate the JSON type before assignment by
checking o_json.contains("EnableParallax") &&
o_json["EnableParallax"].is_boolean() and only then set settings.enableParallax,
otherwise leave the existing default (or fallback) and optionally log/ignore
malformed values; reference TerrainHelper::LoadSettings and
settings.enableParallax to locate where to add the is_boolean() guard and
graceful degradation.
In `@src/Globals.cpp`:
- Around line 291-303: The thunk in Globals.cpp currently skips the DSS rewrite
when pDepthStencilState is null, causing default-state binds
(OMSetDepthStencilState(nullptr,...)) to bypass stereo stencil culling; update
the thunk to still call stereoOpt.GetOrCreateModifiedDSS(pDepthStencilState)
even when pDepthStencilState is null and set StencilRef = 1 when a modified DSS
is returned, and update VRStereoOptimizations::GetOrCreateModifiedDSS to handle
a null originalDSS by creating/reusing a modified default depth-stencil state (a
copy of the engine default with a NOT_EQUAL stencil test) and returning that
modified DSS.
---
Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: Replace the magic numeric literals with the named
constants by including the VRStereoOptimizations/modes.hlsli header and using
MODE_MAIN and MODE_EDGE in the comparison: locate the VR_STEREO_OPT block that
reads StereoOptModeTexture[uint2(dispatchID.xy)] into `mode` and swap the if
(mode == 2 || mode == 1) check to use the symbolic constants (MODE_MAIN,
MODE_EDGE) after adding `#include` "VRStereoOptimizations/modes.hlsli" at the top
of DeferredCompositeCS.hlsl so the stencil-culled reprojected path uses the
defined names rather than raw numbers.
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 24-29: The shader currently early-returns half of threads in
main(uint2 dtid : SV_DispatchThreadID) by computing eyeWidth/eyeHeight from
FrameDim, so change the CPU dispatch in
VRStereoOptimizations::DispatchReprojection() to dispatch only (eyeWidth,
eyeHeight) threads (i.e., half the X dimension) so dtid already maps to Eye 1
local coordinates and no per-thread bounds check is needed; update any dispatch
parameters or computed dispatch sizes to use (FrameDim.x / 2, FrameDim.y) and
remove the redundant any(dtid >= uint2(eyeWidth, eyeHeight)) return in the
shader.
In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl`:
- Around line 22-26: Remove the unused TexCoord interpolator from the PS_INPUT
struct to reduce interpolator bandwidth: in StencilWritePS.hlsl delete the line
declaring "float2 TexCoord: TEXCOORD0;" inside struct PS_INPUT and ensure any
references (none expected since SV_Position.xy is used for sampling) are updated
or removed; keep the Position: SV_Position member intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f24df00-0a6c-44e7-83c8-2c8b901a01da
📒 Files selected for processing (34)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.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/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 (10)
- src/State.cpp
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- package/Shaders/Common/VR.hlsli
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- package/Shaders/RunGrass.hlsl
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- src/Globals.h
- package/Shaders/VRStereoOptimizations/modes.hlsli
- .claude/CLAUDE.md
- package/Shaders/Lighting.hlsl
🚧 Files skipped from review as they are similar to previous changes (13)
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- src/FeatureBuffer.cpp
- src/Features/VR.cpp
- package/Shaders/DistantTree.hlsl
- src/Features/VR/SettingsUI.cpp
- src/Features/TerrainHelper.h
- src/Features/ExtendedMaterials.h
- src/Features/VR.h
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- package/Shaders/VRStereoOptimizations/StencilCS.hlsl
- src/Features/VRStereoOptimizations.cpp
- src/Features/VRStereoOptimizations.h
- src/Features/VR/StereoBlend.cpp
Contributor
Author
|
All CodeRabbit feedback addressed in latest push: Eye 1 jitter removed from VR.hlsli ✓ |
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
from
7feb09a to
e75fdc1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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 | 🟡 MinorLine 326: document/guard new overwrite debug modes to avoid ambiguous behavior.
"Overwrite"/"Overwrite Eye1"are selectable now, but the tooltip still documents only modes 0–3. Please add explicit text that these map to the overwrite path (shared shader variant) and when they are meaningful.Suggested tooltip addition
if (auto _tt = Util::HoverTooltipWrapper()) { ImGui::Text( "Off: Normal rendering.\n\n" "Back-Check: Visualize reprojection outcomes.\n" ... "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 / Overwrite Eye1: use the overwrite compute path.\n" + "Both reuse the same shader variant with different mode parameters."); }Based on learnings: In
src/Features/VR/StereoBlend.cpp, debug modes 4/5 sharestereoBlendOverwriteCSrather than separate shader variants.🤖 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, Update the tooltip text shown for the Debug View combo (linked to settings.StereoBlendDebugMode) to document the new modes "Overwrite" (index 4) and "Overwrite Eye1" (index 5): state that both modes use the shared overwrite shader path (stereoBlendOverwriteCS) rather than unique shader variants, explain the difference (global overwrite vs overwrite for eye1) and when they are meaningful (e.g., for forcing a full-frame overwrite or testing single-eye overwrite behavior), and make clear these map to debug mode values 4 and 5 so the UI description matches the actual implementation in StereoBlend.cpp.
♻️ Duplicate comments (2)
src/Globals.cpp (1)
289-297:⚠️ Potential issue | 🟠 Major
nullptrDSS binds still bypass the stencil rewrite.
OMSetDepthStencilState(nullptr, ...)is a valid D3D11 default-state bind, but this hook only rewrites non-null states. Any draw that hits the null path whilestencilActiveis true will skip theNOT_EQUALtest and repaint pixels the stencil pass intended to cull.Verify whether the render path binds the default DSS during the active stencil window. Expected result: either no
nullptrbinds exist there, or the hook synthesizes a rewritten default DSS for them.#!/bin/bash set -e rg -n -C2 'OMSetDepthStencilState\s*\(\s*(nullptr|NULL|0)\s*,' srcAs per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.cpp` around lines 289 - 297, The thunk in Globals.cpp currently only rewrites non-null pDepthStencilState, so OMSetDepthStencilState(nullptr, ...) bypasses the stencil test; update thunk (and vrStereoOptimizations helper usage) to handle nullptr by synthesizing or retrieving a modified default depth-stencil state when stereoOpt.loaded && stereoOpt.IsStencilActive() is true: call or add a method on globals::features::vrStereoOptimizations (e.g., GetOrCreateModifiedDSSForDefault) that returns a valid ID3D11DepthStencilState* for the default case, assign it to pDepthStencilState, set StencilRef = 1, and ensure the synthesized state is cached and released appropriately to avoid leaks and to gracefully degrade if creation fails (fall back to leaving pDepthStencilState as nullptr).src/Deferred.cpp (1)
376-395:⚠️ Potential issue | 🟠 MajorManage slot
t16unconditionally for anyVR_STEREO_OPTdispatch.The composite shader is compiled with
VR_STEREO_OPTwhenevervrStereoOptimizations.loadedis true, but this function only binds/clears slot 16 whenstereoMode != Offand only ifGetModeTextureSRV()is non-null. In Off mode—or on a null-SRV failure—the Eye 1 skip path can read stale data from a previous resource binding.Verify the slot-management invariant here. Expected result: every
VR_STEREO_OPTdispatch explicitly sets slot 16 to either the current mode SRV ornullptr, and clears it afterward.#!/bin/bash set -e rg -n -C3 'CSSetShaderResources\s*\(\s*16|VR_STEREO_OPT' src/Deferred.cpp package/Shaders/DeferredCompositeCS.hlsl💡 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); }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: 591-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 - 395, The VR stereo-mode SRV slot (t16) is only set/cleared when vrStereoOptimizations.settings.stereoMode != VRStereoOptimizations::StereoMode::Off and when GetModeTextureSRV() returns non-null, which can leave stale bindings for any dispatch compiled with VR_STEREO_OPT; update the code around the Dispatch in Deferred.cpp so that whenever globals::features::vrStereoOptimizations.loaded is true (i.e., any VR_STEREO_OPT dispatch), you always call context->CSSetShaderResources(16, 1, &modeSRV) before Dispatch where modeSRV is either vrStereoOpt.GetModeTextureSRV() or a nullptr (if GetModeTextureSRV() returns null or stereoMode == Off), and always clear slot 16 afterward with a nullptr via context->CSSetShaderResources(16, 1, &nullSRV); ensure this change touches the same blocks that call GetModeTextureSRV(), the Dispatch call, and the post-Dispatch unbind so slot 16 is never left containing stale data.
🧹 Nitpick comments (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
513-513: Optional cleanup: remove redundant fallback assignment.
pixelOffsetis already initialized to0.5at Line 328, so this second write is unnecessary.♻️ Minimal cleanup
- pixelOffset = 0.5; return coords;🤖 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 513, Remove the redundant fallback assignment to pixelOffset at the second occurrence (the assignment "pixelOffset = 0.5;" in ExtendedMaterials.hlsli) because pixelOffset is already initialized to 0.5 earlier (initialization near line where pixelOffset is declared). Edit the block containing that assignment (e.g., within the same shader function or scope where pixelOffset is reassigned) and delete the duplicate line so the variable keeps its original initialization; ensure no other logic depends on reassigning pixelOffset at that point.
🤖 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/DeferredCompositeCS.hlsl`:
- Around line 100-107: The early return in the VR_STEREO_OPT block (the branch
testing eyeIndex and StereoOptModeTexture using dispatchID) skips final writes
to NormalTAAMaskSpecularMaskRW and MotionVectorsRW, leaving stale normals/motion
for downstream shaders like ISReflectionsRayTracing; instead of returning early
from that block, ensure you clear or write safe default values to
NormalTAAMaskSpecularMaskRW and MotionVectorsRW (e.g., zero normal/mask and zero
motion) before exiting for the culled Eye 1 modes (mode == 1 || mode == 2), or
move the stereo cull test after the normal/motion write logic so those UAVs are
always updated; update the block that references StereoOptModeTexture,
dispatchID, and eyeIndex accordingly so StereoBlendCS/ReprojectionCS and
downstream consumers never see stale data unless explicitly handled by
STEREO_OVERWRITE.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 97-143: The inner width is hardcoded (kInnerWidth=2) and maxWidth
is tied to it, so Settings::edgeWidth (VRStereoOptParams::EdgeWidth) never
affects the search or outer-band classification; update the code to use a
configurable innerWidth and an outerWidth from Settings::edgeWidth: replace the
static kInnerWidth with a variable innerWidth (default 2 if needed), set
maxWidth = max(innerWidth, Settings::edgeWidth), and in the post-search
classification add an outer-band branch that sets MODE_EDGE when
nearestWeAreOuter is true and nearestEdgeDist <= Settings::edgeWidth (keep the
existing nearestEdgeDist==1 and inner-band (!nearestWeAreOuter &&
nearestEdgeDist<=innerWidth) branches).
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The current auto-show condition only checks
globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) which omits the tween
menu; update the boolean expression around settings.kAutoHideSeconds,
globals::game::ui, and globals::menu->IsEnabled to also allow when
globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME) is true (i.e., add an OR
for RE::TweenMenu::MENU_NAME alongside RE::MainMenu::MENU_NAME) so the overlay
appears in both main menu and tween menu states while preserving the existing
checks for settings.kAutoHideSeconds and globals::menu->IsEnabled.
---
Outside diff comments:
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 326-345: Update the tooltip text shown for the Debug View combo
(linked to settings.StereoBlendDebugMode) to document the new modes "Overwrite"
(index 4) and "Overwrite Eye1" (index 5): state that both modes use the shared
overwrite shader path (stereoBlendOverwriteCS) rather than unique shader
variants, explain the difference (global overwrite vs overwrite for eye1) and
when they are meaningful (e.g., for forcing a full-frame overwrite or testing
single-eye overwrite behavior), and make clear these map to debug mode values 4
and 5 so the UI description matches the actual implementation in
StereoBlend.cpp.
---
Duplicate comments:
In `@src/Deferred.cpp`:
- Around line 376-395: The VR stereo-mode SRV slot (t16) is only set/cleared
when vrStereoOptimizations.settings.stereoMode !=
VRStereoOptimizations::StereoMode::Off and when GetModeTextureSRV() returns
non-null, which can leave stale bindings for any dispatch compiled with
VR_STEREO_OPT; update the code around the Dispatch in Deferred.cpp so that
whenever globals::features::vrStereoOptimizations.loaded is true (i.e., any
VR_STEREO_OPT dispatch), you always call context->CSSetShaderResources(16, 1,
&modeSRV) before Dispatch where modeSRV is either
vrStereoOpt.GetModeTextureSRV() or a nullptr (if GetModeTextureSRV() returns
null or stereoMode == Off), and always clear slot 16 afterward with a nullptr
via context->CSSetShaderResources(16, 1, &nullSRV); ensure this change touches
the same blocks that call GetModeTextureSRV(), the Dispatch call, and the
post-Dispatch unbind so slot 16 is never left containing stale data.
In `@src/Globals.cpp`:
- Around line 289-297: The thunk in Globals.cpp currently only rewrites non-null
pDepthStencilState, so OMSetDepthStencilState(nullptr, ...) bypasses the stencil
test; update thunk (and vrStereoOptimizations helper usage) to handle nullptr by
synthesizing or retrieving a modified default depth-stencil state when
stereoOpt.loaded && stereoOpt.IsStencilActive() is true: call or add a method on
globals::features::vrStereoOptimizations (e.g.,
GetOrCreateModifiedDSSForDefault) that returns a valid ID3D11DepthStencilState*
for the default case, assign it to pDepthStencilState, set StencilRef = 1, and
ensure the synthesized state is cached and released appropriately to avoid leaks
and to gracefully degrade if creation fails (fall back to leaving
pDepthStencilState as nullptr).
---
Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 513: Remove the redundant fallback assignment to pixelOffset at the
second occurrence (the assignment "pixelOffset = 0.5;" in
ExtendedMaterials.hlsli) because pixelOffset is already initialized to 0.5
earlier (initialization near line where pixelOffset is declared). Edit the block
containing that assignment (e.g., within the same shader function or scope where
pixelOffset is reassigned) and delete the duplicate line so the variable keeps
its original initialization; ensure no other logic depends on reassigning
pixelOffset at that point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aae3f1eb-d8ad-4025-beb2-205e181f59f0
📒 Files selected for processing (28)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/VR/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/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 (9)
- src/State.cpp
- src/Features/ExtendedMaterials.h
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- src/Feature.cpp
- src/Globals.h
- package/Shaders/Common/VR.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
🚧 Files skipped from review as they are similar to previous changes (10)
- package/Shaders/DistantTree.hlsl
- src/Features/VR.h
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
- package/Shaders/Lighting.hlsl
- package/Shaders/RunGrass.hlsl
- src/Features/VR.cpp
- src/Features/VR/StereoBlend.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Features/VRStereoOptimizations.cpp
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
from
11f0c65 to
6fdaf77
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/Globals.cpp (1)
287-296:⚠️ Potential issue | 🟠 MajorHandle
OMSetDepthStencilState(nullptr, …)while stencil culling is active.Line 289 still skips the rewrite when the engine binds a null DSS, but D3D11 treats that as “use the implicit default depth-stencil state.” Those draws bypass the NOT_EQUAL test and repaint pixels the stencil pass already marked for culling. Please synthesize/cache a modified default DSS for the null case and force
StencilRef = 1there as well. 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".#!/bin/bash # Show the current guard and find call sites that bind the implicit default DSS. sed -n '287,296p' src/Globals.cpp rg -nP 'OMSetDepthStencilState\s*\(\s*(nullptr|NULL|0)\b' --type=cpp --type=h -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.cpp` around lines 287 - 296, The thunk currently skips when pDepthStencilState is null, but D3D11 treats null as the implicit default DSS; update thunk (the static thunk wrapper that eventually calls func) to detect pDepthStencilState==nullptr and, when globals::features::vr.stereoOpt.loaded && stereoOpt.IsStencilActive(), obtain or synthesize a modified default depth-stencil state from the stereoOpt (e.g. add or extend GetOrCreateModifiedDSS to accept a nullptr or add GetOrCreateModifiedDefaultDSS), cache that synthesized default in stereoOpt to avoid leaks/duplicates, set StencilRef = 1 in this null-case path, and ensure proper resource management and graceful degradation (validate the returned ID3D11DepthStencilState* before using it and fall back to calling func with the original nullptr if creation fails).src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorRound-trip the two new debug toggles too.
DrawSettings()exposesdebugFullBlendDepthanddebugPOMDepth, but neither flag is serialized here. They reset on restart, which makes tuning the overwrite diagnostics non-reproducible. As per coding guidelines, "When developing graphics features, inherit from theFeatureclass insrc/Feature.hand implementDrawSettings(),LoadSettings(), andSaveSettings()methods".💾 Suggested serialization additions
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("DebugForceAllReprojectCS"); it != o_json.end() && it->is_boolean()) settings.debugForceAllReprojectCS = it->get<bool>(); 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
🤖 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, Save the two new debug toggles so they persist across restarts: in VRStereoOptimizations::SaveSettings add o_json entries for settings.debugFullBlendDepth and settings.debugPOMDepth (keys like "DebugFullBlendDepth" and "DebugPOMDepth"), and mirror these in VRStereoOptimizations::LoadSettings to read them back into settings.debugFullBlendDepth and settings.debugPOMDepth; this aligns with the existing pattern used for other flags (e.g., "DebugDepthMap") and ties into the DrawSettings UI that exposes debugFullBlendDepth and debugPOMDepth.
🧹 Nitpick comments (1)
src/Features/VRStereoOptimizations.h (1)
96-115: Assert the exact cbuffer size as well.
sizeof(VRStereoOptParams) % 16 == 0still allows layout drift that stays aligned but no longer matchespackage/Shaders/VRStereoOptimizations/cbuffers.hlsli. An exact-size assert here would make future C++/HLSL regressions fail fast.🧱 Suggested guard
static_assert(sizeof(VRStereoOptParams) % 16 == 0, "VRStereoOptParams must be 16-byte aligned for HLSL cbuffer."); +static_assert(sizeof(VRStereoOptParams) == 64, "VRStereoOptParams must match the HLSL cbuffer size.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.h` around lines 96 - 115, Add an exact-size assertion to prevent C++/HLSL layout drift: after the VRStereoOptParams definition add a static_assert that sizeof(VRStereoOptParams) == 64 with a message referencing the HLSL cbuffer (package/Shaders/VRStereoOptimizations/cbuffers.hlsli) so any future changes to the struct (VRStereoOptParams) that break the expected 64-byte cbuffer layout fail fast.
🤖 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/Lighting.hlsl`:
- Around line 3180-3185: The current preprocessor guard around setting
psout.Reflectance prevents exporting pixelOffset for complex-material envmap
permutations (pixelOffset is also written in the EMAT_ENVMAP path at the
GetParallaxCoords call), so add that permutation to the condition: update the
guard that checks (defined(PARALLAX) || defined(LANDSCAPE) || defined(TRUE_PBR))
to also include the EMAT/EMAT_ENVMAP case (e.g., || defined(EMAT_ENVMAP) or ||
(defined(EMAT) && defined(EMAT_ENVMAP)) as appropriate for your macros) so that
psout.Reflectance.w is set to (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0
for those complex-material permutations that call GetParallaxCoords.
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 47-49: The reprojection can sample the first column of the right
eye when otherEyeUV.x saturates; in the block using otherEyeUV, clamp the stereo
X to the left-eye half before converting to pixel coords: after computing
eye0StereoUV via Stereo::ConvertToStereoUV(otherEyeUV.xy, 0) ensure its x is
limited to the left-eye range (e.g. min(eye0StereoUV.x, 0.5 - 1.0/FrameDim.x) or
multiply by FrameDim.xy with the X dimension halved) so that eye0Px computed
from eye0StereoUV never becomes eyeWidth; update references around otherEyeUV,
eye0StereoUV and eye0Px to apply this clamp.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 609-612: The dispatch is currently launched across the full
side-by-side width (fullWidth) so half the threads immediately exit in the
shader; change the X dispatch size to the single-eye width used by the HLSL
(compute eyeWidth = texPerPixelMode->desc.Width / 2 or otherwise derive the
per-eye width used by ReprojectionCS.hlsl) and call context->Dispatch((eyeWidth
+ 7) / 8, (fullHeight + 7) / 8, 1) instead of using fullWidth; update the local
variable names (replace fullWidth with eyeWidth where appropriate) so dtid and
eyeWidth in the shader match the runtime dispatch and avoid wasted threads.
---
Duplicate comments:
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: Save the two new debug toggles so they persist across
restarts: in VRStereoOptimizations::SaveSettings add o_json entries for
settings.debugFullBlendDepth and settings.debugPOMDepth (keys like
"DebugFullBlendDepth" and "DebugPOMDepth"), and mirror these in
VRStereoOptimizations::LoadSettings to read them back into
settings.debugFullBlendDepth and settings.debugPOMDepth; this aligns with the
existing pattern used for other flags (e.g., "DebugDepthMap") and ties into the
DrawSettings UI that exposes debugFullBlendDepth and debugPOMDepth.
In `@src/Globals.cpp`:
- Around line 287-296: The thunk currently skips when pDepthStencilState is
null, but D3D11 treats null as the implicit default DSS; update thunk (the
static thunk wrapper that eventually calls func) to detect
pDepthStencilState==nullptr and, when globals::features::vr.stereoOpt.loaded &&
stereoOpt.IsStencilActive(), obtain or synthesize a modified default
depth-stencil state from the stereoOpt (e.g. add or extend
GetOrCreateModifiedDSS to accept a nullptr or add
GetOrCreateModifiedDefaultDSS), cache that synthesized default in stereoOpt to
avoid leaks/duplicates, set StencilRef = 1 in this null-case path, and ensure
proper resource management and graceful degradation (validate the returned
ID3D11DepthStencilState* before using it and fall back to calling func with the
original nullptr if creation fails).
---
Nitpick comments:
In `@src/Features/VRStereoOptimizations.h`:
- Around line 96-115: Add an exact-size assertion to prevent C++/HLSL layout
drift: after the VRStereoOptParams definition add a static_assert that
sizeof(VRStereoOptParams) == 64 with a message referencing the HLSL cbuffer
(package/Shaders/VRStereoOptimizations/cbuffers.hlsli) so any future changes to
the struct (VRStereoOptParams) that break the expected 64-byte cbuffer layout
fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff417bd5-5de4-49f9-b1ef-e2ca9c2cfa93
📒 Files selected for processing (26)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.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/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 (8)
- package/Shaders/Common/VR.hlsli
- src/Features/ExtendedMaterials.h
- src/State.cpp
- package/Shaders/RunGrass.hlsl
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
🚧 Files skipped from review as they are similar to previous changes (9)
- package/Shaders/DeferredCompositeCS.hlsl
- package/Shaders/DistantTree.hlsl
- src/Features/VR.cpp
- src/Features/VR/SettingsUI.cpp
- package/Shaders/VRStereoOptimizations/StencilCS.hlsl
- src/Features/VR.h
- package/Shaders/VR/VRPostProcessCS.hlsl
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Deferred.cpp
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
from
e1c2bde to
1580826
Compare
|
✅ A pre-release build is available for this PR: |
Contributor
Author
|
@doodlum Thanks for the detailed review. All issues addressed in the latest push: VRStereoOptimizations is no longer a standalone Feature — it's folded into VR as an owned helper class (vr.stereoOpt), same pattern as StereoBlend. No separate .ini, no Feature registration, settings serialize under VR's JSON namespace. Contamination cleaned up: .claude/CLAUDE.md — removed Code review responses: "Is this impacting flat?" — All VR-specific changes are gated behind #ifdef VR or runtime REL::Module::IsVR() checks. Flat is unaffected. CI is now green. Ready for re-review. |
Collaborator
|
Please do resolve the open comments so we know you've addressed them. No need to respond to the rabbit unless you feel like it. |
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
4 times, most recently
from
74202dd to
a76685f
Compare
Contributor
Author
|
All review feedback addressed in latest push: Null DSS handling: GetOrCreateModifiedDSS now handles nullptr by synthesizing the D3D11 default state with stencil NOT_EQUAL All CodeRabbit threads should now be resolved. |
vrnord
force-pushed
the
pr/vr-stereo-opt
branch
from
9505880 to
4621bb0
Compare
alandtse
reviewed
Mar 25, 2026
alandtse
changed the title
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>
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
- Remove stray blank line in VR.hlsli before #endif - Revert ExtendedMaterials.h pad field to float pad[1] (no churn) - Restore * scale in SettingsUI.cpp PushTextWrapPos calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
StencilWritePS bound depthSRV (kMAIN D24S8) at t1 while stencilWriteReadOnlyDSV (same texture, D3D11_DSV_READ_ONLY_DEPTH) was the output DSV. D3D11 silently suppressed all stencil writes. Fix: remove depth texture from StencilWritePS entirely. The PS now only reads the mode texture and discards for any mode except MODE_MAIN. This matches ReprojectionCS which only fills MODE_MAIN pixels. Use the normal writable kMAIN DSV; no simultaneous SRV binding means no hazard. Remove stencilWriteReadOnlyDSV — it was only needed to work around the binding conflict that no longer exists. Also increase kInnerWidth 2→4 to widen the MODE_EDGE band at depth discontinuity boundaries, reducing disocclusion ghosting at ~8k VR resolutions where 2px was insufficient. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move modes.hlsli include inside VR_STEREO_OPT guard in DeferredCompositeCS. Replace hardcoded mode values 1/2 with MODE_EDGE/MODE_MAIN named constants. Remove empty PerformLateStencilWrite placeholder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DispatchStencil was reading kMAIN.depthSRV which is unpopulated at StartDeferred time, causing all pixels to classify as MODE_DISOCCLUDED and the feature to be completely dormant. Switch to GetCurrentSceneDepthSRV() which has valid z-prepass data. Also remove MODE_EDGE from the DeferredCompositeCS Eye 1 early-exit. MODE_EDGE pixels are geometry-rendered (not stencil-culled) and must receive full deferred composite before StereoBlendCS bilateral-blends them; skipping composite dropped all specular/IBL/SSGI contributions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
alandtse
force-pushed
the
pr/vr-stereo-opt
branch
from
7dda9df to
73a46c2
Compare
Collaborator
|
I've decided I'm ok with this not working for ps2 vr. If a major issue, we'll just disable by default. |
alandtse
changed the title
ParticleTroned
added a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
Apr 9, 2026
This reverts commit 2017125.
ParticleTroned
added a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
Apr 9, 2026
This reverts commit 2017125.
ParticleTroned
added a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
Apr 9, 2026
This was referenced Apr 10, 2026
This was referenced Apr 19, 2026
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.
Stencil-based Eye 1 culling with compute shader reprojection, folded into the VR feature (not standalone) per maintainer feedback.
What it does
Review feedback addressed (from #1982)
Replaces #1982.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes