Conversation
WalkthroughEnables precision-offset sampling in screen-space shadows, refactors shader loops to fixed bounds with early breaks, removes early-out logic and certain macro guards, and updates default BendSettings values (SurfaceThickness and ShadowContrast). No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CPU as CPU/Engine
participant DP as DispatchParameters
participant CS as RaymarchCS.hlsl
participant SSS as bend_sss_gpu.hlsli
CPU->>DP: Initialize parameters<br/>DynamicSampleCount, DynamicReadCount
CPU->>DP: Set UsePrecisionOffset = true
CPU->>CS: Dispatch Compute (Screen-Space Shadows)
CS->>SSS: WriteScreenSpaceShadow(DP)
rect rgba(220,235,255,0.3)
note over SSS: Fixed-bound loops with early breaks at DynamicSampleCount
SSS->>SSS: [loop] i in [0..READ_COUNT)<br/>break if i == DynamicSampleCount
SSS->>SSS: [loop] i in [0..SAMPLE_COUNT)<br/>break if i == DynamicSampleCount
end
alt UsePrecisionOffset
SSS->>SSS: start_depth = lerp(..., bias)
else No precision offset
SSS->>SSS: start_depth unchanged
end
note over SSS: EarlyOutPixel path removed<br/>hard_shadow removed
SSS-->>CPU: Shadow value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Features/ScreenSpaceShadows.h (1)
32-35: Default tweaks look reasonable; align shader defaults to avoid drift.You bumped C++ defaults to SurfaceThickness=0.02 and ShadowContrast=1.0. In HLSL, DispatchParameters::SetDefaults() still uses 0.005 and 4, respectively. If the CPU path ever skips populating PerFrame fields (dev tools, experiments, or future code), you’ll see confusing mismatches. Suggest syncing HLSL defaults.
Apply this in features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli:
void SetDefaults() { - SurfaceThickness = 0.005; + SurfaceThickness = 0.02; BilinearThreshold = 0.02; - ShadowContrast = 4; + ShadowContrast = 1; IgnoreEdgePixels = false; UsePrecisionOffset = false; BilinearSamplingOffsetMode = false; DebugOutputEdgeMask = false; DebugOutputThreadIndex = false; DebugOutputWaveIndex = false; }features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl (1)
62-63: Make precision-offset runtime-togglable instead of hard-on.Turning UsePrecisionOffset on unconditionally can subtly change all scenes. Prefer plumbing a toggle from C++ (BendSettings) into the PerFrame cbuffer and set parameters.UsePrecisionOffset from it; default it on if that’s the intended new behavior.
Minimal shader-side change (after you add a uint UsePrecisionOffset to PerFrame and wire it from C++):
-parameters.UsePrecisionOffset = true; +parameters.UsePrecisionOffset = (UsePrecisionOffset != 0);I can help with the C++/UI wiring if you want a quick follow-up.
features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli (1)
32-39: Revisit unconditional USE_UV_PIXEL_BIAS and restore a guard for missing macros.Unconditionally forcing USE_UV_PIXEL_BIAS may be fine, but different compilers/hardware can favor Sample(..., offset) when available. Consider making this a compile-time knob again. Also, since this file now relies on SAMPLE_COUNT being defined elsewhere, add an explicit preprocessor check to fail fast when it’s missing.
Apply:
-//#if defined(__HLSL_VERSION) || defined(__hlsl_dx_compiler) -// HLSL enforces that a pixel offset in a Sample() call must be a compile time constant, which isn't always required - and in some cases can give a small perf boost if used. -#define USE_UV_PIXEL_BIAS 1 // Use Sample(uv + bias) instead of Sample(uv, bias) -//#endif +#if !defined(SAMPLE_COUNT) +#error "SAMPLE_COUNT must be defined by the including shader." +#endif + +// HLSL enforces that a pixel offset in a Sample() call must be a compile-time constant. +// Prefer manual UV bias by default; allow projects to override. +#ifndef USE_UV_PIXEL_BIAS +#define USE_UV_PIXEL_BIAS 1 +#endif
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl(1 hunks)features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli(6 hunks)src/Features/ScreenSpaceShadows.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
src/Features/ScreenSpaceShadows.hfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/ScreenSpaceShadows.h
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/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
🧠 Learnings (5)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
📚 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/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
📚 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/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
📚 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:
features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.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: 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 (2)
features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli (2)
352-354: Precision-offset looks fine; confirm half precision granularity.The offset magnitude (-1/0xFFFF) is above half’s subnormal floor, so it should survive fp16 rounding, but please sanity-check banding on very flat gradients.
373-377: Loop bound refactor LGTM.Fixed upper bound with early-break on DynamicSampleCount is clear and compiler-friendly.
If DynamicSampleCount can exceed SAMPLE_COUNT at runtime, ensure the CPU clamps it; otherwise you rely solely on the break here.
| [loop] for (i = 0; i < READ_COUNT; i++) | ||
| { | ||
| if (i == inParameters.DynamicSampleCount) | ||
| break; | ||
|
|
There was a problem hiding this comment.
Wrong early-break: using DynamicSampleCount when filling read tiles can leave DepthData rows uninitialized.
The read loop should honor DynamicReadCount, not DynamicSampleCount. With fewer reads than required, the later sampling loop may consume uninitialized LDS entries, causing flicker/aliasing.
Fix:
-[loop] for (i = 0; i < READ_COUNT; i++)
-{
- if (i == inParameters.DynamicSampleCount)
- break;
+[loop] for (i = 0; i < READ_COUNT; i++)
+{
+ if (i >= inParameters.DynamicReadCount)
+ break;📝 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.
| [loop] for (i = 0; i < READ_COUNT; i++) | |
| { | |
| if (i == inParameters.DynamicSampleCount) | |
| break; | |
| [loop] for (i = 0; i < READ_COUNT; i++) | |
| { | |
| if (i >= inParameters.DynamicReadCount) | |
| break; |
🤖 Prompt for AI Agents
In features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
around lines 226-230, the read loop incorrectly breaks on
inParameters.DynamicSampleCount which can leave DepthData rows uninitialized;
change the loop to honor inParameters.DynamicReadCount (or clamp the loop using
min(READ_COUNT, inParameters.DynamicReadCount)) so the break condition uses
DynamicReadCount and guarantee all required read slots are initialized (or
explicitly initialize remaining entries) before the later sampling loop.
| [loop] for (i = 0; i < READ_COUNT; i++) | ||
| { | ||
| if (i == inParameters.DynamicSampleCount) | ||
| break; | ||
|
|
There was a problem hiding this comment.
Same issue: use DynamicReadCount for the LDS write loop.
Mirrors the bug above; this must also break on DynamicReadCount.
-[loop] for (i = 0; i < READ_COUNT; i++)
-{
- if (i == inParameters.DynamicSampleCount)
- break;
+[loop] for (i = 0; i < READ_COUNT; i++)
+{
+ if (i >= inParameters.DynamicReadCount)
+ break;📝 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.
| [loop] for (i = 0; i < READ_COUNT; i++) | |
| { | |
| if (i == inParameters.DynamicSampleCount) | |
| break; | |
| [loop] for (i = 0; i < READ_COUNT; i++) | |
| { | |
| if (i >= inParameters.DynamicReadCount) | |
| break; |
🤖 Prompt for AI Agents
In features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
around lines 311 to 315, the LDS write loop currently breaks based on
DynamicSampleCount but should break on DynamicReadCount; change the break
condition to compare i against inParameters.DynamicReadCount (e.g., if (i ==
inParameters.DynamicReadCount) break;) so the loop respects the dynamic read
count instead of the sample count.
|
✅ A pre-release build is available for this PR: |
1 participant
Summary by CodeRabbit
New Features
Chores
Impact