feat(vr-dlss): VR DLSS viewport scaling with periphery TAA#2003
Closed
vrnord wants to merge 3 commits into
Closed
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 comprehensive VR upscaling enhancements with stereo optimization via GPU-driven stencil culling and compute-based reprojection. It adds a new TAA reordering pipeline for DLSS periphery compositing, expands the upscaling system with viewport scaling, feathered blending, and dynamic HMD mask handling, and implements multiple new GPU passes across both pixel and compute shaders alongside D3D11 hook interception for stereo depth-stencil management. Changes
Sequence Diagram(s)sequenceDiagram
participant Frame as Frame Loop
participant Deferred as Deferred Rendering
participant StencilCS as Stencil Classification
participant StencilWrite as Stencil Write Pass
participant GeoRender as Geometry Rendering
participant DeferredComp as Deferred Composite
participant Reproject as Reprojection
participant StereoBlend as Stereo Blend
Frame->>Deferred: StartDeferred()
Deferred->>StencilCS: DispatchStencil()
StencilCS->>StencilCS: Classify pixels (MODE_*)
StencilCS->>StencilWrite: Execute stencil write fullscreen triangle
StencilWrite->>StencilWrite: Write stencil=1 for valid pixels
Deferred->>GeoRender: Render geometry (stencil-culled for Eye 1)
GeoRender->>GeoRender: Eye 1 pixels skip rendering via stencil
Deferred->>DeferredComp: Run deferred composite
DeferredComp->>DeferredComp: Read StereoOptModeTexture for optimization
DeferredComp->>DeferredComp: Early-exit if mode == 1 or 2 (Eye 1)
Deferred->>Reproject: DispatchReprojection()
Reproject->>Reproject: Reproject Eye 0 color to Eye 1
Reproject->>Reproject: Write via UAV to stencil-marked pixels
Deferred->>StereoBlend: DrawStereoBlend()
StereoBlend->>StereoBlend: Stereo blend with overwrite mode
sequenceDiagram
participant Main as Main Thread
participant PP as Post-Processing
participant TAA as TAA Reorder
participant DLSS as DLSS
participant Paste as Feathered Paste
Main->>PP: BSImagespaceShader::Render()
alt TAAReorder::ShouldReorderTAA() enabled
PP->>TAA: Capture Phase 2A output → g_postPPCopy
PP->>TAA: Continue to Phase 5
PP->>DLSS: Evaluate DLSS on g_postPPCopy (cropped center)
DLSS->>Paste: Generate upscaled crop
Paste->>Paste: Feather edges per-eye
Paste->>PP: Paste back into submit texture
PP->>Main: Skip TAA for remainder of frame
else Normal upscaling path
PP->>DLSS: Evaluate full-resolution DLSS
PP->>Main: Return
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 15
🧹 Nitpick comments (8)
package/Shaders/DeferredCompositeCS.hlsl (1)
99-107: Good performance optimization with early exit for stencil-culled pixels.The early exit for eye 1 pixels with
mode == 2(MODE_MAIN - stencil-culled) correctly avoids unnecessary G-buffer processing for pixels that have no valid data. This is a sensible optimization.Consider defining a named constant for the mode value to improve maintainability:
♻️ Optional: Use named constant for mode
`#if` defined(VR_STEREO_OPT) Texture2D<uint> StereoOptModeTexture : register(t16); +static const uint STEREO_OPT_MODE_MAIN = 2; // Stencil-culled, no valid G-buffer `#endif` ... `#if` defined(VR_STEREO_OPT) if (eyeIndex == 1) { uint mode = StereoOptModeTexture[uint2(dispatchID.xy)]; - if (mode == 2) { // MODE_MAIN — stencil-culled, no valid G-buffer + if (mode == STEREO_OPT_MODE_MAIN) { 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 99 - 107, Replace the magic literal "2" used for the stereo optimization mode check with a named constant to improve readability and maintainability: define a constant like MODE_MAIN (or STEREO_OPT_MODE_MAIN) and use it in the if-check that reads uint mode = StereoOptModeTexture[uint2(dispatchID.xy)]; then compare if (mode == MODE_MAIN) inside the VR_STEREO_OPT block (referencing eyeIndex, StereoOptModeTexture, dispatchID, and the local variable mode) so future readers know what the value represents.package/Shaders/DistantTree.hlsl (1)
206-214: Consider extracting the VR alpha threshold offset to a named constant or cbuffer.The hardcoded
0.1value for the per-eye alpha threshold adjustment appears in both code paths. Per retrieved learnings,VRAlphaTestThresholdis wired intoDistantTree.hlslandRunGrass.hlsl- consider using that cbuffer value instead of the hardcoded constant for consistency and tunability.That said, if this is intentionally a simpler approach for distant trees specifically, the current implementation is functionally correct.
♻️ Optional: Use a named constant
+#if defined(VR) +static const float VR_ALPHA_OFFSET_PER_EYE = 0.1; +#endif + { float alphaRef = AlphaTestRefRS; # if defined(VR) - alphaRef -= eyeIndex * 0.1; + alphaRef -= eyeIndex * VR_ALPHA_OFFSET_PER_EYE; # endif if ((alpha - alphaRef) < 0) { discard; } }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 offset 0.1 used when computing alphaRef (around the AlphaTestRefRS adjustment in DistantTree.hlsl where VR and eyeIndex are used) with the shared VR alpha threshold value/wrapper (e.g. the VRAlphaTestThreshold value from the existing cbuffer) so the per-eye adjustment is tunable and consistent across shaders; update the calculation to subtract eyeIndex * VRAlphaTestThreshold (or a new named constant referencing that cbuffer) and apply the same change to the other occurrence (the similar block at lines ~222-230) and ensure the cbuffer symbol is declared/imported where this shader is compiled.src/Features/VR/StereoBlend.cpp (1)
142-154: Consider moving sampler creation toSetupResources().The lazy sampler creation inside
DrawStereoBlend()is functionally correct (guarded by null check), but creating D3D11 resources on the first draw call can cause a minor frame hitch. Moving this toSetupResources()would be more consistent with the codebase pattern.🤖 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 142 - 154, The sampler creation currently happens lazily inside DrawStereoBlend() which can hitch on first draw; move the D3D11_SAMPLER_DESC creation and device->CreateSamplerState call into SetupResources() and initialize stereoBlendLinearSampler there (preserve D3D11_FILTER_MIN_MAG_MIP_LINEAR and CLAMP addresses), then remove the CreateSamplerState branch from DrawStereoBlend() and just bind stereoBlendLinearSampler via context->CSSetSamplers(0, 1, &stereoBlendLinearSampler) (use the existing stereoBlendLinearSampler getter/put/get() helpers as in the diff).package/Shaders/VR/VRPostProcessCS.hlsl (1)
56-68: Unused variableuvDbin debug mode 2.The variable
uvDbis computed but never used in this block. The depth is sampled usingdtiddirectly.🧹 Remove unused variable
if (DebugMode == 2) { - float2 uvDb = (dtid + 0.5) * RcpFrameDim; float depthDb = DepthTexture[dtid]; if (depthDb < 1e-5 || depthDb >= 1.0) return;🤖 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 56 - 68, The DebugMode == 2 block computes uvDb but never uses it; remove the unused local variable uvDb (or replace depth sampling to use uvDb if intended) by deleting the declaration "float2 uvDb = (dtid + 0.5) * RcpFrameDim;" in the DebugMode branch in VRPostProcessCS.hlsl so the code uses DepthTexture[dtid] as before (or alternatively sample DepthTexture with uvDb if that was intended), keeping references to dtid, RcpFrameDim, DepthTexture, SharedData::GetScreenDepth, FullBlendDistance, ColorTexture and OutputRW unchanged.package/Shaders/VR/StereoBlendCS.hlsl (1)
160-167: Consider extracting POM depth correction into a helper function.The POM depth correction logic is duplicated between the
MODE_FULL_BLENDpath (lines 160-167) and the Eye 1 reprojection path (lines 226-235). Extracting this to a helper would reduce duplication and ensure consistency.♻️ Extract helper function
+// Apply POM depth correction based on parallax offset stored in reflectance.w +// pomOffset: 0.5 = geometry plane, >0.5 = peak (closer), <0.5 = valley (farther) +float ApplyPOMDepthCorrection(float ndcDepth, float pomOffset, float pomScale) +{ + if (pomOffset <= 1e-2 || pomScale <= 0) + return ndcDepth; + float linDepth = SharedData::GetScreenDepth(ndcDepth); + float depthCorrection = (0.5 - pomOffset) * pomScale; + float newLinDepth = max(linDepth + depthCorrection, 1e-4); + return (SharedData::CameraData.x - SharedData::CameraData.w / newLinDepth) / SharedData::CameraData.z; +}Also applies to: 226-235
🤖 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 160 - 167, Extract the duplicated POM depth correction into a single helper function (e.g., ApplyPOMDepthCorrection) that takes centerDepth, pomOffset (ReflectanceTexture[dtid].w), and POMDepthScale and returns the corrected reproj depth; implement the same logic and thresholds (pomOffset > 1e-2, POMDepthScale > 0, use SharedData::GetScreenDepth, clamp new linear depth to >= 1e-4, and compute reprojDepth via SharedData::CameraData as shown). Replace the block in the MODE_FULL_BLEND path (reprojDepthFB calculation) and the Eye 1 reprojection path (lines ~226-235) to call this helper so both places share the exact same behavior.src/Features/Upscaling.cpp (3)
1553-1558:vrLinearSampleris created redundantly in two locations.The sampler is created both in
FillPeriphery(lines 1553-1558) and inFinalizePerEyeOutputs(lines 1342-1348). While the second creation will be a no-op due to theif (!vrLinearSampler)check, consider consolidating sampler creation to a single location (e.g.,SetupResources()) for clarity.♻️ Consolidate sampler creation
Move the sampler creation to
SetupResources()or a dedicated lazy-init helper to avoid duplicated code:ID3D11SamplerState* Upscaling::GetVRLinearSampler() { if (!vrLinearSampler) { D3D11_SAMPLER_DESC sampDesc = {}; sampDesc.Filter = D3D11_FILTER_MIN_MAG_MIP_LINEAR; sampDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP; sampDesc.AddressV = D3D11_TEXTURE_ADDRESS_CLAMP; sampDesc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP; DX::ThrowIfFailed(globals::d3d::device->CreateSamplerState(&sampDesc, vrLinearSampler.put())); } return vrLinearSampler.get(); }Also applies to: 1342-1349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1553 - 1558, The vrLinearSampler creation is duplicated in FillPeriphery and FinalizePerEyeOutputs; refactor to centralize creation (e.g., in SetupResources or a lazy helper) by moving the D3D11_SAMPLER_DESC + CreateSamplerState logic into a single accessor (suggested name GetVRLinearSampler) on the Upscaling class and replace both original creation sites with calls to that accessor; ensure GetVRLinearSampler lazily creates vrLinearSampler using globals::d3d::device->CreateSamplerState and returns the sampler, and remove the redundant CreateSamplerState blocks from FillPeriphery and FinalizePerEyeOutputs.
2540-2541: Inconsistent VR detection: useglobals::game::isVRfor consistency.Line 2540 uses
REL::Module::IsVR()while the rest of the file consistently usesglobals::game::isVR. Based on coding guidelines, runtime detection should use consistent patterns.♻️ Use consistent VR detection
- if (REL::Module::IsVR() && globals::features::vrStereoOptimizations.loaded) + if (globals::game::isVR && globals::features::vrStereoOptimizations.loaded) globals::features::vrStereoOptimizations.ApplyCAS(a_target);As per coding guidelines: "Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and
REL::RelocateMember()patterns for compatibility."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 2540 - 2541, Replace the direct REL::Module::IsVR() call with the project's runtime VR flag to keep detection consistent: change the conditional that currently checks REL::Module::IsVR() (around the vrStereoOptimizations.ApplyCAS(a_target) call) to use globals::game::isVR instead so vrStereoOptimizations.ApplyCAS(a_target) runs under the same VR detection used elsewhere.
1062-1091: Texture recreation for RTV can be simplified.
vrFinalOutput[i]is created on line 1063-1064 withoutD3D11_BIND_RENDER_TARGET, then immediately checked and recreated with that flag on lines 1066-1091. This creates the texture twice during initialization.Consider modifying
CreateTextureFromSourceor using a direct texture creation path that includesD3D11_BIND_RENDER_TARGETfrom the start.♻️ Avoid double texture creation
- // Full display-res composition target (needs RTV for PS feathered composite) - vrFinalOutput[i] = CreateTextureFromSource(colorSrc, eyeWidthOut, eyeHeightOut, - false, true, true, ("Upscale_FinalOutput_" + suffix).c_str()); - // Add render target bind flag and create RTV for pixel shader composite - { - D3D11_TEXTURE2D_DESC finalDesc; - vrFinalOutput[i]->resource->GetDesc(&finalDesc); - if (!(finalDesc.BindFlags & D3D11_BIND_RENDER_TARGET)) { - // Recreate with render target support - finalDesc.BindFlags |= D3D11_BIND_RENDER_TARGET; - vrFinalOutput[i] = eastl::make_unique<Texture2D>(finalDesc); - // ... SRV/UAV creation ... - } - // RTV creation ... - } + // Full display-res composition target with RTV for PS feathered composite + { + D3D11_TEXTURE2D_DESC finalDesc = {}; + finalDesc.Width = eyeWidthOut; + finalDesc.Height = eyeHeightOut; + // ... copy other fields from colorSrc desc ... + finalDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS | D3D11_BIND_RENDER_TARGET; + vrFinalOutput[i] = eastl::make_unique<Texture2D>(finalDesc); + // Create SRV, UAV, RTV once + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1062 - 1091, The code currently creates vrFinalOutput[i] via CreateTextureFromSource and then checks finalDesc.BindFlags to recreate a Texture2D with D3D11_BIND_RENDER_TARGET if missing, causing double creation; modify the initialization so the texture is created once with render-target support: either update CreateTextureFromSource to accept an extra flag/parameter to request D3D11_BIND_RENDER_TARGET (and use that when calling it for the final composite), or replace the CreateTextureFromSource call for vrFinalOutput[i] with a direct Texture2D construction that includes D3D11_BIND_RENDER_TARGET (ensuring Util::SetResourceName, CreateSRV/CreateUAV as needed) before calling CreateRTV; locate uses of vrFinalOutput, CreateTextureFromSource, Texture2D, CreateSRV/CreateUAV and CreateRTV to implement the single-step creation.
🤖 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 fallback branch currently seeds pixelOffset with 0 which
mismatches the neutral/far value of 0.5 used on the other path; update the
skipped/parallax-walk fallback to set pixelOffset = 0.5 so the encoded value is
continuous across the branch. Locate the pixelOffset assignment in
ExtendedMaterials.hlsli (the non-hit/skip path) and change the literal from 0 to
0.5; ensure no other dependent math assumes zero so the parity with the value
used where the parallax walk returns a far value remains consistent.
In `@features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl`:
- Around line 33-49: Add a tail-thread guard at the top of the compute shader
main so threads outside the actual resource dimensions return early: call
GetDimensions for ColorInOut (and optionally DepthIn/FallbackIn) to obtain the
real width/height and if dispatchID.xy is outside those dimensions return before
computing depthPos; do this instead of relying on ColorWidth/DepthWidth CB
values. Place this check before any accesses to ColorInOut, DepthIn, or
FallbackIn in function main to prevent out-of-bounds reads when ceil-div thread
groups are used.
In `@features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl`:
- Around line 55-61: The current code scales packed stereo UVs before splitting
eyes, shifting the SBS seam when Scale.x < 1; instead compute eye-local UV first
by determining isRight from input.TexCoord.x, remap the packed X into [0,1] per
eye (subtract 0.5 for right eye then multiply by 2.0), apply Scale to that
eye-local UV, clamp in eye-local space, then remap the X back into packed stereo
coordinates (scale by 0.5 and add 0.5 for the right eye). Update uses of uv,
isRight, Scale and halfScale accordingly so the seam remains at x == 0.5.
In `@features/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlsl`:
- Around line 8-11: Guard the compute shader's tail threads by querying the UAV
dimensions and returning early when dispatchID is out of bounds: inside the
compute entrypoint main(uint3 dispatchID : SV_DispatchThreadID) call
ColorInOut.GetDimensions(uint width, uint height) (or the appropriate
GetDimensions overload) into local uint2/dimension variables, then if
(dispatchID.x >= width || dispatchID.y >= height) return; before reading/writing
ColorInOut so the main body (the lines that read ColorInOut[dispatchID.xy], set
c.a and write back) never access out-of-range pixels.
In `@package/Shaders/Common/VR.hlsli`:
- Around line 630-638: GetVRVSOutput applies a post-projection jitter for
a_eyeIndex==1 but the reprojection helpers (ConvertMonoUVToOtherEye,
ResolveMonoUVForEye, ReprojectToOtherEye and the back-check in
FinalizeStereoBlend) compute correspondences from camera matrices only, causing
a systematic mismatch; either remove the eye-1 post-projection jitter from
GetVRVSOutput and apply the jitter only in the final
edge-supersampling/alpha-test path, or update the reprojection helpers to
incorporate the same NDC offset (kJitterNDC scaled by VRPosition.w) when mapping
between eyes so reprojections and back-checks use the same perturbed positions.
Ensure the unique symbol GetVRVSOutput is changed consistently and that
ConvertMonoUVToOtherEye, ResolveMonoUVForEye, ReprojectToOtherEye and the
FinalizeStereoBlend back-checks are updated to use the same offset math if you
choose the latter approach.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3200-3205: Summary: The preprocessor guard that sets
psout.Reflectance.w currently requires PARALLAX (or LANDSCAPE) so materials that
set pixelOffset via complexMaterialParallax or TRUE_PBR displacement end up with
Reflectance.w = 0.0. Fix: update the conditional around the psout.Reflectance
assignment so that the branch that packs pixelOffset runs not only when
PARALLAX/LANDSCAPE is defined but also when TRUE_PBR is present (i.e., change
the inner clause to include TRUE_PBR), ensuring pixelOffset (written by
complexMaterialParallax and TRUE_PBR displacement paths) is preserved in
psout.Reflectance.w; refer to the existing symbols psout.Reflectance,
pixelOffset, indirectLobeWeights.specular, and the preprocessor marks PARALLAX,
LANDSCAPE, EMAT, TRUE_PBR when making the change.
In `@package/Shaders/RunGrass.hlsl`:
- Around line 505-513: alphaRef can go below 0 after the per-eye adjustment,
making the discard check a no-op for one eye; clamp the adjusted alphaRef to
[0,1] before comparing with diffuseAlpha. Locate the blocks that set alphaRef =
AlphaTestRefRS and then subtract convergenceEyeIndex * 0.1 (the one using
Stereo::GetEyeIndexPS with input.HPosition and VPOSOffset and the other similar
block) and apply a saturate/clamp to alphaRef (or use max/min) so the subsequent
if ((diffuseAlpha - alphaRef) < 0) discard; uses a clamped alphaRef.
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 47-52: The reprojection currently converts continuous
otherEyeUV/eye0StereoUV to an int2 (eye0Px) and samples OutputRW at a single
texel, losing subpixel offsets and causing shimmer; instead compute the
floating-point sample position using eye0StereoUV * FrameDim, split it into
integer base pixel (e.g., floor) and fractional blend (frac), then perform
bilinear reconstruction by reading the four neighboring texels from OutputRW
(base, base + (1,0), base + (0,1), base + (1,1)) and blend them with weights
derived from the fractional components to produce reprojectedColor; update the
code around Stereo::ConvertMonoUVToOtherEye / Stereo::ConvertToStereoUV usage,
the eye0StereoUV/otherEyeUV calculations, and the OutputRW accesses to implement
the four-UAV-load bilinear sample instead of a single point-load.
In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl`:
- Around line 36-50: The shader currently leaves MODE_EDGE pixels alive so they
get stencil ref=1 but there is no reconstruction path (ReprojectionCS only
handles MODE_MAIN), causing dropped pixels at depth discontinuities; fix by
ensuring MODE_EDGE is not stencil-marked — either discard MODE_EDGE here (add
"if (mode == MODE_EDGE) discard;" alongside the existing discards around mode
checks) or implement a matching reconstruction path for MODE_EDGE in
ReprojectionCS and update the mode description in cbuffers.hlsli to reflect its
handled behavior; locate the check using the mode variable and symbols
MODE_EDGE, MODE_MAIN, MODE_FULL_BLEND and apply the chosen fix so every
stencil-marked mode has a corresponding repro path.
In `@src/Features/TAAReorder.cpp`:
- Around line 185-190: Reset the per-frame sticky flags at the start of the
image-space shader hook before delegating to the wrapped renderer: in
BSImagespaceShaderHook::thunk clear g_postPPReady, g_dlssReady,
g_phase5Complete, and g_dlssPasteComplete (set to false/0) immediately before
calling func(a_this, a_param) so each frame begins fresh; apply the identical
reset in the other corresponding hook implementation referenced later (the same
function pattern around the second occurrence) to ensure both code paths clear
these globals before running the wrapped render.
- Around line 262-393: Saved pipeline state for blend/shaders/viewports/PS
resources is incomplete; also capture and restore the IA/RS/OM render-target
state you modify. Add calls to RSGetState/RSSetState to save/restore the
rasterizer (oldRS), IAGetInputLayout/IASetInputLayout for input layout (oldIL),
IAGetVertexBuffers/IASetVertexBuffers for vertex buffer bindings (oldVB[],
oldStrides[], oldOffsets[]), IAGetPrimitiveTopology/IASetPrimitiveTopology for
topology (oldTopo), and OMGetRenderTargets/OMSetRenderTargets to save/restore
render targets and depth-stencil view (oldRTV[], oldDSV). Use the same
lifetime/ownership pattern as existing saved objects (raw ID3D11* pointers or
ComPtr) and restore them alongside the existing restores (OMSetBlendState,
RSSetViewports, VS/PS/PS resources/samplers/CB) before returning from the
feathered composite block.
- Around line 60-72: The post-PP copy creation is unchecked and can leave
g_postPPCopy/g_postPPCopySRV null, causing ExecutePassHook to perform a null
CopyResource; modify the CreateTexture2D and CreateShaderResourceView calls (the
CreateTexture2D/CreateShaderResourceView invocations that produce g_postPPCopy
and g_postPPCopySRV) to check their HRESULTs and if either fails release any
partially-created resource, set a capture-disabled flag (or set the capture path
to failed) and skip/avoid calling CopyResource in ExecutePassHook; also
replicate the same HRESULT checks and failure-path handling for the other
instance noted around the second creation (the calls referenced later that also
assign g_postPPCopy/g_postPPCopySRV) so the code gracefully degrades instead of
copying into a null destination.
In `@src/Features/Upscaling.cpp`:
- Around line 1277-1280: Replace manual COM ref management around the OM state
save/restore with RAII com_ptr usage to avoid leaks if an exception occurs:
instead of raw pointers like oldBlendState, oldRTView, oldDepthStencilView and
later Release() calls, capture the results of OMGetBlendState and
OMGetRenderTargets into winrt::com_ptr<ID3D11BlendState>,
winrt::com_ptr<ID3D11RenderTargetView>, and
winrt::com_ptr<ID3D11DepthStencilView> (use .put() when calling OMGet*), store
oldBlendFactor/oldSampleMask as before, and then restore with
OMSetBlendState/OMSetRenderTargets using .get(); remove the manual Release()
calls (or guard them) so cleanup is automatic even if DX::ThrowIfFailed or other
exceptions occur.
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 242-253: Compute and use a single sanitized pair for viewport
scale and crop offset rather than using raw globals everywhere: read
globals::features::upscaling.settings.vrDlssViewportScale and vrDlssCropOffsetX,
clamp vpScale to a safe minimum (e.g., > 0 and <= 1) and clamp crop offset so it
never exceeds (1 - effectiveVpScale)/2, and derive effectiveCropOffset
accordingly; then use effectiveVpScale and effectiveCropOffset when computing
slConstants (e.g., cameraPinholeOffset, FOV/mvec and any other Streamline
constants) and when performing the later eye-crop bounds logic to avoid
inf/zero-sized inputs and mismatches. Apply the same sanitization reuse in the
other Streamline constant build sites mentioned (the sections around the other
occurrences).
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The welcome-overlay gate currently only checks
RE::MainMenu::MENU_NAME (in the bool shouldShow using globals::game::ui) but the
UI text also claims VR controls work from the tween menu, so either extend the
gate to include the tween menu or update the copy; to fix, modify the shouldShow
expression to also accept the tween menu (e.g., add an OR check against the
tween menu identifier used by the engine/UI, similar to RE::MainMenu::MENU_NAME)
so the overlay appears when the tween menu is open, or alternatively change the
onboarding text to remove the tween-menu mention so behavior and copy match
(adjust the string resource where the copy is defined).
---
Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 99-107: Replace the magic literal "2" used for the stereo
optimization mode check with a named constant to improve readability and
maintainability: define a constant like MODE_MAIN (or STEREO_OPT_MODE_MAIN) and
use it in the if-check that reads uint mode =
StereoOptModeTexture[uint2(dispatchID.xy)]; then compare if (mode == MODE_MAIN)
inside the VR_STEREO_OPT block (referencing eyeIndex, StereoOptModeTexture,
dispatchID, and the local variable mode) so future readers know what the value
represents.
In `@package/Shaders/DistantTree.hlsl`:
- Around line 206-214: Replace the hardcoded per-eye offset 0.1 used when
computing alphaRef (around the AlphaTestRefRS adjustment in DistantTree.hlsl
where VR and eyeIndex are used) with the shared VR alpha threshold value/wrapper
(e.g. the VRAlphaTestThreshold value from the existing cbuffer) so the per-eye
adjustment is tunable and consistent across shaders; update the calculation to
subtract eyeIndex * VRAlphaTestThreshold (or a new named constant referencing
that cbuffer) and apply the same change to the other occurrence (the similar
block at lines ~222-230) and ensure the cbuffer symbol is declared/imported
where this shader is compiled.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 160-167: Extract the duplicated POM depth correction into a single
helper function (e.g., ApplyPOMDepthCorrection) that takes centerDepth,
pomOffset (ReflectanceTexture[dtid].w), and POMDepthScale and returns the
corrected reproj depth; implement the same logic and thresholds (pomOffset >
1e-2, POMDepthScale > 0, use SharedData::GetScreenDepth, clamp new linear depth
to >= 1e-4, and compute reprojDepth via SharedData::CameraData as shown).
Replace the block in the MODE_FULL_BLEND path (reprojDepthFB calculation) and
the Eye 1 reprojection path (lines ~226-235) to call this helper so both places
share the exact same behavior.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 56-68: The DebugMode == 2 block computes uvDb but never uses it;
remove the unused local variable uvDb (or replace depth sampling to use uvDb if
intended) by deleting the declaration "float2 uvDb = (dtid + 0.5) *
RcpFrameDim;" in the DebugMode branch in VRPostProcessCS.hlsl so the code uses
DepthTexture[dtid] as before (or alternatively sample DepthTexture with uvDb if
that was intended), keeping references to dtid, RcpFrameDim, DepthTexture,
SharedData::GetScreenDepth, FullBlendDistance, ColorTexture and OutputRW
unchanged.
In `@src/Features/Upscaling.cpp`:
- Around line 1553-1558: The vrLinearSampler creation is duplicated in
FillPeriphery and FinalizePerEyeOutputs; refactor to centralize creation (e.g.,
in SetupResources or a lazy helper) by moving the D3D11_SAMPLER_DESC +
CreateSamplerState logic into a single accessor (suggested name
GetVRLinearSampler) on the Upscaling class and replace both original creation
sites with calls to that accessor; ensure GetVRLinearSampler lazily creates
vrLinearSampler using globals::d3d::device->CreateSamplerState and returns the
sampler, and remove the redundant CreateSamplerState blocks from FillPeriphery
and FinalizePerEyeOutputs.
- Around line 2540-2541: Replace the direct REL::Module::IsVR() call with the
project's runtime VR flag to keep detection consistent: change the conditional
that currently checks REL::Module::IsVR() (around the
vrStereoOptimizations.ApplyCAS(a_target) call) to use globals::game::isVR
instead so vrStereoOptimizations.ApplyCAS(a_target) runs under the same VR
detection used elsewhere.
- Around line 1062-1091: The code currently creates vrFinalOutput[i] via
CreateTextureFromSource and then checks finalDesc.BindFlags to recreate a
Texture2D with D3D11_BIND_RENDER_TARGET if missing, causing double creation;
modify the initialization so the texture is created once with render-target
support: either update CreateTextureFromSource to accept an extra flag/parameter
to request D3D11_BIND_RENDER_TARGET (and use that when calling it for the final
composite), or replace the CreateTextureFromSource call for vrFinalOutput[i]
with a direct Texture2D construction that includes D3D11_BIND_RENDER_TARGET
(ensuring Util::SetResourceName, CreateSRV/CreateUAV as needed) before calling
CreateRTV; locate uses of vrFinalOutput, CreateTextureFromSource, Texture2D,
CreateSRV/CreateUAV and CreateRTV to implement the single-step creation.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 142-154: The sampler creation currently happens lazily inside
DrawStereoBlend() which can hitch on first draw; move the D3D11_SAMPLER_DESC
creation and device->CreateSamplerState call into SetupResources() and
initialize stereoBlendLinearSampler there (preserve
D3D11_FILTER_MIN_MAG_MIP_LINEAR and CLAMP addresses), then remove the
CreateSamplerState branch from DrawStereoBlend() and just bind
stereoBlendLinearSampler via context->CSSetSamplers(0, 1,
&stereoBlendLinearSampler) (use the existing stereoBlendLinearSampler
getter/put/get() helpers as in the diff).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b141013-ce47-44dc-af44-b168f855fc7f
📒 Files selected for processing (40)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlslfeatures/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlslfeatures/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/TAAReorder.cppsrc/Features/TAAReorder.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.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
Comment on lines
+1277
to
+1280
| ID3D11BlendState* oldBlendState = nullptr; | ||
| float oldBlendFactor[4]; | ||
| UINT oldSampleMask; | ||
| context->OMGetBlendState(&oldBlendState, oldBlendFactor, &oldSampleMask); |
Contributor
There was a problem hiding this comment.
OM state save/restore could leak COM references on exception.
The OMGetBlendState/OMGetRenderTargets calls increment COM reference counts, and the cleanup relies on Release() at lines 1374-1380. If any D3D call between save and restore throws (e.g., DX::ThrowIfFailed in a nested call), these COM objects will leak.
Consider using RAII wrappers or winrt::com_ptr for automatic cleanup. This is a minor issue since exceptions are rare in this code path.
🛡️ Suggested RAII pattern
// Use com_ptr for automatic cleanup
winrt::com_ptr<ID3D11BlendState> savedBlendState;
context->OMGetBlendState(savedBlendState.put(), oldBlendFactor, &oldSampleMask);
// ... drawing code ...
// Restore happens via .get(), cleanup is automatic
context->OMSetBlendState(savedBlendState.get(), oldBlendFactor, oldSampleMask);Also applies to: 1373-1380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/Upscaling.cpp` around lines 1277 - 1280, Replace manual COM ref
management around the OM state save/restore with RAII com_ptr usage to avoid
leaks if an exception occurs: instead of raw pointers like oldBlendState,
oldRTView, oldDepthStencilView and later Release() calls, capture the results of
OMGetBlendState and OMGetRenderTargets into winrt::com_ptr<ID3D11BlendState>,
winrt::com_ptr<ID3D11RenderTargetView>, and
winrt::com_ptr<ID3D11DepthStencilView> (use .put() when calling OMGet*), store
oldBlendFactor/oldSampleMask as before, and then restore with
OMSetBlendState/OMSetRenderTargets using .get(); remove the manual Release()
calls (or guard them) so cleanup is automatic even if DX::ThrowIfFailed or other
exceptions occur.
✅ Confirmed as addressed by @vrnord
Contributor
Author
There was a problem hiding this comment.
That was Fix 2003-D — we added IAGetInputLayout/IASetInputLayout, IAGetPrimitiveTopology/IASetPrimitiveTopology, and RSGetState/RSSetState save/restore. Claude Code reported it applied and PR #2003 passed CI.
Contributor
There was a problem hiding this comment.
@vrnord, understood — the pipeline state save/restore (IA and RS) is addressed. The COM reference leak concern for oldBlendState/oldRTV/oldDSV was separately acknowledged as deferred in favor of the existing codebase pattern (as noted in the PR summary). Both aspects are accounted for.
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:29.538Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:25.118Z
Learning: In src/Features/VR/StereoBlend.cpp, treat StereoBlendDebugMode values 4 and 5 as the same shader variant. When vrStereoOptActive is true, stereoBlendOverwriteCS is selected unconditionally; do not create distinct code paths or shader variants for 4 vs 5. Consolidate 4 and 5 into a single path (e.g., a single case) and document the mapping with comments. Update reviews, tests, and any CI/build logic to reflect that 4/5 share the same shader with differing cbuffers, and ensure no behavior differs between them.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Flag potential problems proactively including performance impact on rendering, SE/AE/VR runtime compatibility issues, GPU register conflicts, and security risks from malformed configurations
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:47.322Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:16.698Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: zndxcvbn
Repo: doodlum/skyrim-community-shaders PR: 1929
File: src/Utils/UI.cpp:55-57
Timestamp: 2026-02-22T14:39:02.543Z
Learning: In the Skyrim community shaders project, initialize display-related scaling values once at startup because the resolution cannot be changed at runtime. Do not recompute or refresh these values during a session; store them in a static or application-wide initialization path (e.g., at startup or in a startup singleton) to avoid stale scaling. This applies to UI-related initialization in source files like src/Utils/UI.cpp.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
from
27d1d7e to
11d5e5c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
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 | 🟡 MinorDocument the two new overwrite debug modes.
The combo now exposes
OverwriteandOverwrite Eye1, but the tooltip still stops atEdge Detection. Please add a short description for both so the new options are discoverable.🤖 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, The tooltip text shown inside the Util::HoverTooltipWrapper after the ImGui::Combo for "Debug View" (which uses debugModes and settings.StereoBlendDebugMode) needs two new short paragraphs describing the "Overwrite" and "Overwrite Eye1" modes; update the string passed to ImGui::Text to append concise lines explaining that "Overwrite" forces the composited pixel to come from one eye (or destination) regardless of blending and that "Overwrite Eye1" forces using eye1's image specifically (include brief mention of when these are useful for debugging), so users can discover what those options do.
♻️ Duplicate comments (2)
features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl (1)
33-50:⚠️ Potential issue | 🟠 MajorHarden the remap path before indexing the textures.
There is still no tail-thread guard here, and the scaling branch only checks
DepthWidth > 0even thoughColorWidth/ColorHeightcan still be zero from the caller defaults. A ceil-div dispatch or partially populated CB will send threads into invalid address generation before the mask test.🛠️ Minimal hardening
[numthreads(8, 8, 1)] void main(uint3 dispatchID : SV_DispatchThreadID) { + uint colorWidthTex, colorHeightTex; + ColorInOut.GetDimensions(colorWidthTex, colorHeightTex); + if (dispatchID.x >= colorWidthTex || dispatchID.y >= colorHeightTex) + return; + uint2 colorPos = dispatchID.xy + uint2(ColorOffsetX, 0); uint2 depthPos; - if (DepthWidth > 0) { + if (DepthWidth > 0 && DepthHeight > 0 && ColorWidth > 0 && ColorHeight > 0) { // Scale from display-res color coordinates to render-res depth coordinates depthPos = uint2( (dispatchID.x * DepthWidth) / ColorWidth,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl` around lines 33 - 50, The kernel lacks tail-thread guards and divides by ColorWidth/ColorHeight unsafely; before computing colorPos/depthPos and indexing ColorInOut/DepthIn/FallbackIn inside main, validate that dispatchID.x < ColorWidth && dispatchID.y < ColorHeight (and that ColorWidth/ColorHeight are non-zero) and also guard depth remap by checking DepthWidth/DepthHeight > 0 before doing the scaled division, or else clamp/validate computed depthPos against DepthWidth/DepthHeight; apply these checks around the logic that uses ColorOffsetX, DepthOffsetX/DepthOffsetY and the DepthIn/ColorInOut/FallbackIn accesses to avoid out-of-bounds indexing.src/Features/VR/SettingsUI.cpp (1)
76-76:⚠️ Potential issue | 🟡 MinorKeep the welcome-overlay gate in sync with the tween-menu instructions.
shouldShowonly checksRE::MainMenu::MENU_NAME, but the copy below still says the VR controls work from the tween menu too. Either include the tween menu here or trim the text so behavior and onboarding match.🤖 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 welcome-overlay gate (variable shouldShow) only checks for RE::MainMenu::MENU_NAME but the UI copy mentions the tween menu; update shouldShow to also detect the tween menu (e.g., add an additional IsMenuOpen(RE::TweenMenu::MENU_NAME) check alongside the existing IsMenuOpen(RE::MainMenu::MENU_NAME) and keep the existing globals::menu->IsEnabled logic), or alternatively remove/adjust the tween-menu wording in the overlay text so the onboarding copy matches the current behavior controlled by shouldShow.
🧹 Nitpick comments (3)
src/Features/TAAReorder.h (2)
27-30: Consider removing unused include and forward declaration.
<intrin.h>andstruct Upscaling;do not appear to be referenced in this header's declarations. If only needed by the implementation, they can be moved to the.cppfile to keep the header minimal.♻️ Proposed cleanup
`#include` <Windows.h> `#include` <d3d11.h> -#include <intrin.h> `#include` <winrt/base.h> - -struct Upscaling;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/TAAReorder.h` around lines 27 - 30, Remove the unused header include and forward declaration to keep the header minimal: delete the `#include` <intrin.h> line and the forward declaration "struct Upscaling;" from TAAReorder.h if neither is referenced in the header's class/type/member declarations (leave any usage in the corresponding .cpp file instead); search for identifiers related to intrin (intrinsic functions) and the Upscaling type to ensure they remain available in the implementation file and move those includes/forward-decls into the .cpp where they are actually used (e.g., any functions/methods that reference Upscaling or intrin functions).
85-101: State flags assume single-threaded D3D11 immediate context.The per-frame state flags (
g_postPPReady,g_dlssReady,g_phase5Complete, etc.) are modified across multiple hook points. This is fine for D3D11's single-threaded immediate context, but a brief comment documenting this assumption would help future maintainers avoid introducing multi-threaded access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/TAAReorder.h` around lines 85 - 101, Add a brief comment near the per-frame state flags (g_postPPReady, g_dlssReady, g_dlssPasteComplete, g_phase5Complete, g_insideConductor, g_bsHookCallCount) stating these flags are intentionally accessed/modified across multiple D3D11 hook points under the assumption of a single-threaded D3D11 immediate context and must be protected or reworked if used from multiple threads or deferred contexts; place the comment adjacent to the existing flag declarations so future maintainers see the threading assumption when editing those symbols.src/Features/Upscaling.h (1)
145-150: Consider replacing theClearHMDMaskparameter list with a small config struct.This helper now mixes ten positional region/sizing arguments plus an optional fallback source. It is very easy to swap depth/color rectangles at call sites once every field is just a
uint32_t.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.h` around lines 145 - 150, Replace the long positional parameter list of ClearHMDMask with a small POD config struct (e.g., HMDMaskConfig) containing the previous parameters: colorUAV, depthSRV can remain as separate pointer parameters or be included in the struct (pick one consistent approach), and uint32_t fields depthOffsetX, depthOffsetY, depthWidth, depthHeight, colorOffsetX, colorWidth, colorHeight, fallbackOffsetX plus an optional fallbackSRV; change the ClearHMDMask declaration to accept const HMDMaskConfig& (or HMDMaskConfig&& if intended) and update all callers to construct/populate HMDMaskConfig by name so fields are explicit; ensure any internal use inside ClearHMDMask references config.field names and keep parameter validation/defaults inside the struct or constructor.
🤖 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:
- Line 567: The markdown heading "##Lessons Learned" is missing the required
space after the hash marks; update the heading string "##Lessons Learned" to "##
Lessons Learned" so it renders correctly in markdown parsers (search for the
exact heading text "##Lessons Learned" and add the space).
- Around line 569-576: The markdown bullets in this section use escaped
asterisks (`\*\*`) so bold formatting doesn't render; update the entries
referencing VR.hlsli, BuildRelease.bat / AIO and cmake --install, VS Code Edit
tool, StereoBlendCS, and DeferredCompositeCS to use plain double asterisks
(`**`) around the bolded phrases (e.g., change `\*\*VR.hlsli` to `**VR.hlsli**`)
so the headings render as bold in the document.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: The overwrite-path in StereoBlend.cpp binds only two UAVs
([main.UAV, motionVectors.UAV]) but DeferredCompositeCS.hlsl (VR_STEREO_OPT)
declares three UAVs at u0–u2 (MainRW, NormalTAAMaskSpecularMaskRW,
MotionVectorsRW), causing NormalTAAMaskSpecularMaskRW to receive
motionVectors.UAV and MotionVectorsRW to be unbound; fix by either updating
StereoBlend.cpp’s overwrite path to bind three UAVs in the correct order (bind
main.UAV -> u0, NormalTAAMaskSpecularMaskRW's buffer -> u1, motionVectors.UAV ->
u2) or modify DeferredCompositeCS.hlsl to match the two-UAV scheme
(collapse/remove NormalTAAMaskSpecularMaskRW or remap its usages to existing
UAVs) and update all uses of MainRW, NormalTAAMaskSpecularMaskRW, and
MotionVectorsRW accordingly so shader registers u0–u2 and the bound UAV list are
consistent.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 97-108: The classifier currently ignores the runtime EdgeWidth
parameter because maxWidth is hardcoded to kInnerWidth; update the search to use
the EdgeWidth from VRStereoOptParams (e.g., read params.EdgeWidth or EdgeWidth)
when computing maxWidth (e.g., maxWidth = max(kInnerWidth, params.EdgeWidth)) so
runtime tuning takes effect, or remove EdgeWidth from VRStereoOptParams and any
related code if you prefer to eliminate the unused knob; key symbols to update
are maxWidth, kInnerWidth, and the VRStereoOptParams/EdgeWidth usage in this
stencil classifier.
In `@src/Deferred.cpp`:
- Around line 373-379: The VR_STEREO_OPT shader variant can get out of sync with
slot 16 binding: update the code so the shader variant state and the bound SRV
for slot 16 are always consistent. Either (A) when creating/caching the
VR_STEREO_OPT variants for mainCompositeCS/mainCompositeInteriorCS (the code
paths at the cached-shader sites referenced as lines ~596-597 and ~624-625),
guard the define/caching with the same predicate used when binding
(REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode !=
VRStereoOptimizations::StereoMode::Off) and invalidate/recreate those cached
shaders whenever stereoMode changes; or (B) always bind a valid fallback SRV to
slot 16 in the CSSetShaderResources call site (the block that calls
vrStereoOpt.GetModeTextureSRV() and context->CSSetShaderResources(16, 1,
&modeSRV)) so that when stereoMode is Off the shader still sees a neutral
texture (create or reuse a neutral/fallback SRV from vrStereoOpt or a static
blank texture). Ensure the chosen approach updates both the binding site
(GetModeTextureSRV / CSSetShaderResources) and the shader caching code paths
(mainCompositeCS / mainCompositeInteriorCS) so they remain in sync.
In `@src/FeatureBuffer.cpp`:
- Around line 57-58: The PR contains incomplete VRTerrainBlend stubs that break
compilation — remove or fully implement the feature; either delete all
references to globals::features::vrTerrainBlend.settings (e.g., in
FeatureBuffer.cpp, Globals.cpp, Feature.cpp, Deferred.cpp and any feature
registration lists) and remove any references to VRTerrainBlend header/includes
and shader references, or add the missing implementation: create
Features/VRTerrainBlend.h/cpp with the matching class and settings member, add
vrTerrainBlendSettings into the FeatureData cbuffer in
package/Shaders/Common/SharedData.hlsli, and provide the required shader code
under package/ to match the CPU-side settings. Ensure all uses of
vrTerrainBlendSettings and globals::features::vrTerrainBlend.settings are
consistent and compiled.
In `@src/Features/TAAReorder.h`:
- Line 114: g_submitTexUAVOwner is a non-owning raw pointer that can become
dangling if the texture it points to is released; update the code that releases
or resets g_submitTexUAV (the UAV) to also set g_submitTexUAVOwner = nullptr (or
otherwise clear it) so the owner-tracking pointer is never left pointing at
freed memory, and add a brief comment near g_submitTexUAVOwner and the release
path documenting this clearing behavior (refer to symbols g_submitTexUAVOwner
and g_submitTexUAV).
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 305-320: The perf event begun with
globals::state->BeginPerfEvent("VR Stereo Opt - Stencil") is not closed on the
early bailout when depthSRV is null; update the DispatchStencil code path so
that before returning on !depthSRV you call globals::state->EndPerfEvent() (or
otherwise balance BeginPerfEvent/EndPerfEvent) so frame annotations are always
paired, referencing the existing BeginPerfEvent, EndPerfEvent, globals::state,
and depthSRV symbols.
- Around line 21-36: The SaveSettings function in VRStereoOptimizations does not
serialize the new POM/full-blend debug controls, so add entries for
settings.pomDepthScale, settings.debugFullBlendDepth, and settings.debugPOMDepth
(e.g. o_json["POMDepthScale"] = settings.pomDepthScale;
o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth;
o_json["DebugPOMDepth"] = settings.debugPOMDepth;) to ensure they persist; also
verify the corresponding LoadSettings or constructor that reads these keys
(where settings are restored) includes matching keys so the UI state round-trips
correctly.
---
Outside diff comments:
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 326-345: The tooltip text shown inside the
Util::HoverTooltipWrapper after the ImGui::Combo for "Debug View" (which uses
debugModes and settings.StereoBlendDebugMode) needs two new short paragraphs
describing the "Overwrite" and "Overwrite Eye1" modes; update the string passed
to ImGui::Text to append concise lines explaining that "Overwrite" forces the
composited pixel to come from one eye (or destination) regardless of blending
and that "Overwrite Eye1" forces using eye1's image specifically (include brief
mention of when these are useful for debugging), so users can discover what
those options do.
---
Duplicate comments:
In `@features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl`:
- Around line 33-50: The kernel lacks tail-thread guards and divides by
ColorWidth/ColorHeight unsafely; before computing colorPos/depthPos and indexing
ColorInOut/DepthIn/FallbackIn inside main, validate that dispatchID.x <
ColorWidth && dispatchID.y < ColorHeight (and that ColorWidth/ColorHeight are
non-zero) and also guard depth remap by checking DepthWidth/DepthHeight > 0
before doing the scaled division, or else clamp/validate computed depthPos
against DepthWidth/DepthHeight; apply these checks around the logic that uses
ColorOffsetX, DepthOffsetX/DepthOffsetY and the DepthIn/ColorInOut/FallbackIn
accesses to avoid out-of-bounds indexing.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The welcome-overlay gate (variable shouldShow) only checks for
RE::MainMenu::MENU_NAME but the UI copy mentions the tween menu; update
shouldShow to also detect the tween menu (e.g., add an additional
IsMenuOpen(RE::TweenMenu::MENU_NAME) check alongside the existing
IsMenuOpen(RE::MainMenu::MENU_NAME) and keep the existing
globals::menu->IsEnabled logic), or alternatively remove/adjust the tween-menu
wording in the overlay text so the onboarding copy matches the current behavior
controlled by shouldShow.
---
Nitpick comments:
In `@src/Features/TAAReorder.h`:
- Around line 27-30: Remove the unused header include and forward declaration to
keep the header minimal: delete the `#include` <intrin.h> line and the forward
declaration "struct Upscaling;" from TAAReorder.h if neither is referenced in
the header's class/type/member declarations (leave any usage in the
corresponding .cpp file instead); search for identifiers related to intrin
(intrinsic functions) and the Upscaling type to ensure they remain available in
the implementation file and move those includes/forward-decls into the .cpp
where they are actually used (e.g., any functions/methods that reference
Upscaling or intrin functions).
- Around line 85-101: Add a brief comment near the per-frame state flags
(g_postPPReady, g_dlssReady, g_dlssPasteComplete, g_phase5Complete,
g_insideConductor, g_bsHookCallCount) stating these flags are intentionally
accessed/modified across multiple D3D11 hook points under the assumption of a
single-threaded D3D11 immediate context and must be protected or reworked if
used from multiple threads or deferred contexts; place the comment adjacent to
the existing flag declarations so future maintainers see the threading
assumption when editing those symbols.
In `@src/Features/Upscaling.h`:
- Around line 145-150: Replace the long positional parameter list of
ClearHMDMask with a small POD config struct (e.g., HMDMaskConfig) containing the
previous parameters: colorUAV, depthSRV can remain as separate pointer
parameters or be included in the struct (pick one consistent approach), and
uint32_t fields depthOffsetX, depthOffsetY, depthWidth, depthHeight,
colorOffsetX, colorWidth, colorHeight, fallbackOffsetX plus an optional
fallbackSRV; change the ClearHMDMask declaration to accept const HMDMaskConfig&
(or HMDMaskConfig&& if intended) and update all callers to construct/populate
HMDMaskConfig by name so fields are explicit; ensure any internal use inside
ClearHMDMask references config.field names and keep parameter
validation/defaults inside the struct or constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3fd8cf6-3714-41ee-b9a0-c0e0f7fa3c62
📒 Files selected for processing (46)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlslifeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlslfeatures/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlslfeatures/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/TAAReorder.cppsrc/Features/TAAReorder.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.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 (12)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- src/Features/ExtendedMaterials.h
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- features/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlsl
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/RunGrass.hlsl
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- features/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlsl
🚧 Files skipped from review as they are similar to previous changes (10)
- package/Shaders/DistantTree.hlsl
- src/Features/VR.cpp
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlsl
- features/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlsl
- src/Feature.cpp
- src/Features/Upscaling/Streamline.h
- src/Features/TAAReorder.cpp
- src/Features/Upscaling.cpp
- src/Features/Upscaling/Streamline.cpp
Comment on lines
+97
to
+108
| // --- 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; | ||
|
|
Contributor
There was a problem hiding this comment.
EdgeWidth is effectively a no-op in this classifier.
VRStereoOptParams already carries EdgeWidth, but maxWidth is hardwired to kInnerWidth, so runtime edge-width tuning has no effect in this pass. Either wire EdgeWidth into the search/classification or drop the unused knob.
💡 Minimum fix
static const uint kInnerWidth = 2;
int2 offsets[4] = { int2(-1, 0), int2(1, 0), int2(0, -1), int2(0, 1) };
@@
- uint maxWidth = kInnerWidth;
+ uint maxWidth = max(kInnerWidth, EdgeWidth);📝 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; | |
| // --- 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 = max(kInnerWidth, EdgeWidth); |
🤖 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 - 108,
The classifier currently ignores the runtime EdgeWidth parameter because
maxWidth is hardcoded to kInnerWidth; update the search to use the EdgeWidth
from VRStereoOptParams (e.g., read params.EdgeWidth or EdgeWidth) when computing
maxWidth (e.g., maxWidth = max(kInnerWidth, params.EdgeWidth)) so runtime tuning
takes effect, or remove EdgeWidth from VRStereoOptParams and any related code if
you prefer to eliminate the unused knob; key symbols to update are maxWidth,
kInnerWidth, and the VRStereoOptParams/EdgeWidth usage in this stencil
classifier.
Contributor
Author
There was a problem hiding this comment.
Same issue appears on PR #2002 — EdgeWidth is passed in the cbuffer but the shader hardcodes maxWidth = kInnerWidth. It's a design decision, not a bug. The knob exists for future tuning but isn't wired into the classification loop yet. Low priority.
Contributor
There was a problem hiding this comment.
@vrnord, understood — noted as intentional design. I'll record this so it isn't flagged in future reviews.
( ^ᴗ^ ) ears perk up with curiosity
✏️ Learnings added
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: package/Shaders/VRStereoOptimizations/StencilCS.hlsl:97-108
Timestamp: 2026-03-24T04:37:14.725Z
Learning: In `package/Shaders/VRStereoOptimizations/StencilCS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), `EdgeWidth` is present in `VRStereoOptParams` cbuffer (`cbuffers.hlsli`) as a future-tuning placeholder but is intentionally NOT wired into `maxWidth` in the classification loop — `maxWidth` is hardcoded to `kInnerWidth = 2`. Do not flag this as a bug or suggest `maxWidth = max(kInnerWidth, EdgeWidth)`; the wiring is deferred to a later PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:29.538Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:31.383Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:16.698Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:47.322Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:35:02.326Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
|
|
||
| // ─── Cached UAV for submit texture (ClearHMDMask + ForceAlpha on submit after DLSS paste) ─── | ||
| inline winrt::com_ptr<ID3D11UnorderedAccessView> g_submitTexUAV; | ||
| inline ID3D11Texture2D* g_submitTexUAVOwner = nullptr; // track which texture the UAV belongs to |
Contributor
There was a problem hiding this comment.
Raw pointer for UAV owner tracking could become stale.
g_submitTexUAVOwner is a non-owning raw pointer used to detect when the UAV needs recreation. If the texture is released elsewhere while this pointer still holds its address, comparing against it is undefined behavior.
Consider clearing g_submitTexUAVOwner to nullptr whenever g_submitTexUAV is released, or documenting the lifetime assumption that ensures the texture outlives this tracking pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/TAAReorder.h` at line 114, g_submitTexUAVOwner is a non-owning
raw pointer that can become dangling if the texture it points to is released;
update the code that releases or resets g_submitTexUAV (the UAV) to also set
g_submitTexUAVOwner = nullptr (or otherwise clear it) so the owner-tracking
pointer is never left pointing at freed memory, and add a brief comment near
g_submitTexUAVOwner and the release path documenting this clearing behavior
(refer to symbols g_submitTexUAVOwner and g_submitTexUAV).
This was referenced Mar 23, 2026
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
from
4a98141 to
abf6a39
Compare
Contributor
Author
|
All CodeRabbit feedback addressed in latest push: Bounds guard added in ClearHMDMaskCS (prevents OOB writes on ceil-div dispatch) ✓ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
100-107:⚠️ Potential issue | 🟡 Minor
EdgeWidthis still a no-op here.Line 107 keeps
maxWidthpinned tokInnerWidth, so the runtime edge-width setting never changes the search radius. Please either feed the configured width intomaxWidthor remove the exposed knob so users aren't tuning something this pass ignores.🤖 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 100 - 107, The code pins maxWidth to kInnerWidth, so the runtime EdgeWidth setting is ignored; update the assignment of maxWidth in StencilCS.hlsl to use the configured EdgeWidth (e.g., maxWidth = max(kInnerWidth, EdgeWidth) or directly maxWidth = EdgeWidth if intended) so the search radius actually reflects the user-configured edge width, or if you prefer remove the exposed EdgeWidth knob and any references to it; refer to kInnerWidth, EdgeWidth, and maxWidth in the StencilCS shader to make the change.src/Deferred.cpp (1)
373-379:⚠️ Potential issue | 🟠 MajorVR_STEREO_OPT shader variant binding mismatch persists.
The shader is compiled with
VR_STEREO_OPTwhenevervrStereoOptimizations.loadedis true (Lines 596-597, 624-625), but the mode texture SRV is only bound whenstereoMode != Off(Lines 375-378). If a user loads with VR stereo optimizations enabled then setsstereoMode = Offat runtime, the shader will still contain theVR_STEREO_OPTcode path expecting a valid texture at slot 16, but the SRV won't be bound.🛠️ Suggested fix: align define with runtime condition
Option A: Gate the shader define on the same runtime condition:
- if (REL::Module::IsVR() && globals::features::vrStereoOptimizations.loaded) + if (REL::Module::IsVR() && globals::features::vrStereoOptimizations.loaded && + globals::features::vrStereoOptimizations.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) defines.push_back({ "VR_STEREO_OPT", nullptr });Option B: Invalidate cached shaders when stereoMode changes (requires adding shader cache invalidation logic to VRStereoOptimizations settings).
Option C: Always bind a fallback (1x1 neutral) texture to slot 16 when stereoMode == Off.
Also applies to: 596-597, 624-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 - 379, The VR_STEREO_OPT compile-time define is mismatched with the runtime binding: ensure the shader define is only added when vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off (the same condition used when binding the SRV), or alternatively guarantee a fallback SRV is always bound to slot 16 when the shader was compiled with VR_STEREO_OPT; locate the shader-compile site where VR_STEREO_OPT is added (lines referencing VR_STEREO_OPT and vrStereoOpt.loaded) and either gate the define with vrStereoOpt.settings.stereoMode != Off, add shader cache invalidation when stereoMode changes in VRStereoOptimizations, or make GetModeTextureSRV()/Deferred.cpp bind a 1x1 neutral texture when stereoMode == Off so the shader always sees a valid SRV.src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorDebug settings
debugFullBlendDepthanddebugPOMDepthare not persisted.The
SaveSettingsfunction savesPOMDepthScale(line 36) but does not savedebugFullBlendDepthordebugPOMDepth. These are exposed in the debug UI (lines 253-254) but will reset tofalseon each launch.If this is intentional (debug controls shouldn't persist), consider adding a brief comment. Otherwise, add them to
SaveSettingsandLoadSettings.🐛 Fix to persist debug controls (if desired)
void VRStereoOptimizations::SaveSettings(json& o_json) { // ... existing settings ... o_json["POMDepthScale"] = settings.pomDepthScale; + o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth; + o_json["DebugPOMDepth"] = settings.debugPOMDepth; }And corresponding
LoadSettingsentries.🤖 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, The SaveSettings implementation for VRStereoOptimizations omits persisting the debug flags debugFullBlendDepth and debugPOMDepth so they reset on startup; update VRStereoOptimizations::SaveSettings to add o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth and o_json["DebugPOMDepth"] = settings.debugPOMDepth (matching the naming convention used like "POMDepthScale"), and also update the corresponding LoadSettings logic to read those keys into settings.debugFullBlendDepth and settings.debugPOMDepth (or add a brief comment in both SaveSettings/LoadSettings if the debug controls are intentionally non-persistent).
🧹 Nitpick comments (6)
src/Features/VR/StereoBlend.cpp (1)
143-153: Consider moving sampler creation to SetupResources.The lazy sampler creation works, but
CreateSamplerStateis relatively expensive to call at runtime. Moving this toVR::SetupResources()would be more efficient and consistent with how other D3D11 resources are initialized in this codebase.🤖 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 143 - 153, The sampler (stereoBlendLinearSampler) is being lazily created in StereoBlend.cpp during runtime which is expensive; move its creation into VR::SetupResources() so it is created once during initialization. In SetupResources(), allocate and initialize a D3D11_SAMPLER_DESC with D3D11_FILTER_MIN_MAG_MIP_LINEAR and CLAMP address modes and call device->CreateSamplerState to assign stereoBlendLinearSampler, removing the CreateSamplerState call from the runtime path in the function that currently calls context->CSSetSamplers(0, 1, samplers); ensure the runtime code only sets the sampler via CSSetSamplers using the already-created stereoBlendLinearSampler and keep destruction/cleanup consistent with other resources.features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl (1)
42-50: Potential precision loss in coordinate scaling.Integer division for coordinate scaling (
(dispatchID.x * DepthWidth) / ColorWidth) may cause off-by-one errors at boundaries due to truncation. Consider whether sub-pixel precision matters for mask lookup.🔧 Alternative using floating-point scaling
if (DepthWidth > 0) { - depthPos = uint2( - (dispatchID.x * DepthWidth) / ColorWidth, - (dispatchID.y * DepthHeight) / ColorHeight) + - uint2(DepthOffsetX, DepthOffsetY); + // Use float for sub-pixel precision, round to nearest + float scaleX = (float)DepthWidth / (float)ColorWidth; + float scaleY = (float)DepthHeight / (float)ColorHeight; + depthPos = uint2( + (uint)(dispatchID.x * scaleX + 0.5f), + (uint)(dispatchID.y * scaleY + 0.5f)) + + uint2(DepthOffsetX, DepthOffsetY); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl` around lines 42 - 50, The integer division in the depth coordinate computation (used in the block that sets depthPos from dispatchID, DepthWidth, ColorWidth, DepthHeight) can truncate and produce off-by-one errors; change the scaling to use floating-point math and then convert to integer with an explicit rounding strategy (e.g., compute float scaledX = (dispatchID.x + 0.5f) * DepthWidth / ColorWidth and similarly for Y, then cast/round to uint and add uint2(DepthOffsetX, DepthOffsetY)), and also clamp the resulting depthPos to valid depth bounds to avoid out-of-range lookups; adjust the code paths that reference depthPos (same block using dispatchID, DepthWidth, ColorWidth, DepthHeight, DepthOffsetX/Y) accordingly.src/Features/VR/SettingsUI.cpp (1)
1001-1001: Consider keeping UI scaling for modal windows.The "Record Combo" modal initial size is now fixed at 400×200 pixels. This may be too small on high-resolution displays.
🤖 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 1001, The fixed 400×200 size should be scaled for high-DPI displays: replace the hardcoded ImVec2 used in ImGui::SetNextWindowSize with a size multiplied by the UI scale from ImGui::GetIO() (e.g., multiply ImVec2(400,200) by ImGui::GetIO().DisplayFramebufferScale or an appropriate FontGlobalScale) so the "Record Combo" modal scales on high-resolution screens; update the SetNextWindowSize call that currently uses ImVec2(400, 200) accordingly.src/Features/Upscaling/Streamline.cpp (1)
242-253: Consider consolidating repeated vpScale/nasalFrac clamping.The
vrDlssViewportScaleandvrDlssCropOffsetXvalues are clamped identically at multiple call sites (lines 244-245, 269, 370-371, 600-601). While functionally correct, this creates maintenance overhead and risks inconsistent bounds if one site is updated but others are not.Consider computing these once at the start of each public method or creating inline helpers.
♻️ Suggested helper pattern
// Add to Streamline class or as local helper inline std::pair<float, float> GetSanitizedVRScaleParams() { return { std::clamp(globals::features::upscaling.settings.vrDlssViewportScale, 0.5f, 1.0f), std::clamp(globals::features::upscaling.settings.vrDlssCropOffsetX, 0.0f, 0.3f) }; }Also applies to: 269-283, 370-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling/Streamline.cpp` around lines 242 - 253, Multiple sites in Streamline.cpp repeat clamping of globals::features::upscaling.settings.vrDlssViewportScale and vrDlssCropOffsetX (e.g., the block that sets slConstants.cameraPinholeOffset using eyeIndex), which risks divergence; extract a single helper (inline method on Streamline or local static helper like GetSanitizedVRScaleParams/GetSanitizedVRCropParams) that returns the clamped viewportScale and nasalFrac and replace all repeated std::clamp calls with calls to that helper so every use (including the cameraPinholeOffset calculation and other occurrences) uses the same validated values.src/Features/Upscaling.h (1)
145-150: Consider a parameter struct for ClearHMDMask.The
ClearHMDMaskfunction now has 11 parameters, many with default values. This makes call sites harder to read and increases the risk of argument order mistakes.♻️ Suggested refactor using a parameter struct
struct ClearHMDMaskParams { ID3D11UnorderedAccessView* colorUAV; ID3D11ShaderResourceView* depthSRV; uint32_t eyeWidth; uint32_t eyeHeight; uint32_t depthOffsetX; uint32_t colorOffsetX; uint32_t depthOffsetY = 0; uint32_t depthWidth = 0; uint32_t depthHeight = 0; uint32_t colorWidth = 0; uint32_t colorHeight = 0; ID3D11ShaderResourceView* fallbackSRV = nullptr; uint32_t fallbackOffsetX = 0; }; void ClearHMDMask(const ClearHMDMaskParams& params);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.h` around lines 145 - 150, The ClearHMDMask declaration has 11 parameters (many defaults) which is error-prone and hard to read; replace the parameter list with a single const ClearHMDMaskParams& parameter by introducing a struct (e.g., ClearHMDMaskParams) containing the current parameters (colorUAV, depthSRV, eyeWidth, eyeHeight, depthOffsetX, colorOffsetX, depthOffsetY, depthWidth, depthHeight, colorWidth, colorHeight, fallbackSRV, fallbackOffsetX) with the same default values, update the ClearHMDMask signature to take that struct by const reference, and update all call sites to construct/populate ClearHMDMaskParams to preserve behavior and readability.src/Features/Upscaling.cpp (1)
2466-2473: Verify TAAReorder state reset is complete.The per-frame flags are reset unconditionally before the periphery TAA branch, which is correct. However, if
TAAReorder::g_initializedis false, the diagnostic counter increment (lines 2458-2464) still runs, which is harmless but could be gated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 2466 - 2473, The diagnostic counter increment that runs regardless of initialization should be gated by the TAAReorder initialization check: ensure the diagnostic increment block (the code that increments the diagnostic counters near the call to TAAReorder::ShouldReorderTAA()) only executes when TAAReorder::g_initialized is true; leave the unconditional per-frame resets (TAAReorder::g_postPPReady, g_dlssReady, g_dlssPasteComplete, g_phase5Complete, g_bsHookCallCount) as-is, but wrap the diagnostic-counter increment in an if (TAAReorder::g_initialized) { ... } so it won’t run when g_initialized 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/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlsl`:
- Around line 24-28: The compute shader's right/bottom edge distances
(distRight, distBottom) use "- 1" and thus are off by one compared to the pixel
shader variant; update the calculations in FeatheredCompositeCS.hlsl so
distRight = (float)(CropW - cropLocal.x) and distBottom = (float)(CropH -
cropLocal.y) (keeping distLeft/distTop and distFromEdge as-is) to match the
pixel shader logic (which uses (CropOrigin + CropSize) - pixelPos).
In `@features/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlsl`:
- Around line 16-24: The FeatheredCompositeCB constant buffer size is not padded
to a 16-byte multiple; update the FeatheredCompositeCB definition so its total
size rounds up to 48 bytes (for safe 16-byte alignment) by adding appropriate
padding after SrcUVScale (e.g., an extra float2 or two floats) so that the
buffer members (CropOrigin, CropSize, FeatherWidth, _pad0, SrcUVOrigin,
SrcUVScale) remain in the same order but the struct total is padded to 48 bytes.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: The early return when eyeIndex == 1 and
StereoOptModeTexture[...] indicates MODE_MAIN/MODE_EDGE skips writing to
NormalTAAMaskSpecularMaskRW, leaving u1 stale; modify DeferredCompositeCS.hlsl
so that before returning in that conditional you still update the
NormalTAAMaskSpecularMaskRW's u1 component for those reprojected pixels (or
alternatively update StereoBlendCS.hlsl to copy u1 from the Eye0 source
alongside color and motion during the overwrite), ensuring
NormalTAAMaskSpecularMaskRW, u1, StereoOptModeTexture, eyeIndex, and the
MODE_MAIN/MODE_EDGE path are handled.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 219-236: The POM branch currently overwrites the first-pass
reprojection result r and then bails if the POM-adjusted hit is sky/clear,
removing the valid fallback; instead, compute the POM-reprojection into a
temporary (e.g., rPOM) using the same math (pomOffset, linearDepth,
depthCorrection, newLinearDepth, Stereo::ReprojectToOtherEye), then only assign
r = rPOM if rPOM.valid AND the sampled sourceDepth from
DepthTexture[rPOM.otherPx] is a real geometry depth (not >= 1.0 or < 1e-5);
otherwise keep the original r (and only return if the original r was invalid).
Reference symbols: pomOffset, ReflectanceTexture, SharedData::GetScreenDepth,
POMDepthScale, Stereo::ReprojectToOtherEye, r.otherPx, DepthTexture.
In `@src/Features/TerrainHelper.cpp`:
- Around line 186-188: The early return when settings.enableParallax is false
skips clearing the THLandHasDisplacement descriptor bits, so toggle-off leaves
bits 0-5 and bit 9 set; update the logic in TerrainHelper.cpp (around the
function handling descriptor flags where settings.enableParallax is checked) to
explicitly clear the THLandHasDisplacement bits when enableParallax is false
before returning (i.e., compute/modify the descriptor/mask that holds
THLandHasDisplacement and clear bits 0-5 and bit 9 or reset the
THLandHasDisplacement flag) so shader permutation selection reflects the current
setting even when parallax was previously enabled.
---
Duplicate comments:
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 100-107: The code pins maxWidth to kInnerWidth, so the runtime
EdgeWidth setting is ignored; update the assignment of maxWidth in
StencilCS.hlsl to use the configured EdgeWidth (e.g., maxWidth =
max(kInnerWidth, EdgeWidth) or directly maxWidth = EdgeWidth if intended) so the
search radius actually reflects the user-configured edge width, or if you prefer
remove the exposed EdgeWidth knob and any references to it; refer to
kInnerWidth, EdgeWidth, and maxWidth in the StencilCS shader to make the change.
In `@src/Deferred.cpp`:
- Around line 373-379: The VR_STEREO_OPT compile-time define is mismatched with
the runtime binding: ensure the shader define is only added when
vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off (the
same condition used when binding the SRV), or alternatively guarantee a fallback
SRV is always bound to slot 16 when the shader was compiled with VR_STEREO_OPT;
locate the shader-compile site where VR_STEREO_OPT is added (lines referencing
VR_STEREO_OPT and vrStereoOpt.loaded) and either gate the define with
vrStereoOpt.settings.stereoMode != Off, add shader cache invalidation when
stereoMode changes in VRStereoOptimizations, or make
GetModeTextureSRV()/Deferred.cpp bind a 1x1 neutral texture when stereoMode ==
Off so the shader always sees a valid SRV.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: The SaveSettings implementation for VRStereoOptimizations
omits persisting the debug flags debugFullBlendDepth and debugPOMDepth so they
reset on startup; update VRStereoOptimizations::SaveSettings to add
o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth and
o_json["DebugPOMDepth"] = settings.debugPOMDepth (matching the naming convention
used like "POMDepthScale"), and also update the corresponding LoadSettings logic
to read those keys into settings.debugFullBlendDepth and settings.debugPOMDepth
(or add a brief comment in both SaveSettings/LoadSettings if the debug controls
are intentionally non-persistent).
---
Nitpick comments:
In `@features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl`:
- Around line 42-50: The integer division in the depth coordinate computation
(used in the block that sets depthPos from dispatchID, DepthWidth, ColorWidth,
DepthHeight) can truncate and produce off-by-one errors; change the scaling to
use floating-point math and then convert to integer with an explicit rounding
strategy (e.g., compute float scaledX = (dispatchID.x + 0.5f) * DepthWidth /
ColorWidth and similarly for Y, then cast/round to uint and add
uint2(DepthOffsetX, DepthOffsetY)), and also clamp the resulting depthPos to
valid depth bounds to avoid out-of-range lookups; adjust the code paths that
reference depthPos (same block using dispatchID, DepthWidth, ColorWidth,
DepthHeight, DepthOffsetX/Y) accordingly.
In `@src/Features/Upscaling.cpp`:
- Around line 2466-2473: The diagnostic counter increment that runs regardless
of initialization should be gated by the TAAReorder initialization check: ensure
the diagnostic increment block (the code that increments the diagnostic counters
near the call to TAAReorder::ShouldReorderTAA()) only executes when
TAAReorder::g_initialized is true; leave the unconditional per-frame resets
(TAAReorder::g_postPPReady, g_dlssReady, g_dlssPasteComplete, g_phase5Complete,
g_bsHookCallCount) as-is, but wrap the diagnostic-counter increment in an if
(TAAReorder::g_initialized) { ... } so it won’t run when g_initialized is false.
In `@src/Features/Upscaling.h`:
- Around line 145-150: The ClearHMDMask declaration has 11 parameters (many
defaults) which is error-prone and hard to read; replace the parameter list with
a single const ClearHMDMaskParams& parameter by introducing a struct (e.g.,
ClearHMDMaskParams) containing the current parameters (colorUAV, depthSRV,
eyeWidth, eyeHeight, depthOffsetX, colorOffsetX, depthOffsetY, depthWidth,
depthHeight, colorWidth, colorHeight, fallbackSRV, fallbackOffsetX) with the
same default values, update the ClearHMDMask signature to take that struct by
const reference, and update all call sites to construct/populate
ClearHMDMaskParams to preserve behavior and readability.
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 242-253: Multiple sites in Streamline.cpp repeat clamping of
globals::features::upscaling.settings.vrDlssViewportScale and vrDlssCropOffsetX
(e.g., the block that sets slConstants.cameraPinholeOffset using eyeIndex),
which risks divergence; extract a single helper (inline method on Streamline or
local static helper like GetSanitizedVRScaleParams/GetSanitizedVRCropParams)
that returns the clamped viewportScale and nasalFrac and replace all repeated
std::clamp calls with calls to that helper so every use (including the
cameraPinholeOffset calculation and other occurrences) uses the same validated
values.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 1001: The fixed 400×200 size should be scaled for high-DPI displays:
replace the hardcoded ImVec2 used in ImGui::SetNextWindowSize with a size
multiplied by the UI scale from ImGui::GetIO() (e.g., multiply ImVec2(400,200)
by ImGui::GetIO().DisplayFramebufferScale or an appropriate FontGlobalScale) so
the "Record Combo" modal scales on high-resolution screens; update the
SetNextWindowSize call that currently uses ImVec2(400, 200) accordingly.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 143-153: The sampler (stereoBlendLinearSampler) is being lazily
created in StereoBlend.cpp during runtime which is expensive; move its creation
into VR::SetupResources() so it is created once during initialization. In
SetupResources(), allocate and initialize a D3D11_SAMPLER_DESC with
D3D11_FILTER_MIN_MAG_MIP_LINEAR and CLAMP address modes and call
device->CreateSamplerState to assign stereoBlendLinearSampler, removing the
CreateSamplerState call from the runtime path in the function that currently
calls context->CSSetSamplers(0, 1, samplers); ensure the runtime code only sets
the sampler via CSSetSamplers using the already-created stereoBlendLinearSampler
and keep destruction/cleanup consistent with other resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 256caeb0-5d85-4356-9024-aa450d6ca8c9
📒 Files selected for processing (47)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlslifeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlslfeatures/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlslfeatures/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/TAAReorder.cppsrc/Features/TAAReorder.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.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)
- src/State.cpp
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- package/Shaders/Common/VR.hlsli
- package/Shaders/DistantTree.hlsl
- src/Features/ExtendedMaterials.h
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- features/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlsl
- package/Shaders/RunGrass.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- src/Globals.h
- src/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- src/FeatureBuffer.cpp
- src/Features/TerrainHelper.h
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- package/Shaders/Common/SharedData.hlsli
- .claude/CLAUDE.md
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- src/Features/TerrainBlending.cpp
- features/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlsl
- features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl
- package/Shaders/Lighting.hlsl
- src/Features/TAAReorder.cpp
Comment on lines
+24
to
+28
| float distLeft = (float)cropLocal.x; | ||
| float distRight = (float)(CropW - 1 - cropLocal.x); | ||
| float distTop = (float)cropLocal.y; | ||
| float distBottom = (float)(CropH - 1 - cropLocal.y); | ||
| float distFromEdge = min(min(distLeft, distRight), min(distTop, distBottom)); |
Contributor
There was a problem hiding this comment.
Edge distance calculation differs from pixel shader variant.
This compute shader uses CropW - 1 - cropLocal.x for the right edge distance, while FeatheredCompositePS.hlsl uses (CropOrigin.x + CropSize.x) - pixelPos.x. The CS version produces distances offset by 1 pixel at the right/bottom edges, which may cause a subtle visual discontinuity if both shaders are used in the same pipeline.
🔧 Suggested fix for consistency
// Distance from nearest crop edge (positive = inside)
float distLeft = (float)cropLocal.x;
- float distRight = (float)(CropW - 1 - cropLocal.x);
+ float distRight = (float)(CropW - cropLocal.x);
float distTop = (float)cropLocal.y;
- float distBottom = (float)(CropH - 1 - cropLocal.y);
+ float distBottom = (float)(CropH - cropLocal.y);📝 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
| float distLeft = (float)cropLocal.x; | |
| float distRight = (float)(CropW - 1 - cropLocal.x); | |
| float distTop = (float)cropLocal.y; | |
| float distBottom = (float)(CropH - 1 - cropLocal.y); | |
| float distFromEdge = min(min(distLeft, distRight), min(distTop, distBottom)); | |
| float distLeft = (float)cropLocal.x; | |
| float distRight = (float)(CropW - cropLocal.x); | |
| float distTop = (float)cropLocal.y; | |
| float distBottom = (float)(CropH - cropLocal.y); | |
| float distFromEdge = min(min(distLeft, distRight), min(distTop, distBottom)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlsl` around lines
24 - 28, The compute shader's right/bottom edge distances (distRight,
distBottom) use "- 1" and thus are off by one compared to the pixel shader
variant; update the calculations in FeatheredCompositeCS.hlsl so distRight =
(float)(CropW - cropLocal.x) and distBottom = (float)(CropH - cropLocal.y)
(keeping distLeft/distTop and distFromEdge as-is) to match the pixel shader
logic (which uses (CropOrigin + CropSize) - pixelPos).
Contributor
Author
There was a problem hiding this comment.
leaving open. Feathered Edge is still being worked on.
Contributor
There was a problem hiding this comment.
@vrnord, understood — acknowledged. The off-by-one consistency check between FeatheredCompositeCS.hlsl and FeatheredCompositePS.hlsl can be revisited once the feathered edge implementation stabilises. Good luck with the ongoing work on it!
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:29.538Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: package/Shaders/VRStereoOptimizations/StencilCS.hlsl:97-108
Timestamp: 2026-03-24T04:37:14.725Z
Learning: In `package/Shaders/VRStereoOptimizations/StencilCS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), `EdgeWidth` is present in `VRStereoOptParams` cbuffer (`cbuffers.hlsli`) as a future-tuning placeholder but is intentionally NOT wired into `maxWidth` in the classification loop — `maxWidth` is hardcoded to `kInnerWidth = 2`. Do not flag this as a bug or suggest `maxWidth = max(kInnerWidth, EdgeWidth)`; the wiring is deferred to a later PR.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:31.383Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to features/**/Shaders/**/*.{hlsl,fxh} : Add shader files for graphics features to the `features/YourFeature/Shaders/` directory and use shader compilation templates for deployment
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:35:02.326Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:47.322Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:16.698Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Comment on lines
+100
to
+107
| #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 | ||
| return; | ||
| } | ||
| } | ||
| #endif |
Contributor
There was a problem hiding this comment.
Don't skip NormalTAAMaskSpecularMaskRW for reprojected Eye 1 pixels.
Lines 100-107 return before the writes on Lines 306-307, so Eye 1 MODE_MAIN/MODE_EDGE pixels never get refreshed in NormalTAAMaskSpecularMaskRW. In the provided overwrite path, StereoBlendCS.hlsl only backfills color and motion, not u1, so downstream temporal passes will read stale TAA-mask/normal data for the reprojected region. Either keep writing u1 before this return or have the overwrite pass copy u1 from the Eye 0 source alongside color/motion.
🤖 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, The early
return when eyeIndex == 1 and StereoOptModeTexture[...] indicates
MODE_MAIN/MODE_EDGE skips writing to NormalTAAMaskSpecularMaskRW, leaving u1
stale; modify DeferredCompositeCS.hlsl so that before returning in that
conditional you still update the NormalTAAMaskSpecularMaskRW's u1 component for
those reprojected pixels (or alternatively update StereoBlendCS.hlsl to copy u1
from the Eye0 source alongside color and motion during the overwrite), ensuring
NormalTAAMaskSpecularMaskRW, u1, StereoOptModeTexture, eyeIndex, and the
MODE_MAIN/MODE_EDGE path are handled.
Contributor
Author
There was a problem hiding this comment.
being addressed in 2002.
Contributor
There was a problem hiding this comment.
@vrnord, understood — thanks for the context. I'll note this for future reviews so it doesn't get re-flagged on this PR.
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:29.538Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:31.383Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: package/Shaders/VRStereoOptimizations/StencilCS.hlsl:97-108
Timestamp: 2026-03-24T04:37:14.725Z
Learning: In `package/Shaders/VRStereoOptimizations/StencilCS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), `EdgeWidth` is present in `VRStereoOptParams` cbuffer (`cbuffers.hlsli`) as a future-tuning placeholder but is intentionally NOT wired into `maxWidth` in the classification loop — `maxWidth` is hardcoded to `kInnerWidth = 2`. Do not flag this as a bug or suggest `maxWidth = max(kInnerWidth, EdgeWidth)`; the wiring is deferred to a later PR.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:37.608Z
Learning: In package/Shaders/DeferredCompositeCS.hlsl (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with skylightingSpecular: dalcScaled is computed as IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular), and finalIrradiance is lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular). This is original engine logic preserved from before PR `#1947`. Do not flag this as a double-attenuation bug during code reviews. If reviewing similar shader paths, apply this exact calculation pattern and treat skylightingSpecular as the controlling factor for both the pre- and post-irradiance blend.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:35:02.326Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:47.322Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:16.698Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
60a2e29 to
dc355ea
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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 | 🟡 MinorDocument the new overwrite debug modes.
The combo now exposes
OverwriteandOverwrite Eye1, but the tooltip still stops atEdge Detection. Add a short description here so users know what those two modes do.🤖 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, The tooltip text for the debug mode combo (see debugModes array and ImGui::Combo using settings.StereoBlendDebugMode) is missing descriptions for the newly added "Overwrite" and "Overwrite Eye1" modes; update the ImGui::Text string inside the Util::HoverTooltipWrapper block to add concise lines explaining what "Overwrite" does (force destination pixels to be taken from the source/other eye) and what "Overwrite Eye1" does (force overwrite using eye1/source specifically), keeping the same style/formatting as the other mode descriptions so users see these modes in the tooltip when hovering the "Debug View" combo.
♻️ Duplicate comments (7)
src/Features/VR/SettingsUI.cpp (1)
76-76:⚠️ Potential issue | 🟡 MinorAlign the welcome-overlay gate with the copy.
shouldShownow only acceptsRE::MainMenu::MENU_NAME, but this file still tells users the VR controls work from the tween menu too. Either widen the gate again or trim the strings so onboarding matches behavior.🤖 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 welcome-overlay gate (variable shouldShow in SettingsUI.cpp) currently only checks IsMenuOpen(RE::MainMenu::MENU_NAME) but the onboarding copy claims VR controls also work from the tween menu; either broaden the gate to include the tween menu or update the copy. To fix: modify the shouldShow expression (referencing settings.kAutoHideSeconds, globals::game::ui, IsMenuOpen, RE::MainMenu::MENU_NAME, globals::menu, IsEnabled) to also accept the tween menu (e.g., || globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME)) or alternatively update the onboarding strings to remove the tween-menu claim so behavior matches text.package/Shaders/DeferredCompositeCS.hlsl (1)
100-105:⚠️ Potential issue | 🟠 MajorDon't skip
MODE_EDGEin Deferred Composite.
ExecuteStencilWritePass()only marksMODE_MAIN, andReprojectionCS.hlslonly refillsMODE_MAIN, so returning here formode == 1drops the Eye 1 composite writes for edge pixels with no later producer. This branch also skips theNormalTAAMaskSpecularMaskRWrefresh for the skipped region, so theMODE_MAINpath still needs a writer foru1.💡 Minimum fix
`#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 == 2) { // MODE_MAIN — later reprojection path refills this class 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 - 105, The current early return in DeferredCompositeCS.hlsl inside the VR_STEREO_OPT block skips processing when mode == 1 (MODE_EDGE), which drops eye-1 composite writes and skips refreshing NormalTAAMaskSpecularMaskRW; change the condition so only the intended mode is skipped (keep the return for the MODE_MAIN value only, e.g., check for mode == 2) and remove the branch that returns for mode == 1 so the shader continues to run for EDGE pixels; refer to eyeIndex, StereoOptModeTexture, ExecuteStencilWritePass(), ReprojectionCS.hlsl, and NormalTAAMaskSpecularMaskRW to verify EDGE now gets written and refreshed.package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
97-143:⚠️ Potential issue | 🟡 MinorWire
EdgeWidthinto the actual classifier.
EdgeWidthis still uploaded viaVRStereoOptParams, but this shader hardcodesmaxWidth = kInnerWidthand never emitsMODE_EDGE_NEIGHBOURin the real path. That makes the runtime setting ineffective.💡 Minimum fix
// Use the larger of inner/outer widths for the search - uint maxWidth = kInnerWidth; + 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 for post-process blending + ModeTextureRW[dtid] = MODE_EDGE_NEIGHBOUR; + return; } }src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorPersist the new full-blend/POM debug toggles.
debugFullBlendDepthanddebugPOMDepthare exposed inDrawSettings(), but they are still missing from bothSaveSettings()andLoadSettings(), so they reset every launch.💾 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; } void VRStereoOptimizations::LoadSettings(json& o_json) { if (o_json.contains("StereoMode")) settings.stereoMode = o_json["StereoMode"].get<StereoMode>(); @@ 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, 253-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 and LoadSettings are missing persistence for the two new toggles debugFullBlendDepth and debugPOMDepth; update VRStereoOptimizations::SaveSettings to write o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth and o_json["DebugPOMDepth"] = settings.debugPOMDepth, and update VRStereoOptimizations::LoadSettings to read those keys back into settings.debugFullBlendDepth and settings.debugPOMDepth (use existing patterns for similar fields like DebugDepthMap/DebugSkipMerge to handle missing keys and defaults).src/Features/TAAReorder.cpp (2)
47-74:⚠️ Potential issue | 🟠 MajorFail the capture path when
g_postPPCopyor its SRV cannot be created.
EnsurePostPPCopy()still ignores both HRESULTs, andExecutePassHook()immediatelyCopyResources intog_postPPCopyafterward. On allocation/SRV failure this becomes either a null-destination copy or a later feathered composite with a null source SRV instead of a clean fallback.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".🛡️ Suggested fix
void EnsurePostPPCopy(ID3D11Texture2D* sourceTex) { D3D11_TEXTURE2D_DESC srcDesc; sourceTex->GetDesc(&srcDesc); @@ D3D11_TEXTURE2D_DESC desc = srcDesc; desc.BindFlags = D3D11_BIND_SHADER_RESOURCE; desc.MiscFlags = 0; g_postPPCopy = nullptr; g_postPPCopySRV = nullptr; - globals::d3d::device->CreateTexture2D(&desc, nullptr, g_postPPCopy.put()); + HRESULT hr = globals::d3d::device->CreateTexture2D(&desc, nullptr, g_postPPCopy.put()); + if (FAILED(hr)) + return; if (g_postPPCopy) { D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc = {}; srvDesc.Format = desc.Format; srvDesc.ViewDimension = D3D11_SRV_DIMENSION_TEXTURE2D; srvDesc.Texture2D.MipLevels = 1; - globals::d3d::device->CreateShaderResourceView(g_postPPCopy.get(), &srvDesc, g_postPPCopySRV.put()); + hr = globals::d3d::device->CreateShaderResourceView(g_postPPCopy.get(), &srvDesc, g_postPPCopySRV.put()); + if (FAILED(hr)) { + g_postPPCopy = nullptr; + g_postPPCopySRV = nullptr; + return; + } Util::SetResourceName(g_postPPCopy.get(), "TAAReorder_PostPPCopy"); } } @@ - EnsurePostPPCopy(postTex); - globals::d3d::context->CopyResource(g_postPPCopy.get(), postTex); - g_postPPReady = true; + EnsurePostPPCopy(postTex); + if (g_postPPCopy && g_postPPCopySRV) { + globals::d3d::context->CopyResource(g_postPPCopy.get(), postTex); + g_postPPReady = true; + }Also applies to: 145-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/TAAReorder.cpp` around lines 47 - 74, EnsurePostPPCopy currently ignores the HRESULTs from CreateTexture2D and CreateShaderResourceView and leaves g_postPPCopy/g_postPPCopySRV null, allowing ExecutePassHook to attempt CopyResource or use a null SRV; change EnsurePostPPCopy to capture and check the HRESULTs returned by globals::d3d::device->CreateTexture2D and CreateShaderResourceView, on failure release/clear g_postPPCopy and g_postPPCopySRV, log the error, and return a failure indicator (or set a boolean/state) so callers like ExecutePassHook can skip copying and use a safe fallback path; update ExecutePassHook to test g_postPPCopy and g_postPPCopySRV (or the failure indicator) before CopyResource/using the SRV and perform a graceful fallback when allocation/SRV creation failed.
262-318:⚠️ Potential issue | 🟠 MajorThe feathered path still leaks OM/IA state.
This branch now restores blend/VS/PS/RS/input-layout/topology, but it still drops the currently bound DSV with
OMSetRenderTargets(..., nullptr)and clears IA vertex buffers without saving/restoring either. That mutated state survives back into the rest ofBSImagespaceShader::Render().Also applies to: 393-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/TAAReorder.cpp` around lines 262 - 318, The feathered-path branch fails to save/restore the current depth-stencil view and IA vertex buffer bindings, so calling OMSetRenderTargets(..., nullptr) and IASetVertexBuffers(...) mutates global pipeline state; fix by calling OMGetRenderTargets to capture the current RTV/DSV (e.g., store an ID3D11DepthStencilView* oldDSV) before OMSetRenderTargets and restore it with OMSetRenderTargets when done, and call IAGetVertexBuffers to capture the bound vertex buffer(s)/strides/offsets (store arrays like oldVBs/oldStrides/oldOffsets and a UINT oldNumVBs) before IASetVertexBuffers(nullptr) and restore them with IASetVertexBuffers afterwards; apply the same save/restore for the other occurrence noted around the 393-403 region.src/Deferred.cpp (1)
376-382:⚠️ Potential issue | 🟠 MajorKeep
VR_STEREO_OPTgated by the same predicate as slott16.These paths still diverge: the shader variant is cached when the feature is merely
loaded, but the resource binding only happens whenstereoMode != Off. That leavesmainCompositeCS/mainCompositeInteriorCScompiled forVR_STEREO_OPTwhile slot 16 goes unbound whenever the mode is off until the cache is rebuilt. Please drive both the define and the binding from the sameloaded && stereoMode != Offcheck, and invalidate the cached shaders if that setting can change at runtime.Also applies to: 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 - 382, The VR_STEREO_OPT define and the SRV binding are gated inconsistently: the shader variant is enabled when globals::features::vrStereoOptimizations.loaded is true, but the slot t16 binding (via vrStereoOpt.GetModeTextureSRV() and context->CSSetShaderResources(16,...)) only occurs when vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off; synchronize these checks so both the define (where VR_STEREO_OPT is set for mainCompositeCS / mainCompositeInteriorCS) and the binding are driven by the same predicate (globals::features::vrStereoOptimizations.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off), and add logic to invalidate or rebuild the cached mainComposite shader variants when that setting changes at runtime so cached shaders without VR_STEREO_OPT aren’t reused incorrectly.
🧹 Nitpick comments (1)
src/Features/VR.h (1)
366-378: Add a layout guard forStereoBlendCB.This struct now mirrors
package/Shaders/VR/StereoBlendCS.hlsl, but there’s nostatic_asserthere to catch packing drift. A later field/type edit will compile cleanly and only fail on-GPU.♻️ Suggested guard
struct alignas(16) StereoBlendCB { float FrameDim[2]; float RcpFrameDim[2]; float DepthSigma; float MaxBlendFactor; float ColorDiffThreshold; float DebugEdgeTint; uint32_t DebugMode; float FullBlendDistance; float POMDepthScale; float _pad; }; + static_assert(sizeof(StereoBlendCB) == 48, "StereoBlendCB must match package/Shaders/VR/StereoBlendCS.hlsl.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VR.h` around lines 366 - 378, StereoBlendCB lacks a compile-time layout guard; add a static_assert verifying its size matches the HLSL struct to catch packing drift (e.g. static_assert(sizeof(StereoBlendCB) == 48, "StereoBlendCB size mismatch with package/Shaders/VR/StereoBlendCS.hlsl")). Place the static_assert near the StereoBlendCB definition (respecting the existing alignas(16)) so future field/type edits will fail at compile time if sizes diverge.
🤖 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 fallback assignments of pixelOffset currently set to 0.5 on
non-parallax/non‑POM paths violate the POM sentinel contract and force
unnecessary stereo depth correction; update all non-POM/fallback branches that
assign pixelOffset (the pixelOffset variable in ExtendedMaterials.hlsli where
used for parallax/POM fallback) to use 0.0 instead of 0.5 so that downstream
packing into Reflectance.w and the StereoBlendCS.hlsl pomOffsetFB > 1e-2 test
correctly treats these pixels as non-POM; ensure any comments or related
fallback logic reflect this change and retain exact packing behavior expected by
Lighting.hlsl and StereoBlendCS.hlsl.
In `@src/Features/Upscaling.cpp`:
- Around line 69-75: Replace early uses of globals::game::isVR in the
startup/device-init branches with REL::Module::IsVR() so VR is detected at
runtime before globals are initialized; specifically update the swapchain branch
(pSwapChainDesc->SwapEffect = DXGI_SWAP_EFFECT_FLIP_DISCARD) and the shouldProxy
logic (and other occurrences around the startup/device-init and the
TAAReorder::InitEarly()/Streamline wrapping sites) to call REL::Module::IsVR()
instead of globals::game::isVR, ensuring VR-only behavior (no FLIP_DISCARD, no
proxy/wrapping, and TAAReorder::InitEarly() execution) is gated by
REL::Module::IsVR() so detection works across SE/AE/VR variants.
---
Outside diff comments:
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 326-345: The tooltip text for the debug mode combo (see debugModes
array and ImGui::Combo using settings.StereoBlendDebugMode) is missing
descriptions for the newly added "Overwrite" and "Overwrite Eye1" modes; update
the ImGui::Text string inside the Util::HoverTooltipWrapper block to add concise
lines explaining what "Overwrite" does (force destination pixels to be taken
from the source/other eye) and what "Overwrite Eye1" does (force overwrite using
eye1/source specifically), keeping the same style/formatting as the other mode
descriptions so users see these modes in the tooltip when hovering the "Debug
View" combo.
---
Duplicate comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-105: The current early return in DeferredCompositeCS.hlsl
inside the VR_STEREO_OPT block skips processing when mode == 1 (MODE_EDGE),
which drops eye-1 composite writes and skips refreshing
NormalTAAMaskSpecularMaskRW; change the condition so only the intended mode is
skipped (keep the return for the MODE_MAIN value only, e.g., check for mode ==
2) and remove the branch that returns for mode == 1 so the shader continues to
run for EDGE pixels; refer to eyeIndex, StereoOptModeTexture,
ExecuteStencilWritePass(), ReprojectionCS.hlsl, and NormalTAAMaskSpecularMaskRW
to verify EDGE now gets written and refreshed.
In `@src/Deferred.cpp`:
- Around line 376-382: The VR_STEREO_OPT define and the SRV binding are gated
inconsistently: the shader variant is enabled when
globals::features::vrStereoOptimizations.loaded is true, but the slot t16
binding (via vrStereoOpt.GetModeTextureSRV() and
context->CSSetShaderResources(16,...)) only occurs when
vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off;
synchronize these checks so both the define (where VR_STEREO_OPT is set for
mainCompositeCS / mainCompositeInteriorCS) and the binding are driven by the
same predicate (globals::features::vrStereoOptimizations.loaded &&
vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off), and
add logic to invalidate or rebuild the cached mainComposite shader variants when
that setting changes at runtime so cached shaders without VR_STEREO_OPT aren’t
reused incorrectly.
In `@src/Features/TAAReorder.cpp`:
- Around line 47-74: EnsurePostPPCopy currently ignores the HRESULTs from
CreateTexture2D and CreateShaderResourceView and leaves
g_postPPCopy/g_postPPCopySRV null, allowing ExecutePassHook to attempt
CopyResource or use a null SRV; change EnsurePostPPCopy to capture and check the
HRESULTs returned by globals::d3d::device->CreateTexture2D and
CreateShaderResourceView, on failure release/clear g_postPPCopy and
g_postPPCopySRV, log the error, and return a failure indicator (or set a
boolean/state) so callers like ExecutePassHook can skip copying and use a safe
fallback path; update ExecutePassHook to test g_postPPCopy and g_postPPCopySRV
(or the failure indicator) before CopyResource/using the SRV and perform a
graceful fallback when allocation/SRV creation failed.
- Around line 262-318: The feathered-path branch fails to save/restore the
current depth-stencil view and IA vertex buffer bindings, so calling
OMSetRenderTargets(..., nullptr) and IASetVertexBuffers(...) mutates global
pipeline state; fix by calling OMGetRenderTargets to capture the current RTV/DSV
(e.g., store an ID3D11DepthStencilView* oldDSV) before OMSetRenderTargets and
restore it with OMSetRenderTargets when done, and call IAGetVertexBuffers to
capture the bound vertex buffer(s)/strides/offsets (store arrays like
oldVBs/oldStrides/oldOffsets and a UINT oldNumVBs) before
IASetVertexBuffers(nullptr) and restore them with IASetVertexBuffers afterwards;
apply the same save/restore for the other occurrence noted around the 393-403
region.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The welcome-overlay gate (variable shouldShow in SettingsUI.cpp)
currently only checks IsMenuOpen(RE::MainMenu::MENU_NAME) but the onboarding
copy claims VR controls also work from the tween menu; either broaden the gate
to include the tween menu or update the copy. To fix: modify the shouldShow
expression (referencing settings.kAutoHideSeconds, globals::game::ui,
IsMenuOpen, RE::MainMenu::MENU_NAME, globals::menu, IsEnabled) to also accept
the tween menu (e.g., ||
globals::game::ui->IsMenuOpen(RE::TweenMenu::MENU_NAME)) or alternatively update
the onboarding strings to remove the tween-menu claim so behavior matches text.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: SaveSettings and LoadSettings are missing persistence for
the two new toggles debugFullBlendDepth and debugPOMDepth; update
VRStereoOptimizations::SaveSettings to write o_json["DebugFullBlendDepth"] =
settings.debugFullBlendDepth and o_json["DebugPOMDepth"] =
settings.debugPOMDepth, and update VRStereoOptimizations::LoadSettings to read
those keys back into settings.debugFullBlendDepth and settings.debugPOMDepth
(use existing patterns for similar fields like DebugDepthMap/DebugSkipMerge to
handle missing keys and defaults).
---
Nitpick comments:
In `@src/Features/VR.h`:
- Around line 366-378: StereoBlendCB lacks a compile-time layout guard; add a
static_assert verifying its size matches the HLSL struct to catch packing drift
(e.g. static_assert(sizeof(StereoBlendCB) == 48, "StereoBlendCB size mismatch
with package/Shaders/VR/StereoBlendCS.hlsl")). Place the static_assert near the
StereoBlendCB definition (respecting the existing alignas(16)) so future
field/type edits will fail at compile time if sizes diverge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16a92a66-0e41-46c0-bf04-5462a6828592
📒 Files selected for processing (41)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlslfeatures/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlslfeatures/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/TAAReorder.cppsrc/Features/TAAReorder.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.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
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- src/Features/ExtendedMaterials.h
- package/Shaders/DistantTree.hlsl
- src/Globals.h
- src/Feature.cpp
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- features/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlsl
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
🚧 Files skipped from review as they are similar to previous changes (13)
- package/Shaders/RunGrass.hlsl
- features/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlsl
- package/Shaders/Common/SharedData.hlsli
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl
- features/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlsl
- features/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
- package/Shaders/Lighting.hlsl
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Features/Upscaling.h
- src/Features/Upscaling/Streamline.cpp
- src/Features/TAAReorder.h
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
from
6658413 to
d53fa72
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
package/Shaders/VRStereoOptimizations/StencilCS.hlsl (1)
97-108:⚠️ Potential issue | 🟡 Minor
EdgeWidthis still a dead knob in this classifier.The search radius is still hard-wired to
kInnerWidth, so the runtime edge-width setting never expands classification beyond 2 pixels. Either feed that parameter into the search/classification path or drop it from the cbuffer to avoid exposing a setting that has no effect.🤖 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 - 108, The classifier currently hardcodes the search radius to kInnerWidth (2), ignoring the runtime EdgeWidth parameter; update the search logic so maxWidth uses the EdgeWidth value from the cbuffer (e.g., set maxWidth = max(kInnerWidth, EdgeWidth) or simply maxWidth = EdgeWidth if kInnerWidth is not required), ensuring the edge search loop that uses nearestEdgeDist/nearestWeAreOuter honors the runtime EdgeWidth; alternatively, remove the unused EdgeWidth from the cbuffer if you intend to keep the fixed kInnerWidth to avoid exposing a no-op setting.src/Features/VR/SettingsUI.cpp (1)
76-76:⚠️ Potential issue | 🟡 MinorKeep the welcome-overlay copy aligned with the new gate.
shouldShownow auto-shows only from the main menu, but the welcome overlay still tells users the tween menu is an entry point. Either restore the tween-menu branch or trim that onboarding copy so behavior and messaging match.🤖 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 onboarding text no longer matches the new auto-show logic: update the logic around shouldShow (which currently checks settings.kAutoHideSeconds, globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME), globals::menu and globals::menu->IsEnabled) and the welcome overlay copy to be consistent—either reintroduce the previous tween-menu branch into the shouldShow condition so the overlay can auto-show from the tween menu as before, or keep the current main-menu-only condition and trim the welcome-overlay text to remove references to the tween menu/entry point; locate usages of shouldShow and the welcome-overlay rendering to apply the corresponding code or copy change.package/Shaders/DeferredCompositeCS.hlsl (1)
100-106:⚠️ Potential issue | 🟠 MajorDon't return before Eye 1
u1has been refreshed.This branch exits before the only visible
NormalTAAMaskSpecularMaskRWwrite in this shader. If the overwrite path still only backfills color and motion, reprojected Eye 1MODE_MAIN/MODE_EDGEpixels keep stale normal/TAA-mask data.Expected result: the overwrite path in
package/Shaders/VR/StereoBlendCS.hlslshould also write theu1payload for these pixels. If it only writes color/motion, this early return still leavesu1stale.#!/bin/bash set -euo pipefail echo "=== DeferredComposite early-return branch ===" sed -n '98,108p' package/Shaders/DeferredCompositeCS.hlsl echo echo "=== StereoBlendCS resource declarations / writes ===" rg -n -C3 'RWTexture2D|NormalTAAMaskSpecularMaskRW|MotionRW|MainRW|EncodeNormalVanilla|MODE_MAIN|MODE_EDGE' package/Shaders/VR/StereoBlendCS.hlsl || true echo echo "=== Check specifically for any u1 / normal-mask write in StereoBlendCS ===" rg -n -C2 'NormalTAAMaskSpecularMaskRW|EncodeNormalVanilla|RWTexture2D<float4>' package/Shaders/VR/StereoBlendCS.hlsl || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 100 - 106, The early return in DeferredCompositeCS.hlsl (the eyeIndex == 1 branch that checks StereoOptModeTexture and returns for MODE_MAIN/MODE_EDGE) causes NormalTAAMaskSpecularMaskRW (u1) to remain stale; either move or delay the return until after the shader updates u1 for the pixel, or update the overwrite path in VR/StereoBlendCS.hlsl so that for MODE_MAIN and MODE_EDGE it also writes the NormalTAAMaskSpecularMaskRW/u1 payload (and any EncodeNormalVanilla logic) in addition to color/motion; locate the branch using eyeIndex and StereoOptModeTexture and ensure consistency with the StereoBlendCS.hlsl write paths for MODE_MAIN/MODE_EDGE so u1 is always refreshed.src/Features/TAAReorder.cpp (1)
145-157:⚠️ Potential issue | 🟠 MajorDon't set
g_postPPReadyunless the capture texture exists.
EnsurePostPPCopy()can still leaveg_postPPCopynull, but this block always callsCopyResource()and marks the frame ready. That reintroduces the null-destination copy / stale-ready-path failure the earlier review called out. 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/TAAReorder.cpp` around lines 145 - 157, The code sets g_postPPReady and calls CopyResource unconditionally after EnsurePostPPCopy(postTex), which can leave g_postPPCopy null; modify the block in TAAReorder.cpp so that after calling EnsurePostPPCopy(postTex) you check that g_postPPCopy (the ID3D11Texture2D/ID3D11Resource wrapped by EnsurePostPPCopy) is non-null before calling globals::d3d::context->CopyResource(...) and before setting g_postPPReady; if g_postPPCopy is null, skip the CopyResource and do not set g_postPPReady (optionally log or handle the degraded path), ensuring all uses of g_postPPCopy and g_postPPReady only occur when the resource is valid.src/Features/VRStereoOptimizations.cpp (1)
21-37:⚠️ Potential issue | 🟡 MinorPersist the new debug toggles too.
debugFullBlendDepthanddebugPOMDepthare exposed later inDrawSettings(), but neither key is saved nor loaded here. Those switches reset every launch, so the debug UI can't round-trip its own state.Suggested fix
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("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, 243-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, Save the two missing debug toggles so their UI state persists: in VRStereoOptimizations::SaveSettings add o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth and o_json["DebugPOMDepth"] = settings.debugPOMDepth; then in VRStereoOptimizations::LoadSettings (the counterpart that reads settings) read those keys into settings.debugFullBlendDepth and settings.debugPOMDepth (use existing key-reading pattern used for the other debug flags), ensuring the same JSON key names are used so the toggles displayed by DrawSettings round-trip across launches.package/Shaders/VR/StereoBlendCS.hlsl (1)
219-236:⚠️ Potential issue | 🟠 MajorKeep the original reprojection when the POM-adjusted hit misses.
This branch overwrites
rwith the POM-adjusted reprojection and then bails if that second hit is invalid or samples clear depth. For Eye 1MODE_MAIN/MODE_EDGEpixels, that drops a valid first-pass source and leaves the skipped pixel stale.Suggested fix
- float pomOffset = ReflectanceTexture[r.otherPx].w; + Stereo::StereoBilateralResult baseR = r; + float pomOffset = ReflectanceTexture[r.otherPx].w; if (pomOffset > 1e-2) { // Re-reproject with POM-adjusted depth centered at geometry plane float linearDepth = SharedData::GetScreenDepth(centerDepth); float depthCorrection = (0.5 - pomOffset) * POMDepthScale; float newLinearDepth = max(linearDepth + depthCorrection, 1e-4); reprojDepth = (SharedData::CameraData.x - SharedData::CameraData.w / newLinearDepth) / SharedData::CameraData.z; - r = Stereo::ReprojectToOtherEye(uv, reprojDepth, eyeIndex, FrameDim); - if (!r.valid) - return; + Stereo::StereoBilateralResult pomR = Stereo::ReprojectToOtherEye(uv, reprojDepth, eyeIndex, FrameDim); + if (pomR.valid) { + float pomSourceDepth = DepthTexture[pomR.otherPx]; + if (pomSourceDepth > 1e-5 && pomSourceDepth < 1.0) + r = pomR; + } } // Skip if the Eye 0 source pixel is sky/unrendered (depth at clear value). float sourceDepth = DepthTexture[r.otherPx]; - if (sourceDepth >= 1.0 || sourceDepth < 1e-5) - return; + if (sourceDepth >= 1.0 || sourceDepth < 1e-5) { + r = baseR; + sourceDepth = DepthTexture[r.otherPx]; + if (sourceDepth >= 1.0 || sourceDepth < 1e-5) + return; + }🤖 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 219 - 236, The POM branch currently overwrites the original reprojection result (r) with the POM-adjusted reprojection and returns on failure, dropping a previously valid first-pass source; capture the original reprojection result (e.g., r_orig = r) before calling Stereo::ReprojectToOtherEye with the POM-adjusted reprojDepth and only replace r with the POM result if that new r is valid and yields a non-clear sourceDepth (DepthTexture[r.otherPx] in range); if the POM reprojection is invalid or samples clear depth, restore/keep r_orig so the original reprojection (and its valid source) is retained.package/Shaders/Lighting.hlsl (1)
3180-3185:⚠️ Potential issue | 🟠 MajorKeep packing
pixelOffsetfor ENVMAP complex-material parallax.
complexMaterialParallaxcan still populatepixelOffsetearlier in this shader withoutPARALLAX,LANDSCAPE, orTRUE_PBRbeing defined. With the current guard, those permutations still forceReflectance.wto0.0, so the overwrite stereo path loses the POM depth correction again.Suggested fix
-# if (defined(EMAT) || defined(TRUE_PBR)) && (defined(PARALLAX) || defined(LANDSCAPE) || defined(TRUE_PBR)) +# if defined(EMAT) || defined(TRUE_PBR) psout.Reflectance = float4(indirectLobeWeights.specular, (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0); # else🤖 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 current preprocessor guard prevents packing pixelOffset into psout.Reflectance.w unless PARALLAX, LANDSCAPE, or TRUE_PBR are defined, but complexMaterialParallax can still set pixelOffset earlier and ENVMAP stereo/overwrite paths need that POM depth correction; update the condition so psout.Reflectance = float4(indirectLobeWeights.specular, (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0) is used whenever ENVMAP complex-material parallax may run (i.e., base it on whether pixelOffset can be populated—refer to symbols pixelOffset, psout.Reflectance, complexMaterialParallax and the ENVMAP/complex-material path—instead of tying it only to (PARALLAX || LANDSCAPE || TRUE_PBR) macros) so the .w component carries the POM correction for those permutations.
🧹 Nitpick comments (2)
src/Features/VR/SettingsUI.cpp (1)
102-110: Keep these dialog widths tied to UI scale.Hard-coding
520/500/400x200here makes the welcome overlay and combo recorder ignore the configured UI scale, so they wrap much earlier on larger-scale setups. If the per-frame scale lookup was the concern, cache the startup value and keep the multipliers. Based on learnings, initialize display-related scaling values once at startup because the resolution cannot be changed at runtime; do not recompute or refresh these values during a session.Also applies to: 126-126, 1004-1004
🤖 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 102 - 110, Hard-coded UI sizes (overlaySize = ImVec2(520,0), PushTextWrapPos +500.0f, and other literal widths like 400x200) ignore configured UI scale; cache the display/UI scale once at startup (e.g., a startup UI scale variable initialized from io.DisplayFramebufferScale or io.FontGlobalScale) and replace literal constants with multiplied values using that cached scale (e.g., overlaySize.x = baseOverlayWidth * uiScaleStartup, wrapPos = baseWrapWidth * uiScaleStartup). Update usages in SettingsUI.cpp (symbols: overlaySize, overlayPos calculation, ImGui::PushTextWrapPos, and the other occurrences noted) to use the cached startup scale variable so sizes are computed once and remain stable during the session.src/Features/Upscaling.cpp (1)
1066-1091: Texture recreation for RTV support is handled correctly but adds runtime overhead.The code recreates
vrFinalOutputto addD3D11_BIND_RENDER_TARGETif missing. This is necessary for the PS feathered composite path. However, the outerneedsRecreatecheck at line 994 doesn't account for missing RTV bind flags, so this inner recreation will occur every frame until format/size changes trigger a full recreation.Consider adding the RTV bind flag check to
needsRecreateto avoid the redundant bind-flag check per frame:💡 Suggested optimization
bool needsRecreate = !vrIntermediateColorIn[0] || !vrCropColorIn[0] || !vrIntermediateDepth[0] || - !vrIntermediateColorOut[0] || !vrFinalOutput[0]; + !vrIntermediateColorOut[0] || !vrFinalOutput[0] || !vrFinalOutput[0]->rtv;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1066 - 1091, The per-frame recreation happens because the existing needsRecreate check doesn't consider missing RTV bind support; update the logic that computes needsRecreate (the variable/condition referenced as needsRecreate) to also detect when vrFinalOutput textures lack D3D11_BIND_RENDER_TARGET and force a full recreate in that case. Concretely, when comparing/storing the Texture2D desc for vrFinalOutput (or when iterating vrFinalOutput to decide recreation), inspect each texture's D3D11_TEXTURE2D_DESC.BindFlags and set needsRecreate = true if (BindFlags & D3D11_BIND_RENDER_TARGET) == 0 so the texture is recreated once with the correct bind flags instead of being fixed every frame.
🤖 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/Features/TAAReorder.cpp`:
- Around line 520-588: Add an early VR-only guard in InitEarly() and Init() so
the VR-specific detours and singleton pointer aren't applied on SE/AE; at the
top of both functions check REL::Module::IsVR() and return immediately if false.
Specifically, wrap the DepthStencilRegHook, ForceTAASetter, TAAStateMachine,
ExecutePassHook, SubmitHook, the g_pRendererSingleton assignment, and the
stl::write_thunk_call<...> calls behind that guard. Also replace raw base+RVA
usages with REL relocation helpers (e.g., REL::RelocateMember or equivalent REL
patterns) for each symbol (DepthStencilRegHook, ForceTAASetter, TAAStateMachine,
ExecutePassHook, SubmitHook, g_pRendererSingleton, BSImagespaceShaderHook,
ConductorCallHook) so offsets resolve correctly across SE/AE/VR variants.
- Around line 291-299: The CreateBuffer and subsequent Map/memcpy calls for
upscaling.vrFeatheredCompositeCB must gracefully fail and fall back to the
non-feathered path instead of letting DX::ThrowIfFailed or an unchecked Map
result crash the app: when calling globals::d3d::device->CreateBuffer(...) do
not unconditionally throw—check the HRESULT and if creation fails clear/reset
upscaling.vrFeatheredCompositeCB and set a flag to disable the feathered path;
likewise, when mapping the buffer (ID3D11DeviceContext::Map used in the
feathered update code), check the returned HRESULT (or the Map result) and
ensure mapped.pData is valid before memcpy, and on any failure
unmap/release/reset the buffer and fall back to the hard-copy code path rather
than calling memcpy or continuing with the feathered logic (apply the same
pattern to the other block at lines ~361-381).
In `@src/Features/VR.cpp`:
- Around line 123-126: stereoOpt.loaded is being set unconditionally which can
enable the overwrite path in StereoBlend.cpp even if SetupResources() failed;
change SetupResources() (or add a new readiness checker) to return a bool
indicating success and set stereoOpt.loaded = stereoOpt.SetupResources() here,
or after SetupResources() return derive loaded by verifying the required DirectX
resources (e.g., presence/non-null of the shader and texture members used in
StereoBlend.cpp such as the vertex/pixel shader and stereo textures) and only
set loaded when all required members are valid; also ensure SetupResources()
cleans up on partial failure so the object is left in a consistent state.
---
Duplicate comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-106: The early return in DeferredCompositeCS.hlsl (the
eyeIndex == 1 branch that checks StereoOptModeTexture and returns for
MODE_MAIN/MODE_EDGE) causes NormalTAAMaskSpecularMaskRW (u1) to remain stale;
either move or delay the return until after the shader updates u1 for the pixel,
or update the overwrite path in VR/StereoBlendCS.hlsl so that for MODE_MAIN and
MODE_EDGE it also writes the NormalTAAMaskSpecularMaskRW/u1 payload (and any
EncodeNormalVanilla logic) in addition to color/motion; locate the branch using
eyeIndex and StereoOptModeTexture and ensure consistency with the
StereoBlendCS.hlsl write paths for MODE_MAIN/MODE_EDGE so u1 is always
refreshed.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3180-3185: The current preprocessor guard prevents packing
pixelOffset into psout.Reflectance.w unless PARALLAX, LANDSCAPE, or TRUE_PBR are
defined, but complexMaterialParallax can still set pixelOffset earlier and
ENVMAP stereo/overwrite paths need that POM depth correction; update the
condition so psout.Reflectance = float4(indirectLobeWeights.specular,
(pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0) is used whenever ENVMAP
complex-material parallax may run (i.e., base it on whether pixelOffset can be
populated—refer to symbols pixelOffset, psout.Reflectance,
complexMaterialParallax and the ENVMAP/complex-material path—instead of tying it
only to (PARALLAX || LANDSCAPE || TRUE_PBR) macros) so the .w component carries
the POM correction for those permutations.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 219-236: The POM branch currently overwrites the original
reprojection result (r) with the POM-adjusted reprojection and returns on
failure, dropping a previously valid first-pass source; capture the original
reprojection result (e.g., r_orig = r) before calling
Stereo::ReprojectToOtherEye with the POM-adjusted reprojDepth and only replace r
with the POM result if that new r is valid and yields a non-clear sourceDepth
(DepthTexture[r.otherPx] in range); if the POM reprojection is invalid or
samples clear depth, restore/keep r_orig so the original reprojection (and its
valid source) is retained.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 97-108: The classifier currently hardcodes the search radius to
kInnerWidth (2), ignoring the runtime EdgeWidth parameter; update the search
logic so maxWidth uses the EdgeWidth value from the cbuffer (e.g., set maxWidth
= max(kInnerWidth, EdgeWidth) or simply maxWidth = EdgeWidth if kInnerWidth is
not required), ensuring the edge search loop that uses
nearestEdgeDist/nearestWeAreOuter honors the runtime EdgeWidth; alternatively,
remove the unused EdgeWidth from the cbuffer if you intend to keep the fixed
kInnerWidth to avoid exposing a no-op setting.
In `@src/Features/TAAReorder.cpp`:
- Around line 145-157: The code sets g_postPPReady and calls CopyResource
unconditionally after EnsurePostPPCopy(postTex), which can leave g_postPPCopy
null; modify the block in TAAReorder.cpp so that after calling
EnsurePostPPCopy(postTex) you check that g_postPPCopy (the
ID3D11Texture2D/ID3D11Resource wrapped by EnsurePostPPCopy) is non-null before
calling globals::d3d::context->CopyResource(...) and before setting
g_postPPReady; if g_postPPCopy is null, skip the CopyResource and do not set
g_postPPReady (optionally log or handle the degraded path), ensuring all uses of
g_postPPCopy and g_postPPReady only occur when the resource is valid.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The onboarding text no longer matches the new auto-show logic: update
the logic around shouldShow (which currently checks settings.kAutoHideSeconds,
globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME), globals::menu and
globals::menu->IsEnabled) and the welcome overlay copy to be consistent—either
reintroduce the previous tween-menu branch into the shouldShow condition so the
overlay can auto-show from the tween menu as before, or keep the current
main-menu-only condition and trim the welcome-overlay text to remove references
to the tween menu/entry point; locate usages of shouldShow and the
welcome-overlay rendering to apply the corresponding code or copy change.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 21-37: Save the two missing debug toggles so their UI state
persists: in VRStereoOptimizations::SaveSettings add
o_json["DebugFullBlendDepth"] = settings.debugFullBlendDepth and
o_json["DebugPOMDepth"] = settings.debugPOMDepth; then in
VRStereoOptimizations::LoadSettings (the counterpart that reads settings) read
those keys into settings.debugFullBlendDepth and settings.debugPOMDepth (use
existing key-reading pattern used for the other debug flags), ensuring the same
JSON key names are used so the toggles displayed by DrawSettings round-trip
across launches.
---
Nitpick comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1066-1091: The per-frame recreation happens because the existing
needsRecreate check doesn't consider missing RTV bind support; update the logic
that computes needsRecreate (the variable/condition referenced as needsRecreate)
to also detect when vrFinalOutput textures lack D3D11_BIND_RENDER_TARGET and
force a full recreate in that case. Concretely, when comparing/storing the
Texture2D desc for vrFinalOutput (or when iterating vrFinalOutput to decide
recreation), inspect each texture's D3D11_TEXTURE2D_DESC.BindFlags and set
needsRecreate = true if (BindFlags & D3D11_BIND_RENDER_TARGET) == 0 so the
texture is recreated once with the correct bind flags instead of being fixed
every frame.
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 102-110: Hard-coded UI sizes (overlaySize = ImVec2(520,0),
PushTextWrapPos +500.0f, and other literal widths like 400x200) ignore
configured UI scale; cache the display/UI scale once at startup (e.g., a startup
UI scale variable initialized from io.DisplayFramebufferScale or
io.FontGlobalScale) and replace literal constants with multiplied values using
that cached scale (e.g., overlaySize.x = baseOverlayWidth * uiScaleStartup,
wrapPos = baseWrapWidth * uiScaleStartup). Update usages in SettingsUI.cpp
(symbols: overlaySize, overlayPos calculation, ImGui::PushTextWrapPos, and the
other occurrences noted) to use the cached startup scale variable so sizes are
computed once and remain stable during the session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35b44ce6-ce58-4cef-aa33-021902414ca7
📒 Files selected for processing (39)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlslfeatures/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlslfeatures/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlslfeatures/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlslpackage/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/TAAReorder.cppsrc/Features/TAAReorder.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.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 (14)
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- features/Upscaling/Shaders/Upscaling/ForceAlphaCS.hlsl
- src/Features/ExtendedMaterials.h
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- features/Upscaling/Shaders/Upscaling/VRPeripheryFillCS.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- features/Upscaling/Shaders/Upscaling/DLSSCompositePS.hlsl
- features/Upscaling/Shaders/Upscaling/FeatheredCompositeCS.hlsl
- features/Upscaling/Shaders/Upscaling/FeatheredCompositePS.hlsl
- src/Features/VR/StereoBlend.cpp
- src/Features/TAAReorder.h
🚧 Files skipped from review as they are similar to previous changes (8)
- package/Shaders/DistantTree.hlsl
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
- features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl
- src/Features/VR.h
- src/Deferred.cpp
- src/Features/VRStereoOptimizations.h
- src/Features/Upscaling/Streamline.cpp
Comment on lines
+291
to
+299
| // Ensure CB exists (lazy create, matching Upscaling.cpp pattern) | ||
| if (!upscaling.vrFeatheredCompositeCB) { | ||
| D3D11_BUFFER_DESC cbDesc = {}; | ||
| cbDesc.ByteWidth = 48; | ||
| cbDesc.Usage = D3D11_USAGE_DYNAMIC; | ||
| cbDesc.BindFlags = D3D11_BIND_CONSTANT_BUFFER; | ||
| cbDesc.CPUAccessFlags = D3D11_CPU_ACCESS_WRITE; | ||
| DX::ThrowIfFailed(globals::d3d::device->CreateBuffer(&cbDesc, nullptr, upscaling.vrFeatheredCompositeCB.put())); | ||
| } |
Contributor
There was a problem hiding this comment.
Fall back from the feathered path when constant-buffer setup fails.
The optional feathered path still hard-fails here: CreateBuffer throws, and Map() is used without checking its HRESULT before memcpy(mapped.pData, ...). If either call fails, you crash instead of taking the existing hard-copy fallback. 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: 361-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/TAAReorder.cpp` around lines 291 - 299, The CreateBuffer and
subsequent Map/memcpy calls for upscaling.vrFeatheredCompositeCB must gracefully
fail and fall back to the non-feathered path instead of letting
DX::ThrowIfFailed or an unchecked Map result crash the app: when calling
globals::d3d::device->CreateBuffer(...) do not unconditionally throw—check the
HRESULT and if creation fails clear/reset upscaling.vrFeatheredCompositeCB and
set a flag to disable the feathered path; likewise, when mapping the buffer
(ID3D11DeviceContext::Map used in the feathered update code), check the returned
HRESULT (or the Map result) and ensure mapped.pData is valid before memcpy, and
on any failure unmap/release/reset the buffer and fall back to the hard-copy
code path rather than calling memcpy or continuing with the feathered logic
(apply the same pattern to the other block at lines ~361-381).
Contributor
Author
There was a problem hiding this comment.
This is about the feathered composite path in BSImagespaceShaderHook::thunk() where CreateBuffer uses DX::ThrowIfFailed (crashes on failure) and Map() return value isn't checked before memcpy. In practice these only fail on catastrophic GPU issues (out of memory, device lost), but the comment is technically correct that a graceful fallback to the hard-copy path would be more robust. Will revisit.
Contributor
There was a problem hiding this comment.
@vrnord, that's a fair assessment — these paths do only fail on device-lost / OOM, but wrapping them in a graceful fallback is straightforward. Here's the pattern to apply in both spots:
CreateBuffer (lines ~291–299): replace DX::ThrowIfFailed with an HRESULT check and disable the feathered path on failure:
if (!upscaling.vrFeatheredCompositeCB) {
D3D11_BUFFER_DESC cbDesc = {};
cbDesc.ByteWidth = 48;
cbDesc.Usage = D3D11_USAGE_DYNAMIC;
cbDesc.BindFlags = D3D11_BIND_CONSTANT_BUFFER;
cbDesc.CPUAccessFlags = D3D11_CPU_ACCESS_WRITE;
HRESULT hr = globals::d3d::device->CreateBuffer(&cbDesc, nullptr, upscaling.vrFeatheredCompositeCB.put());
if (FAILED(hr)) {
logger::warn("[TAAReorder] vrFeatheredCompositeCB creation failed (0x{:X}), falling back to hard-copy", (uint32_t)hr);
upscaling.vrFeatheredCompositeCB = nullptr;
useFeathered = false;
}
}Map/memcpy (lines ~361–381): guard the mapped data pointer:
D3D11_MAPPED_SUBRESOURCE mapped = {};
HRESULT hr = context->Map(upscaling.vrFeatheredCompositeCB.get(), 0, D3D11_MAP_WRITE_DISCARD, 0, &mapped);
if (FAILED(hr) || !mapped.pData) {
logger::warn("[TAAReorder] vrFeatheredCompositeCB Map failed (0x{:X}), falling back to hard-copy", (uint32_t)hr);
// fall through to hard-copy path below
useFeathered = false;
} else {
memcpy(mapped.pData, &cbData, sizeof(cbData));
context->Unmap(upscaling.vrFeatheredCompositeCB.get(), 0);
}Since useFeathered gates the entire feathered block, resetting it to false on either failure will naturally fall through to your existing CopySubresourceRegion hard-copy path with no additional restructuring needed.
🧠 Learnings used
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:29.538Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:25.118Z
Learning: In src/Features/VR/StereoBlend.cpp, treat StereoBlendDebugMode values 4 and 5 as the same shader variant. When vrStereoOptActive is true, stereoBlendOverwriteCS is selected unconditionally; do not create distinct code paths or shader variants for 4 vs 5. Consolidate 4 and 5 into a single path (e.g., a single case) and document the mapping with comments. Update reviews, tests, and any CI/build logic to reflect that 4/5 share the same shader with differing cbuffers, and ensure no behavior differs between them.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: package/Shaders/Lighting.hlsl:3089-3095
Timestamp: 2026-03-16T13:13:11.490Z
Learning: In `package/Shaders/Lighting.hlsl` (doodlum/skyrim-community-shaders), the `TREE_ANIM` alpha test path intentionally uses a hardcoded `0.1` floor check (`if (alpha < 0.1) discard;`) and `AlphaTestRefRS` rather than `SharedData::VRAlphaTestThreshold`. `VRAlphaTestThreshold` is wired into `DistantTree.hlsl` and `RunGrass.hlsl` only. The threshold wiring for `Lighting.hlsl`'s `TREE_ANIM` path is deferred to a separate VR foliage PR.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Flag potential problems proactively including performance impact on rendering, SE/AE/VR runtime compatibility issues, GPU register conflicts, and security risks from malformed configurations
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/Features/**/*.{h,hpp,cpp,hlsl} : Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance
Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: package/Shaders/VRStereoOptimizations/StencilCS.hlsl:97-108
Timestamp: 2026-03-24T04:37:14.725Z
Learning: In `package/Shaders/VRStereoOptimizations/StencilCS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), `EdgeWidth` is present in `VRStereoOptParams` cbuffer (`cbuffers.hlsli`) as a future-tuning placeholder but is intentionally NOT wired into `maxWidth` in the classification loop — `maxWidth` is hardcoded to `kInnerWidth = 2`. Do not flag this as a bug or suggest `maxWidth = max(kInnerWidth, EdgeWidth)`; the wiring is deferred to a later PR.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-25T04:43:36.075Z
Learning: Applies to src/**/*.{h,hpp,cpp} : Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and `REL::RelocateMember()` patterns for compatibility
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:47.322Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:16.698Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1880
File: src/Features/VR/InSceneOverlay.cpp:603-604
Timestamp: 2026-02-15T08:13:49.292Z
Learning: The skyrim-community-shaders repository uses OpenVR SDK version 1.0.10, where IVRCompositor::Submit is at vtable index 5 (0-based). Do not suggest changing this to index 6, as that is only correct for newer OpenVR versions (2.x).
Learnt from: zndxcvbn
Repo: doodlum/skyrim-community-shaders PR: 1929
File: src/Utils/UI.cpp:55-57
Timestamp: 2026-02-22T14:39:02.543Z
Learning: In the Skyrim community shaders project, initialize display-related scaling values once at startup because the resolution cannot be changed at runtime. Do not recompute or refresh these values during a session; store them in a static or application-wide initialization path (e.g., at startup or in a startup singleton) to avoid stale scaling. This applies to UI-related initialization in source files like src/Utils/UI.cpp.
a75cf9f to
b18e4fc
Compare
|
✅ A pre-release build is available for this PR: |
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
5 times, most recently
from
89d316a to
05bea19
Compare
Collaborator
|
Taking to draft until dependency #2002 merged. |
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
2 times, most recently
from
414de2b to
b8f877a
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>
Process a configurable central crop through DLSS, reducing GPU cost. Periphery filled with bilinear upscale or TAA'd content via conductor. Feathered composite at crop boundary hides quality transition. Nasal crop offset shifts DLSS region toward nose for higher acuity. Key components: - Per-eye DLSS evaluation with viewport-scaled projection matrices - ClearHMDMask zeroes hidden area mesh pixels before DLSS - VRPeripheryFillCS bilinear upscales render-res to display-res - FeatheredCompositePS alpha-blends DLSS crop onto periphery - TAAReorder conductor architecture for periphery TAA - REL::Module::IsVR() in device-creation hook (timing fix) - Bounds guards in ClearHMDMaskCS and ForceAlphaCS - Per-frame flag resets in TAAReorder - Complete pipeline state save/restore in BSImagespaceShaderHook - Crop parameter clamping before Streamline constants Based on PureDark's Skyrim-Upscaler VR conductor architecture (MIT). Depends on: feat(vr): VR stereo reprojection optimizations Replaces PR community-shaders#1983. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vrnord
force-pushed
the
pr/vr-dlss-viewport
branch
from
d5ca601 to
a12530b
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
Author
|
Closing this PR. The DLSS viewport scaling + periphery TAA approach of intercepting the post-processing conductor has proven fragile — UI timing issues, format mismatches, and motion vector reprojection artifacts when stacking with VRStereoOptimizations. |
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.
Process a configurable central crop of each eye through DLSS, reducing GPU cost. Periphery filled with bilinear upscale or TAA via conductor architecture.
What it does
Based on PureDark's Skyrim-Upscaler VR conductor architecture (MIT).
Depends on: #2002 (VR stereo reprojection optimizations)
Replaces #1983.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements