feat: stochastic screen space reflections#1843
Conversation
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
📝 WalkthroughWalkthroughAdds a new Stochastic Screen Space Reflections (SSSR) feature: shared shader utilities, multiple compute shaders (preprocess, raymarch, temporal, variance, spatial), C++ feature class with settings/resources, and integration into deferred composition and global feature registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Frame as Frame Input (color, depth, motion)
participant Prepass as Preprocess / Hi-Z
participant Raymarch as Raymarch CS
participant Temporal as Temporal CS
participant Variance as Variance CS
participant Spatial as Spatial (SVGF) CS
participant Composite as Deferred Composite
participant Output as Final Frame
Frame->>Prepass: depth + color -> build Hi-Z / downsample
Prepass->>Raymarch: Hi-Z mips, prepared color, normals, motion
Raymarch->>Raymarch: hierarchical ray traversal, hit tests
Raymarch->>Temporal: SSR color + hit PDF -> history reprojection
Temporal->>Variance: blended color + moments
Variance->>Spatial: variance + color
Spatial->>Composite: filtered SSR color
Composite->>Output: apply SSSR override (if enabled) -> final irradiance
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)
58-77:⚠️ Potential issue | 🔴 CriticalCritical:
ilis never assigned whenSSSRis defined butEnableSpecularis false.When the shader is compiled with
SSSRdefined, the#elsebranch (lines 64–76) containing the SSGI specular computation is excluded. IfEnableSpecularisfalseat runtime, theifblock is skipped and theoutparameterilis never written — leaving it undefined. This silently drops SSGI specular contribution whenever the SSSR feature is loaded, regardless of whether SSR specular is actually enabled.The fix should use a runtime fallback rather than a compile-time
#else:Proposed fix
# if defined(SSSR) if (SharedData::sssrSettings.EnableSpecular) { il = 0; return; } -# else +# endif float4 ssgiIlYSh = SsgiYTexture[pixCoord]; float ssgiIlY = SphericalHarmonics::FuncProductIntegral(ssgiIlYSh, lobe); float2 ssgiIlCoCg = SsgiCoCgTexture[pixCoord].xy; // pi to compensate for the /pi in specularLobe // i don't think there really should be a 1/PI but without it the specular is too strong // reflectance being ambient reflectance doesn't help either il = max(0, Color::YCoCgToRGB(float3(ssgiIlY, ssgiIlCoCg / Math::PI))); // HQ spec float4 hq_spec = SsgiSpecularTexture[pixCoord]; ao *= 1 - hq_spec.a; il += hq_spec.rgb; -# endif }
🤖 Fix all issues with AI agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl:
- Around line 489-498: The local variable occlusion is used uninitialized when
valid_hit is false (confidence becomes 0 and later code reads occlusion);
initialize occlusion to a safe default before it's used or ensure the ternary
assigns it in both branches. Specifically, in the block where occlusion is
declared (the float occlusion declaration) set it to 0 (e.g., float occlusion =
0;) or change the conditional assignment using SSSR_ValidateHit so the false
branch also sets occlusion to 0; this ensures SSSR_ValidateHit, confidence,
valid_hit, and subsequent dynamic-cubemap fallback code all see a defined
occlusion value.
- Around line 564-568: The non-specular DYNAMIC_CUBEMAPS path uses an undeclared
albedo when calling Color::MultiBounceAO (envColor *= multiBounceAO), causing
compile failures; either fetch/declare the surface albedo from the GBuffer
(where other permutations read it) before line 566 and use that variable, or
wrap the whole MultiBounceAO call and envColor multiplication in the same
SSSR_SPECULAR guard so that MultiBounceAO(albedo, ao) is only compiled when
albedo exists — update the shader so the identifier albedo is either
declared/populated (matching the GBuffer read used elsewhere) or removed/guarded
to avoid referencing it in non-specular permutations.
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_spatial.hlsl:
- Around line 74-122: The shader currently leaves blendedColor at (0,0,0) and
always writes alpha=1.0, causing sky/invalid pixels (where depthCenter <= 0) to
be marked as valid; update the logic around depthCenter and FilteredOutput so
that when depthCenter <= 0 you set the output alpha to 0 (or otherwise mark
invalid) rather than 1.0 and ensure blendedColor is set to a sensible fallback
(e.g., ssrColor.rgb or leave unchanged) only for valid depths; look for symbols
depthCenter, blendedColor, ssrColor, and FilteredOutput[DTid.xy] and change the
final write to conditionally set alpha based on depthCenter (or assign a
validity flag) so sky pixels are written with alpha = 0.0.
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_temporal.hlsl:
- Around line 56-60: The current moment scaling is inconsistent: curMoment is
multiplied by 0.5 but prevMoment (computed from prevColor's luminance) is not,
which corrupts variance blending; fix by making both moments use the same
scale—either remove the "* 0.5" from curMoment or multiply prevMoment by 0.5
when computed from prevColor so curMoment and prevMoment are consistently scaled
before they are blended (references: curMoment, prevMoment, prevColor,
luminance).
- Around line 151-168: The code recomputes prevMoment from prevColor's
luminance, discarding accumulated history; replace that recomputation and use
the previously-read accumulated prevMoments as the source for the lerp.
Specifically, in the valid branch set prevMoment = prevMoments.xy (or the
appropriate components of prevMoments read earlier) instead of computing it from
prevColor, then continue to lerp(prevMoment, curMoment, momentAlpha), compute
variance, and write MomentsOutput/FilteredOutput as before so temporal moment
accumulation is preserved.
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_variance.hlsl:
- Around line 76-78: The variance computation uses "variance *= 2 / history"
which can divide by zero when MomentsTexture[DTid.xy].z (history) is 0; update
the code around momentsSum, history and VarianceOutput to guard against zero
history by using a safeHistory = max(1, history) or conditional (if history > 0)
before performing the division, so variance is computed with 2/safeHistory (or
skipped when history==0) to avoid inf/NaN propagation.
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 288-293: The current code uses a hard binary swap via
any(ssrIrradiance.rgb > 0) which creates visible seams and discards valid dark
SSR results; update the blend to use the SSR alpha/confidence instead: when
SharedData::sssrSettings.EnableSpecular is true, sample ssrIrradiance =
SSSRexture[dispatchID.xy] and compute a lerp between ssrIrradiance.rgb and
finalIrradiance using ssrIrradiance.a (or a confidence mask derived from the
denoiser/variance) as the weight so that low-confidence SSR smoothly fades to
the cubemap while preserving legitimately dark reflections (refer to SSSR,
SSSRexture, ssrIrradiance, finalIrradiance, dispatchID).
- Around line 90-92: The declaration and uses of the SSSR texture are misspelled
as SSSRexture; rename the symbol to SSSRTexture in the Texture2D declaration
(currently "SSSRexture : register(t16)") and update every reference/usage of
SSSRexture to SSSRTexture (preserve the register binding register(t16)). Ensure
all shader reads/writes use the new SSSRTexture identifier so references
compile.
In `@src/Features/StochasticScreenSpaceReflections.cpp`:
- Around line 228-244: The ClearShaderCache function fails to reset
spatialSpecularCS leaving it stale; update the static vector in
StochasticScreenSpaceReflections::ClearShaderCache to include &spatialSpecularCS
alongside &raymarchSpecularCS, &prepareColorCS, &preprocessDepthCS,
&depthDownsampleCS, &temporalCS, &varianceCS, &spatialCS so all eight compute
shader com_ptrs are set to nullptr before calling CompileComputeShaders().
In `@src/Features/StochasticScreenSpaceReflections.h`:
- Around line 39-56: The Settings struct's MaxSteps slider is missing
ImGuiSliderFlags_AlwaysClamp and LoadSettings() does not validate deserialized
values, allowing out-of-range values (e.g., MaxSteps=999999) to be used; update
the UI code that creates the MaxSteps slider to add ImGuiSliderFlags_AlwaysClamp
(consistent with MaxMips/MaxAccumulatedFrames/AtrousIterations) and modify
LoadSettings() to clamp incoming numeric fields to safe ranges—specifically
clamp MaxSteps to [1,256], MaxMips to [1, maxMips] (use your runtime maxMips
variable), MaxAccumulatedFrames to [1,64], and AtrousIterations to [1,5]—apply
these clamps when assigning into Settings (the Settings struct and
LoadSettings() function) so deserialized configs cannot produce unsafe GPU
workloads.
- Line 36: The HasShaderDefine implementation in
StochasticScreenSpaceReflections currently returns true for every
RE::BSShader::Type causing the SSSR define to be injected into unrelated
shaders; change HasShaderDefine(RE::BSShader::Type) to return false (or only
true for the specific shader type that actually uses SSSR if such an enum
exists) so the "SSSR" define is not added to standard shader types—update the
StochasticScreenSpaceReflections::HasShaderDefine method accordingly to prevent
unnecessary shader permutations.
🧹 Nitpick comments (11)
package/Shaders/Common/SharedData.hlsli (1)
241-246: Inconsistent indentation: mixed tabs and spaces.The struct body uses spaces for indentation on most lines but tabs on line 245 (
float2 _padding). The rest of the file consistently uses tabs.🧹 Normalize indentation to tabs
struct SSSRSettings - { - uint EnableSpecular; - float SpecularMult; + { + uint EnableSpecular; + float SpecularMult; float2 _padding; - }; + };src/Features/StochasticScreenSpaceReflections.cpp (2)
126-128:D3D11_RESOURCE_MISC_GENERATE_MIPSis set butMipLevelsis 1 for color textures.Line 127 sets
MipLevels = 1and line 128 addsGENERATE_MIPS. This flag is only useful when there are multiple mip levels. For single-mip textures (texColor, texSSRColor, texHistory, etc.), it adds a minor driver overhead for no benefit. The depth texture later correctly setsMipLevels = maxMips.Proposed fix — move the GENERATE_MIPS flag to just before depth texture creation
texDesc.MipLevels = 1; -texDesc.MiscFlags |= D3D11_RESOURCE_MISC_GENERATE_MIPS; texDesc.Format = DXGI_FORMAT_R16G16B16A16_FLOAT;Then before the depth texture block:
texDesc.Format = srvDesc.Format = uavDesc.Format = DXGI_FORMAT_R32_FLOAT; +texDesc.MiscFlags |= D3D11_RESOURCE_MISC_GENERATE_MIPS; texDesc.MipLevels = maxMips;
334-344: Hi-Z dispatch count can reach zero, silently skipping higher mip levels.
dispatchCount.x >> ieventually becomes 0 for largei, causingDispatch(0, ...)which is a no-op. For typical screen sizes this is fine (e.g., 1920→240 groups, usable through ~8 shifts), but ifmaxMipsis larger than expected this could leave high mips uninitialized. Consider clamping to at least 1:Proposed fix
-context->Dispatch((uint)dispatchCount.x >> i, (uint)dispatchCount.y >> i, 1); +context->Dispatch(max(1u, (uint)dispatchCount.x >> i), max(1u, (uint)dispatchCount.y >> i), 1);features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_spatial.hlsl (2)
62-62:historyColoris read but never used.
HistoryTextureis sampled intohistoryColoron this line, but the variable is never referenced again in the shader. This is a wasted texture fetch on every thread.Proposed fix
- float4 historyColor = HistoryTexture[DTid.xy];
17-46:GaussianBlurcan divide by zero if all neighbors are out of bounds.If
kernelSumremains zero (all samples out of bounds), Line 45 divides by zero. In practice, this is guarded by the early-out inmainat Lines 56–57 ensuring the center pixel is valid, so the center kernel weight always contributes. Still, a defensive guard would be cheap insurance.Optional defensive fix
- return sum / kernelSum; + return kernelSum > 0.f ? sum / kernelSum : 0.f;features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli (3)
255-274: Dead camera-reprojection code inReprojectHit.Lines 258–265 perform a full inverse-project → world-space → previous-clip reprojection to compute
prevScreen, but Line 269 immediately overwritesprevScreenusingthisClip.xy + velocity. The expensive matrix multiplications (twomulcalls, two perspective divides) are never used.Either remove the dead path or clarify the intent (e.g., was the plan to blend camera-reprojected motion with per-pixel velocity?).
Proposed cleanup
void ReprojectHit(Texture2D MotionTexture, SamplerState s, float3 hitUVz, uint eyeIndex, out float2 outPrevUV) { // Camera motion for pixel (in ScreenPos space). float2 thisScreen = (hitUVz.xy - 0.5f) * float2(2.0f, -2.0f); float4 thisClip = float4(thisScreen, hitUVz.z, 1); - float4 thisView = mul(FrameBuffer::CameraProjUnjitteredInverse[eyeIndex], thisClip); - thisView.xyz = thisView.xyz / thisView.w; - float4 thisWorld = mul(FrameBuffer::CameraViewInverse[eyeIndex], float4(thisView.xyz, 1.0f)); - thisWorld.xyz = thisWorld.xyz / thisWorld.w; - float4 prevClip = mul(FrameBuffer::CameraPreviousViewProjUnjittered[eyeIndex], float4(thisWorld.xyz, 1.0f)); - float2 prevScreen = prevClip.xy / prevClip.w; - float2 velocity = MotionTexture.SampleLevel(s, hitUVz.xy * FrameBuffer::DynamicResolutionParams1.xy, 0).xy; - prevScreen = thisClip.xy + velocity * float2(2.f, -2.f); + float2 prevScreen = thisClip.xy + velocity * float2(2.f, -2.f); float2 prevUV = prevScreen.xy * float2(0.5f, -0.5f) + 0.5f; outPrevUV = prevUV; }
242-242: LocalM_PIredefinition instead of usingMath::PI.
SampleGGXVNDFdefines a localconst float M_PI(Line 242) while the rest of the file consistently usesMath::PI. This is a minor inconsistency.- const float M_PI = 3.14159265358979f; - float phi = 2.0 * M_PI * U2; + float phi = 2.0 * Math::PI * U2;
295-311:CalculateWeight: division byphiLwithout zero guard.Line 308 divides by
phiLunconditionally. While the current caller insssr_spatial.hlslensuresphiLuminance >= VAR_EPSILON,CalculateWeightis a shared utility exposed in the common header—a future caller could passphiL = 0. Consider adding the same pattern used forphiDon Line 302.Proposed fix
// Luminance weight - float weightLuminance = abs(luminanceCenter - luminanceP) / phiL; + float weightLuminance = (phiL == 0) ? 0.f : abs(luminanceCenter - luminanceP) / phiL;features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl (3)
64-68:SSSR_FLOAT_MAXis defined in bothsssr_common.hlsli(Line 11) and here (Line 66).This duplicate
#definewill produce a macro redefinition warning. Since the value is identical, remove it from this file and rely on the common header.Proposed fix
`#define` HIZ_MAX_ITERATIONS MaxSteps `#define` HIZ_MIN_MIP 0 -#define SSSR_FLOAT_MAX 3.402823466e+38 `#define` SSSR_DEPTH_HIERARCHY_MAX_MIP MaxMips `#define` SAMPLES_PER_PIXEL 1
423-423: Unused variables:debugandweightsgroupshared array.
debug(Line 423) is declared but never written to or read.weights[64][SAMPLES_PER_PIXEL](Line 390) is populated at Line 468 but never read anywhere in the shader.These are dead code left over from development. Removing them frees a register and 1 KB of groupshared memory.
Also applies to: 466-468
313-331:CreateTBNdivides by zero for degenerate normals where both checked components are zero.If
N = (0, 0, 0)(degenerate), the first branch computesk = sqrt(0) = 0and divides by it. Normalized input prevents this, but amax(k, 1e-8)guard would be cheap insurance given this is a utility function.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)
58-77:⚠️ Potential issue | 🔴 Critical
ilis left uninitialized whenSSSRis compiled in butEnableSpecularisfalseat runtime.The
#if defined(SSSR) …#else`` structure compiles out the entire SSGI specular path (lines 64-76) whenever theSSSRpermutation is active. If `EnableSpecular` is toggled off at runtime, the early-return on line 61 is skipped, but `il` (an `out` parameter) is never assigned — producing undefined/garbage specular lighting.The SSGI specular fallback needs to remain reachable at runtime. One way:
Proposed fix
# if defined(SSSR) if (SharedData::sssrSettings.EnableSpecular) { il = 0; return; } -# else +# endif float4 ssgiIlYSh = SsgiYTexture[pixCoord]; float ssgiIlY = SphericalHarmonics::FuncProductIntegral(ssgiIlYSh, lobe); float2 ssgiIlCoCg = SsgiCoCgTexture[pixCoord].xy; il = max(0, Color::YCoCgToRGB(float3(ssgiIlY, ssgiIlCoCg / Math::PI))); float4 hq_spec = SsgiSpecularTexture[pixCoord]; ao *= 1 - hq_spec.a; il += hq_spec.rgb; -# endif }
🤖 Fix all issues with AI agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl:
- Around line 313-325: CreateTBN currently computes a tangent vector U by
dividing by k which can be zero for degenerate normals (e.g., N ==
float3(0,0,0)), producing NaNs; guard the division by clamping k (e.g., k =
max(k, 1e-6)) before using it in both branches and ensure U is
initialized/normalized afterwards so CreateTBN always returns a valid
orthonormal basis even for bad GBuffer normals.
- Around line 558-566: The AO value (ao) is computed but only applied to
envColor inside the SSSR_SPECULAR branch, so when SSSR_SPECULAR is not defined
diffuse envColor ignores AO; update the shader to apply ao to envColor for the
non-specular path (either move the envColor *= ao out of the `#if`
defined(SSSR_SPECULAR) guard or add an `#else` branch that multiplies envColor by
ao after SsgiAoTexture modification), ensuring the same ao variable (and any
SSGI adjustments) affects envColor before the sampleColor lerp; reference the
variables/envColor, ao, SsgiAoTexture, GetSpecularOcclusionFromAmbientOcclusion,
and the sampleColor assignment to locate the change.
- Around line 401-414: The LocalBRDF call is passing L and N swapped; update
calls that currently pass LocalBRDF(-view_space_ray_direction,
view_space_surface_normal, view_space_reflected_direction, roughness) to instead
pass the reflected (light) direction as L and the surface normal as N, i.e. swap
the second and third arguments so the call becomes
LocalBRDF(-view_space_ray_direction, view_space_reflected_direction,
view_space_surface_normal, roughness); apply the same swap to the other nearby
calls mentioned.
- Line 512: The dot uses camera-to-surface direction but NdotV expects
surface-to-camera: negate the direction before normalizing so NdotV =
saturate(dot(normalize(-view_space_ray), view_space_surface_normal)); update the
expression where NdotV is computed (reference: normalize(view_space_ray),
view_space_surface_normal) so GetSpecularOcclusionFromAmbientOcclusion receives
the correct NdotV value used at its call sites (lines calling
GetSpecularOcclusionFromAmbientOcclusion).
- Around line 47-50: Remove or correctly bind the unused SHARC UAVs declared in
sssr_raymarch.hlsl: u_SharcHashEntriesBuffer, u_HashCopyOffsetBuffer,
u_SharcVoxelDataBuffer, and u_SharcVoxelDataBufferPrev (registers u2–u5). Locate
these declarations in sssr_raymarch.hlsl and either delete them if they are not
referenced anywhere, or update the C++ dispatch (where DrawSSSRSpecular binds
UAVs) to bind the corresponding GPU buffers into the shader so the register
layout matches (ensure the UAV array bound in DrawSSSRSpecular includes u2–u5
and uses the correct buffer resources/names). Ensure no other included HLSL
files expect those registers before removing.
In `@src/Features/StochasticScreenSpaceReflections.cpp`:
- Around line 35-36: The MaxSteps slider (ImGui::SliderInt call that sets
settings.MaxSteps) lacks ImGuiSliderFlags_AlwaysClamp; update that call to pass
ImGuiSliderFlags_AlwaysClamp (like the MaxMips slider) so typed or Ctrl+clicked
values are clamped to the intended [1, 256] range and cannot produce
out-of-range HIZ_MAX_ITERATIONS values.
- Around line 394-396: The UAV array is too small for the shader which expects
u0–u5: update the uavs declaration (currently
std::array<ID3D11UnorderedAccessView*, 2> uavs) to include six entries and
populate it with the SHARC buffer UAVs so that u2–u5 are bound (mirror whatever
SHARC UAV variables you have), then update the raymarch dispatch bind call to
pass all six UAVs (u0..u5) rather than only u0 and u1; alternatively, if you
intend to only use two UAVs, change the shader to remove references to u2–u5.
Ensure you adjust the code that constructs/sets the UAV array (variable name
uavs) and the dispatch binding so shader and runtime UAV counts match.
🧹 Nitpick comments (3)
features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl (1)
96-102: Commented-out code throughout the shader.Multiple blocks of commented-out code remain (Lines 98–101, 337–340, 359, 502–503). These are debug/WIP remnants that reduce readability. Consider removing them before merge.
Also applies to: 337-340, 359-359, 502-503
src/Features/StochasticScreenSpaceReflections.cpp (2)
126-129:D3D11_RESOURCE_MISC_GENERATE_MIPSis set butMipLevels = 1.
GENERATE_MIPShas no effect with a single mip level. The flag is only meaningful fortexDepth(which getsmaxMipsat Line 182). Consider setting this flag only on textures that actually have multiple mip levels to avoid confusion.
514-515:atrousIterationsCB field is overloaded to mean both "total iterations" and "current iteration index".Line 472 sets
denoiserCBData.atrousIterations = settings.AtrousIterations(total count), but Line 515 overwrites it with the loop indexi. If any shader other than spatial reads this field expecting the total, it will get stale data. This works because the CB is re-uploaded each iteration, but it's fragile. Consider using a separate field for the iteration index.
| RWStructuredBuffer<uint2> u_SharcHashEntriesBuffer : register(u2); | ||
| RWStructuredBuffer<uint> u_HashCopyOffsetBuffer : register(u3); | ||
| RWStructuredBuffer<uint4> u_SharcVoxelDataBuffer : register(u4); | ||
| RWStructuredBuffer<uint4> u_SharcVoxelDataBufferPrev : register(u5); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the shader file and check its structure
echo "=== Finding SSSR shader files ==="
fd -t f "sssr_raymarch" --extension hlsl
echo ""
echo "=== Searching for SHARC buffer usage in shader files ==="
rg -n "u_Sharc|u_HashCopy" --type-add 'hlsl:*.hlsl' --type-add 'hlsli:*.hlsli' --type=hlsl --type=hlsli -A 2 -B 2
echo ""
echo "=== Locating DrawSSSRSpecular function in C++ ==="
rg -n "DrawSSSRSpecular" --type cpp --type hppRepository: doodlum/skyrim-community-shaders
Length of output: 1578
🏁 Script executed:
#!/bin/bash
# Read the shader file to see what's included and how buffers are used
echo "=== Reading sssr_raymarch.hlsl to check buffer usage ==="
cat -n "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" | head -80
echo ""
echo "=== Searching for buffer usage within the shader file ==="
rg "u_SharcHashEntriesBuffer|u_HashCopyOffsetBuffer|u_SharcVoxelDataBuffer|u_SharcVoxelDataBufferPrev" "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" -n
echo ""
echo "=== Finding DrawSSSRSpecular in C++ files ==="
rg "DrawSSSRSpecular" -t c -t cpp -t h
echo ""
echo "=== Finding SSSR C++ implementation files ==="
fd -t f "sssr" --extension cpp --extension h | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 4569
🏁 Script executed:
#!/bin/bash
# Check the included sssr_common.hlsli for buffer usage
echo "=== Reading sssr_common.hlsli to check for SHARC buffer usage ==="
fd -t f "sssr_common.hlsli" -exec cat {} \;
echo ""
echo "=== Checking DrawSSSRSpecular C++ implementation ==="
rg -A 50 "void StochasticScreenSpaceReflections::DrawSSSRSpecular" src/Features/StochasticScreenSpaceReflections.cppRepository: doodlum/skyrim-community-shaders
Length of output: 2393
🏁 Script executed:
#!/bin/bash
# Find and read sssr_common.hlsli
echo "=== Reading sssr_common.hlsli ==="
find . -name "sssr_common.hlsli" -type f | xargs cat
echo ""
echo "=== Full DrawSSSRSpecular function (up to 150 lines) ==="
rg -A 150 "void StochasticScreenSpaceReflections::DrawSSSRSpecular" src/Features/StochasticScreenSpaceReflections.cppRepository: doodlum/skyrim-community-shaders
Length of output: 5815
🏁 Script executed:
#!/bin/bash
# List all files in the SSSR shader directory
echo "=== Files in SSSR shader directory ==="
find "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections" -type f
echo ""
echo "=== Check if sssr_common.hlsli exists and its content ==="
ls -la "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli" 2>&1 || echo "File not found"
echo ""
echo "=== Search for any SHARC function calls in shader files ==="
rg "Sharc|SHARC" "features/Stochastic Screen Space Reflections/Shaders/" -i
echo ""
echo "=== Check what shaders are compiled in SSSR ==="
rg "prepareColorCS|raymarchSpecularCS|temporalCS|varianceCS|atrousCS" src/Features/StochasticScreenSpaceReflections.cpp -B 2 -A 2 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 3669
🏁 Script executed:
#!/bin/bash
# Read sssr_common.hlsli
echo "=== Reading sssr_common.hlsli ==="
wc -l "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli"
echo ""
cat -n "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 13903
Remove unused SHARC buffer declarations from sssr_raymarch.hlsl (u2–u5) or bind them in the C++ dispatch.
The shader declares u_SharcHashEntriesBuffer, u_HashCopyOffsetBuffer, u_SharcVoxelDataBuffer, and u_SharcVoxelDataBufferPrev on registers u2–u5, but these buffers are never referenced in the shader code or included files, and DrawSSSRSpecular only binds a 2-element UAV array (u0–u1). Leaving unused register declarations creates confusion about resource binding state. If these buffers are truly unused, remove the declarations; otherwise, properly bind them in the C++ code.
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl around
lines 47 - 50, Remove or correctly bind the unused SHARC UAVs declared in
sssr_raymarch.hlsl: u_SharcHashEntriesBuffer, u_HashCopyOffsetBuffer,
u_SharcVoxelDataBuffer, and u_SharcVoxelDataBufferPrev (registers u2–u5). Locate
these declarations in sssr_raymarch.hlsl and either delete them if they are not
referenced anywhere, or update the C++ dispatch (where DrawSSSRSpecular binds
UAVs) to bind the corresponding GPU buffers into the shader so the register
layout matches (ensure the UAV array bound in DrawSSSRSpecular includes u2–u5
and uses the correct buffer resources/names). Ensure no other included HLSL
files expect those registers before removing.
| float3x3 CreateTBN(float3 N) { | ||
| float3 U; | ||
| if (abs(N.z) > 0.0) { | ||
| float k = sqrt(N.y * N.y + N.z * N.z); | ||
| U.x = 0.0; | ||
| U.y = -N.z / k; | ||
| U.z = N.y / k; | ||
| } else { | ||
| float k = sqrt(N.x * N.x + N.y * N.y); | ||
| U.x = N.y / k; | ||
| U.y = -N.x / k; | ||
| U.z = 0.0; | ||
| } |
There was a problem hiding this comment.
CreateTBN divides by k which can be zero if the input normal is degenerate.
If N is (0, 0, 0) or close to it (e.g., bad GBuffer data), k will be 0 in both branches, producing NaN. A guard like k = max(k, 1e-6) would make this robust.
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl around
lines 313 - 325, CreateTBN currently computes a tangent vector U by dividing by
k which can be zero for degenerate normals (e.g., N == float3(0,0,0)), producing
NaNs; guard the division by clamping k (e.g., k = max(k, 1e-6)) before using it
in both branches and ensure U is initialized/normalized afterwards so CreateTBN
always returns a valid orthonormal basis even for bad GBuffer normals.
| float LocalBRDF(float3 V, float3 L, float3 N, float roughness) { | ||
| #if defined(SSSR_SPECULAR) // D_GGX only | ||
| float3 H = normalize(V + L); | ||
| float NdotL = saturate(dot(N, L)); | ||
| float NdotV = saturate(dot(N, V)); | ||
| float NdotH = saturate(dot(N, H)); | ||
| float D = BRDF::D_GGX(roughness, NdotH); | ||
| float G = BRDF::Vis_SmithJointApprox(roughness, NdotV, NdotL); | ||
| return D * G; | ||
| #else // Lambert | ||
| float NdotL = saturate(dot(N, L)); | ||
| return NdotL * BRDF::Diffuse_Lambert(); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Bug: LocalBRDF called with swapped L and N arguments.
At Line 467, the call is:
LocalBRDF(-view_space_ray_direction, view_space_surface_normal, view_space_reflected_direction, roughness)
which maps to V = -ray_dir, L = normal, N = reflected_dir. The second and third arguments are swapped — L should be the reflected (light) direction and N should be the surface normal.
Proposed fix
- float localWeight = pdf == 0 ? 0 : LocalBRDF(-view_space_ray_direction, view_space_surface_normal, view_space_reflected_direction, roughness) / max(pdf, 1e-4);
+ float localWeight = pdf == 0 ? 0 : LocalBRDF(-view_space_ray_direction, view_space_reflected_direction, view_space_surface_normal, roughness) / max(pdf, 1e-4);Also applies to: 466-468
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl around
lines 401 - 414, The LocalBRDF call is passing L and N swapped; update calls
that currently pass LocalBRDF(-view_space_ray_direction,
view_space_surface_normal, view_space_reflected_direction, roughness) to instead
pass the reflected (light) direction as L and the surface normal as N, i.e. swap
the second and third arguments so the call becomes
LocalBRDF(-view_space_ray_direction, view_space_reflected_direction,
view_space_surface_normal, roughness); apply the same swap to the other nearby
calls mentioned.
| outPDF.xyz += hit * confidence; | ||
| outPDF.w += pdf * confidence; | ||
| } | ||
| const float NdotV = saturate(dot(normalize(view_space_ray), view_space_surface_normal)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the sssr_raymarch.hlsl file
find . -name "sssr_raymarch.hlsl" -type fRepository: doodlum/skyrim-community-shaders
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Read the file around line 512 to understand the context
if [ -f "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" ]; then
echo "=== Context around line 512 (view_space_ray definition and NdotV) ==="
sed -n '480,550p' "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" | cat -n
fiRepository: doodlum/skyrim-community-shaders
Length of output: 5016
🏁 Script executed:
#!/bin/bash
# Check the definition of view_space_ray earlier in the file
if [ -f "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" ]; then
echo "=== Searching for view_space_ray definition ==="
rg -n "view_space_ray\s*=" "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" -A 2
fiRepository: doodlum/skyrim-community-shaders
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Check GetSpecularOcclusionFromAmbientOcclusion function to understand what it expects
if [ -f "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" ]; then
echo "=== Searching for GetSpecularOcclusionFromAmbientOcclusion function ==="
rg -n "GetSpecularOcclusionFromAmbientOcclusion" "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" -B 5 -A 10
fiRepository: doodlum/skyrim-community-shaders
Length of output: 2137
🏁 Script executed:
#!/bin/bash
# Check for NdotV patterns across all shaders to understand the convention
echo "=== NdotV patterns in other shaders ==="
rg -n "NdotV" --type-add 'hlsl:*.hlsl' --type=hlsl -C 2 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 6946
🏁 Script executed:
#!/bin/bash
# Find the GetSpecularOcclusionFromAmbientOcclusion function definition
echo "=== Looking for GetSpecularOcclusionFromAmbientOcclusion definition ==="
rg -n "GetSpecularOcclusionFromAmbientOcclusion" -A 5 --type=hlsl --type=hlsliRepository: doodlum/skyrim-community-shaders
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Find ScreenSpaceToViewSpace function
echo "=== Looking for ScreenSpaceToViewSpace definition ==="
rg -n "ScreenSpaceToViewSpace" -B 2 -A 10 --type=hlsl --type=hlsliRepository: doodlum/skyrim-community-shaders
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Check the header files for Color namespace functions
fd "Color" --type hlsl --type hlsli | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Look for the file structure and find include files
if [ -f "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" ]; then
echo "=== Finding includes and dependencies ==="
head -50 "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl" | grep -E "#include|#define"
fiRepository: doodlum/skyrim-community-shaders
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Search for GetSpecularOcclusionFromAmbientOcclusion without type restriction
echo "=== Looking for GetSpecularOcclusionFromAmbientOcclusion definition ==="
rg -n "GetSpecularOcclusionFromAmbientOcclusion" -A 8 | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 4455
🏁 Script executed:
#!/bin/bash
# Search for ScreenSpaceToViewSpace definition
echo "=== Looking for ScreenSpaceToViewSpace definition ==="
rg -n "ScreenSpaceToViewSpace" -B 1 -A 8 | head -80Repository: doodlum/skyrim-community-shaders
Length of output: 6890
🏁 Script executed:
#!/bin/bash
# Check the sssr_common.hlsli file
if [ -f "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli" ]; then
echo "=== Reading sssr_common.hlsli ==="
cat "features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_common.hlsli"
fiRepository: doodlum/skyrim-community-shaders
Length of output: 11610
🏁 Script executed:
#!/bin/bash
# Search for Color namespace SpecularAOLagarde (similar function to understand NdotV expectation)
echo "=== Looking for SpecularAOLagarde and similar Color functions ==="
rg -n "SpecularAOLagarde\|SpecularOcclusion" -B 2 -A 8 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 141
Negate the camera-to-surface direction to match NdotV convention.
normalize(view_space_ray) gives the camera-to-surface direction, but NdotV conventionally expects surface-to-camera. With the current code, surfaces facing the camera produce negative dot products that saturate to 0, causing GetSpecularOcclusionFromAmbientOcclusion (lines 541, 563) to always receive NdotV ≈ 0 regardless of view angle.
Proposed fix
- const float NdotV = saturate(dot(normalize(view_space_ray), view_space_surface_normal));
+ const float NdotV = saturate(dot(-normalize(view_space_ray), view_space_surface_normal));📝 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.
| const float NdotV = saturate(dot(normalize(view_space_ray), view_space_surface_normal)); | |
| const float NdotV = saturate(dot(-normalize(view_space_ray), view_space_surface_normal)); |
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl at line
512, The dot uses camera-to-surface direction but NdotV expects
surface-to-camera: negate the direction before normalizing so NdotV =
saturate(dot(normalize(-view_space_ray), view_space_surface_normal)); update the
expression where NdotV is computed (reference: normalize(view_space_ray),
view_space_surface_normal) so GetSpecularOcclusionFromAmbientOcclusion receives
the correct NdotV value used at its call sites (lines calling
GetSpecularOcclusionFromAmbientOcclusion).
| float ao = lerp(1.0, occlusion, OcclusionStrength); | ||
| # if defined(SSGI) | ||
| ao *= 1 - saturate(SsgiAoTexture[coords.xy].x); | ||
| # endif | ||
| # if defined(SSSR_SPECULAR) | ||
| ao = GetSpecularOcclusionFromAmbientOcclusion(NdotV, ao, roughness); | ||
| envColor *= ao; | ||
| # endif | ||
| sampleColor.xyz = lerp(envColor, sampleColor.xyz, confidence); |
There was a problem hiding this comment.
AO is computed but never applied to envColor in the non-specular (diffuse) path.
When SSSR_SPECULAR is not defined, ao is calculated at Line 558 (and modified by SSGI at Line 560), but the envColor *= ao multiplication at Line 564 is guarded by #if defined(SSSR_SPECULAR). The diffuse cubemap fallback therefore ignores AO entirely.
If this is intentional (diffuse AO handled elsewhere), consider adding a comment. Otherwise:
Proposed fix
# if defined(SSSR_SPECULAR)
ao = GetSpecularOcclusionFromAmbientOcclusion(NdotV, ao, roughness);
- envColor *= ao;
# endif
+ envColor *= ao;📝 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.
| float ao = lerp(1.0, occlusion, OcclusionStrength); | |
| # if defined(SSGI) | |
| ao *= 1 - saturate(SsgiAoTexture[coords.xy].x); | |
| # endif | |
| # if defined(SSSR_SPECULAR) | |
| ao = GetSpecularOcclusionFromAmbientOcclusion(NdotV, ao, roughness); | |
| envColor *= ao; | |
| # endif | |
| sampleColor.xyz = lerp(envColor, sampleColor.xyz, confidence); | |
| float ao = lerp(1.0, occlusion, OcclusionStrength); | |
| # if defined(SSGI) | |
| ao *= 1 - saturate(SsgiAoTexture[coords.xy].x); | |
| # endif | |
| # if defined(SSSR_SPECULAR) | |
| ao = GetSpecularOcclusionFromAmbientOcclusion(NdotV, ao, roughness); | |
| # endif | |
| envColor *= ao; | |
| sampleColor.xyz = lerp(envColor, sampleColor.xyz, confidence); |
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_raymarch.hlsl around
lines 558 - 566, The AO value (ao) is computed but only applied to envColor
inside the SSSR_SPECULAR branch, so when SSSR_SPECULAR is not defined diffuse
envColor ignores AO; update the shader to apply ao to envColor for the
non-specular path (either move the envColor *= ao out of the `#if`
defined(SSSR_SPECULAR) guard or add an `#else` branch that multiplies envColor by
ao after SsgiAoTexture modification), ensuring the same ao variable (and any
SSGI adjustments) affects envColor before the sampleColor lerp; reference the
variables/envColor, ao, SsgiAoTexture, GetSpecularOcclusionFromAmbientOcclusion,
and the sampleColor assignment to locate the change.
| ImGui::SliderInt("Max Steps", (int*)&settings.MaxSteps, 1, 256); | ||
| ImGui::SliderInt("Max Mip Level", (int*)&settings.MaxMips, 1, maxMips, "%d", ImGuiSliderFlags_AlwaysClamp); |
There was a problem hiding this comment.
MaxSteps slider is missing ImGuiSliderFlags_AlwaysClamp.
MaxMips (Line 36) and other integer sliders correctly use AlwaysClamp, but MaxSteps does not. A user could Ctrl+Click and type a value outside [1, 256], which would be passed directly to the GPU as HIZ_MAX_ITERATIONS — potentially causing very long or zero-iteration raymarches.
Proposed fix
- ImGui::SliderInt("Max Steps", (int*)&settings.MaxSteps, 1, 256);
+ ImGui::SliderInt("Max Steps", (int*)&settings.MaxSteps, 1, 256, "%d", ImGuiSliderFlags_AlwaysClamp);🤖 Prompt for AI Agents
In `@src/Features/StochasticScreenSpaceReflections.cpp` around lines 35 - 36, The
MaxSteps slider (ImGui::SliderInt call that sets settings.MaxSteps) lacks
ImGuiSliderFlags_AlwaysClamp; update that call to pass
ImGuiSliderFlags_AlwaysClamp (like the MaxMips slider) so typed or Ctrl+clicked
values are clamped to the intended [1, 256] range and cannot produce
out-of-range HIZ_MAX_ITERATIONS values.
| std::array<ID3D11ShaderResourceView*, 12> srvs = { nullptr }; | ||
| std::array<ID3D11UnorderedAccessView*, 2> uavs = { nullptr }; | ||
|
|
There was a problem hiding this comment.
UAV array sized to 2 but shader declares u0–u5.
The raymarch dispatch binds only u0 and u1 (Lines 439–440), leaving the SHARC buffers declared at u2–u5 in the shader unbound. See the corresponding shader-side comment for details.
Also applies to: 439-460
🤖 Prompt for AI Agents
In `@src/Features/StochasticScreenSpaceReflections.cpp` around lines 394 - 396,
The UAV array is too small for the shader which expects u0–u5: update the uavs
declaration (currently std::array<ID3D11UnorderedAccessView*, 2> uavs) to
include six entries and populate it with the SHARC buffer UAVs so that u2–u5 are
bound (mirror whatever SHARC UAV variables you have), then update the raymarch
dispatch bind call to pass all six UAVs (u0..u5) rather than only u0 and u1;
alternatively, if you intend to only use two UAVs, change the shader to remove
references to u2–u5. Ensure you adjust the code that constructs/sets the UAV
array (variable name uavs) and the dispatch binding so shader and runtime UAV
counts match.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_variance.hlsl:
- Around line 26-29: The current code writes ssrColor directly to VarianceOutput
for history > 2, leaving an invalid variance; fix by always reading momentsSum
from MomentsTexture (use MomentsTexture[DTid.xy]), compute variance =
momentsSum.y - (momentsSum.x * momentsSum.x) and write VarianceOutput[DTid.xy] =
float4(blendedColor, variance) before the existing conditional so both the
history > 2 and history <= 2 paths use a valid variance value consistent with
the lower-history computation.
| float4 ssrColor = SSRColorTexture[DTid.xy]; | ||
| float3 blendedColor = ssrColor.xyz; | ||
| float depthCenter = DepthTexture[DTid.xy]; | ||
| VarianceOutput[DTid.xy] = ssrColor; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the temporal pass shader file
fd -e hlsl -e hlsli | grep -i 'temporal'Repository: doodlum/skyrim-community-shaders
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Read the variance shader context around the flagged lines
fd -path '*/sssr_variance.hlsl' -exec cat {} \;Repository: doodlum/skyrim-community-shaders
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Search for writes to .w or .a in temporal pass
rg -n '\.w\s*=' --type-add 'hlsl:*.hlsl' -g '*temporal*' -C2Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Read the temporal shader
cat -n 'features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_temporal.hlsl'Repository: doodlum/skyrim-community-shaders
Length of output: 7036
🏁 Script executed:
#!/bin/bash
# Read the variance shader
cat -n 'features/Stochastic Screen Space Reflections/Shaders/StochasticScreenSpaceReflections/sssr_variance.hlsl'Repository: doodlum/skyrim-community-shaders
Length of output: 3548
Write valid variance to VarianceOutput when history > 2.
Lines 29–79 contain a logic error: when history exceeds 2, the function returns after line 29, writing ssrColor (with its unrelated .w channel) directly to VarianceOutput without any variance computation. The conditional block (lines 33–79) only executes when history ≤ 2, leaving the high-history case with invalid variance data.
Compute variance from moments even when history > 2. For consistency with the history <= 2 path, compute variance = momentsSum.y - (momentsSum.x * momentsSum.x) from MomentsTexture and write float4(blendedColor, variance) to VarianceOutput before the conditional.
🤖 Prompt for AI Agents
In `@features/Stochastic` Screen Space
Reflections/Shaders/StochasticScreenSpaceReflections/sssr_variance.hlsl around
lines 26 - 29, The current code writes ssrColor directly to VarianceOutput for
history > 2, leaving an invalid variance; fix by always reading momentsSum from
MomentsTexture (use MomentsTexture[DTid.xy]), compute variance = momentsSum.y -
(momentsSum.x * momentsSum.x) and write VarianceOutput[DTid.xy] =
float4(blendedColor, variance) before the existing conditional so both the
history > 2 and history <= 2 paths use a valid variance value consistent with
the lower-history computation.
There was a problem hiding this comment.
Should any of these be in an existing common include? Pow2(x)?
There was a problem hiding this comment.
could be. math.hlsli could include more math helpers. i'll think about this and move some there
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Do we need shader unit tests so if anyone mucks with your hlsl functions it doesn't break silently? It caught some of Doodlum's changes to PBR breaking stuff.
i don't think i have much functions working on their own without actual context. maybe on some helper functions? idk if they'll ever be modified tho. |
If you have an AI, they can give you some unit tests pretty easily. The goal is to stop regressions because someone changes your functions internally and they suddenly spit out different values then before. As you know, a lot of stuff gets "tweaked" because it doesn't look right to random people. |
i'd try add some |
2 participants
Summary by CodeRabbit