feat(vr): optimize SSGI VR#2005
Draft
vrnord wants to merge 3 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a comprehensive VR stereo rendering optimization system that classifies per-pixel reprojection viability via stencil buffer, implements early-exit logic in compute shaders to skip unnecessary processing on eye 1, dispatches lateral reprojection for culled pixels, and provides debug visualization support with supporting shader infrastructure and C++ integration. Changes
Sequence DiagramsequenceDiagram
participant Renderer as Deferred Renderer
participant StencilPass as Stencil Classification
participant SSGIShaders as SSGI Compute Shaders
participant ModeTexture as Mode Texture
participant StereoBlend as StereoBlend/Composite
participant Reprojection as Reprojection CS
participant VRPostProc as VR PostProcess CS
participant Output as Final Output
Renderer->>StencilPass: DispatchStencil()
Note over StencilPass: Sample depth, classify pixels<br/>(disoccluded/edge/main/full-blend)
StencilPass->>ModeTexture: Write pixel modes
Renderer->>SSGIShaders: Dispatch SSGI passes
SSGIShaders->>ModeTexture: Sample StereoOptModeTexture
alt eyeIndex == 1 and mode ∈ {1,2}
SSGIShaders->>SSGIShaders: Early return (skip GI work)
else eyeIndex == 0 or mode not in {1,2}
SSGIShaders->>Output: Perform full GI computation
end
Renderer->>StereoBlend: DrawStereoBlend() / Composite
StereoBlend->>ModeTexture: Sample mode classification
StereoBlend->>Output: Apply stereo blend logic
Renderer->>Reprojection: DispatchReprojection()
Reprojection->>ModeTexture: Read pixel modes
Reprojection->>Output: Copy Eye 0 → Eye 1 (lateral reproject)
Renderer->>VRPostProc: VRPostProcess (optional post-composite)
VRPostProc->>ModeTexture: Sample modes
VRPostProc->>Output: Depth-agreement weighted blend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 8
🧹 Nitpick comments (10)
package/Shaders/DeferredCompositeCS.hlsl (1)
100-107: Use named constant instead of magic number for mode comparison.The comparison
mode == 2uses a magic number. For clarity and maintainability, consider includingcbuffers.hlsliand usingMODE_MAIN:+#if defined(VR_STEREO_OPT) +#include "VRStereoOptimizations/cbuffers.hlsli" +#endif ... - if (mode == 2) { // MODE_MAIN — stencil-culled, no valid G-buffer + if (mode == MODE_MAIN) { // stencil-culled, no valid G-buffer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 100 - 107, Replace the magic literal compare in the VR stereo branch with the named constant by including the header that defines MODE_MAIN; specifically, add an include for cbuffers.hlsli and change the check in the DeferredCompositeCS shader where StereoOptModeTexture is read (the block guarded by VR_STEREO_OPT and using eyeIndex and StereoOptModeTexture) so it compares mode == MODE_MAIN instead of mode == 2 to improve clarity and maintainability.package/Shaders/VR/VRPostProcessCS.hlsl (2)
29-33: MODE_ constants duplicated from cbuffers.hlsli.*These mode constants are also defined in
VRStereoOptimizations/cbuffers.hlsli. Consider including that header instead to maintain a single source of truth.♻️ Suggested change
`#include` "Common/FrameBuffer.hlsli" `#include` "Common/SharedData.hlsli" `#include` "Common/VR.hlsli" +#include "VRStereoOptimizations/cbuffers.hlsli" Texture2D<float4> ColorTexture : register(t0); // Copy of final composited image ... -#define MODE_DISOCCLUDED 0 -#define MODE_EDGE 1 -#define MODE_MAIN 2 -#define MODE_EDGE_NEIGHBOUR 3 -#define MODE_FULL_BLEND 4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/VRPostProcessCS.hlsl` around lines 29 - 33, The MODE_* constants (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here; remove these definitions from VRPostProcessCS.hlsl and include the canonical header VRStereoOptimizations/cbuffers.hlsli instead so there is a single source of truth; update any references in VRPostProcessCS.hlsl to rely on the imported constants and ensure the include appears before any use of those MODE_* symbols.
101-106: Consider using linearized depth for geometrically accurate bilateral weighting.The depth agreement calculation uses raw NDC depth values:
float depthAgreement = 1.0 - saturate(abs(centerDepth - otherDepth) / maxDepth / 0.02);NDC depth is non-linear (hyperbolic), so equal NDC differences don't correspond to equal world-space distances. For more perceptually consistent blending, consider using
SharedData::GetScreenDepth()to linearize both depths before computing the agreement, similar to howStencilCS.hlslhandles depth comparisons.This may be intentional for performance, but could cause inconsistent blending behavior at different depth ranges.
🤖 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 101 - 106, The depth agreement is computed using raw NDC depths (centerDepth, otherDepth) which are non-linear; replace the raw-depth comparison with linearized depths by calling SharedData::GetScreenDepth() on both centerDepth and otherDepth, then compute maxDepth and depthAgreement from those linear depths (keep the same threshold logic or adjust if needed), and use that depthAgreement in blendWeight before the lerp to ensure geometrically consistent bilateral blending in VRPostProcessCS.hlsl.package/Shaders/VRStereoOptimizations/StencilCS.hlsl (2)
76-79: Inconsistent depth comparison methodology between disocclusion and edge detection.Disocclusion detection (lines 76-79) uses raw NDC depth:
float relativeDepthDiff = abs(centerDepth - otherDepth) / maxDepth;But edge detection (lines 115-117) uses linearized screen depth:
float linNeighbor = neighborIsSky ? 999999.0 : SharedData::GetScreenDepth(neighborDepth); float relDepthDiff = abs(linCenter - linNeighbor) / maxLin;This inconsistency means the
DisocclusionThresholdandEdgeDepthThresholdparameters operate on different depth spaces, making tuning non-intuitive. Consider linearizing depth for disocclusion detection as well for consistent behavior.🤖 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 76 - 79, Disocclusion uses raw NDC depth while edge detection uses linearized depth, so change the disocclusion block to linearize both center and other depths before comparison: read DepthTexture[reproj.otherPx] as before, call SharedData::GetScreenDepth(...) for centerDepth and otherDepth (handling sky like edge code if needed), compute maxLin and relative depth diff from the linearized values, and set isDisoccluded based on (relativeDepthDiff > DisocclusionThreshold); update references to centerDepth/otherDepth in that block to use the linearized names so DisocclusionThreshold and EdgeDepthThreshold operate in the same depth space.
96-96: Consider makingkInnerWidthconfigurable via cbuffer.
kInnerWidth = 2is hardcoded. SinceEdgeWidthalready exists in the cbuffer (though unused in this shader), consider using it or a similar cbuffer parameter to allow runtime tuning of the edge band width.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl` at line 96, The hardcoded constant kInnerWidth = 2 should be made configurable from a cbuffer so edge band width can be tuned at runtime; modify the shader to remove the static const kInnerWidth and instead read a uint (or int) parameter from the existing cbuffer (e.g., reuse EdgeWidth or add a new field like InnerWidth) and replace uses of kInnerWidth with that cbuffer value in the StencilCS shader; ensure the cbuffer declaration is updated to include the new parameter and that any callers that set the cbuffer (host/CPU-side code) are updated to supply a sensible default if needed.src/Features/ScreenSpaceGI.cpp (1)
888-892: Consider consolidating the duplicated stereo opt check.The
useStereoOptcondition (lines 888-891) duplicates thestereoOptActivecheck from lines 751-752. You could reusestereoOptActivehere instead.♻️ Suggested simplification
- bool useStereoOpt = REL::Module::IsVR() && - globals::features::vrStereoOptimizations.loaded && - globals::features::vrStereoOptimizations.settings.stereoMode != VRStereoOptimizations::StereoMode::Off; - - if (REL::Module::IsVR() && stereoSyncCompute && !useStereoOpt) { + if (REL::Module::IsVR() && stereoSyncCompute && !stereoOptActive) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceGI.cpp` around lines 888 - 892, The duplicated VR/stereo optimization check should be consolidated: replace the local useStereoOpt computation and the later REL::Module::IsVR() && stereoSyncCompute && !useStereoOpt condition with the already-existing stereoOptActive boolean (or make stereoOptActive available in this scope) so the second conditional becomes REL::Module::IsVR() && stereoSyncCompute && !stereoOptActive; remove the redundant useStereoOpt variable or its computation to avoid duplicate logic and ensure the same stereo optimization check is used throughout (verify stereoOptActive is defined before use and adjust scope if necessary).features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl (1)
20-22: Replace the raw mode literals with shared constants.
1and2are duplicated here while the stencil/reprojection shaders already use namedMODE_*values. Pull the skip mask from a shared include, or at least name the values locally, so this fast path cannot drift from the classifier encoding. Based on learnings, simple scalar constants in this repository's HLSL are typically expressed as#definevalues.Also applies to: 66-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl around lines 20 - 22, Replace the hard-coded numeric mode literals with named constants to match the existing MODE_* naming used by the stencil/reprojection shaders: either include the shared HLSL header that defines MODE_* and the skip mask, or add local `#define` constants (e.g. MODE_SKIP, MODE_REPROJECT and SKIP_MASK) and use those identifiers wherever the file reads StereoOptModeTexture (the Texture2D<uint> StereoOptModeTexture : register(t16) usage and the branches around lines 66–75) so the fast path can’t drift from the classifier encoding.src/Features/VR/SettingsUI.cpp (1)
326-345: Document the two new overwrite debug modes in the tooltip.The combo now exposes
OverwriteandOverwrite Eye1, but the help text still stops atEdge Detection, so users get two undocumented modes. Based on learnings, modes 4/5 share the same shader variant and their mapping should be documented.🤖 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 for the Debug View combo (see debugModes[] and ImGui::Combo("Debug View", &settings.StereoBlendDebugMode, ...)) is missing entries for the two new modes ("Overwrite" and "Overwrite Eye1"); update the ImGui::Text block that shows the hover tooltip (used in Util::HoverTooltipWrapper()) to add short descriptions for "Overwrite" and "Overwrite Eye1", noting that both use the same shader variant and explaining their mapping (e.g. "Overwrite: force destination color, uses overwrite shader variant" and "Overwrite Eye1: same shader variant but maps to eye1 as source/target" or equivalent phrasing that documents which eye is overwritten). Ensure you reference settings.StereoBlendDebugMode semantics so the tooltip matches the combo order.package/Shaders/VR/StereoBlendCS.hlsl (1)
28-34: Deduplicate the mode IDs behind a shared include.These macros now hand-copy the classification contract from
VRStereoOptimizations/cbuffers.hlsli. One reorder on either side will silently reinterpret every pixel class. Please split the shared constants into a cbuffer-free include and consume that from both shaders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/StereoBlendCS.hlsl` around lines 28 - 34, The mode macros (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here and in VRStereoOptimizations/cbuffers.hlsli which risks silent reordering bugs; extract these constants into a new cbuffer-free include (e.g., StereoModes.hlsli) that only defines the mode IDs in a single place, remove the local `#define` block from StereoBlendCS.hlsl and instead `#include` that shared file, and update VRStereoOptimizations/cbuffers.hlsli to consume the same include so both shaders use the identical ordering (keeping StereoBlendCB and any b1 cbuffer usage unchanged).src/Features/VRStereoOptimizations.h (1)
94-95: ReplacereprojectionModemagic numbers with a typed enum.
0/4/5is hard to audit in a serialized settings surface, and the current comment implies more structural distinction than there really is. A named enum would make the config self-documenting and avoid implying that 4 and 5 are separate shader variants.Example refactor
+ enum class ReprojectionMode : uint32_t + { + Blend = 0, + Overwrite = 4, + OverwriteEye1Only = 5, + }; ... - int reprojectionMode = 5; // 0=Blend, 4=Overwrite, 5=Overwrite Eye1 Only + ReprojectionMode reprojectionMode = ReprojectionMode::OverwriteEye1Only;Based on learnings, debug modes 4/5 in
src/Features/VR/StereoBlend.cppuse the samestereoBlendOverwriteCSshader and differ only by cbuffer parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.h` around lines 94 - 95, Replace the magic-number int reprojectionMode with a typed enum (e.g., enum class ReprojectionMode { Blend = 0, Overwrite = 4, OverwriteEye1Only = 5 }) and use that enum type where reprojectionMode is declared and serialized; update any serialization/setting code to read/write the enum value instead of raw ints and adjust any switch/if logic that checks reprojectionMode to compare against the new enum constants. Ensure callers in src/Features/VR/StereoBlend.cpp (notably where stereoBlendOverwriteCS is chosen/parameterized) are updated to use the enum and pass the correct cbuffer parameters for Overwrite vs OverwriteEye1Only, while noting they share the same shader; keep the original numeric values for backward compatibility during deserialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl:
- Around line 83-89: The VR_STEREO_OPT fast-path for eyeIndex==1 returns early
for modes 1 and 2 without writing UAVs, leaving stale temporal outputs; before
the early return in that block (the if (mode == 1 || mode == 2) branch)
write/reset the temporal outputs (clear outAccumFrames and the outRemapped* UAVs
— e.g. outRemappedDepth/outRemappedColor or their actual variable names used in
this shader) to a neutral/zero state so culled pixels do not retain history,
then return. Ensure the reset uses the same output formats/types as the normal
code paths so layout remains consistent.
In `@package/Shaders/Lighting.hlsl`:
- Around line 3200-3205: The current `#if` branch zeros the entire
psout.Reflectance vector for non-parallax materials which clears RGB used later
for specular; change the assignment so only the .w component depends on the
parallax condition while leaving Reflectance.rgb set to
indirectLobeWeights.specular for all cases. Concretely, update the logic around
psout.Reflectance so that Reflectance.rgb = indirectLobeWeights.specular always
and Reflectance.w = (pixelOffset > 0.0) ? saturate(pixelOffset) : 0.0 only when
(EMAT||TRUE_PBR) && (PARALLAX||LANDSCAPE), keeping the original values for other
builds (so deferred composite still sees valid specular data).
In `@package/Shaders/RunGrass.hlsl`:
- Around line 505-513: Replace the hardcoded VR eye offset (0.1) with the shared
VR alpha-threshold value: compute the per-eye offset using
SharedData::VRAlphaTestThreshold and Stereo::GetEyeIndexPS (same place where
convergenceEyeIndex is obtained) and subtract that scaled value from
AlphaTestRefRS before the alpha test; update the block around AlphaTestRefRS,
convergenceEyeIndex (from input.HPosition, VPOSOffset) and the if ((diffuseAlpha
- alphaRef) < 0) discard check to use SharedData::VRAlphaTestThreshold-derived
offset instead of 0.1 (also apply the same change in the other occurrence
mentioned).
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 47-49: The reprojection clamp currently uses the full SBS FrameDim
when computing eye0Px after calling Stereo::ConvertMonoUVToOtherEye and
Stereo::ConvertToStereoUV; change the clamp so the x coordinate is limited to
Eye 0 bounds (half the stereo width) rather than the full surface. Specifically,
compute an eye0 width (e.g., eyeWidth = FrameDim.x / 2) and clamp eye0StereoUV *
FrameDim using int2(eyeWidth, FrameDim.y) (or the equivalent) before converting
to int2 for eye0Px, ensuring reprojections cannot spill into Eye 1 and
preserving the read-left/write-right guarantee.
In `@src/Deferred.cpp`:
- Around line 372-378: The VR_STEREO_OPT shader contract expects a SRV at slot
t16 but the code in Deferred.cpp only binds vrStereoOpt.GetModeTextureSRV() when
stereoMode != VRStereoOptimizations::StereoMode::Off, which lets the compute
shader hit its stereo-opt path with an unbound texture; update the logic in the
block that references globals::features::vrStereoOptimizations (and similar
blocks at the other locations) to either: 1) only define/compile the
VR_STEREO_OPT variant when vrStereoOpt.settings.stereoMode !=
VRStereoOptimizations::StereoMode::Off, or 2) always bind a valid "neutral" mode
SRV (a fallback SRV) to slot 16 via context->CSSetShaderResources(16, 1,
&neutralSRV) when stereoMode == Off so the shader contract is satisfied even for
MIP-bias-only loads, and ensure GetComputeMainComposite*() and VR_STEREO_OPT are
kept consistent.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 76: The visibility predicate for the welcome overlay only checks
IsMenuOpen(RE::MainMenu::MENU_NAME) and thus omits the tween menu; update the
condition in the expression that sets shouldShow (which references
settings.kAutoHideSeconds, globals::game::ui,
globals::game::ui->IsMenuOpen(...), globals::menu and globals::menu->IsEnabled)
to also consider the tween menu (e.g., OR in a check for
RE::TweenMenu::MENU_NAME or the appropriate tween menu identifier) so the
overlay appears when either the main menu or the tween menu is open while
preserving the existing kAutoHideSeconds and globals::menu enablement checks.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 173-182: The code creates a read-only DSV into
stencilWriteReadOnlyDSV but still sets stencilActive and runs DispatchStencil()
even when CreateDepthStencilView fails or depthData is missing; adjust the logic
in the block that handles
renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGETS_DEPTHSTENCIL::kMAIN]
so that if depthData.views[0] or depthData.texture is missing or
device->CreateDepthStencilView fails, you do not enable stencil mode: ensure
stencilWriteReadOnlyDSV remains null and bail out (do not set stencilActive =
true and do not call DispatchStencil()), and only activate stencilActive and
call DispatchStencil() after successful creation of stencilWriteReadOnlyDSV (use
the existence of stencilWriteReadOnlyDSV as the guard).
In `@src/Features/VRStereoOptimizations.h`:
- Around line 3-7: This header (VRStereoOptimizations.h) directly uses
fixed-width integer types and standard templates but relies on transitive
includes; add the missing direct includes: `#include` <cstdint> to provide
uint8_t/uint32_t, `#include` <string_view> to provide std::string_view, and
`#include` <utility> to provide std::pair so symbols like uint8_t, uint32_t,
std::string_view and std::pair resolve correctly in declarations/typedefs within
VRStereoOptimizations.h.
---
Nitpick comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl:
- Around line 20-22: Replace the hard-coded numeric mode literals with named
constants to match the existing MODE_* naming used by the stencil/reprojection
shaders: either include the shared HLSL header that defines MODE_* and the skip
mask, or add local `#define` constants (e.g. MODE_SKIP, MODE_REPROJECT and
SKIP_MASK) and use those identifiers wherever the file reads
StereoOptModeTexture (the Texture2D<uint> StereoOptModeTexture : register(t16)
usage and the branches around lines 66–75) so the fast path can’t drift from the
classifier encoding.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: Replace the magic literal compare in the VR stereo branch
with the named constant by including the header that defines MODE_MAIN;
specifically, add an include for cbuffers.hlsli and change the check in the
DeferredCompositeCS shader where StereoOptModeTexture is read (the block guarded
by VR_STEREO_OPT and using eyeIndex and StereoOptModeTexture) so it compares
mode == MODE_MAIN instead of mode == 2 to improve clarity and maintainability.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 28-34: The mode macros (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN,
MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here and in
VRStereoOptimizations/cbuffers.hlsli which risks silent reordering bugs; extract
these constants into a new cbuffer-free include (e.g., StereoModes.hlsli) that
only defines the mode IDs in a single place, remove the local `#define` block from
StereoBlendCS.hlsl and instead `#include` that shared file, and update
VRStereoOptimizations/cbuffers.hlsli to consume the same include so both shaders
use the identical ordering (keeping StereoBlendCB and any b1 cbuffer usage
unchanged).
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 29-33: The MODE_* constants (MODE_DISOCCLUDED, MODE_EDGE,
MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) are duplicated here; remove
these definitions from VRPostProcessCS.hlsl and include the canonical header
VRStereoOptimizations/cbuffers.hlsli instead so there is a single source of
truth; update any references in VRPostProcessCS.hlsl to rely on the imported
constants and ensure the include appears before any use of those MODE_* symbols.
- Around line 101-106: The depth agreement is computed using raw NDC depths
(centerDepth, otherDepth) which are non-linear; replace the raw-depth comparison
with linearized depths by calling SharedData::GetScreenDepth() on both
centerDepth and otherDepth, then compute maxDepth and depthAgreement from those
linear depths (keep the same threshold logic or adjust if needed), and use that
depthAgreement in blendWeight before the lerp to ensure geometrically consistent
bilateral blending in VRPostProcessCS.hlsl.
In `@package/Shaders/VRStereoOptimizations/StencilCS.hlsl`:
- Around line 76-79: Disocclusion uses raw NDC depth while edge detection uses
linearized depth, so change the disocclusion block to linearize both center and
other depths before comparison: read DepthTexture[reproj.otherPx] as before,
call SharedData::GetScreenDepth(...) for centerDepth and otherDepth (handling
sky like edge code if needed), compute maxLin and relative depth diff from the
linearized values, and set isDisoccluded based on (relativeDepthDiff >
DisocclusionThreshold); update references to centerDepth/otherDepth in that
block to use the linearized names so DisocclusionThreshold and
EdgeDepthThreshold operate in the same depth space.
- Line 96: The hardcoded constant kInnerWidth = 2 should be made configurable
from a cbuffer so edge band width can be tuned at runtime; modify the shader to
remove the static const kInnerWidth and instead read a uint (or int) parameter
from the existing cbuffer (e.g., reuse EdgeWidth or add a new field like
InnerWidth) and replace uses of kInnerWidth with that cbuffer value in the
StencilCS shader; ensure the cbuffer declaration is updated to include the new
parameter and that any callers that set the cbuffer (host/CPU-side code) are
updated to supply a sensible default if needed.
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 888-892: The duplicated VR/stereo optimization check should be
consolidated: replace the local useStereoOpt computation and the later
REL::Module::IsVR() && stereoSyncCompute && !useStereoOpt condition with the
already-existing stereoOptActive boolean (or make stereoOptActive available in
this scope) so the second conditional becomes REL::Module::IsVR() &&
stereoSyncCompute && !stereoOptActive; remove the redundant useStereoOpt
variable or its computation to avoid duplicate logic and ensure the same stereo
optimization check is used throughout (verify stereoOptActive is defined before
use and adjust scope if necessary).
In `@src/Features/VR/SettingsUI.cpp`:
- Around line 326-345: The tooltip for the Debug View combo (see debugModes[]
and ImGui::Combo("Debug View", &settings.StereoBlendDebugMode, ...)) is missing
entries for the two new modes ("Overwrite" and "Overwrite Eye1"); update the
ImGui::Text block that shows the hover tooltip (used in
Util::HoverTooltipWrapper()) to add short descriptions for "Overwrite" and
"Overwrite Eye1", noting that both use the same shader variant and explaining
their mapping (e.g. "Overwrite: force destination color, uses overwrite shader
variant" and "Overwrite Eye1: same shader variant but maps to eye1 as
source/target" or equivalent phrasing that documents which eye is overwritten).
Ensure you reference settings.StereoBlendDebugMode semantics so the tooltip
matches the combo order.
In `@src/Features/VRStereoOptimizations.h`:
- Around line 94-95: Replace the magic-number int reprojectionMode with a typed
enum (e.g., enum class ReprojectionMode { Blend = 0, Overwrite = 4,
OverwriteEye1Only = 5 }) and use that enum type where reprojectionMode is
declared and serialized; update any serialization/setting code to read/write the
enum value instead of raw ints and adjust any switch/if logic that checks
reprojectionMode to compare against the new enum constants. Ensure callers in
src/Features/VR/StereoBlend.cpp (notably where stereoBlendOverwriteCS is
chosen/parameterized) are updated to use the enum and pass the correct cbuffer
parameters for Overwrite vs OverwriteEye1Only, while noting they share the same
shader; keep the original numeric values for backward compatibility during
deserialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d978c17-4590-4237-9f23-ee8d5ebbd25a
📒 Files selected for processing (33)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.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/ScreenSpaceGI.cppsrc/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
+83
to
+89
| #if defined(VR_STEREO_OPT) | ||
| if (eyeIndex == 1) { | ||
| uint2 fullResPx = uint2(uv * FrameDim); | ||
| uint mode = StereoOptModeTexture[fullResPx]; | ||
| if (mode == 1 || mode == 2) | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Reset the temporal outputs before taking this Eye 1 fast path.
Unlike the curr_depth < FP_Z branch below, this path returns before touching any UAVs. That leaves stale outAccumFrames/outRemapped* data behind for culled pixels, so a pixel that later stops being culled can pick up old history instead of starting clean.
💡 Proposed fix
`#if` defined(VR_STEREO_OPT)
if (eyeIndex == 1) {
uint2 fullResPx = uint2(uv * FrameDim);
uint mode = StereoOptModeTexture[fullResPx];
- if (mode == 1 || mode == 2)
- return;
+ if (mode == 1 || mode == 2) {
+#ifdef GI
+ outRadianceDisocc[pixCoord] = 0;
+#endif
+#ifdef TEMPORAL_DENOISER
+ outAccumFrames[pixCoord] = 1.0 / 255.0;
+ outRemappedAo[pixCoord] = 0;
+ outRemappedIlY[pixCoord] = 0;
+ outRemappedIlCoCg[pixCoord] = 0;
+ outRemappedPrevGISpecular[pixCoord] = 0;
+#endif
+ return;
+ }
}
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl around
lines 83 - 89, The VR_STEREO_OPT fast-path for eyeIndex==1 returns early for
modes 1 and 2 without writing UAVs, leaving stale temporal outputs; before the
early return in that block (the if (mode == 1 || mode == 2) branch) write/reset
the temporal outputs (clear outAccumFrames and the outRemapped* UAVs — e.g.
outRemappedDepth/outRemappedColor or their actual variable names used in this
shader) to a neutral/zero state so culled pixels do not retain history, then
return. Ensure the reset uses the same output formats/types as the normal code
paths so layout remains consistent.
Comment on lines
+505
to
+513
| { | ||
| float alphaRef = AlphaTestRefRS; | ||
| # if defined(VR) | ||
| uint convergenceEyeIndex = Stereo::GetEyeIndexPS(input.HPosition, VPOSOffset); | ||
| alphaRef -= convergenceEyeIndex * 0.1; | ||
| # endif | ||
| if ((diffuseAlpha - alphaRef) < 0) { | ||
| discard; | ||
| } |
Contributor
There was a problem hiding this comment.
Use the shared VR alpha-threshold setting here.
Hardcoding 0.1 makes AlphaTestThreshold a no-op for grass and breaks the existing foliage tuning path in both depth passes. Please derive the eye-specific offset from SharedData::VRAlphaTestThreshold instead.
💡 Suggested change
- alphaRef -= convergenceEyeIndex * 0.1;
+ alphaRef -= convergenceEyeIndex * SharedData::VRAlphaTestThreshold;Based on learnings, VRAlphaTestThreshold is wired into DistantTree.hlsl and RunGrass.hlsl only.
Also applies to: 876-884
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/RunGrass.hlsl` around lines 505 - 513, Replace the hardcoded
VR eye offset (0.1) with the shared VR alpha-threshold value: compute the
per-eye offset using SharedData::VRAlphaTestThreshold and Stereo::GetEyeIndexPS
(same place where convergenceEyeIndex is obtained) and subtract that scaled
value from AlphaTestRefRS before the alpha test; update the block around
AlphaTestRefRS, convergenceEyeIndex (from input.HPosition, VPOSOffset) and the
if ((diffuseAlpha - alphaRef) < 0) discard check to use
SharedData::VRAlphaTestThreshold-derived offset instead of 0.1 (also apply the
same change in the other occurrence mentioned).
Comment on lines
+47
to
+49
| float3 otherEyeUV = Stereo::ConvertMonoUVToOtherEye(float3(monoUV, depth), 1); | ||
| float2 eye0StereoUV = Stereo::ConvertToStereoUV(otherEyeUV.xy, 0); | ||
| int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(FrameDim) - 1); |
Contributor
There was a problem hiding this comment.
Clamp this reprojection to Eye 0 bounds, not the full SBS surface.
ConvertToStereoUV(..., 0) is supposed to land in the left half, but the current clamp allows x >= eyeWidth to spill into Eye 1 on out-of-range reprojections. That breaks the “read left / write right” guarantee and can turn edge cases into self-reads from the UAV.
🔒 Proposed fix
float3 otherEyeUV = Stereo::ConvertMonoUVToOtherEye(float3(monoUV, depth), 1);
-float2 eye0StereoUV = Stereo::ConvertToStereoUV(otherEyeUV.xy, 0);
-int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(FrameDim) - 1);
+float2 eye0StereoUV = Stereo::ClampToEyeUV(Stereo::ConvertToStereoUV(otherEyeUV.xy, 0), 0);
+int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(int(eyeWidth) - 1, int(eyeHeight) - 1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl` around lines 47 -
49, The reprojection clamp currently uses the full SBS FrameDim when computing
eye0Px after calling Stereo::ConvertMonoUVToOtherEye and
Stereo::ConvertToStereoUV; change the clamp so the x coordinate is limited to
Eye 0 bounds (half the stereo width) rather than the full surface. Specifically,
compute an eye0 width (e.g., eyeWidth = FrameDim.x / 2) and clamp eye0StereoUV *
FrameDim using int2(eyeWidth, FrameDim.y) (or the equivalent) before converting
to int2 for eye0Px, ensuring reprojections cannot spill into Eye 1 and
preserving the read-left/write-right guarantee.
| static LARGE_INTEGER freq = { 0 }; | ||
|
|
||
| bool shouldShow = settings.kAutoHideSeconds > 0 && globals::state->isMainMenuOpen && globals::menu && !globals::menu->IsEnabled; | ||
| bool shouldShow = settings.kAutoHideSeconds > 0 && globals::game::ui && globals::game::ui->IsMenuOpen(RE::MainMenu::MENU_NAME) && globals::menu && !globals::menu->IsEnabled; |
Contributor
There was a problem hiding this comment.
This drops tween-menu visibility for the welcome overlay.
The new predicate only checks RE::MainMenu::MENU_NAME, but the overlay text in this file still says the VR controls work from the main menu or tween menu. That leaves one advertised entry point without the onboarding overlay.
🤖 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 visibility predicate for the
welcome overlay only checks IsMenuOpen(RE::MainMenu::MENU_NAME) and thus omits
the tween menu; update the condition in the expression that sets shouldShow
(which references settings.kAutoHideSeconds, globals::game::ui,
globals::game::ui->IsMenuOpen(...), globals::menu and globals::menu->IsEnabled)
to also consider the tween menu (e.g., OR in a check for
RE::TweenMenu::MENU_NAME or the appropriate tween menu identifier) so the
overlay appears when either the main menu or the tween menu is open while
preserving the existing kAutoHideSeconds and globals::menu enablement checks.
Comment on lines
+3
to
+7
| #include "Feature.h" | ||
|
|
||
| #include <d3d11.h> | ||
| #include <unordered_map> | ||
| #include <winrt/base.h> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Features/VRStereoOptimizations.hRepository: doodlum/skyrim-community-shaders
Length of output: 11830
🏁 Script executed:
cat -n src/Feature.h | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 1972
🏁 Script executed:
head -20 src/FeatureConstraints.h src/FeatureVersions.hRepository: doodlum/skyrim-community-shaders
Length of output: 674
Add the missing standard-library includes this header directly uses.
This header uses uint8_t/uint32_t, std::string_view, and std::pair directly, but only includes transitively through Feature.h for std::string and std::vector. The missing includes are:
<cstdint>for fixed-width integer types (lines 29, 36, 125, 128, 231)<string_view>forstd::string_view(lines 50, 51)<utility>forstd::pair(line 63)
Proposed include cleanup
`#include` "Feature.h"
+#include <cstdint>
`#include` <d3d11.h>
+#include <string_view>
`#include` <unordered_map>
+#include <utility>
`#include` <winrt/base.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VRStereoOptimizations.h` around lines 3 - 7, This header
(VRStereoOptimizations.h) directly uses fixed-width integer types and standard
templates but relies on transitive includes; add the missing direct includes:
`#include` <cstdint> to provide uint8_t/uint32_t, `#include` <string_view> to
provide std::string_view, and `#include` <utility> to provide std::pair so symbols
like uint8_t, uint32_t, std::string_view and std::pair resolve correctly in
declarations/typedefs within VRStereoOptimizations.h.
vrnord
force-pushed
the
pr/vr-ssgi-optimization
branch
from
58b1e20 to
b25bd46
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
package/Shaders/Common/SharedData.hlsli (1)
55-55: Consider using explicit padding type for clarity.Changing
pad0fromfloat1toboolis unconventional for padding. While both are typically 4 bytes in HLSL constant buffers, usingfloat1oruintmakes the padding intent clearer. Based on learnings, this codebase usesuintfor boolean flags in C++/HLSL interfaces for compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/SharedData.hlsli` at line 55, The field pad0 in SharedData.hlsli is declared as a float1 for padding which is unclear—change the type of pad0 to uint (or another explicit 4-byte integer like uint32) to make the padding intent and C++/HLSL compatibility explicit; update any corresponding C++/HLSL interface definitions that reference SharedData::pad0 so sizes and alignment remain identical and rebuild/validate constant buffer layout after the change.package/Shaders/VR/StereoBlendCS.hlsl (1)
28-34: Mode constants duplicated from cbuffers.hlsli — consider adding a sync comment.The mode constants are duplicated here due to the cbuffer conflict with
StereoBlendCB. This is a valid workaround, but the values must stay in sync withVRStereoOptimizations/cbuffers.hlsli. Consider adding a reference to the exact line range or a build-time assertion if feasible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VR/StereoBlendCS.hlsl` around lines 28 - 34, The duplicated mode constants (MODE_DISOCCLUDED, MODE_EDGE, MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) must be explicitly linked to their source to avoid drift: update the block in StereoBlendCS.hlsl to add a clear sync comment that points to the exact symbol source in VRStereoOptimizations/cbuffers.hlsli (include filename and approximate line range or commit hash), and if your build system supports it, add a compile-time check or preprocessor guard that verifies these numeric values match the definitions in VRStereoOptimizations/cbuffers.hlsli (or document the manual synchronization step and owner) so future edits stay consistent with StereoBlendCB.src/Features/VR/SettingsUI.cpp (1)
102-102: UI scale removed from overlay dimensions.The overlay size and text wrap positions changed from scaled values (e.g.,
520 * scale) to fixed pixel values (e.g.,520,500.0f). This may affect readability on high-DPI displays. If intentional, consider documenting why scaling was removed.Also applies to: 110-110, 126-126
🤖 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 102, The overlay and wrap sizes were changed to fixed pixels, which removes DPI scaling; restore use of the UI scale variable by multiplying dimensions and wrap positions by the existing scale (e.g., change ImVec2 overlaySize(520, 0) back to using 520 * scale and adjust the related wrap values such as 500.0f and any other hardcoded sizes on the same code paths); update occurrences referenced by the symbols ImVec2 overlaySize and the wrap position variables used at the other locations so the overlay and text wrapping respect high-DPI scaling.src/Features/VR/StereoBlend.cpp (1)
110-120: DSV save/restore pattern has a subtle issue with RTV array reuse.The
savedRTVsarray is retrieved and released at lines 114-119, then reused at line 174 to get RTVs again. However, the first retrieval's purpose is only to pass them back unchanged viaOMSetRenderTargetsat line 115 to unbind the DSV. The Release calls at lines 116-119 are correct for that retrieval.The pattern is functionally correct but could be clearer. Consider adding a comment explaining that the first retrieval is just to preserve RTVs while unbinding DSV.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VR/StereoBlend.cpp` around lines 110 - 120, The savedRTVs/savedDSV retrieval when isOverwriteMode is true (using context->OMGetRenderTargets) is only to preserve current RTVs so we can call context->OMSetRenderTargets(..., savedRTVs, nullptr) to unbind the DSV; the savedRTVs are released immediately afterward, and the array is reused later—add a concise comment above the OMGetRenderTargets/OMSetRenderTargets block (referencing savedRTVs, savedDSV, isOverwriteMode, context->OMGetRenderTargets and context->OMSetRenderTargets) explaining that this first retrieval only preserves RTV pointers for temporarily unbinding the DSV and that the pointers are released on purpose to clarify why the array is reused later.
🤖 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: GetParallaxCoords and the fallback/init path must preserve the
zero=no-parallax contract: change the fallback/initial pixelOffset from 0.5 back
to 0.0 so Reflectance.w receives 0 for no-parallax pixels (and thus
StereoBlendCS.hlsl’s check (<=1e-2) will classify them correctly); update the
same pattern wherever pixelOffset is set in GetParallaxCoords (and the related
blocks around lines ~500-514) so any distance/fallback logic fades toward 0.0
not 0.5 and ensure callers that write to Reflectance.w continue to propagate the
zero value for true no-parallax cases.
In `@src/FeatureBuffer.cpp`:
- Around line 57-58: Add the missing VRTerrainBlend declaration and matching
HLSL struct: create VRTerrainBlend.h that declares the VRTerrainBlend class
(with a public member 'settings' of type VRTerrainBlendSettings) and defines the
POD struct VRTerrainBlendSettings used by C++ code; update all includes in
Globals.cpp, FeatureBuffer.cpp, Feature.cpp, and Deferred.cpp to include this
new header. Then update SharedData.hlsli: add a VRTerrainBlendSettings struct
inside the FeatureData cbuffer (matching field order, types, and size/alignment
of the C++ VRTerrainBlendSettings) so the GPU FeatureData layout matches the C++
push of globals::features::vrTerrainBlend.settings. Ensure names and member
types exactly match between the C++ struct and the HLSL struct.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 142-154: The code creates a sampler with
globals::d3d::device->CreateSamplerState but doesn't check the HRESULT, so
stereoBlendLinearSampler can remain null and then be passed into
context->CSSetSamplers; update the block inside the isOverwriteMode check to
capture the HRESULT from CreateSamplerState, verify SUCCEEDED(hr) before using
stereoBlendLinearSampler, and if creation fails either log the error and skip
calling CSSetSamplers or use a safe fallback sampler; specifically reference the
CreateSamplerState call and guard the subsequent use of stereoBlendLinearSampler
when calling CSSetSamplers (and avoid dereferencing a null sampler).
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 38-66: The POMDepthScale value referenced by DrawSettings is not
persisted; update SaveSettings to write a numeric "POMDepthScale" entry from
settings.pomDepthScale, and update LoadSettings to read "POMDepthScale" (use
auto it = o_json.find("POMDepthScale"); check it != o_json.end() &&
it->is_number()) and assign into settings.pomDepthScale using
std::clamp(it->get<float>(), /*min*/, /*max*/); this ensures the POMDepthScale
used by DrawSettings is saved and restored across sessions.
---
Nitpick comments:
In `@package/Shaders/Common/SharedData.hlsli`:
- Line 55: The field pad0 in SharedData.hlsli is declared as a float1 for
padding which is unclear—change the type of pad0 to uint (or another explicit
4-byte integer like uint32) to make the padding intent and C++/HLSL
compatibility explicit; update any corresponding C++/HLSL interface definitions
that reference SharedData::pad0 so sizes and alignment remain identical and
rebuild/validate constant buffer layout after the change.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 28-34: The duplicated mode constants (MODE_DISOCCLUDED, MODE_EDGE,
MODE_MAIN, MODE_EDGE_NEIGHBOUR, MODE_FULL_BLEND) must be explicitly linked to
their source to avoid drift: update the block in StereoBlendCS.hlsl to add a
clear sync comment that points to the exact symbol source in
VRStereoOptimizations/cbuffers.hlsli (include filename and approximate line
range or commit hash), and if your build system supports it, add a compile-time
check or preprocessor guard that verifies these numeric values match the
definitions in VRStereoOptimizations/cbuffers.hlsli (or document the manual
synchronization step and owner) so future edits stay consistent with
StereoBlendCB.
In `@src/Features/VR/SettingsUI.cpp`:
- Line 102: The overlay and wrap sizes were changed to fixed pixels, which
removes DPI scaling; restore use of the UI scale variable by multiplying
dimensions and wrap positions by the existing scale (e.g., change ImVec2
overlaySize(520, 0) back to using 520 * scale and adjust the related wrap values
such as 500.0f and any other hardcoded sizes on the same code paths); update
occurrences referenced by the symbols ImVec2 overlaySize and the wrap position
variables used at the other locations so the overlay and text wrapping respect
high-DPI scaling.
In `@src/Features/VR/StereoBlend.cpp`:
- Around line 110-120: The savedRTVs/savedDSV retrieval when isOverwriteMode is
true (using context->OMGetRenderTargets) is only to preserve current RTVs so we
can call context->OMSetRenderTargets(..., savedRTVs, nullptr) to unbind the DSV;
the savedRTVs are released immediately afterward, and the array is reused
later—add a concise comment above the OMGetRenderTargets/OMSetRenderTargets
block (referencing savedRTVs, savedDSV, isOverwriteMode,
context->OMGetRenderTargets and context->OMSetRenderTargets) explaining that
this first retrieval only preserves RTV pointers for temporarily unbinding the
DSV and that the pointers are released on purpose to clarify why the array is
reused later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 073e3f6f-f300-4fd7-a769-4dafc1fec52f
📒 Files selected for processing (39)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlslifeatures/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/VR/StereoBlendCS.hlslpackage/Shaders/VR/VRPostProcessCS.hlslpackage/Shaders/VRStereoOptimizations/ReprojectionCS.hlslpackage/Shaders/VRStereoOptimizations/StencilCS.hlslpackage/Shaders/VRStereoOptimizations/StencilWritePS.hlslpackage/Shaders/VRStereoOptimizations/StencilWriteVS.hlslpackage/Shaders/VRStereoOptimizations/cbuffers.hlslisrc/Deferred.cppsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceGI.cppsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cppsrc/Globals.hsrc/State.cpp
✅ Files skipped from review due to trivial changes (9)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- src/State.cpp
- src/Features/ExtendedMaterials.h
- .claude/CLAUDE.md
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- package/Shaders/Common/VR.hlsli
🚧 Files skipped from review as they are similar to previous changes (12)
- src/Features/VR.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- src/Globals.h
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl
- src/Feature.cpp
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- package/Shaders/VR/VRPostProcessCS.hlsl
- src/Features/ScreenSpaceGI.cpp
- package/Shaders/RunGrass.hlsl
Comment on lines
+57
to
+58
| globals::features::exponentialHeightFog.settings, | ||
| globals::features::vrTerrainBlend.settings); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if VRTerrainBlendSettings exists in SharedData.hlsli and FeatureData cbuffer
rg -n 'VRTerrainBlend|vrTerrainBlend' package/Shaders/Common/SharedData.hlsli
# Also check the VRTerrainBlend header for the settings struct
fd -t f 'VRTerrainBlend' src/Features --exec cat {}Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Check the actual FeatureBuffer.cpp file context
head -80 src/FeatureBuffer.cpp | tail -30
# Search more broadly for VRTerrainBlend with case-insensitive matching
rg -i 'vrterrain|vr.*terrain' --type cpp --type h --type hlsl
# Check the structure of SharedData.hlsli to understand cbuffer layout
rg -n 'cbuffer FeatureData' package/Shaders/Common/SharedData.hlsli -A 50Repository: doodlum/skyrim-community-shaders
Length of output: 3133
🏁 Script executed:
# Search for VRTerrainBlendSettings struct definition
rg -i 'struct.*VRTerrainBlendSettings|VRTerrainBlendSettings.*struct'
# Check VRTerrainBlend.h for the settings struct
find src/Features -iname '*vrterrain*' -o -iname '*terrain*blend*' 2>/dev/null
# Verify the complete SharedData.hlsli doesn't have this struct elsewhere
rg 'VRTerrainBlendSettings' package/Shaders/Repository: doodlum/skyrim-community-shaders
Length of output: 139
🏁 Script executed:
# List actual files in src/Features to see what features exist
ls -la src/Features/ | grep -i 'vr\|terrain'
# Search Globals.h for vrTerrainBlend
rg 'vrTerrainBlend' src/Globals.h
# Check if VRTerrainBlend.h exists
find src -name '*VRTerrainBlend*' -o -name '*vrTerrain*'
# Look for vrTerrainBlend in FeatureBuffer.cpp more broadly
rg -n 'vrTerrainBlend' src/FeatureBuffer.cppRepository: doodlum/skyrim-community-shaders
Length of output: 1028
🏁 Script executed:
# Search in VR directory for VRTerrainBlend class
rg 'class VRTerrainBlend|struct VRTerrainBlend' src/Features/VR/
# Check what files are in VR directory
ls -la src/Features/VR/
# Search for VRTerrainBlendSettings in all VR files
rg 'VRTerrainBlendSettings' src/Features/VR/Repository: doodlum/skyrim-community-shaders
Length of output: 721
🏁 Script executed:
# Search Globals.cpp for vrTerrainBlend implementation
rg 'VRTerrainBlend' src/Globals.cpp
# Check entire src directory for any VRTerrainBlend references
rg 'class VRTerrainBlend|VRTerrainBlend vrTerrainBlend' src/
# Look at Globals.h to see the complete declaration
rg -B2 -A2 'extern VRTerrainBlend' src/Globals.hRepository: doodlum/skyrim-community-shaders
Length of output: 415
🏁 Script executed:
# Find VRTerrainBlend.h file with flexible search
find src -iname '*vrterrainblend*' -o -name 'VRTerrainBlend.h'
# Also search with rg for the file content pattern
rg -l 'class VRTerrainBlend' src/
# Check if file exists but with different casing
find src -name '*terrain*blend*' -iname '*vr*'Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search entire repository for VRTerrainBlend.h
find . -name '*VRTerrainBlend*' 2>/dev/null
# Check git for tracked files with VRTerrainBlend
git ls-files | grep -i vrterrainblend
# List all .h files in src/Features
ls -la src/Features/*.h
# Check if the file is simply missing from the working directory
git status | grep -i terrainRepository: doodlum/skyrim-community-shaders
Length of output: 2867
🏁 Script executed:
# Check the actual content of Globals.cpp include statement
rg -B5 -A5 '#include.*VRTerrainBlend' src/Globals.cpp
# Check git status for any new/untracked files
git status -s | head -20
# Check git diff to see what files are being added/modified
git diff --name-status --cached | grep -i terrain
# Look for any pending additions in git
git diff --name-only --cachedRepository: doodlum/skyrim-community-shaders
Length of output: 563
🏁 Script executed:
# Check git history for VRTerrainBlend.h
git log --all --full-history -- src/Features/VRTerrainBlend.h | head -20
# Check if file is staged for commit
git ls-files --stage | grep -i vrterrain
# Look for any untracked or deleted files related to terrain blending
git status --short
# Check if there's a different branch with the file
git branch -a
# See if the file might be in a different location
find . -name '*.h' -exec grep -l 'class VRTerrainBlend' {} \;Repository: doodlum/skyrim-community-shaders
Length of output: 257
🏁 Script executed:
# Check Globals.cpp for the vrTerrainBlend instantiation
rg -n 'VRTerrainBlend vrTerrainBlend' src/Globals.cpp
# Verify the exact line where it's defined
rg -B2 -A2 'VRTerrainBlend vrTerrainBlend\{\}' src/Globals.cpp
# Check if there are any other references to VRTerrainBlend in the codebase
rg 'VRTerrainBlend' src/ --type cpp --type h | grep -v '^Binary'Repository: doodlum/skyrim-community-shaders
Length of output: 679
🏁 Script executed:
# Read FeatureBuffer.cpp to see the complete context of the change
head -60 src/FeatureBuffer.cpp | tail -20
# Check Deferred.cpp for how vrTerrainBlend is used
rg -B5 -A5 'vrTerrainBlend.DrawVRTerrainBlend' src/Deferred.cpp
# Check if the file might be in a completely different location
find . -type f -name '*.h' | xargs grep -l 'class VRTerrainBlend' 2>/dev/nullRepository: doodlum/skyrim-community-shaders
Length of output: 1420
🏁 Script executed:
# Verify SharedData.hlsli doesn't have VRTerrainBlendSettings
grep -n 'VRTerrainBlendSettings\|vrTerrainBlend' package/Shaders/Common/SharedData.hlsli
# Check the actual cbuffer structure in SharedData.hlsli one more time
rg -n 'cbuffer FeatureData' package/Shaders/Common/SharedData.hlsli -A 25Repository: doodlum/skyrim-community-shaders
Length of output: 1199
VRTerrainBlend.h is missing from src/Features/ — this will cause compilation failure.
The file is included in Globals.cpp, FeatureBuffer.cpp, Feature.cpp, and Deferred.cpp but does not exist. Additionally, the HLSL-side VRTerrainBlendSettings struct is missing from the cbuffer FeatureData in SharedData.hlsli (lines 264-282). The C++ code pushes vrTerrainBlend.settings to the feature buffer at line 58, but the HLSL shader has no corresponding struct to receive it, causing a buffer layout mismatch.
Add the missing VRTerrainBlend.h header file with the VRTerrainBlend class and VRTerrainBlendSettings struct definition, and ensure VRTerrainBlendSettings is added to the FeatureData cbuffer in SharedData.hlsli.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FeatureBuffer.cpp` around lines 57 - 58, Add the missing VRTerrainBlend
declaration and matching HLSL struct: create VRTerrainBlend.h that declares the
VRTerrainBlend class (with a public member 'settings' of type
VRTerrainBlendSettings) and defines the POD struct VRTerrainBlendSettings used
by C++ code; update all includes in Globals.cpp, FeatureBuffer.cpp, Feature.cpp,
and Deferred.cpp to include this new header. Then update SharedData.hlsli: add a
VRTerrainBlendSettings struct inside the FeatureData cbuffer (matching field
order, types, and size/alignment of the C++ VRTerrainBlendSettings) so the GPU
FeatureData layout matches the C++ push of
globals::features::vrTerrainBlend.settings. Ensure names and member types
exactly match between the C++ struct and the HLSL struct.
Comment on lines
+142
to
+154
| // Bind linear sampler for hardware bilinear color sampling in overwrite mode | ||
| if (isOverwriteMode) { | ||
| if (!stereoBlendLinearSampler) { | ||
| 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; | ||
| globals::d3d::device->CreateSamplerState(&sampDesc, stereoBlendLinearSampler.put()); | ||
| } | ||
| ID3D11SamplerState* samplers[] = { stereoBlendLinearSampler.get() }; | ||
| context->CSSetSamplers(0, 1, samplers); | ||
| } |
Contributor
There was a problem hiding this comment.
Sampler creation lacks error handling.
CreateSamplerState can fail, but the result is not checked. If creation fails, stereoBlendLinearSampler remains null and samplers[0] will be nullptr.
🛡️ Proposed fix to add error handling
if (isOverwriteMode) {
if (!stereoBlendLinearSampler) {
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;
- globals::d3d::device->CreateSamplerState(&sampDesc, stereoBlendLinearSampler.put());
+ HRESULT hr = globals::d3d::device->CreateSamplerState(&sampDesc, stereoBlendLinearSampler.put());
+ if (FAILED(hr)) {
+ logger::warn("[VR StereoBlend] Failed to create linear sampler (HRESULT: {:`#x`})", static_cast<uint32_t>(hr));
+ }
}
- ID3D11SamplerState* samplers[] = { stereoBlendLinearSampler.get() };
- context->CSSetSamplers(0, 1, samplers);
+ if (stereoBlendLinearSampler) {
+ ID3D11SamplerState* samplers[] = { stereoBlendLinearSampler.get() };
+ context->CSSetSamplers(0, 1, samplers);
+ }
}As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources."
📝 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
| // Bind linear sampler for hardware bilinear color sampling in overwrite mode | |
| if (isOverwriteMode) { | |
| if (!stereoBlendLinearSampler) { | |
| 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; | |
| globals::d3d::device->CreateSamplerState(&sampDesc, stereoBlendLinearSampler.put()); | |
| } | |
| ID3D11SamplerState* samplers[] = { stereoBlendLinearSampler.get() }; | |
| context->CSSetSamplers(0, 1, samplers); | |
| } | |
| // Bind linear sampler for hardware bilinear color sampling in overwrite mode | |
| if (isOverwriteMode) { | |
| if (!stereoBlendLinearSampler) { | |
| 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; | |
| HRESULT hr = globals::d3d::device->CreateSamplerState(&sampDesc, stereoBlendLinearSampler.put()); | |
| if (FAILED(hr)) { | |
| logger::warn("[VR StereoBlend] Failed to create linear sampler (HRESULT: {:`#x`})", static_cast<uint32_t>(hr)); | |
| } | |
| } | |
| if (stereoBlendLinearSampler) { | |
| ID3D11SamplerState* samplers[] = { stereoBlendLinearSampler.get() }; | |
| context->CSSetSamplers(0, 1, samplers); | |
| } | |
| } |
🤖 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 code creates a
sampler with globals::d3d::device->CreateSamplerState but doesn't check the
HRESULT, so stereoBlendLinearSampler can remain null and then be passed into
context->CSSetSamplers; update the block inside the isOverwriteMode check to
capture the HRESULT from CreateSamplerState, verify SUCCEEDED(hr) before using
stereoBlendLinearSampler, and if creation fails either log the error and skip
calling CSSetSamplers or use a safe fallback sampler; specifically reference the
CreateSamplerState call and guard the subsequent use of stereoBlendLinearSampler
when calling CSSetSamplers (and avoid dereferencing a null sampler).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Features/ScreenSpaceGI.cpp (1)
888-892: Consider reusingstereoOptActiveto avoid redundant condition evaluation.
useStereoOptcomputes the same condition asstereoOptActive(line 751). You could reuse the existing variable.♻️ Proposed simplification
- bool useStereoOpt = REL::Module::IsVR() && - globals::features::vrStereoOptimizations.loaded && - globals::features::vrStereoOptimizations.settings.stereoMode != VRStereoOptimizations::StereoMode::Off; - - if (REL::Module::IsVR() && stereoSyncCompute && !useStereoOpt) { + if (REL::Module::IsVR() && stereoSyncCompute && !stereoOptActive) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceGI.cpp` around lines 888 - 892, The code recomputes the same VR/stereo-optimization condition in useStereoOpt that was already computed in stereoOptActive; replace the redundant expression by reusing stereoOptActive (e.g., remove the duplicate computation or set useStereoOpt = stereoOptActive) and update the if that uses useStereoOpt (the if with REL::Module::IsVR() && stereoSyncCompute && !useStereoOpt) to refer to stereoOptActive to avoid duplicated condition evaluation; ensure the variable you reuse (stereoOptActive) is in scope where useStereoOpt was declared.package/Shaders/Common/SharedData.hlsli (1)
1-1: Add explicit issue keyword for dependency traceability.Since this feature depends on
#2002, add a PR description keyword likeAddresses#2002(or `Related to `#2002) for GitHub linkage.As per coding guidelines: “Suggest adding appropriate GitHub keywords … ‘Addresses
#123’ for features.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/SharedData.hlsli` at line 1, Add the GitHub linkage keyword for the dependent feature by inserting a brief comment at the top of SharedData.hlsli (near the existing include guard symbol __SHARED_DATA_DEPENDENCY_HLSL__) that contains "Addresses `#2002`" or "Related to `#2002`" so the PR will link to issue/PR `#2002`; place it immediately above or below the existing `#ifndef` line to make the dependency explicit without changing any logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/Common/SharedData.hlsli`:
- Line 55: The shader constant buffer field pad0 in SharedData.hlsli is declared
as bool but must match the C++ constant buffer mirror (uint pad0 in
ExtendedMaterials.h) for 4-byte size/alignment; change the type of pad0 from
bool to uint in SharedData.hlsli (and verify any other padding fields use uint)
so the HLSL layout matches the C++ struct for proper host/shader interop.
---
Nitpick comments:
In `@package/Shaders/Common/SharedData.hlsli`:
- Line 1: Add the GitHub linkage keyword for the dependent feature by inserting
a brief comment at the top of SharedData.hlsli (near the existing include guard
symbol __SHARED_DATA_DEPENDENCY_HLSL__) that contains "Addresses `#2002`" or
"Related to `#2002`" so the PR will link to issue/PR `#2002`; place it immediately
above or below the existing `#ifndef` line to make the dependency explicit without
changing any logic.
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 888-892: The code recomputes the same VR/stereo-optimization
condition in useStereoOpt that was already computed in stereoOptActive; replace
the redundant expression by reusing stereoOptActive (e.g., remove the duplicate
computation or set useStereoOpt = stereoOptActive) and update the if that uses
useStereoOpt (the if with REL::Module::IsVR() && stereoSyncCompute &&
!useStereoOpt) to refer to stereoOptActive to avoid duplicated condition
evaluation; ensure the variable you reuse (stereoOptActive) is in scope where
useStereoOpt was declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 688b70b4-4477-48cf-9f99-9cb0d2c8c8bb
📒 Files selected for processing (7)
.claude/CLAUDE.mdfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslpackage/Shaders/Common/SharedData.hlslisrc/FeatureBuffer.cppsrc/Features/ScreenSpaceGI.cppsrc/Features/TerrainBlending.cppsrc/Features/VRStereoOptimizations.cpp
✅ Files skipped from review due to trivial changes (2)
- src/Features/TerrainBlending.cpp
- .claude/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/FeatureBuffer.cpp
- src/Features/VRStereoOptimizations.cpp
6 tasks
vrnord
force-pushed
the
pr/vr-ssgi-optimization
branch
from
25b7434 to
e6428e2
Compare
Contributor
Author
|
CodeRabbit feedback addressed in latest push and via PR #2002 base: SSGI temporal outputs now reset to neutral values before VR_STEREO_OPT early-exit (prevents stale outAccumFrames/outRemapped data) ✓ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl (1)
47-49:⚠️ Potential issue | 🟠 MajorClamp to Eye 0 bounds to prevent potential read from Eye 1 region.
The current clamp allows
eye0Px.xup toFrameDim.x - 1, which includes Eye 1 territory[eyeWidth, FrameDim.x - 1]. If reprojection produces edge-case UVs slightly out of Eye 0's range, the clamped coordinate could land in Eye 1, breaking the read-left/write-right guarantee.🔒 Proposed fix
float3 otherEyeUV = Stereo::ConvertMonoUVToOtherEye(float3(monoUV, depth), 1); float2 eye0StereoUV = Stereo::ConvertToStereoUV(otherEyeUV.xy, 0); -int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(FrameDim) - 1); +int2 eye0Px = clamp(int2(eye0StereoUV * FrameDim), int2(0, 0), int2(eyeWidth - 1, eyeHeight - 1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl` around lines 47 - 49, Clamp the computed eye0Px x-coordinate to Eye 0's horizontal bounds so reprojection never reads into Eye 1; after computing eye0StereoUV and before clamping to pixel integers (symbols: Stereo::ConvertToStereoUV, eye0StereoUV, eye0Px, FrameDim), cap the x component to the Eye 0 width (use the existing eyeWidth variable if present, otherwise compute eyeWidth = FrameDim.x / 2) and clamp x to [0, eyeWidth - 1] while clamping y to [0, FrameDim.y - 1].src/Deferred.cpp (1)
373-378:⚠️ Potential issue | 🟠 Major
VR_STEREO_OPTis still compiled for the loaded/off state.Lines 596-597 and 624-625 still build the stereo-opt variant whenever
vrStereoOptimizations.loadedis true, but Lines 375-378 only bind slott16whenstereoMode != Off. That leaves the loaded/off path on a divergent shader contract and keeps the extra branch/register pressure in the inactive path. Either recompile when the mode flips, or bind a neutral mode SRV while the feature stays loaded.As per coding guidelines "Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance".
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 - 378, The code currently leaves the VR stereo optimization shader variant compiled when vrStereoOptimizations.loaded is true even if vrStereoOptimizations.settings.stereoMode == VRStereoOptimizations::StereoMode::Off, causing a divergent shader contract because only CS slot t16 is conditionally bound via modeSRV and context->CSSetShaderResources(16,...). Fix by either: 1) ensure the stereo variant is only compiled/created when globals::features::vrStereoOptimizations.settings.stereoMode != VRStereoOptimizations::StereoMode::Off (so no variant exists when Off), or 2) always bind a neutral/empty SRV to slot 16 (using vrStereoOptimizations.GetModeTextureSRV() fallback or a null/neutral SRV) when the mode is Off so the shader contract remains consistent; update creation/compile code paths that reference vrStereoOptimizations to recompile when mode flips if you choose option 1.
🧹 Nitpick comments (3)
package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl (2)
44-46: Depth threshold constants are reasonable.The
depth >= 1.0check for sky anddepth < 1e-5check for HMD-mask pixels are well-documented in the header comments. If these thresholds are used elsewhere in the VRStereoOptimizations system, consider extracting them tocbuffers.hlslifor consistency. Otherwise, the inline values with comments are acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl` around lines 44 - 46, Extract the magic numbers used in StencilWritePS.hlsl (the checks on DepthTexture[modeCoord] stored in local variable depth and the subsequent discard) into named constants in cbuffers.hlsli (e.g., DEPTH_SKY_THRESHOLD and DEPTH_HMD_MASK_THRESHOLD), include that header in StencilWritePS.hlsl, and replace the inline literals 1.0 and 1e-5 with those constants so other VRStereoOptimizations shaders can reuse the thresholds consistently.
22-26:TexCoordis declared but unused.The shader uses
SV_Position.xydirectly for texture indexing (line 32) and never referencesTexCoord. Consider removing it to keep the input struct minimal.🧹 Suggested cleanup
struct PS_INPUT { float4 Position: SV_Position; - float2 TexCoord: TEXCOORD0; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl` around lines 22 - 26, PS_INPUT currently defines an unused member TexCoord; remove the TexCoord: TEXCOORD0 entry from the PS_INPUT struct and ensure the vertex shader output no longer writes TEXCOORD0 (or update matching semantics) so the pixel shader uses only Position: SV_Position (SV_Position.xy is used for texture indexing) and no unused input semantics remain.package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl (1)
24-29: Consider optimizing dispatch dimensions in the C++ dispatch code.The shader handles over-dispatch correctly via early return, but per the context snippet at
src/Features/VRStereoOptimizations.cpp:609-612, the dispatch covers the full SBS texture while only Eye 1 (right half) needs processing. This results in ~50% of threads being culled immediately.The dispatch should be changed to
(eyeWidth + 7) / 8instead of(fullWidth + 7) / 8in the C++ code to avoid wasted GPU threads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl` around lines 24 - 29, The dispatch is over the full SBS texture causing ~50% early-returned threads; update the C++ dispatch that launches this compute (in the VRStereoOptimizations dispatch code) to use the per-eye width when computing threadgroup counts: compute threadGroupX = (eyeWidth + 7) / 8 (where eyeWidth = FrameDim.x / 2) instead of using the fullFrameWidth formula, and keep threadGroupY = (eyeHeight + 7) / 8; adjust the dispatch call that currently references fullWidth so it passes these reduced thread group counts to the shader that expects uint eyeWidth/eyeHeight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl:
- Around line 350-356: The VR stereo early-return is comparing the raw
StereoOptModeTexture value which may include auxiliary bits; in the
VR_STEREO_OPT block (checking eyeIndex) load the texture value into a local uint
(from StereoOptModeTexture[fullResPx]) and mask it with 0x0F (e.g., mode = mode
& 0x0F) before comparing to 1 or 2 so the low nibble is decoded the same way as
DeferredCompositeCS.hlsl; update the comparisons to use the masked value (still
returning when equal to 1 or 2) to keep this SSGI shader consistent with its
siblings.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 100-107: The early return for reprojected Eye 1 in
DeferredCompositeCS.hlsl (guarded by VR_STEREO_OPT, checking eyeIndex and
StereoOptModeTexture using dispatchID) prevents writes to
NormalTAAMaskSpecularMaskRW, leaving deferred normals/TAA/specular-mask stale
for modes 1–2; fix by either writing appropriate reprojection values into
NormalTAAMaskSpecularMaskRW inside the same branch (mirroring how StereoBlendCS
updates MainRW and MotionVectorsRW) or, if intentional, add a clear explanatory
comment at that return referencing StereoOptModeTexture, StereoBlendCS, and why
downstream passes do not require fresh NormalTAAMaskSpecularMaskRW so reviewers
understand the decision.
In `@package/Shaders/DistantTree.hlsl`:
- Around line 206-213: The VR-specific alpha offset currently uses a hardcoded
0.1; change it to use the shared tuning variable so VR billboards stay in sync:
replace occurrences where you subtract eyeIndex * 0.1 (near AlphaTestRefRS usage
and the second block later) with eyeIndex * SharedData::VRAlphaTestThreshold (or
the equivalent name used for the shared VR alpha threshold) in DistantTree.hlsl
so the alphaRef calculation is driven by the shared VRAlphaTestThreshold rather
than a magic constant.
In `@package/Shaders/VR/VRPostProcessCS.hlsl`:
- Around line 58-61: The proximity computation uses max(FullBlendDistance, 1.0)
which skews the fade for thresholds < 1.0; change the divisor to the actual
FullBlendDistance but guard against zero to avoid division-by-zero (e.g. use
(FullBlendDistance > 0 ? FullBlendDistance : 1.0) or a tiny epsilon). Update the
line computing proximity in the block using linDepth, FullBlendDistance, and
proximity so the saturate(1.0 - linDepth / <guarded FullBlendDistance>) uses the
real threshold value and the visualization reaches zero at the configured
boundary.
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 749-756: The stereo activation check currently uses
REL::Module::IsVR() and vrStereoOptimizations flags but ignores whether
vrStereoOptimizations.GetModeTextureSRV() actually returned a non-null SRV,
causing SSGI to skip legacy stereoSyncCompute while lacking the replacement
mask; update the predicate used where stereoOptActive is computed (the variable
and its uses) to require modeSRV != nullptr as well (or alternatively, if
modeSRV is null, explicitly fall back to invoking the legacy stereoSyncCompute
path) so that both the binding (CSSetShaderResources with modeSRV) and the
decision to bypass stereoSyncCompute are consistent; reference
vrStereoOptimizations, GetModeTextureSRV, modeSRV, and stereoSyncCompute when
making the change.
---
Duplicate comments:
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 47-49: Clamp the computed eye0Px x-coordinate to Eye 0's
horizontal bounds so reprojection never reads into Eye 1; after computing
eye0StereoUV and before clamping to pixel integers (symbols:
Stereo::ConvertToStereoUV, eye0StereoUV, eye0Px, FrameDim), cap the x component
to the Eye 0 width (use the existing eyeWidth variable if present, otherwise
compute eyeWidth = FrameDim.x / 2) and clamp x to [0, eyeWidth - 1] while
clamping y to [0, FrameDim.y - 1].
In `@src/Deferred.cpp`:
- Around line 373-378: The code currently leaves the VR stereo optimization
shader variant compiled when vrStereoOptimizations.loaded is true even if
vrStereoOptimizations.settings.stereoMode ==
VRStereoOptimizations::StereoMode::Off, causing a divergent shader contract
because only CS slot t16 is conditionally bound via modeSRV and
context->CSSetShaderResources(16,...). Fix by either: 1) ensure the stereo
variant is only compiled/created when
globals::features::vrStereoOptimizations.settings.stereoMode !=
VRStereoOptimizations::StereoMode::Off (so no variant exists when Off), or 2)
always bind a neutral/empty SRV to slot 16 (using
vrStereoOptimizations.GetModeTextureSRV() fallback or a null/neutral SRV) when
the mode is Off so the shader contract remains consistent; update
creation/compile code paths that reference vrStereoOptimizations to recompile
when mode flips if you choose option 1.
---
Nitpick comments:
In `@package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl`:
- Around line 24-29: The dispatch is over the full SBS texture causing ~50%
early-returned threads; update the C++ dispatch that launches this compute (in
the VRStereoOptimizations dispatch code) to use the per-eye width when computing
threadgroup counts: compute threadGroupX = (eyeWidth + 7) / 8 (where eyeWidth =
FrameDim.x / 2) instead of using the fullFrameWidth formula, and keep
threadGroupY = (eyeHeight + 7) / 8; adjust the dispatch call that currently
references fullWidth so it passes these reduced thread group counts to the
shader that expects uint eyeWidth/eyeHeight.
In `@package/Shaders/VRStereoOptimizations/StencilWritePS.hlsl`:
- Around line 44-46: Extract the magic numbers used in StencilWritePS.hlsl (the
checks on DepthTexture[modeCoord] stored in local variable depth and the
subsequent discard) into named constants in cbuffers.hlsli (e.g.,
DEPTH_SKY_THRESHOLD and DEPTH_HMD_MASK_THRESHOLD), include that header in
StencilWritePS.hlsl, and replace the inline literals 1.0 and 1e-5 with those
constants so other VRStereoOptimizations shaders can reuse the thresholds
consistently.
- Around line 22-26: PS_INPUT currently defines an unused member TexCoord;
remove the TexCoord: TEXCOORD0 entry from the PS_INPUT struct and ensure the
vertex shader output no longer writes TEXCOORD0 (or update matching semantics)
so the pixel shader uses only Position: SV_Position (SV_Position.xy is used for
texture indexing) and no unused input semantics remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5eeb238-8477-4274-8951-8d27b17e0998
📒 Files selected for processing (40)
.claude/CLAUDE.mdfeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlslifeatures/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.inipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/VR/StereoBlendCS.hlslpackage/Shaders/VR/VRPostProcessCS.hlslpackage/Shaders/VRStereoOptimizations/ReprojectionCS.hlslpackage/Shaders/VRStereoOptimizations/StencilCS.hlslpackage/Shaders/VRStereoOptimizations/StencilWritePS.hlslpackage/Shaders/VRStereoOptimizations/StencilWriteVS.hlslpackage/Shaders/VRStereoOptimizations/cbuffers.hlslipackage/Shaders/VRStereoOptimizations/modes.hlslisrc/Deferred.cppsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/ExtendedMaterials.hsrc/Features/ScreenSpaceGI.cppsrc/Features/TerrainBlending.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cppsrc/Globals.hsrc/State.cpp
✅ Files skipped from review due to trivial changes (12)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- package/Shaders/VRStereoOptimizations/modes.hlsli
- features/Terrain Blending/Shaders/TerrainBlending/TerrainBlending.hlsli
- src/Globals.h
- src/Features/ExtendedMaterials.h
- package/Shaders/RunGrass.hlsl
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- .claude/CLAUDE.md
- package/Shaders/VRStereoOptimizations/StencilCS.hlsl
- src/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (16)
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- src/FeatureBuffer.cpp
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
- package/Shaders/Common/SharedData.hlsli
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- src/Feature.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- src/Features/TerrainHelper.h
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
- src/Features/TerrainHelper.cpp
- src/Features/VR.cpp
- src/Features/VR/StereoBlend.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Features/VRStereoOptimizations.h
- src/Features/VRStereoOptimizations.cpp
Comment on lines
+206
to
+213
| { | ||
| float alphaRef = AlphaTestRefRS; | ||
| # if defined(VR) | ||
| alphaRef -= eyeIndex * 0.1; | ||
| # endif | ||
| if ((alpha - alphaRef) < 0) { | ||
| discard; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the VR alpha offset tied to the shared threshold.
The hardcoded 0.1 on Lines 209 and 225 bypasses the shared VR foliage alpha-threshold tuning. If SharedData::VRAlphaTestThreshold is adjusted, distant-tree billboards will drift out of sync with the rest of the foliage path in VR.
💡 Suggested change
float alphaRef = AlphaTestRefRS;
# if defined(VR)
- alphaRef -= eyeIndex * 0.1;
+ alphaRef -= eyeIndex * SharedData::VRAlphaTestThreshold;
# endifBased on learnings: VRAlphaTestThreshold is wired into DistantTree.hlsl and RunGrass.hlsl only.
Also applies to: 222-229
🤖 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 - 213, The VR-specific
alpha offset currently uses a hardcoded 0.1; change it to use the shared tuning
variable so VR billboards stay in sync: replace occurrences where you subtract
eyeIndex * 0.1 (near AlphaTestRefRS usage and the second block later) with
eyeIndex * SharedData::VRAlphaTestThreshold (or the equivalent name used for the
shared VR alpha threshold) in DistantTree.hlsl so the alphaRef calculation is
driven by the shared VRAlphaTestThreshold rather than a magic constant.
Comment on lines
+58
to
+61
| if (linDepth < FullBlendDistance) { | ||
| float4 c = ColorTexture[dtid]; | ||
| float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0)); | ||
| OutputRW[dtid] = float4(lerp(c.rgb, float3(0, 1, 1), proximity * 0.4), c.a); |
Contributor
There was a problem hiding this comment.
Normalize debug proximity against the actual threshold value.
max(FullBlendDistance, 1.0) makes the fade non-linear for thresholds below 1.0, so the visualization no longer reaches zero at the configured boundary.
Suggested fix
- float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0));
+ float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1e-5));📝 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
| if (linDepth < FullBlendDistance) { | |
| float4 c = ColorTexture[dtid]; | |
| float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1.0)); | |
| OutputRW[dtid] = float4(lerp(c.rgb, float3(0, 1, 1), proximity * 0.4), c.a); | |
| if (linDepth < FullBlendDistance) { | |
| float4 c = ColorTexture[dtid]; | |
| float proximity = saturate(1.0 - linDepth / max(FullBlendDistance, 1e-5)); | |
| OutputRW[dtid] = float4(lerp(c.rgb, float3(0, 1, 1), proximity * 0.4), c.a); |
🤖 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 58 - 61, The proximity
computation uses max(FullBlendDistance, 1.0) which skews the fade for thresholds
< 1.0; change the divisor to the actual FullBlendDistance but guard against zero
to avoid division-by-zero (e.g. use (FullBlendDistance > 0 ? FullBlendDistance :
1.0) or a tiny epsilon). Update the line computing proximity in the block using
linDepth, FullBlendDistance, and proximity so the saturate(1.0 - linDepth /
<guarded FullBlendDistance>) uses the real threshold value and the visualization
reaches zero at the configured boundary.
ab40cae to
b39b7ff
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
328-328:⚠️ Potential issue | 🟠 MajorRestore the zero=no-parallax
pixelOffsetcontract.Line 328, Line 500, and Line 513 now use
0.5as the default/far target, which makes non-parallax/far pixels look like parallax pixels downstream (Reflectance.w > 0) and breaks VR stereo reprojection classification.💡 Suggested fix
- pixelOffset = 0.5; + pixelOffset = 0.0; ... - pixelOffset = saturate(lerp(parallaxAmount, 0.5, nearBlendToFar)); + pixelOffset = saturate(lerp(parallaxAmount, 0.0, nearBlendToFar)); ... - pixelOffset = 0.5; + pixelOffset = 0.0;Also applies to: 500-500, 513-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli at line 328, The default assignment of pixelOffset was changed to 0.5, breaking the zero=no-parallax contract and causing non-parallax pixels to be misclassified (affecting downstream Reflectance.w logic); locate every assignment of the variable pixelOffset in ExtendedMaterials.hlsli (the three places currently setting pixelOffset = 0.5) and restore the contract by setting pixelOffset = 0.0 so that zero means no parallax/far pixel.src/Deferred.cpp (1)
376-396:⚠️ Potential issue | 🟠 MajorAlways populate
t16when theVR_STEREO_OPTpermutation is active.The composite shaders are compiled with
VR_STEREO_OPTwhenevervrStereoOptimizations.loadedis true, but this block only writes slot 16 when stereo mode is enabled. In the off path,StereoOptModeTextureis whatever some earlier CS left int16, so Eye 1 can still early-out against stale mode data.🧩 Suggested fix
- if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { - ID3D11ShaderResourceView* modeSRV = vrStereoOpt.GetModeTextureSRV(); - if (modeSRV) - context->CSSetShaderResources(16, 1, &modeSRV); - } + if (REL::Module::IsVR() && vrStereoOpt.loaded) { + ID3D11ShaderResourceView* modeSRV = + vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off ? vrStereoOpt.GetModeTextureSRV() : nullptr; + context->CSSetShaderResources(16, 1, &modeSRV); + } @@ - if (REL::Module::IsVR() && vrStereoOpt.loaded && vrStereoOpt.settings.stereoMode != VRStereoOptimizations::StereoMode::Off) { + if (REL::Module::IsVR() && vrStereoOpt.loaded) { ID3D11ShaderResourceView* nullSRV = nullptr; context->CSSetShaderResources(16, 1, &nullSRV); }Also applies to: 591-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 376 - 396, The VR stereo mode SRV (t16) must be explicitly set whenever the VR_STEREO_OPT permutation is used: if globals::features::vrStereoOptimizations.loaded is true, always call context->CSSetShaderResources(16, 1, &modeSRV) with either the real ID3D11ShaderResourceView* from vrStereoOpt.GetModeTextureSRV() or a nullptr to unbind, regardless of vrStereoOpt.settings.stereoMode; update both places around the compute dispatches that select GetComputeMainComposite()/GetComputeMainCompositeInterior() (and the duplicate block later) so t16 is never left holding stale data between dispatches.
🧹 Nitpick comments (1)
features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl (1)
66-75: Dead code:VR_STEREO_OPTearly-return is never executed.Per
ScreenSpaceGI.cpp:
VR_STEREO_OPTdefine is added whenvrStereoOptimizations.loaded(Lines 597-598)stereoSyncComputeis dispatched only when!useStereoOpt(Line 892)These conditions are mutually exclusive. When VR stereo optimization is active, the entire
stereoSyncComputedispatch is skipped, so this early-return path never runs.If this is intentional future-proofing (allowing the shader to work if dispatch conditions change), consider adding a comment. Otherwise, the
VR_STEREO_OPTblock in this file can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl around lines 66 - 75, The VR_STEREO_OPT early-return block in stereoSync.cs.hlsl (the `#if` defined(VR_STEREO_OPT) guard that checks eyeIndex, reads StereoOptModeTexture using FrameDim/uv and calls Passthrough(dtid)) is dead because stereoSyncCompute is never dispatched when VR_STEREO_OPT is defined; remove this entire guarded block to eliminate dead code, or if you intend to keep it for future-proofing, replace it with a short clarifying comment above the `#if` (referencing VR_STEREO_OPT, stereoSyncCompute, Passthrough, StereoOptModeTexture) explaining why the guard remains despite current dispatch logic so reviewers won’t delete it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/Lighting.hlsl`:
- Line 1867: Replace the generic gamma conversions with Skyrim-specific
conversions for the FACEGEN roundtrip: in the expression using
GetFacegenBaseColor (the call that currently reads baseColor.xyz =
Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz),
uv))); swap Color::LinearToGamma -> Color::SkyrimLinearToGamma (or the project’s
Skyrim-specific linearize name) for the input and swap Color::GammaToLinear ->
Color::SkyrimGammaToLinear for the output so the roundtrip uses Skyrim's curve;
make the identical replacement for the other FACEGEN/FACEGEN_RGB_TINT occurrence
that currently uses Color::GammaToLinear/Color::LinearToGamma.
- Line 2255: The env-cubemap base/F0 is being converted with the generic
Color::GammaToLinear which alters legacy Skyrim reflectance; replace the generic
conversion when assigning material.F0 from the envmap sample (the assignment
using material.F0 and envColorBase.rgb) with the Skyrim-specific gamma curve
conversion (use the project’s Skyrim gamma helper instead of
Color::GammaToLinear — e.g. Color::SkyrimGammaToLinear or the existing Skyrim
gamma helper function) and make the identical change for the other occurrence
that converts envmap samples (the similar assignment around the other envmap
sample). Ensure both places use the Skyrim gamma conversion or leave sampling on
the Skyrim curve so legacy reflectance remains unchanged.
In `@package/Shaders/VR/StereoBlendCS.hlsl`:
- Around line 34-44: SampleReprojectedColor currently clamps sampleUV into the
full SBS frame, allowing bilinear taps to cross the center seam; fix by clamping
X into the source-eye subregion instead: compute eyeWidth = frameDim.x * 0.5,
determine eyeIndex based on reprojPx.x (e.g. eyeIndex = reprojPx.x < eyeWidth ?
0 : 1), then build per-eye min/max U: eyeMinU = (eyeIndex*eyeWidth + 0.5) /
texSize.x and eyeMaxU = ((eyeIndex+1)*eyeWidth - 0.5) / texSize.x, keep Y
clamping as before, and replace minUV.x/maxUV.x with these eye bounds before
calling ColorTexture.SampleLevel in SampleReprojectedColor.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 295-357: DispatchStencil can early-return while leaving
texPerPixelMode containing stale data; ensure we clear the UAV and mark the
result invalid before any early return. For each early-return path in
VRStereoOptimizations::DispatchStencil (e.g., after the depthSRV null check and
other pre-checks), clear the per-pixel-mode UAV (texPerPixelMode->uav.get()) to
a neutral mode using the D3D11 context
ClearUnorderedAccessViewUint/ClearUnorderedAccessViewFloat call, and set
stencilActive = false (or flip a new per-frame flag like texPerPixelModeValid =
false) so downstream users (Deferred::DeferredPasses()/VR::DrawStereoBlend) can
gate on validity; conversely, on successful dispatch set texPerPixelModeValid =
true (and keep stencilActive as now) and ensure no stale SRV remains bound when
bailing out. Ensure these changes reference DispatchStencil, texPerPixelMode,
ExecuteStencilWritePass, and stencilActive (or a new validity flag) so behavior
is deterministic per-frame.
- Around line 41-42: In LoadSettings() in VRStereoOptimizations.cpp the
StereoMode deserialization uses o_json.contains("StereoMode") then directly
calls o_json["StereoMode"].get<StereoMode>(), which can throw on malformed JSON;
update the block to validate the JSON type first (e.g., check
o_json["StereoMode"].is_string() or is_number() as appropriate), only call
get<>() after the type check, convert from string/number to the StereoMode enum
as other settings do, and on invalid type fallback to a safe default for
settings.stereoMode and optionally log the bad value; reference symbols:
LoadSettings(), settings.stereoMode, StereoMode, and o_json["StereoMode"].
---
Duplicate comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Line 328: The default assignment of pixelOffset was changed to 0.5, breaking
the zero=no-parallax contract and causing non-parallax pixels to be
misclassified (affecting downstream Reflectance.w logic); locate every
assignment of the variable pixelOffset in ExtendedMaterials.hlsli (the three
places currently setting pixelOffset = 0.5) and restore the contract by setting
pixelOffset = 0.0 so that zero means no parallax/far pixel.
In `@src/Deferred.cpp`:
- Around line 376-396: The VR stereo mode SRV (t16) must be explicitly set
whenever the VR_STEREO_OPT permutation is used: if
globals::features::vrStereoOptimizations.loaded is true, always call
context->CSSetShaderResources(16, 1, &modeSRV) with either the real
ID3D11ShaderResourceView* from vrStereoOpt.GetModeTextureSRV() or a nullptr to
unbind, regardless of vrStereoOpt.settings.stereoMode; update both places around
the compute dispatches that select
GetComputeMainComposite()/GetComputeMainCompositeInterior() (and the duplicate
block later) so t16 is never left holding stale data between dispatches.
---
Nitpick comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl:
- Around line 66-75: The VR_STEREO_OPT early-return block in stereoSync.cs.hlsl
(the `#if` defined(VR_STEREO_OPT) guard that checks eyeIndex, reads
StereoOptModeTexture using FrameDim/uv and calls Passthrough(dtid)) is dead
because stereoSyncCompute is never dispatched when VR_STEREO_OPT is defined;
remove this entire guarded block to eliminate dead code, or if you intend to
keep it for future-proofing, replace it with a short clarifying comment above
the `#if` (referencing VR_STEREO_OPT, stereoSyncCompute, Passthrough,
StereoOptModeTexture) explaining why the guard remains despite current dispatch
logic so reviewers won’t delete it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2b107f8-268e-4059-8e45-560ac576a917
📒 Files selected for processing (34)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.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/ScreenSpaceGI.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cppsrc/Globals.hsrc/State.cpp
✅ Files skipped from review due to trivial changes (9)
- features/VR Stereo Optimizations/Shaders/Features/VRStereoOptimizations.ini
- package/Shaders/Common/VR.hlsli
- src/State.cpp
- src/Features/ExtendedMaterials.h
- package/Shaders/RunGrass.hlsl
- package/Shaders/VRStereoOptimizations/modes.hlsli
- package/Shaders/VRStereoOptimizations/StencilWriteVS.hlsl
- package/Shaders/DistantTree.hlsl
- package/Shaders/VRStereoOptimizations/cbuffers.hlsli
🚧 Files skipped from review as they are similar to previous changes (7)
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- src/Globals.h
- src/Feature.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- package/Shaders/DeferredCompositeCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- package/Shaders/VRStereoOptimizations/ReprojectionCS.hlsl
| baseColor.xyz = GetFacegenBaseColor(baseColor.xyz, uv); | ||
| } else { | ||
| baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv)); | ||
| baseColor.xyz = Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz), uv)); |
Contributor
There was a problem hiding this comment.
Use Skyrim gamma conversion for FACEGEN linear-lighting roundtrip.
Line 1867 and Line 1873 switch FACEGEN/FACEGEN_RGB_TINT from Skyrim gamma conversions to generic gamma conversions, which changes the curve and can shift skin tint/brightness.
💡 Suggested fix
- baseColor.xyz = Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz), uv));
+ baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv));
...
- baseColor.xyz = Color::GammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToGamma(baseColor.xyz), uv));
+ baseColor.xyz = Color::SkyrimGammaToLinear(GetFacegenRGBTintBaseColor(Color::LinearToSkyrimGamma(baseColor.xyz), uv));Also applies to: 1873-1873
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Lighting.hlsl` at line 1867, Replace the generic gamma
conversions with Skyrim-specific conversions for the FACEGEN roundtrip: in the
expression using GetFacegenBaseColor (the call that currently reads
baseColor.xyz =
Color::GammaToLinear(GetFacegenBaseColor(Color::LinearToGamma(baseColor.xyz),
uv))); swap Color::LinearToGamma -> Color::SkyrimLinearToGamma (or the project’s
Skyrim-specific linearize name) for the input and swap Color::GammaToLinear ->
Color::SkyrimGammaToLinear for the output so the roundtrip uses Skyrim's curve;
make the identical replacement for the other FACEGEN/FACEGEN_RGB_TINT occurrence
that currently uses Color::GammaToLinear/Color::LinearToGamma.
|
|
||
| if (envColorBase.a < 1.0) { | ||
| material.F0 = Color::SkyrimGammaToLinear(envColorBase.rgb); | ||
| material.F0 = Color::GammaToLinear(envColorBase.rgb); |
Contributor
There was a problem hiding this comment.
Keep env-cubemap F0/base sampling on Skyrim gamma curve.
Line 2255 and Line 2279 now convert envmap samples with generic gamma helpers; this can alter reflectance intensity for legacy envmap content.
💡 Suggested fix
- material.F0 = Color::GammaToLinear(envColorBase.rgb);
+ material.F0 = Color::SkyrimGammaToLinear(envColorBase.rgb);
...
- float3 envColorBase = Color::GammaToLinear(TexEnvSampler.Sample(SampEnvSampler, envSamplingPoint).xyz);
+ float3 envColorBase = Color::SkyrimGammaToLinear(TexEnvSampler.Sample(SampEnvSampler, envSamplingPoint).xyz);Also applies to: 2279-2279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Lighting.hlsl` at line 2255, The env-cubemap base/F0 is being
converted with the generic Color::GammaToLinear which alters legacy Skyrim
reflectance; replace the generic conversion when assigning material.F0 from the
envmap sample (the assignment using material.F0 and envColorBase.rgb) with the
Skyrim-specific gamma curve conversion (use the project’s Skyrim gamma helper
instead of Color::GammaToLinear — e.g. Color::SkyrimGammaToLinear or the
existing Skyrim gamma helper function) and make the identical change for the
other occurrence that converts envmap samples (the similar assignment around the
other envmap sample). Ensure both places use the Skyrim gamma conversion or
leave sampling on the Skyrim curve so legacy reflectance remains unchanged.
Comment on lines
+41
to
+42
| if (o_json.contains("StereoMode")) | ||
| settings.stereoMode = o_json["StereoMode"].get<StereoMode>(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file to understand the validation context
wc -l src/Features/VRStereoOptimizations.cppRepository: doodlum/skyrim-community-shaders
Length of output: 118
🏁 Script executed:
# Read the LoadSettings method around lines 41-42 and surrounding context
sed -n '1,100p' src/Features/VRStereoOptimizations.cpp | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 5381
🏁 Script executed:
# Check what other JSON field validations look like in this file
rg "o_json\.contains|get<" src/Features/VRStereoOptimizations.cpp -A 2 -B 2Repository: doodlum/skyrim-community-shaders
Length of output: 2230
🏁 Script executed:
# Verify the StereoMode enum definition and how it's used
rg "enum.*StereoMode|StereoMode\s*:" src/Features/VRStereoOptimizations.cpp -B 2 -A 5Repository: doodlum/skyrim-community-shaders
Length of output: 1277
🏁 Script executed:
# Check if nlohmann::json is used and understand its behavior
fd "\.h$" src/Features/VRStereoOptimizations | head -1 | xargs catRepository: doodlum/skyrim-community-shaders
Length of output: 197
Add type validation for StereoMode JSON deserialization to match the pattern used for all other settings.
Lines 41–42 skip the type-check pattern used everywhere else in LoadSettings(). The contains() call only proves the key exists; malformed JSON (e.g., a number instead of a string) will still throw during get<StereoMode>() deserialization. All other fields validate type via is_number(), is_boolean(), etc. before calling get<>().
Suggested fix
- if (o_json.contains("StereoMode"))
- settings.stereoMode = o_json["StereoMode"].get<StereoMode>();
+ if (auto it = o_json.find("StereoMode"); it != o_json.end() && it->is_string())
+ settings.stereoMode = it->get<StereoMode>();Per coding guideline: "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/VRStereoOptimizations.cpp` around lines 41 - 42, In
LoadSettings() in VRStereoOptimizations.cpp the StereoMode deserialization uses
o_json.contains("StereoMode") then directly calls
o_json["StereoMode"].get<StereoMode>(), which can throw on malformed JSON;
update the block to validate the JSON type first (e.g., check
o_json["StereoMode"].is_string() or is_number() as appropriate), only call
get<>() after the type check, convert from string/number to the StereoMode enum
as other settings do, and on invalid type fallback to a safe default for
settings.stereoMode and optionally log the bad value; reference symbols:
LoadSettings(), settings.stereoMode, StereoMode, and o_json["StereoMode"].
Comment on lines
+295
to
+357
| void VRStereoOptimizations::DispatchStencil() | ||
| { | ||
| if (!REL::Module::IsVR()) | ||
| return; | ||
| if (settings.stereoMode == StereoMode::Off) | ||
| return; | ||
| if (!stencilCS || !stencilWriteVS || !stencilWritePS || !texPerPixelMode || !paramsCB || | ||
| !stencilWriteReadOnlyDSV || !stencilWriteDSS || !stencilWriteRS) | ||
| return; | ||
|
|
||
| ZoneScoped; | ||
| TracyD3D11Zone(globals::state->tracyCtx, "VR Stereo Opt - Stencil"); | ||
|
|
||
| if (globals::state->frameAnnotations) | ||
| globals::state->BeginPerfEvent("VR Stereo Opt - Stencil"); | ||
|
|
||
| auto context = globals::d3d::context; | ||
|
|
||
| UpdateConstantBuffer(); | ||
| auto cbPtr = paramsCB->CB(); | ||
| // Use live depth buffer (kMAIN) instead of kPOST_ZPREPASS_COPY — at StartDeferred time, | ||
| // kPOST_ZPREPASS_COPY is stale (previous frame). kMAIN has fresh z-prepass depth so | ||
| // StencilCS can correctly detect sky-vs-geometry edges in the current frame. | ||
| auto renderer = globals::game::renderer; | ||
| auto* depthSRV = renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGETS_DEPTHSTENCIL::kMAIN].depthSRV; | ||
| if (!depthSRV) { | ||
| logger::warn("[VRStereoOptimizations] DispatchStencil: depthSRV is null, skipping"); | ||
| if (globals::state->frameAnnotations) | ||
| globals::state->EndPerfEvent(); | ||
| return; | ||
| } | ||
|
|
||
| // Dispatch classification CS over Eye 1 region | ||
| // Input: t0 = depth, b1 = params CB | ||
| // Output: u0 = per-pixel mode texture | ||
| { | ||
| ID3D11ShaderResourceView* srvs[1]{ depthSRV }; | ||
| ID3D11UnorderedAccessView* uavs[1]{ texPerPixelMode->uav.get() }; | ||
|
|
||
| context->CSSetConstantBuffers(1, 1, &cbPtr); | ||
| context->CSSetShaderResources(0, 1, srvs); | ||
| context->CSSetUnorderedAccessViews(0, 1, uavs, nullptr); | ||
| auto* activeStencilCS = (settings.debugDepthMap && stencilDebugDepthMapCS) ? stencilDebugDepthMapCS.get() : stencilCS.get(); | ||
| context->CSSetShader(activeStencilCS, nullptr, 0); | ||
|
|
||
| uint32_t fullWidth = texPerPixelMode->desc.Width; | ||
| uint32_t fullHeight = texPerPixelMode->desc.Height; | ||
| context->Dispatch((fullWidth + 7) / 8, (fullHeight + 7) / 8, 1); | ||
|
|
||
| // Cleanup CS bindings | ||
| ID3D11ShaderResourceView* nullSRV = nullptr; | ||
| ID3D11UnorderedAccessView* nullUAV = nullptr; | ||
| ID3D11Buffer* nullCB = nullptr; | ||
| context->CSSetShaderResources(0, 1, &nullSRV); | ||
| context->CSSetUnorderedAccessViews(0, 1, &nullUAV, nullptr); | ||
| context->CSSetConstantBuffers(1, 1, &nullCB); | ||
| context->CSSetShader(nullptr, nullptr, 0); | ||
| } | ||
|
|
||
| // Transfer classification to hardware stencil buffer | ||
| ExecuteStencilWritePass(); | ||
|
|
||
| stencilActive = true; |
Contributor
There was a problem hiding this comment.
Don't leave the previous mode texture live after a stencil-dispatch bailout.
Every early return here leaves texPerPixelMode untouched. On the first failed frame that can be undefined contents; on later frames it's last frame's classification, while Deferred::DeferredPasses() and VR::DrawStereoBlend() still consume the SRV whenever the feature is loaded. Clear the UAV to a neutral mode or gate downstream consumers on a per-frame validity flag.
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/VRStereoOptimizations.cpp` around lines 295 - 357,
DispatchStencil can early-return while leaving texPerPixelMode containing stale
data; ensure we clear the UAV and mark the result invalid before any early
return. For each early-return path in VRStereoOptimizations::DispatchStencil
(e.g., after the depthSRV null check and other pre-checks), clear the
per-pixel-mode UAV (texPerPixelMode->uav.get()) to a neutral mode using the
D3D11 context ClearUnorderedAccessViewUint/ClearUnorderedAccessViewFloat call,
and set stencilActive = false (or flip a new per-frame flag like
texPerPixelModeValid = false) so downstream users
(Deferred::DeferredPasses()/VR::DrawStereoBlend) can gate on validity;
conversely, on successful dispatch set texPerPixelModeValid = true (and keep
stencilActive as now) and ensure no stale SRV remains bound when bailing out.
Ensure these changes reference DispatchStencil, texPerPixelMode,
ExecuteStencilWritePass, and stencilActive (or a new validity flag) so behavior
is deterministic per-frame.
3d09d35 to
9a9e5e5
Compare
|
✅ A pre-release build is available for this PR: |
vrnord
force-pushed
the
pr/vr-ssgi-optimization
branch
5 times, most recently
from
1e71aae to
fc923f6
Compare
Collaborator
|
Taking to draft until dependency merged. |
vrnord
force-pushed
the
pr/vr-ssgi-optimization
branch
from
fea7189 to
1bdda28
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>
Bypass stereoSync when VRStereoOptimizations active to prevent eye 0 contamination from sparse eye 1 data (saves 0.1-0.3ms). Mode texture early-out in compute passes for culled eye 1 pixels. Depends on: feat(vr): VR stereo reprojection optimizations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vrnord
force-pushed
the
pr/vr-ssgi-optimization
branch
2 times, most recently
from
99c981f to
5c590cd
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bypasses redundant SSGI work when VR stereo optimizations are active.
Depends on: #2002 (VR stereo reprojection optimizations)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements