feat(sss): add burley normalized sss#1393
Conversation
WalkthroughAdds a Burley-based subsurface scattering (SSS) path alongside existing separable SSS. Introduces a new shader Burley.hlsli, extends SeparableSSSCS.hlsl with BURLEY conditional flow, adds epsilon macro in Math.hlsli, and updates C++ SSS feature for settings/UI, constant buffer, shader compilation/dispatch, and resource bindings (albedo/normal). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Renderer
participant SSS as SubsurfaceScattering (C++)
participant D3D as D3D11
participant CS as SeparableSSSCS.hlsl
App->>SSS: DrawSSS()
SSS->>SSS: Read Settings.SSMode
alt Mode 0 (Separable)
SSS->>D3D: Bind H-pass CS + SRVs (color, depth, mask)
SSS->>D3D: Update CB (blur params)
SSS->>D3D: Dispatch H-pass
SSS->>D3D: Bind V-pass CS + SRVs
SSS->>D3D: Dispatch V-pass
else Mode 1 (Burley)
SSS->>SSS: GetComputeShaderBurley()
SSS->>D3D: Compile/Cache CS with BURLEY (if needed)
SSS->>D3D: Bind CS + SRVs (color, depth, mask, albedo, normal)
SSS->>D3D: Update CB (BurleySamples, MFP base/human, FOVY)
SSS->>D3D: Dispatch Burley
SSS->>D3D: Copy result to main texture
end
D3D-->>App: Completed SSS pass
sequenceDiagram
autonumber
participant Thread as CS Thread (pixel)
participant GBuf as GBuffer/Textures
participant RNG as Random.hlsli
Thread->>GBuf: Load center depth/color/albedo/normal/mask
alt sssAmount == 0 or invalid depth
Thread-->>Thread: Return center color
else Burley
Thread->>Thread: Pick MFP (base/human), scale by sssAmount
Thread->>RNG: Seed per-pixel RNG
loop i in BurleySamples
Thread->>Thread: Sample radius/angle via CDF/PDF
Thread->>GBuf: Fetch neighbor sample (color/depth/normal)
Thread->>Thread: Weight by profile and normal/depth terms
Thread->>Thread: Accumulate color/weight
end
Thread->>Thread: Normalize, blend with center
Thread-->>Thread: Write clamped output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
6-7: NormalTexture is declared but never used.The
NormalTextureat register(t4) is declared but not referenced anywhere in the shader code. Consider removing it if it's not needed, or ensure it's properly integrated for the Burley implementation.If the normal texture is not needed, apply this diff:
Texture2D<float4> AlbedoTexture : register(t3); -Texture2D<float4> NormalTexture : register(t4);src/Features/SubsurfaceScattering.cpp (1)
33-35: Consider adding tooltips for SSS mode selection.The radio buttons for SSS mode selection would benefit from tooltips explaining the differences between Separable SSS and Burley modes to help users make informed choices.
Add tooltips after each radio button:
ImGui::RadioButton("Separatable SSS", &settings.SSMode, 0); +if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::Text("Traditional separable subsurface scattering using Gaussian blur profiles."); +} ImGui::SameLine(); ImGui::RadioButton("Burley", &settings.SSMode, 1); +if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::Text("Physically-based Burley reflectance profile with better color bleeding."); +}features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (2)
30-35: Fix variable naming in RadiusApprox function.The parameter
dconflicts with the local variabledused in the formula comment. Consider renaming for clarity. Also, the approximation formula needs validation against the referenced paper.-float RadiusApprox(float d, float rand) +float RadiusApprox(float dmfp, float rand) { // g(ξ) = d((2 − c)ξ − 2)log(1 − ξ) // minimal mean squared error when c = 2.5715 - return d * ((2 - 2.5715f) * rand - 2) * log(1 - rand); + return dmfp * ((2 - 2.5715f) * rand - 2) * log(1 - rand); }
92-131: Sampling loop implementation looks solid.The Monte Carlo sampling approach is well-implemented with:
- Proper random number generation
- Importance sampling with PDF weighting
- Normal-based weighting for geometric consistency
- Mask-based color bleeding
One minor optimization opportunity: pre-compute
2.0f * Math::PIas a constant.+ const float TWO_PI = 2.0f * Math::PI; [loop] for (int i = 0; i < BurleySamples; ++i) { seed.z = seedStart++; float2 rand = float2(Random::pcg3d(seed).xy) / 4294967296.0f; // to [0, 1) rand.x = centerRadiusCDF + rand.x * (1.0f - centerRadiusCDF); // generate radius & angle for sampling float radius = max(RadiusApprox(d, rand.x), 1e-5f); - float theta = rand.y * 2.0f * Math::PI; + float theta = rand.y * TWO_PI;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli(1 hunks)features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli(2 hunks)features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(3 hunks)package/Shaders/Common/Math.hlsli(1 hunks)src/Features/SubsurfaceScattering.cpp(6 hunks)src/Features/SubsurfaceScattering.h(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/Common/Math.hlsli
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslsrc/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (14)
package/Shaders/Common/Math.hlsli (1)
4-4: LGTM! Appropriate constant definition following project conventions.The epsilon constant is correctly defined using
#definewhich aligns with the project's convention for simple scalar constants.features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli (1)
162-162: Verify the max operation for blend factor.The change from
profile.ytomax(profile.y, 0.0001)ensures a minimum blend factor, but consider if this aligns with the intended SSS behavior when profile thickness is very small.features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
60-60: Verify the albedo multiplication order in vertical pass.The albedo is applied after converting from linear to gamma space. Ensure this is the intended behavior, as typically albedo modulation should occur in linear space before gamma conversion.
Consider if the albedo multiplication should happen before gamma conversion:
- color.rgb = Color::LinearToGamma(color.rgb) * AlbedoTexture[DTid.xy].rgb; + color.rgb = Color::LinearToGamma(color.rgb * AlbedoTexture[DTid.xy].rgb);src/Features/SubsurfaceScattering.h (1)
23-28: LGTM! Well-structured settings for Burley SSS mode.The addition of SSMode selector and Burley-specific parameters (samples count and mean free path) provides appropriate control over the new rendering path with sensible defaults.
src/Features/SubsurfaceScattering.cpp (2)
296-297: Array size mismatch in cleanup.The views array is declared with size 3 but you're clearing 5 elements with
CSSetShaderResources(0, 5, views). This could lead to undefined behavior.Fix the array size to match the number of resources being cleared:
- ID3D11ShaderResourceView* views[5]{ nullptr, nullptr, nullptr }; + ID3D11ShaderResourceView* views[5]{ nullptr, nullptr, nullptr, nullptr, nullptr }; context->CSSetShaderResources(0, 5, views);Likely an incorrect or invalid review comment.
278-290: Confirm UAV binding for Burley mode
I wasn’t able to locate thesrc/Features/SubsurfaceScattering.cppfile or thesettings.SSMode == 1block in the repo. Please verify that theblurHorizontalTempUAV is bound before dispatch in Burley mode:
- Ensure the file
src/Features/SubsurfaceScattering.cpp(or its actual path) exists and contains the SSMode == 1 branch.- In that branch, immediately before
context->Dispatch(...), there must be a call to
context->CSSetUnorderedAccessViews(..., blurHorizontalTemp->resource->GetUAV(), ...)(or equivalent)- After dispatch, copying from
blurHorizontalTemptomain.texturewithout binding the UAV first will result in no data being written.features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (8)
7-11: LGTM! Clean utility function for depth conversion.The function correctly retrieves depth and converts it to linear space using the shared utility.
20-25: PDF calculation looks correct.The probability density function implementation aligns with the Burley paper's formulation. The
max(pdf, 1e-5f)guard prevents division by zero in downstream calculations.
37-44: Excellent implementation of the Burley profile.The function correctly implements the reflectance profile from the paper with appropriate numerical guards to prevent underflow.
52-60: Early exit logic is correct.The function properly handles edge cases where SSS should not be applied.
62-63: Good gamma correction handling.Properly converts from gamma space and handles division by zero with the epsilon guard.
133-140: Final color composition is well-structured.The normalization, blending, and gamma correction pipeline is correctly implemented. The function properly preserves the alpha channel from the original color texture.
65-74: Validate Mean Free Path in the Configuration LayerClamping to 1e-5f in the HLSL shader avoids division by zero, but invalid or zero-valued parameters may still slip through and only manifest as subtle rendering artifacts. Please review and add explicit validation where these fields are populated:
• features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (lines 20–21) define
– MeanFreePathBase
– MeanFreePathHuman• Locate the CPU-side code (constant-buffer setup or configuration parser) that writes to these fields and ensure it enforces a sensible minimum (e.g. >0) or emits an error/warning for out-of-range values before they reach the GPU.
82-83: Please verify the GAME_UNIT_TO_CM constant and the mm conversionI wasn’t able to locate the definition of
GAME_UNIT_TO_CMin the repo—before shipping, please confirm:
- Where and how
GAME_UNIT_TO_CMis defined (should be the factor from game units to centimeters).- That multiplying by
0.1factually converts the result to millimeters. If 1 cm = 10 mm, the multiplier would typically be 10, not 0.1. Either adjust the factor or update the comment to match the true unit scaling.Affected location:
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (around line 82)
float2 uvScale = (GAME_UNIT_TO_CM * 0.1f * SSSScaleX) / centerDepth; // Scale in mm
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli (2)
105-105: Inconsistent albedo sampling for center pixel in horizontal path.The center pixel color is divided by its own albedo (from
DTid.xy), but neighbor samples at line 155 are divided by the center pixel's albedo instead of their own.
155-155: Inconsistent albedo sampling for neighbor pixels.In the neighbor sample loop, you're using
AlbedoTexture[DTid.xy].rgb(the center pixel's albedo) instead ofAlbedoTexture[coords].rgb(the neighbor pixel's albedo). This appears inconsistent with the intent of albedo normalization for each sample.Apply this diff to use the correct albedo coordinates:
- color.rgb = Color::GammaToLinear(color.rgb / max(AlbedoTexture[DTid.xy].rgb, EPSILON_SSS_ALBEDO)); + color.rgb = Color::GammaToLinear(color.rgb / max(AlbedoTexture[coords].rgb, EPSILON_SSS_ALBEDO));features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (4)
15-18: Address CDF numerical stability issues.The CDF function can produce values outside [0,1] when
ris very small relative tod, potentially causing negative values that could destabilize the sampling logic.Consider clamping the result and protecting against division by zero:
float3 GetCDF(float3 d, float3 r, float rand) { - return 1 - 0.25 * exp(-r / d) - 0.75 * exp(-r / (3 * d)) - rand; + d = max(d, 1e-6f); + return saturate(1 - 0.25 * exp(-r / d) - 0.75 * exp(-r / (3 * d)) - rand); }
46-55: Document the magic constants in scaling factor calculation.The scaling factor function uses unexplained constants that need documentation for maintainability.
float3 GetScalingFactor(float3 albedo) { - // we have three methods for calculating the scaling factor - // d = l / (1.85 − A + 7|A − 0.8|^3) - // d = l / (1.9 − A + 3.5(A − 0.8)^2) - // d = l / (3.5 + 100(A − 0.33)^4) - // here we choose the third to use diffuse mean free path as parameter. + // Based on Burley 2015 paper, method 3: d = l / (3.5 + 100(A − 0.33)^4) + // 0.33f: albedo offset for optimal fitting + // 3.5f: baseline scaling factor + // 100.f: coefficient controlling profile curvature float3 value = albedo - 0.33f; return 3.5f + 100.f * value * value * value * value; }
87-92: Protect against division by zero in UV scale calculations.The
uvScalecomponents could be zero whencenterDepthis very large, leading to division by zero in thecenterRadiuscalculation.- float2 uvScale = (GAME_UNIT_TO_CM * 0.1f * SSSScaleX) / centerDepth; // Scale in mm + float2 uvScale = (GAME_UNIT_TO_CM * 0.1f * SSSScaleX) / max(centerDepth, 1e-5f); // Scale in mm // center sample weight - float centerRadius = 0.5f * (SharedData::BufferDim.z / uvScale.x + SharedData::BufferDim.w / uvScale.y); + float centerRadius = 0.5f * (SharedData::BufferDim.z / max(uvScale.x, 1e-5f) + SharedData::BufferDim.w / max(uvScale.y, 1e-5f));
138-140: Protect against division by zero in sample normalization.If
BurleySamplesis 0, thercp(BurleySamples)will cause division by zero.- colorSum *= bleedSum * rcp(BurleySamples); + colorSum *= bleedSum * rcp(max(BurleySamples, 1));
🧹 Nitpick comments (1)
src/Features/SubsurfaceScattering.cpp (1)
296-297: Update resource cleanup count to match binding count.The shader resource cleanup should match the expanded resource binding count.
- ID3D11ShaderResourceView* views[5]{ nullptr, nullptr, nullptr }; - context->CSSetShaderResources(0, 5, views); + ID3D11ShaderResourceView* views[5]{ nullptr, nullptr, nullptr, nullptr, nullptr }; + context->CSSetShaderResources(0, 5, views);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli(1 hunks)features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli(2 hunks)features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(3 hunks)package/Shaders/Common/Math.hlsli(1 hunks)src/Features/SubsurfaceScattering.cpp(6 hunks)src/Features/SubsurfaceScattering.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Features/SubsurfaceScattering.h
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/Common/Math.hlsli
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlslisrc/Features/SubsurfaceScattering.cppfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (10)
package/Shaders/Common/Math.hlsli (1)
4-4: LGTM! Epsilon constant follows established patterns.The
EPSILON_SSS_ALBEDOconstant follows the existing naming convention and value format used in this file. The #define approach is consistent with other epsilon constants and appropriate for scalar constants as established in the project.features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli (1)
162-162: LGTM! Improved numerical stability for profile scaling.The addition of
max(profile.y, 0.0001)prevents potential division by zero when profile.y is very small, improving the numerical stability of the depth difference blending calculation.src/Features/SubsurfaceScattering.cpp (7)
15-20: LGTM! Settings structure properly extended for Burley mode.The serialization properly includes all new settings fields for Burley SSS mode including the mode selector and Burley-specific parameters.
33-84: Well-structured dual-mode UI implementation.The UI properly separates separable SSS and Burley modes with appropriate controls for each. The radio button selection and conditional UI sections provide a clean user experience.
201-217: LGTM! Proper constant buffer setup for both modes.The constant buffer correctly populates both the existing separable SSS parameters and the new Burley-specific parameters including projection scaling and mean free path settings.
232-246: Resource binding properly extended for Burley mode.The shader resource views array is correctly expanded to include albedo and normal textures required by the Burley implementation, maintaining backward compatibility with the separable SSS path.
249-290: Well-implemented dual rendering pipeline.The conditional rendering logic properly separates the two-pass separable SSS from the single-pass Burley implementation. The resource copying from temporary to main texture for Burley mode is handled correctly.
378-381: LGTM! Shader cache management properly updated.The Burley compute shader is correctly added to the cache cleanup routine to ensure proper resource management.
402-409: LGTM! Lazy compilation pattern maintained for Burley shader.The Burley compute shader follows the established lazy compilation pattern used by the horizontal and vertical blur shaders.
features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (1)
57-145: Well-structured Monte Carlo sampling implementation.The
BurleyNormalizedSSfunction implements a comprehensive Monte Carlo sampling approach for subsurface scattering with proper:
- Early exit conditions for invalid depth or zero SSS amount
- Albedo-based scaling and gamma correction
- World-space normal transformations
- Proper PDF/CDF-based sampling
- Depth-aware radius corrections
- Normal-weighted blending
The implementation correctly follows the Burley BSSRDF model with appropriate physical parameterization.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/SubsurfaceScattering.cpp (1)
201-217: Verify projection matrix access pattern and consider caching expensive calculations.The code accesses
cameraData.projMat.m[0][0]to get the projection distance, then performs trigonometric calculations each frame. While functionally correct, this could be optimized.Consider caching the field of view calculation if the projection matrix doesn't change frequently:
const float distanceToProjectionWindow = cameraData.projMat.m[0][0]; - -blurCBData.SSSS_FOVY = atan(1.0f / distanceToProjectionWindow) * 2.0f * (180.0f / 3.14159265359f); + +// Cache FOV calculation if projection matrix is stable +static float cachedFOVY = 0.0f; +static float cachedProjectionDistance = 0.0f; +if (cachedProjectionDistance != distanceToProjectionWindow) { + cachedProjectionDistance = distanceToProjectionWindow; + cachedFOVY = atan(1.0f / distanceToProjectionWindow) * 2.0f * (180.0f / 3.14159265359f); +} +blurCBData.SSSS_FOVY = cachedFOVY;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SubsurfaceScattering.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (6)
src/Features/SubsurfaceScattering.cpp (6)
15-20: LGTM! Clean addition of new configuration fields.The serialization fields for the new Burley SSS parameters are correctly added to the JSON mapping with appropriate types and naming conventions.
33-84: Well-structured UI implementation with proper mode separation.The UI correctly implements a radio button mode selector and conditionally shows appropriate controls based on the selected SSS mode. The parameter ranges and tooltips are appropriate for their respective use cases.
296-297: Update resource cleanup to match new binding count.The cleanup code should match the number of bound shader resources (now 5 instead of the previous count).
378-381: LGTM! Proper shader cache management.The Burley compute shader is correctly added to the cache cleanup routine, following the same pattern as existing shaders.
402-409: LGTM! Consistent shader compilation pattern.The Burley compute shader compilation follows the established pattern and correctly uses the BURLEY define to compile the specialized variant.
232-246: Ensure ALBEDO and NORMALROUGHNESS are always initialized before binding
The code now bindsALBEDOandNORMALROUGHNESSSRVs unconditionally in both SSS modes, but only Burley mode consumes them. You must guarantee these render targets exist whenever subsurface scattering is enabled to avoid null‐pointer binds at runtime.• src/Features/SubsurfaceScattering.cpp (lines 232–246): SRV binding of
albedoandnormal
• Renderer initialization (e.g. your RenderTargetsManager or similar): verify thatALBEDOandNORMALROUGHNESSare registered inrenderTargets[]whenever SSS is activeConsider adding explicit null‐checks or assertions to catch missing targets early.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Features/SubsurfaceScattering.cpp (2)
33-36: Typo and clarity: “Separatable” → “Separable”Minor labeling nit. Also consider a hover tooltip to explain each mode succinctly.
Apply this diff:
- ImGui::RadioButton("Separatable SSS", &settings.SSMode, 0); + ImGui::RadioButton("Separable SSS", &settings.SSMode, 0); ImGui::SameLine(); ImGui::RadioButton("Burley", &settings.SSMode, 1);
290-301: Burley path should write directly to main UAV; remove extra CopyResource (and avoid SRV/UAV hazard)Currently, UAV 0 is still blurHorizontalTemp (set at Line 248), so Burley writes to temp and then copies to main. This is unnecessary overhead. Bind main.UAV before dispatch and remove the copy. If BURLEY doesn’t sample t0, unbind t0 to avoid SRV/UAV aliasing on main.
Apply this diff:
} else if (settings.SSMode == 1) { // Burley pass to main texture { TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); + // Write directly to main UAV and avoid extra copy + ID3D11UnorderedAccessView* uavMain[1] = { main.UAV }; + context->CSSetUnorderedAccessViews(0, 1, uavMain, nullptr); + + // If BURLEY path doesn't read main.SRV (t0), prevent read-write hazard + // by unbinding SRV slot 0. Leave t1..t4 (depth/mask/albedo/normal) intact. + ID3D11ShaderResourceView* nullSRV[1]{ nullptr }; + context->CSSetShaderResources(0, 1, nullSRV); + auto shader = GetComputeShaderBurley(); context->CSSetShader(shader, nullptr, 0); context->Dispatch(dispatchCount.x, dispatchCount.y, 1); - - context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); } }To confirm whether BURLEY samples main (t0), please run:
#!/bin/bash # Check if BURLEY path samples main SRV (t0) in SeparableSSSCS.hlsl fd -a SeparableSSSCS.hlsl Burley.hlsli rg -n "register\\(t0\\)|Texture2D.*main|MainTexture|gColor|t0" -A 6 -B 3 features rg -n "BURLEY" -A 20 -B 5 features/Subsurface\ Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
- If t0 is read under BURLEY, we must keep writing to a different UAV (temp) or ping-pong, or rework to a different input buffer to avoid SRV/UAV aliasing. In that case, we can still eliminate the copy by swapping handles or ping-ponging textures.
- If t0 isn’t read, the above diff is safe and optimal.
I can provide a handle-swap variant if we confirm aliasing is required for input.
🧹 Nitpick comments (2)
src/Features/SubsurfaceScattering.cpp (2)
76-77: Tooltip grammar: “channel” → “channels”Minor wording nit to improve clarity.
Apply this diff:
- ImGui::Text("Controls how far light goes into the subsurface in the red, green, and blue channel. It is scaled by the Mean Free Path Distance."); + ImGui::Text("Controls how far light goes into the subsurface in the red, green, and blue channels. It is scaled by the Mean Free Path Distance.");(Apply in both Base and Human profile tooltips.)
Also applies to: 88-89
244-246: Bind albedo/normal only when in Burley mode (minor CPU overhead reduction)Albedo and normal SRVs are only needed for Burley. Binding them unconditionally is harmless but avoidable work.
I can sketch a small refactor to move their binding under the SSMode == 1 branch if desired.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Features/SubsurfaceScattering.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (7)
src/Features/SubsurfaceScattering.cpp (7)
15-20: Settings JSON: confirm defaults/back-compat for new fieldsAdding SSMode, BurleySamples, MeanFreePathBase, and MeanFreePathHuman looks fine with NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT, but please ensure:
- Settings defaults initialize SSMode to separable (0) to preserve old behavior.
- MeanFreePathBase/Human have sane defaults (e.g., human MFP tuned values) when loading existing configs lacking these fields.
If helpful, I can scan SubsurfaceScattering.h and existing saved JSONs to verify defaults and back-compat.
252-259: LGTM on SRV fan-out and ARRAYSIZE usageUsing an SRV array with ARRAYSIZE(views) is tidy and consistent with engine patterns.
261-289: Separable path looks correct (UAV ping-pong + main writeback)Horizontal writes to temp, vertical reads from temp and writes to main. Unbinds are handled. Good.
308-310: LGTM on complete SRV unbindNulling all five SRV slots after dispatch avoids lingering bindings and hazards across passes.
390-393: LGTM on shader cache cleanup for BurleyProperly releasing burleySS alongside existing shaders is correct.
414-421: LGTM on lazy BURLEY shader compileCompiling SeparableSSSCS.hlsl with BURLEY define on demand is consistent with existing pattern.
225-229: HLSL declares BurleySamples as float — the UB concern is incorrect; verify C++ CB layoutPerFrameSSS cbuffer in features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl declares:
- float BurleySamples
- float4 MeanFreePathBase
- float4 MeanFreePathHuman
Action items:
- Check the C++ constant-buffer struct (blurCBData / BlurCB) to ensure it uses matching types (float for BurleySamples; a 4-component float type for MeanFreePathBase/MeanFreePathHuman) and respects 16-byte packing for CBs.
- I could not find blurCBData or the C++ CB struct in the repository search — please point me to the C++ file if you want me to verify the C++ side.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Features/SubsurfaceScattering.cpp (2)
72-72: Use integer slider for “Burley Samples”.Samples are counts; using a float invites fractional states and type drift vs HLSL.
Apply:
- ImGui::SliderFloat("Burley Samples", &settings.BurleySamples, 1.0f, 64.0f, "%.0f", ImGuiSliderFlags_AlwaysClamp); + ImGui::SliderInt("Burley Samples", &settings.BurleySamples, 1, 64);If the backing C++/HLSL types are float, consider switching them to int/uint consistently in C++ (Settings/BlurCB) and HLSL (cbuffer) to remove implicit casts.
213-220: FOVy and projection scales are computed from fx (m[0][0]); should use fy (m[1][1]).This currently stores FOVx into SSSS_FOVY and derives aspect-incorrect scales. Pass per-axis projection scales.
Apply:
- const float distanceToProjectionWindow = cameraData.projMat.m[0][0]; - - blurCBData.SSSS_FOVY = atan(1.0f / distanceToProjectionWindow) * 2.0f * (180.0f / 3.14159265359f); - const float SSSScaleZ = distanceToProjectionWindow; - const float SSSScaleX = SSSScaleZ * 0.5f; - - blurCBData.SSSScaleX = SSSScaleX; - blurCBData.SSSScaleZ = SSSScaleZ; + const float fx = cameraData.projMat.m[0][0]; + const float fy = cameraData.projMat.m[1][1]; + + // FOVy = 2 * atan(1 / fy) + blurCBData.SSSS_FOVY = 2.0f * atan(1.0f / fy) * (180.0f / 3.14159265358979323846f); + // Pass projection scale terms per-axis for correct aspect handling. + blurCBData.SSSScaleX = fx; + blurCBData.SSSScaleZ = fy;Note: If shader semantics allow, consider renaming SSSScaleZ to SSSScaleY for clarity in a follow-up.
🧹 Nitpick comments (1)
src/Features/SubsurfaceScattering.cpp (1)
414-421: On-demand BURLEY compile looks correct; optional: set debug name for easier GPU debugging.Consider calling SetPrivateData(WKPDID_D3DDebugObjectName, ...) on the shader for PIX/NSight clarity, mirroring other shaders if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Features/SubsurfaceScattering.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (6)
src/Features/SubsurfaceScattering.cpp (6)
15-20: Settings JSON schema extension looks good; confirm defaults for new fields.The WITH_DEFAULT macro will tolerate missing keys in old configs. Ensure the C++ struct initializes sane defaults for SSMode, BurleySamples, MeanFreePathBase/Human to avoid undefined runtime behavior on first run.
Would you like me to scan SubsurfaceScattering.h and the HLSL CBs to verify default values and type alignment for these new fields?
33-36: Mode selector UI is clear and minimal.Radio buttons for Separable vs Burley are a good UX fit.
308-309: Good cleanup: unbinding all SRVs via ARRAYSIZE.Prevents SRV leakage into subsequent passes.
390-393: Shader cache: correctly releasing burleySS.Matches existing cache-clearing pattern.
225-229: Verify CB types/packing for BurleySamples and MeanFreePath in PerFrameSSSHLSL shows PerFrameSSS defines:
- float BurleySamples;
- float4 MeanFreePathBase;
- float4 MeanFreePathHuman
(in features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl).I could not find the CPU-side constant-buffer struct (the blurCBData / BlurCB definition or src/Features/SubsurfaceScattering.cpp is not present in this branch). Please confirm the C++ CB field types and packing match the HLSL layout. If BurleySamples is an integer sample count, change the CPU and shader fields to int/uint (and keep proper 16‑byte packing) to avoid implicit float→int casts and packing misalignment.
Please check:
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (PerFrameSSS)
- CPU-side CB definition that populates blurCBData (wherever blurCBData/BlurCB is defined)
247-249: Fix Burley pass comment and unbind UAV before CopyResourceShort: SeparableSSSCS.hlsl writes to u0 (SSSRW) and reads t0..t4, so the temp→main pattern makes sense; I couldn't find the referenced C++ file to verify the exact binding — please confirm the file path or apply the patch below.
Files of interest:
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl — declares RWTexture2D SSSRW : register(u0) and reads ColorTexture (t0), MaskTexture (t2), AlbedoTexture (t3), etc.
- src/Features/SubsurfaceScattering.cpp — not found in the repo (original review referenced lines 247-249 and 291-301); I couldn't verify the C++ binding/COPY calls.
Suggested change (update comment + unbind UAV before CopyResource):
- } else if (settings.SSMode == 1) { - // Burley pass to main texture + } else if (settings.SSMode == 1) { + // Burley pass to temporary texture, then copy to main { TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); auto shader = GetComputeShaderBurley(); context->CSSetShader(shader, nullptr, 0); context->Dispatch(dispatchCount.x, dispatchCount.y, 1); + // Unbind UAV before copying resources to avoid driver hazards. + uav = nullptr; + context->CSSetUnorderedAccessViews(0, 1, &uav, nullptr); + context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); } }Notes: If the BURLEY shader definitely does NOT sample main.SRV, you can avoid the copy by binding main.UAV for the dispatch (ensure no SRV slot still references main). I can locate and patch the exact C++ file if you provide the correct path or allow me to search for the binding code.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/Features/SubsurfaceScattering.h (1)
41-51: Mirror CB layout change: make BurleySamples integral in BlurCBKeep C++<->HLSL CB in sync to avoid undefined behavior from mismatched layouts.
float4 HumanProfile; float SSSS_FOVY; - float BurleySamples; + int BurleySamples; float2 pad; float4 MeanFreePathBase; float4 MeanFreePathHuman;Note: int is 32-bit; packing with float2 pad still preserves 16-byte alignment for the subsequent float4s.
♻️ Duplicate comments (3)
src/Features/SubsurfaceScattering.cpp (3)
71-96: Use an integer slider for “Burley Samples” to prevent fractional valuesSamples are counts and should be integral. This also aligns with the proposed HLSL/C++ type change.
- ImGui::SliderFloat("Burley Samples", &settings.BurleySamples, 1.0f, 64.0f, "%.0f", ImGuiSliderFlags_AlwaysClamp); + ImGui::SliderInt("Burley Samples", &settings.BurleySamples, 1, 64);If you keep BurleySamples as uint, use a local int mirror for UI and clamp/assign back.
213-216: Compute FOVy from projMat.m[1][1] (fy), not m[0][0] (fx)Current math yields FOVx but stores in SSSS_FOVY. Use fy for correctness.
- const float distanceToProjectionWindow = cameraData.projMat.m[0][0]; - - blurCBData.SSSS_FOVY = atan(1.0f / distanceToProjectionWindow) * 2.0f * (180.0f / 3.14159265359f); + const float fy = cameraData.projMat.m[1][1]; + blurCBData.SSSS_FOVY = 2.0f * atan(1.0f / fy) * (180.0f / 3.14159265358979323846f);If shaders rely on SSSS_FOVY to scale taps, this improves kernel correctness across aspect ratios.
285-296: Avoid post-dispatch CopyResource in Burley path; bind main UAV and read from temp SRVThe current flow writes to blurHorizontalTemp and then copies to main, adding an extra pass. You can flip it: seed temp with main (once), read from temp, and write directly to main — same copy count but no post-dispatch stall. Also avoids SRV/UAV hazards on main.
- } else if (settings.SSMode == 1) { - // Burley pass to main texture - { - TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); - - auto shader = GetComputeShaderBurley(); - context->CSSetShader(shader, nullptr, 0); - - context->Dispatch(dispatchCount.x, dispatchCount.y, 1); - - context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); - } + } else if (settings.SSMode == 1) { + // Burley pass: copy main -> temp (SRV), write directly to main (UAV) + { + TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); + + // Seed temp with current main color for SRV reads + context->CopyResource(blurHorizontalTemp->resource.get(), main.texture); + + // Bind temp as input color (t0) to avoid SRV/UAV hazard on main + views[0] = blurHorizontalTemp->srv.get(); + context->CSSetShaderResources(0, 1, views); + + // Set main as output UAV + ID3D11UnorderedAccessView* uavs[1] = { main.UAV }; + context->CSSetUnorderedAccessViews(0, 1, uavs, nullptr); + + auto shader = GetComputeShaderBurley(); + context->CSSetShader(shader, nullptr, 0); + + context->Dispatch(dispatchCount.x, dispatchCount.y, 1); + }If Burley doesn’t read ColorTexture at all, you can skip the pre-copy, null out SRV slot 0, and bind main.UAV directly.
🧹 Nitpick comments (3)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (2)
6-7: NormalTexture is declared but not usedBinding an unused SRV wastes a bind slot and may incur unnecessary bandwidth. If Burley doesn't consume normals yet, defer adding/binding it or guard behind the BURLEY path where it’s actually needed.
Apply one of these diffs:
Option A — keep declaration but bind only for BURLEY builds:
-Texture2D<float4> NormalTexture : register(t4); +#if defined(BURLEY) +Texture2D<float4> NormalTexture : register(t4); +#endifOption B — remove until used:
-Texture2D<float4> NormalTexture : register(t4);
38-45: Prefer explicit float literal in max to avoid implicit int→float conversionsMinor clarity/readability nit.
- SSSRW[DTid.xy] = max(0, color); + SSSRW[DTid.xy] = max(0.0f, color);src/Features/SubsurfaceScattering.cpp (1)
33-36: Add brief hover tooltips for SSS mode choiceNon-experts may not know the difference between Separable and Burley. A one-liner tooltip improves UX without clutter.
- ImGui::RadioButton("Separable SSS", &settings.SSMode, 0); + if (ImGui::RadioButton("Separable SSS", &settings.SSMode, 0)) {} + if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::Text("Faster two-pass blur approximation."); + } ImGui::SameLine(); - ImGui::RadioButton("Burley", &settings.SSMode, 1); + if (ImGui::RadioButton("Burley", &settings.SSMode, 1)) {} + if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::Text("Physically-based Burley diffusion (more accurate, costlier)."); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli(1 hunks)features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(3 hunks)src/Features/SubsurfaceScattering.cpp(6 hunks)src/Features/SubsurfaceScattering.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/SubsurfaceScattering.cppfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (6)
src/Features/SubsurfaceScattering.h (2)
64-65: LGTM: Dedicated compute shader handle for BurleyNew handle and accessor are appropriate and scoped alongside existing blur shaders.
106-107: LGTM: Public accessor for Burley compute shaderConsistent with the existing horizontal/vertical getters.
src/Features/SubsurfaceScattering.cpp (4)
15-21: Ensure Settings JSON mapping includes new fieldsThe new fields are correctly listed in the macro. Keep in mind any future type change for BurleySamples (int) must be reflected here, but nlohmann supports it transparently.
303-307: Good state cleanupNulling CBs/SRVs/UAVs after use avoids binding hazards down the line.
375-389: Shader cache clears include BurleyReleasing burleySS here matches existing pattern; safe and complete.
409-416: On-demand Burley shader compilation is consistent with othersBuilds SeparableSSSCS.hlsl with BURLEY define — good reuse of the entrypoint.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Features/SubsurfaceScattering.h (1)
26-26: Use integer type for “BurleySamples” in Settings (counts shouldn’t be float)Sample count is discrete; store it as an int in Settings to avoid fractional states and type drift. Keep BlurCB.BurleySamples as float if HLSL expects float, casting on upload.
- float BurleySamples = 16.0f; + int BurleySamples = 16;src/Features/SubsurfaceScattering.cpp (3)
213-213: Fix FOVY computation: use projMat.m[1][1] (fy), not m[0][0] (fx)Current code computes FOVx but stores in SSSS_FOVY.
- blurCBData.SSSS_FOVY = atan(1.0f / cameraData.projMat.m[0][0]) * 2.0f * (180.0f / 3.14159265359f); + blurCBData.SSSS_FOVY = 2.0f * atan(1.0f / cameraData.projMat.m[1][1]) * (180.0f / 3.14159265359f);
71-73: Make “Burley Samples” an integer sliderSamples are counts; using float permits fractional UI states and mismatches Settings if you switch it to int.
- ImGui::SliderFloat("Burley Samples", &settings.BurleySamples, 1.0f, 64.0f, "%.0f", ImGuiSliderFlags_AlwaysClamp); + ImGui::SliderInt("Burley Samples", &settings.BurleySamples, 1, 64);
245-248: Compile error: trailing comma instead of semicolonThis line won’t compile.
- views[1] = terrainBlending.loaded ? terrainBlending.blendedDepthTexture16->srv.get() : depth.depthSRV, + views[1] = terrainBlending.loaded ? terrainBlending.blendedDepthTexture16->srv.get() : depth.depthSRV;
🧹 Nitpick comments (2)
src/Features/SubsurfaceScattering.cpp (2)
74-95: Avoid aliasing cast in ColorEdit3; pass the x component pointerMinor clarity/readability improvement; avoids reinterpret cast on float4.
- ImGui::ColorEdit3("Mean Free Path Color", (float*)&settings.MeanFreePathBase); + ImGui::ColorEdit3("Mean Free Path Color", &settings.MeanFreePathBase.x); ... - ImGui::ColorEdit3("Mean Free Path Color", (float*)&settings.MeanFreePathHuman); + ImGui::ColorEdit3("Mean Free Path Color", &settings.MeanFreePathHuman.x);
240-242: Bind UAVs inside mode branches; optionally write Burley pass directly to main (avoid copy) with SRV/UAV safetyUnconditionally binding blurHorizontalTemp UAV (Line 240) means Burley also writes to temp and then copies. Move UAV binding into the per‑mode branches. If Burley doesn’t sample main.SRV, bind main.UAV and remove the CopyResource. If it does sample main.SRV, keep the temp (or unbind SRV 0 before binding main.UAV).
Proposed refactor:
- ID3D11UnorderedAccessView* uav = blurHorizontalTemp->uav.get(); - context->CSSetUnorderedAccessViews(0, 1, &uav, nullptr); + // Defer UAV binding to per-mode branchesBind for separable horizontal pass only:
// Horizontal pass to temporary texture { TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Horizontal"); + // Output to temp + ID3D11UnorderedAccessView* uavH[1] = { blurHorizontalTemp->uav.get() }; + context->CSSetUnorderedAccessViews(0, 1, uavH, nullptr); auto shader = GetComputeShaderHorizontalBlur(); context->CSSetShader(shader, nullptr, 0); context->Dispatch(dispatchCount.x, dispatchCount.y, 1); }For Burley, write directly to main.UAV (only if the shader doesn’t read main SRV; otherwise keep current temp+copy):
} else if (settings.SSMode == 1) { // Burley pass to main texture { TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); - auto shader = GetComputeShaderBurley(); - context->CSSetShader(shader, nullptr, 0); - - context->Dispatch(dispatchCount.x, dispatchCount.y, 1); - - context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); + // If Burley path does NOT sample main.SRV: unbind SRV[0] to avoid SRV/UAV conflict + ID3D11ShaderResourceView* nullSRV[1]{ nullptr }; + context->CSSetShaderResources(0, 1, nullSRV); + + // Set main UAV as output + ID3D11UnorderedAccessView* uavMain[1]{ main.UAV }; + context->CSSetUnorderedAccessViews(0, 1, uavMain, nullptr); + + auto shader = GetComputeShaderBurley(); + context->CSSetShader(shader, nullptr, 0); + + context->Dispatch(dispatchCount.x, dispatchCount.y, 1); } }Verification needed:
- If the BURLEY shader samples SRV slot 0 (main), keep the current temp + CopyResource approach, or switch SRV slot 0 to a non-conflicting texture and unbind it before binding main.UAV.
- Run with D3D11 debug layer to catch SRV/UAV conflicts during Burley.
Also applies to: 254-283, 283-295
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Features/SubsurfaceScattering.cpp(6 hunks)src/Features/SubsurfaceScattering.h(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (11)
src/Features/SubsurfaceScattering.h (5)
23-23: SS mode field addition looks goodPublic setting is integer (mode selector), consistent with UI and shader branching.
27-28: Mean Free Path defaults: sensible and self‑documentingRGB in xyz and distance in w is clear. Good defaults.
47-51: CB layout: addition and alignment look correctKeeping BurleySamples as float with pad[2] preserves 16‑byte alignment before the two float4s. Matches typical HLSL packing.
64-64: Compute shader handle for Burley pathNew handle is properly introduced; ensure it’s released (verified in ClearShaderCache).
106-107: Accessor for Burley compute shaderNecessary for lazy compilation and selection at runtime.
src/Features/SubsurfaceScattering.cpp (6)
15-20: JSON (de)serialization updated correctlyNew fields are included in NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT; defaults handle backward compat.
33-36: Mode selector UI: clear and consistentRadio buttons for Separable vs Burley are straightforward.
218-222: Per-frame CB fields are populated correctlyValues mapped 1:1 from Settings to CB; implicit int→float cast for BurleySamples is fine if Settings switches to int.
252-252: Nice: using ARRAYSIZE for SRV binding countPrevents drift if views array changes.
383-386: Shader cache cleanup includes Burley shaderPrevents leaks and ensures recompile on cache clear.
407-414: Lazy compilation of Burley compute shader with defineCompiles SeparableSSSCS.hlsl with BURLEY define — matches header and runtime branching.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Features/SubsurfaceScattering.cpp (1)
25-31: Avoid casting uint to bool in ImGui checkbox (UB risk)*Casting a uint* to bool* is unsafe; bool is 1 byte vs uint 4 bytes. Use a local bool and write back.
- ImGui::Checkbox("Enable Character Lighting", (bool*)&settings.EnableCharacterLighting); + { + bool enabled = settings.EnableCharacterLighting != 0u; + if (ImGui::Checkbox("Enable Character Lighting", &enabled)) { + settings.EnableCharacterLighting = enabled ? 1u : 0u; + } + }
♻️ Duplicate comments (4)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
62-63: Fix color-space math: multiply in linear, then encode to gammaCurrently multiplies gamma(color) by gamma(albedo). Convert albedo to linear, multiply in linear, then convert the final result if the target expects gamma.
- color.rgb = Color::LinearToGamma(color.rgb) * AlbedoTexture[DTid.xy].rgb; - SSSRW[DTid.xy] = float4(color.rgb, 1.0); + float3 albedoLin = Color::GammaToLinear(saturate(AlbedoTexture[DTid.xy].rgb)); + float3 outLin = color.rgb * albedoLin; + SSSRW[DTid.xy] = float4(Color::LinearToGamma(outLin), 1.0f);If this stage already operates in linear and writes to a linear target, drop the gamma conversions and write outLin directly.
src/Features/SubsurfaceScattering.cpp (3)
213-213: Compute FOVy from projMat.m[1][1] (fy), not m[0][0] (fx)You’re currently storing FOVx into SSSS_FOVY. Use fy for FOVy.
- blurCBData.SSSS_FOVY = atan(1.0f / cameraData.projMat.m[0][0]) * 2.0f * (180.0f / 3.14159265359f); + blurCBData.SSSS_FOVY = 2.0f * atan(1.0f / cameraData.projMat.m[1][1]) * (180.0f / 3.14159265359f);
72-72: Don’t cast uint to int for SliderInt**Avoid pointer type punning. Use a local int and write back to the uint.
- ImGui::SliderInt("Burley Samples", (int*)&settings.BurleySamples, 1, 64, "%d", ImGuiSliderFlags_AlwaysClamp); + { + int samples = static_cast<int>(settings.BurleySamples); + if (ImGui::SliderInt("Burley Samples", &samples, 1, 64, "%d", ImGuiSliderFlags_AlwaysClamp)) { + settings.BurleySamples = static_cast<uint>(samples); + } + }
283-295: Fix D3D11 resource hazard: CopyResource while main is bound as SRVIn the Burley path you write to blurHorizontalTemp UAV and then CopyResource to main.texture while main.SRV is still bound (and blurHorizontalTemp UAV is also bound). This is illegal with the D3D11 debug layer and can cause undefined behavior.
Either:
- Keep writing to temp (to avoid read/write hazards with ColorTexture=t0=main.SRV), but unbind SRVs/UAVs referencing the src/dst before CopyResource; or
- Rebind main.UAV and avoid the copy, but only if you don’t read from main.SRV in the same dispatch (not true here).
Apply the safe unbind-before-copy fix:
} else if (settings.SSMode == 1) { // Burley pass to main texture { TracyD3D11Zone(globals::state->tracyCtx, "Subsurface Scattering - Burley"); auto shader = GetComputeShaderBurley(); context->CSSetShader(shader, nullptr, 0); context->Dispatch(dispatchCount.x, dispatchCount.y, 1); - context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); + // Unbind SRVs/UAVs that alias the copy destination/source before copying. + ID3D11ShaderResourceView* nullSRVs[5]{ nullptr, nullptr, nullptr, nullptr, nullptr }; + context->CSSetShaderResources(0, ARRAYSIZE(nullSRVs), nullSRVs); + ID3D11UnorderedAccessView* nullUAV{ nullptr }; + context->CSSetUnorderedAccessViews(0, 1, &nullUAV, nullptr); + + context->CopyResource(main.texture, blurHorizontalTemp->resource.get()); } }Optional follow-up: to save bandwidth, consider binding main.UAV for the Burley dispatch and sampling ColorTexture from a history or pre-SSS buffer so you can eliminate the copy altogether.
🧹 Nitpick comments (1)
src/Features/SubsurfaceScattering.h (1)
56-59: Remove unused ‘validMaterial’ (singular) to avoid confusion with ‘validMaterials’There are both validMaterial and validMaterials; only validMaterials is used. Keep one.
- bool validMaterial = true; bool updateKernels = true; bool validMaterials = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(3 hunks)src/Features/SubsurfaceScattering.cpp(6 hunks)src/Features/SubsurfaceScattering.h(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslsrc/Features/SubsurfaceScattering.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (4)
6-7: Bindings and include selection look correct
- SRV slots t0–t4 map as expected from C++ bindings (Color, Depth, Mask, Albedo, Normal).
- Conditional include via BURLEY/HORIZONTAL matches the dispatch paths compiled in C++.
Also applies to: 27-31
17-20: Good: BurleySamples moved to uint; CB alignment preservedUsing uint for sample count matches semantics and avoids float→int casts in HLSL. The padding keeps the 16-byte alignment intact.
36-37: Eye index retrieval is fine (VR-safe, no-op in mono)Stereo::GetEyeIndexFromTexCoord is cheap and benign in mono; okay to keep here.
38-45: Burley and horizontal branches look consistent; clamping to non-negative is sensible
- Burley path uses mask channels as sssAmount/humanProfile and writes max(0, color).
- Horizontal pass switches direction to float2(1,0) and also clamps negative values.
Also applies to: 46-53
src/Features/SubsurfaceScattering.h (3)
23-28: Settings types align with shader usage
- SSMode as int is fine for UI branching.
- BurleySamples as uint matches the HLSL CB (PerFrameSSS::BurleySamples).
- Defaults for mean free paths look reasonable.
40-51: CB layout mirrors HLSL (two 21-sample kernels + new fields)BaseKernel/HumanKernel map cleanly to the single HLSL float4 Kernels[42] array; added fields and padding maintain alignment.
64-64: Burley shader handle and accessor are appropriateShader handle addition and lazy compilation accessor for Burley mode align with the dispatch logic in cpp.
Also applies to: 106-107
src/Features/SubsurfaceScattering.cpp (3)
245-252: SRV array size matches shader declarations (t0–t4)Five SRVs are bound in order: main, depth (or blended), mask, albedo, normal. Matches the HLSL t0–t4 layout.
373-386: Shader cache cleanup correctly releases burleySSEnsures the new shader is released in ClearShaderCache.
407-414: Lazy compilation path for Burley shader is soundCompiles SeparableSSSCS.hlsl with BURLEY define, consistent with HLSL conditional includes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
58-64: Fix color-space: multiply SSS result by albedo in linear, then gamma-encode; also clamp negativesRight now you convert the SSS result to gamma and then multiply by AlbedoTexture, which yields incorrect results whether the albedo fetch is linear or gamma. Multiply in linear, clamp, then convert once at the end.
Apply:
- float4 color = SSSSBlurCS(DTid.xy, texCoord, float2(0.0, 1.0), sssAmount, humanProfile); - color.rgb = Color::LinearToGamma(color.rgb) * AlbedoTexture[DTid.xy].rgb; - SSSRW[DTid.xy] = float4(color.rgb, 1.0); + float4 colorLin = SSSSBlurCS(DTid.xy, texCoord, float2(0.0, 1.0), sssAmount, humanProfile); + // If AlbedoTexture SRV is not sRGB-typed, this explicit decode is required. If it is sRGB-typed, keep albedoLin = AlbedoTexture[...].rgb. + float3 albedoLin = Color::GammaToLinear(saturate(AlbedoTexture[DTid.xy].rgb)); + float3 outLin = max(0.0, colorLin.rgb) * albedoLin; + SSSRW[DTid.xy] = float4(Color::LinearToGamma(outLin), 1.0f);If your render target expects linear, skip the LinearToGamma and write outLin directly.
🧹 Nitpick comments (3)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (3)
36-41: Compute eyeIndex only when BURLEY is enabledeyeIndex is only used by the Burley path; compute it inside that branch to avoid unnecessary work and potential compile issues if Stereo utilities are not available in other variants.
Apply:
- uint eyeIndex = Stereo::GetEyeIndexFromTexCoord(texCoord); - -#if defined(BURLEY) - - float sssAmount = MaskTexture[DTid.xy].x; - bool humanProfile = MaskTexture[DTid.xy].y > 0.0; +#if defined(BURLEY) + uint eyeIndex = Stereo::GetEyeIndexFromTexCoord(texCoord); + float2 mask = MaskTexture[DTid.xy].xy; + float sssAmount = mask.x; + bool humanProfile = mask.y > 0.0;
38-45: Unify MaskTexture fetch; standardize output alpha and color space with the vertical path
- Avoid two texture reads; fetch MaskTexture.xy once.
- Alpha handling is inconsistent: vertical path writes alpha=1, Burley path preserves whatever BurleyNormalizedSS returns. Prefer alpha=1.0 for consistency unless alpha is deliberately carrying metadata.
- Color space: vertical path gamma-encodes at write. Burley path currently writes without explicit encoding. If BurleyNormalizedSS returns linear (expected), gamma-encode before write to match the vertical path.
Proposed change (assumes BurleyNormalizedSS returns linear RGB and RT expects gamma):
- float sssAmount = MaskTexture[DTid.xy].x; - bool humanProfile = MaskTexture[DTid.xy].y > 0.0; - - float4 color = BurleyNormalizedSS(DTid.xy, texCoord, eyeIndex, sssAmount, humanProfile); - SSSRW[DTid.xy] = max(0, color); + float2 mask = MaskTexture[DTid.xy].xy; + float sssAmount = mask.x; + bool humanProfile = mask.y > 0.0; + + float4 colorLin = BurleyNormalizedSS(DTid.xy, texCoord, eyeIndex, sssAmount, humanProfile); + SSSRW[DTid.xy] = float4(Color::LinearToGamma(max(0.0, colorLin.rgb)), 1.0f);If your SSSRW target is linear, drop LinearToGamma and keep the max(0.0, ...) with alpha=1.0.
46-53: Reduce redundant MaskTexture reads and standardize alpha in HORIZONTAL passTwo reads from MaskTexture for .x and .y are unnecessary; also, consider writing alpha=1.0 like the vertical path.
Apply:
- float sssAmount = MaskTexture[DTid.xy].x; - bool humanProfile = MaskTexture[DTid.xy].y > 0.0; + float2 mask = MaskTexture[DTid.xy].xy; + float sssAmount = mask.x; + bool humanProfile = mask.y > 0.0; - float4 color = SSSSBlurCS(DTid.xy, texCoord, float2(1.0, 0.0), sssAmount, humanProfile); - SSSRW[DTid.xy] = max(0, color); + float4 color = SSSSBlurCS(DTid.xy, texCoord, float2(1.0, 0.0), sssAmount, humanProfile); + SSSRW[DTid.xy] = float4(max(0.0, color.rgb), 1.0f);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
🧠 Learnings (3)
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (4)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (4)
17-20: Good: BurleySamples switched to uint; CB alignment stays 16‑byte cleanThe uint type matches sampling semantics and avoids float→int casts. With SSSS_FOVY (float) + BurleySamples (uint) + pad (uint2) you fill a 16‑byte slot, then the float4s remain aligned.
Confirm the corresponding C++ struct(s)/UI were updated to uint/int and that serialization preserves the new type boundaries.
27-31: Conditional include is correct; prevents symbol clashes between Burley vs. SeparableIncluding only one implementation per compile keeps function/CB namespaces clean. Ensure both headers agree on external resource expectations (e.g., Albedo/Normal presence).
11-21: No register(b1) conflicts found
A ripgrep search for anycbuffer … : register(b1)across allfeatures/**/Shaders/**/*.hlsl*files returned no matches, confirming no other constant buffers are bound to b1. This usage is safe.
6-7: Verify SRV slots t3/t4 and sRGB decode for AlbedoTexture/NormalTexture• Declarations in SeparableSSSCS.hlsl (lines 6–7):
Texture2D<float4> AlbedoTexture : register(t3); Texture2D<float4> NormalTexture : register(t4);• t3/t4 are also bound in multiple Screen Space GI shaders (e.g., upsample.cs.hlsl, radianceDisocc.cs.hlsl, blur.cs.hlsl, gi.cs.hlsl). Ensure your SSS compute‐shader root signature or descriptor tables rebind these slots per‐pass to avoid conflicts.
• Call sites (e.g., line 62 in SeparableSSSCS.hlsl) must bind the correct SRVs for each SSS mode—double‐check your dispatch setup.
• If AlbedoTexture is using an sRGB UNORM format, decide whether to rely on hardware‐linear fetch or to decode manually to prevent double‐decoding in your color-space math.
• NormalTexture is consumed in this CS; the earlier suggestion to remove it if unused elsewhere isn’t applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
56-64: Consolidate Mask fetch and clamp before gamma conversionThe vertical‐pass should sample the mask only once and clamp to non‐negative before applying gamma to avoid NaNs. The gamma conversion belongs only in this final pass—BURLEY and horizontal passes remain linear. If you instead target a linear RT, remove this
LinearToGammahere and mirror that change in the BURLEY path so all modes match.• In
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl, replace the twoMaskTexturefetches with a singlefloat2 mask = MaskTexture[DTid.xy].xy;
• Clampcolor.rgbusingmax(color.rgb, 0.0f)before callingColor::LinearToGamma
• Leave gamma conversion only in this final (vertical) pass; BURLEY and horizontal passes output linear values- float sssAmount = MaskTexture[DTid.xy].x; - if (sssAmount > 0.0) { - bool humanProfile = MaskTexture[DTid.xy].y > 0.0; - - float4 color = SSSSBlurCS(DTid.xy, texCoord, float2(0.0, 1.0), sssAmount, humanProfile); - color.rgb = Color::LinearToGamma(color.rgb); - SSSRW[DTid.xy] = float4(color.rgb, 1.0); - } + float2 mask = MaskTexture[DTid.xy].xy; + float sssAmount = mask.x; + if (sssAmount > 0.0f) { + bool humanProfile = mask.y > 0.0f; + float4 color = SSSSBlurCS(DTid.xy, texCoord, float2(0.0, 1.0), sssAmount, humanProfile); + color.rgb = max(color.rgb, 0.0f); // clamp negative before gamma + SSSRW[DTid.xy] = float4(Color::LinearToGamma(color.rgb), 1.0f); + }
♻️ Duplicate comments (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
17-21: CB layout LGTM; uint for BurleySamples matches semantics and alignmentUsing uint for BurleySamples avoids float→int casts in loops and keeps 16-byte packing correct with the added uint2 pad and MFP vectors. Nice.
Please confirm the C++ PerFrameSSS/Settings structs and UI slider now use an unsigned integer for BurleySamples:
#!/bin/bash set -euo pipefail rg -nP -C3 '\b(BurleySamples|PerFrameSSS|MeanFreePath(Base|Human))\b' src || true
🧹 Nitpick comments (3)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (3)
27-31: Guard shader permutations to prevent invalid macro combosAdd a small preprocessor guard to avoid accidental compilation with both BURLEY and HORIZONTAL defined.
Apply:
#include "Common/SharedData.hlsli" +#if defined(BURLEY) && defined(HORIZONTAL) +#error "BURLEY and HORIZONTAL cannot be defined together for SeparableSSSCS." +#endif + #if defined(BURLEY) # include "SubsurfaceScattering/Burley.hlsli" #else # include "SubsurfaceScattering/SeparableSSS.hlsli" #endifAlso ensure the build system emits three permutations only: BURLEY, HORIZONTAL (separable pass 1), and default vertical (separable pass 2).
36-37: Avoid unused local in separable permutations; move eyeIndex inside BURLEY blockeyeIndex is computed for all permutations but only used in BURLEY. Move it under the BURLEY branch to avoid warnings and a tiny amount of redundant work.
Apply:
-uint eyeIndex = Stereo::GetEyeIndexFromTexCoord(texCoord); - -#if defined(BURLEY) +#if defined(BURLEY) + uint eyeIndex = Stereo::GetEyeIndexFromTexCoord(texCoord);
46-53: Literal type nit: prefer 0.0f in max to avoid implicit int→float promotionSmall readability tweak in both BURLEY and HORIZONTAL writes.
Apply:
- SSSRW[DTid.xy] = max(0, color); + SSSRW[DTid.xy] = max(0.0f, color);And the same change in the HORIZONTAL block’s write.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
🧠 Learnings (3)
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
6-7: Bind correctness: verify t3/t4 SRV slots and NormalTexture usageOur scan of the HLSL sources shows that
NormalTexture : register(t4)is only declared in
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl(line 7) and never referenced elsewhere. Please:
Remove the unused SRV to avoid wasted binds/reads:
- Texture2D<float4> NormalTexture : register(t4);Confirm your C++ binding code sets shader resource slots in the same order:
t3 →AlbedoTexture, t4 →NormalTexturefor this compute shader.Scan all HLSL under
features/Subsurface Scattering/Shadersfor any otherregister(t3)/register(t4)usages to prevent conflicts.
3 participants
This pull request adds a new Burley-based subsurface scattering (SSS) method alongside the existing separable SSS blur, providing more physically accurate skin rendering and greater user control. It introduces new shader code for the Burley profile, updates the configuration and UI to support both SSS methods, and refactors resource bindings and compute shader selection to accommodate the changes.
Burley SSS Implementation
Burley.hlslishader implementing the Burley reflectance profile, including physically-based sampling, color bleed, and profile calculations for improved realism. (features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli)SeparableSSSCS.hlsl, including new texture inputs (albedo, normal), constants (sample count, mean free path), and conditional execution based on mode. (features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl) [1] [2] [3]Configuration and UI Enhancements
src/Features/SubsurfaceScattering.cpp,src/Features/SubsurfaceScattering.h) [1] [2] [3] [4]Resource Binding and Shader Dispatch
src/Features/SubsurfaceScattering.cpp,src/Features/SubsurfaceScattering.h) [1] [2] [3] [4] [5] [6] [7]Separable SSS Improvements
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSS.hlsli,package/Shaders/Common/Math.hlsli) [1] [2] [3] [4]Refactoring and Maintenance
src/Features/SubsurfaceScattering.cpp,src/Features/SubsurfaceScattering.h) [1] [2]These changes collectively enable a more advanced, configurable, and physically plausible subsurface scattering effect for character and skin rendering.
Summary by CodeRabbit