feat(UI): add gaussian blur shader core files #1595
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Background Blur Module src/Menu/BackgroundBlur.h, src/Menu/BackgroundBlur.cpp |
New public API and implementation for background blur: Initialize(), RenderBackgroundBlur(), CreateBlurTextures(width,height,format), PerformBlur(...), Cleanup(), SetEnabled/GetEnabled/IsEnabled(), GetTextureDimensions(). Compiles shaders, creates/manages D3D11 resources (textures, RTV/SRV, constant buffers, sampler, blend/rasterizer states), performs downsample + separable blur passes and final upscale/composite with scissor per ImGui window, mutex-protected resource access, error handling and dynamic resizing. |
Horizontal Blur Shader package/Shaders/Menu/BackgroundBlurHorizontal.hlsl |
New HLSL vertex and pixel shaders for the horizontal pass: BlurBuffer cbuffer (TexelSize, BlurParams), LinearSampler, InputTexture, precomputed Gaussian weights, symmetric horizontal sampling and accumulation. |
Vertical Blur Shader package/Shaders/Menu/BackgroundBlurVertical.hlsl |
New HLSL vertex and pixel shaders for the vertical pass mirroring the horizontal pass: BlurBuffer, LinearSampler, InputTexture, precomputed weights, symmetric vertical sampling and accumulation. |
Sequence Diagram(s)
sequenceDiagram
participant ImGui
participant BG as BackgroundBlur
participant Sh as Shaders
participant DX as D3D11
rect rgb(240,248,255)
Note over BG: Initialization
ImGui->>BG: Initialize()
BG->>Sh: Compile H & V shaders
Sh-->>BG: Bytecode
BG->>DX: Create textures, RTV/SRV, CB, sampler, blend, rasterizer
DX-->>BG: Resources ready
end
rect rgb(240,255,240)
Note over BG: Per-frame render
ImGui->>BG: RenderBackgroundBlur()
BG->>DX: Capture source RT (downsample)
BG->>DX: Bind H shader, draw fullscreen -> ping RT (horizontal)
DX-->>BG: H-pass result
BG->>DX: Bind V shader, draw fullscreen -> pong RT (vertical)
DX-->>BG: V-pass result
BG->>DX: Composite upscaled blurred texture into scissor per-window
DX-->>ImGui: Blurred composition
end
rect rgb(255,240,240)
Note over BG: Cleanup
ImGui->>BG: Cleanup()
BG->>DX: Release resources
DX-->>BG: Freed
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Check HLSL <-> CPU constant buffer layout and TexelSize / sample count matching.
- Verify D3D11 resource creation/resizing and safe COM release paths in
BackgroundBlur.cpp. - Inspect render-target state save/restore, scissor setup, per-window bounds calculation, and composition blending.
- Review mutex usage for thread safety around shared resources.
Suggested reviewers
- jiayev
Poem
🐇 I hop behind the UI and blur each seam,
Left-right then up-down, a soft pixel dream.
Downsampled whispers, then two-pass cream,
I stitch back the glow — a rabbit's gleam.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: adding core shader files for a Gaussian blur feature (BackgroundBlur.cpp/h and two HLSL shader files). |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Menu/BackgroundBlur.cpp(1 hunks)src/Menu/BackgroundBlur.h(1 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:
src/Menu/BackgroundBlur.hsrc/Menu/BackgroundBlur.cpp
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/Menu/BackgroundBlur.hsrc/Menu/BackgroundBlur.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
🧬 Code graph analysis (1)
src/Menu/BackgroundBlur.h (1)
src/Menu/BackgroundBlur.cpp (18)
Initialize(237-349)Initialize(237-237)RenderBackgroundBlur(636-731)RenderBackgroundBlur(636-636)CreateBlurTextures(351-440)CreateBlurTextures(351-351)PerformBlur(442-585)PerformBlur(442-442)Cleanup(587-611)Cleanup(587-587)SetIntensity(613-617)SetIntensity(613-613)GetIntensity(619-622)GetIntensity(619-619)IsEnabled(624-627)IsEnabled(624-624)GetTextureDimensions(629-634)GetTextureDimensions(629-629)
🪛 Clang (14.0.6)
src/Menu/BackgroundBlur.h
[error] 3-3: 'd3d11.h' file not found
(clang-diagnostic-error)
⏰ 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). (1)
- GitHub Check: Build plugin and addons
|
✅ A pre-release build is available for this PR: |
davo0411
changed the title
|
in cs we don't bake shaders like this |
cryingcat.gif. do you want a refactor with separate .hlsl for this function? I can do that if it means it will get merged / reviewed |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Menu/BackgroundBlur.cpp (2)
344-349: Source SRV leak on early returns persists.The
sourceSRVis still a raw pointer that leaks if any early return occurs before the cleanup block. This was flagged in a previous review.Wrap in
winrt::com_ptr:- ID3D11ShaderResourceView* sourceSRV = nullptr; - HRESULT hr = globals::d3d::device->CreateShaderResourceView(sourceTexture, nullptr, &sourceSRV); + winrt::com_ptr<ID3D11ShaderResourceView> sourceSRV; + HRESULT hr = globals::d3d::device->CreateShaderResourceView(sourceTexture, nullptr, sourceSRV.put());Then update usages accordingly.
376-391: IA input layout mismatch will cause draw failure.The vertex shader uses only
SV_VertexID, but ImGui's input layout (POSITION/TEXCOORD/COLOR) is still bound. D3D11 will reject theDraw(3, 0)calls. This was flagged in a previous review.Clear the input layout before blur passes:
+ ID3D11InputLayout* originalInputLayout = nullptr; + context->IAGetInputLayout(&originalInputLayout); + context->IASetInputLayout(nullptr); + context->VSSetShader(vertexShader.get(), nullptr, 0);Then restore it during state cleanup.
🧹 Nitpick comments (5)
src/Menu/BackgroundBlur.cpp (4)
554-574: Raw pointers in RenderBackgroundBlur risk leaks on unexpected failures.Multiple raw D3D11 pointers (
currentRTV,currentRT,currentTexture) are manually managed. While the explicit cleanup handles known failure paths, wrapping these inwinrt::com_ptrwould provide exception safety and simpler code.- ID3D11RenderTargetView* currentRTV = nullptr; - context->OMGetRenderTargets(1, ¤tRTV, nullptr); - - if (!currentRTV) { - return; - } - - // Get render target texture and its dimensions - ID3D11Resource* currentRT = nullptr; - currentRTV->GetResource(¤tRT); - - ID3D11Texture2D* currentTexture = nullptr; - HRESULT hr = currentRT->QueryInterface(__uuidof(ID3D11Texture2D), (void**)¤tTexture); + winrt::com_ptr<ID3D11RenderTargetView> currentRTV; + context->OMGetRenderTargets(1, currentRTV.put(), nullptr); + + if (!currentRTV) { + return; + } + + winrt::com_ptr<ID3D11Resource> currentRT; + currentRTV->GetResource(currentRT.put()); + + winrt::com_ptr<ID3D11Texture2D> currentTexture; + HRESULT hr = currentRT->QueryInterface(__uuidof(ID3D11Texture2D), currentTexture.put_void());
463-478: Incomplete D3D11 state restoration may affect subsequent rendering.The function saves/restores render targets, viewport, and rasterizer state, but doesn't save/restore vertex/pixel shaders, samplers, constant buffers, or the previous blend state. If ImGui (or other rendering code) expects its state to persist, this could cause issues.
Consider saving and restoring the full pipeline state, or document that callers must rebind their state after this function. ImGui typically rebinds its state before each draw, so this may be acceptable in practice.
95-101: Initialization failure is permanent with no recovery path.Once
initializationFailedis set,Initialize()will always returnfalsewithout retrying. If the failure was transient (e.g., device temporarily busy), the blur feature is permanently disabled until the game is restarted.Consider adding a
Reset()API or allowing retry under specific conditions to support graceful recovery. Based on coding guidelines recommending graceful degradation.
39-42: Unused constants declared.
BLUR_TEXT_CONTRAST_FACTORandGAUSSIAN_BLUR_SIGMAare declared but never used in the implementation. Remove them or add the intended functionality.-// Blur System Constants -// --------------------- -// Text contrast boost per unit blur: Compensates for reduced clarity behind blurred backgrounds -constexpr float BLUR_TEXT_CONTRAST_FACTOR = 0.05f; // 5% brightness boost at max blur - -// Gaussian blur sigma: Controls blur kernel spread (standard deviation) -constexpr float GAUSSIAN_BLUR_SIGMA = 2.0f;package/Shaders/Menu/BackgroundBlurHorizontal.hlsl (1)
1-61: Consider extracting shared code to a common .hlsli file.The vertex shader,
WEIGHTSarray, andVS_OUTPUTstruct are duplicated between horizontal and vertical shaders. Per maintainer feedback ("in cs we don't bake shaders like this"), consider extracting common definitions to a shared include file:// BackgroundBlurCommon.hlsli cbuffer BlurBuffer : register(b0) { float4 TexelSize; int4 BlurParams; }; SamplerState LinearSampler : register(s0); Texture2D InputTexture : register(t0); struct VS_OUTPUT { ... }; VS_OUTPUT VS_Main(uint vertexID : SV_VertexID) { ... } static const float WEIGHTS[8] = { ... };Then each pass shader only defines its unique
PS_Main.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package/Shaders/Menu/BackgroundBlurHorizontal.hlsl(1 hunks)package/Shaders/Menu/BackgroundBlurVertical.hlsl(1 hunks)src/Menu/BackgroundBlur.cpp(1 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:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslpackage/Shaders/Menu/BackgroundBlurVertical.hlslsrc/Menu/BackgroundBlur.cpp
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/Menu/BackgroundBlur.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslpackage/Shaders/Menu/BackgroundBlurVertical.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslpackage/Shaders/Menu/BackgroundBlurVertical.hlslsrc/Menu/BackgroundBlur.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 (2)
package/Shaders/Menu/BackgroundBlurVertical.hlsl (1)
4-26: Shader setup and vertex shader look correct.The cbuffer layout, resource bindings, and fullscreen triangle generation via
SV_VertexIDare correctly implemented and consistent with the horizontal pass shader.package/Shaders/Menu/BackgroundBlurHorizontal.hlsl (1)
29-38: Same weight normalization issue as vertical shader.The
WEIGHTSarray andPS_Mainhave the same normalization problem as the vertical pass—output will be brightened when all samples are used. See the vertical shader review for the fix.Also applies to: 40-60
⛔ Skipped due to learnings
Learnt from: CR Repo: doodlum/skyrim-community-shaders PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-17T18:37:35.839Z Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)Learnt from: jiayev Repo: doodlum/skyrim-community-shaders PR: 0 File: :0-0 Timestamp: 2025-08-03T18:37:19.690Z Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Menu/BackgroundBlur.cpp (1)
345-472: Bind a null IA input layout for the SV_VertexID fullscreen pass (otherwise Draw can fail)This VS only consumes
SV_VertexID, butPerformBlurnever clears ImGui’s POSITION/TEXCOORD/COLOR input layout. On D3D11 this typically yieldsE_INVALIDARGfromDraw, and the blur won’t render. Capture the existing input layout, set it tonullptrfor the blur passes, then restore it during state restore. This is the same issue previously flagged on this file and still appears unresolved.Suggested patch:
@@ - // Save current state - ID3D11RenderTargetView* originalRTV = nullptr; - ID3D11DepthStencilView* originalDSV = nullptr; - context->OMGetRenderTargets(1, &originalRTV, &originalDSV); - - D3D11_VIEWPORT originalViewport; - UINT numViewports = 1; - context->RSGetViewports(&numViewports, &originalViewport); - - ID3D11RasterizerState* originalRS = nullptr; - context->RSGetState(&originalRS); + // Save current state + ID3D11RenderTargetView* originalRTV = nullptr; + ID3D11DepthStencilView* originalDSV = nullptr; + context->OMGetRenderTargets(1, &originalRTV, &originalDSV); + + D3D11_VIEWPORT originalViewport; + UINT numViewports = 1; + context->RSGetViewports(&numViewports, &originalViewport); + + ID3D11RasterizerState* originalRS = nullptr; + context->RSGetState(&originalRS); + + // Fullscreen triangle VS only uses SV_VertexID – IA layout must be null + ID3D11InputLayout* originalInputLayout = nullptr; + context->IAGetInputLayout(&originalInputLayout); + context->IASetInputLayout(nullptr); @@ - // Restore state - context->OMSetRenderTargets(1, &originalRTV, originalDSV); - context->OMSetBlendState(nullptr, nullptr, 0xFFFFFFFF); - context->PSSetShaderResources(0, 1, &nullSRV); - context->RSSetState(originalRS); - context->RSSetScissorRects(0, nullptr); + // Restore state + context->OMSetRenderTargets(1, &originalRTV, originalDSV); + context->OMSetBlendState(nullptr, nullptr, 0xFFFFFFFF); + context->PSSetShaderResources(0, 1, &nullSRV); + context->RSSetState(originalRS); + context->RSSetScissorRects(0, nullptr); + context->IASetInputLayout(originalInputLayout); @@ - // Cleanup - if (sourceSRV) - sourceSRV->Release(); - if (originalRTV) - originalRTV->Release(); - if (originalDSV) - originalDSV->Release(); - if (originalRS) - originalRS->Release(); + // Cleanup + if (sourceSRV) + sourceSRV->Release(); + if (originalRTV) + originalRTV->Release(); + if (originalDSV) + originalDSV->Release(); + if (originalRS) + originalRS->Release(); + if (originalInputLayout) + originalInputLayout->Release();This keeps ImGui’s IA state intact while making the fullscreen blur draws valid for the SV_VertexID‑only vertex shader.
🧹 Nitpick comments (2)
package/Shaders/Menu/BackgroundBlurHorizontal.hlsl (1)
28-38: Clarify weights comment to match runtime normalizationThe weights are described as “precomputed normalized Gaussian weights”, but you also normalize at runtime based on
samples. Consider rephrasing to something like “Precomputed Gaussian weights (normalized per‑sample at runtime)” to avoid confusion for future tweaks.src/Menu/BackgroundBlur.cpp (1)
21-23: Make downsample/blur resolution comments consistent (eighth vs quarter res)The code uses
DOWNSAMPLE_FACTOR = 8(one‑eighth resolution) anddownsampledWidth/Heighteverywhere, but some comments still refer to “quarter resolution” / “quarter‑res”. To avoid confusion during future tuning, consider aligning the wording to “eighth‑resolution” in all these spots.Also applies to: 41-42, 357-363, 390-392, 434-452
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package/Shaders/Menu/BackgroundBlurHorizontal.hlsl(1 hunks)package/Shaders/Menu/BackgroundBlurVertical.hlsl(1 hunks)src/Menu/BackgroundBlur.cpp(1 hunks)src/Menu/BackgroundBlur.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Menu/BackgroundBlur.h
🧰 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:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslsrc/Menu/BackgroundBlur.cpppackage/Shaders/Menu/BackgroundBlurVertical.hlsl
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/Menu/BackgroundBlur.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslsrc/Menu/BackgroundBlur.cpppackage/Shaders/Menu/BackgroundBlurVertical.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Menu/BackgroundBlurHorizontal.hlslpackage/Shaders/Menu/BackgroundBlurVertical.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 (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 (4)
package/Shaders/Menu/BackgroundBlurVertical.hlsl (2)
19-26: Fullscreen triangle VS is correct for this passThe SV_VertexID‑based fullscreen triangle setup and Y‑flip look standard and compatible with the PS expectations; no issues here.
40-68: Vertical blur + runtime normalization look correctPer‑pixel normalization over only the actually used symmetric samples keeps brightness stable for any
BlurParams.x ≤ 15, and edge handling relies cleanly on clamp sampling; this implementation looks solid.package/Shaders/Menu/BackgroundBlurHorizontal.hlsl (1)
40-68: Horizontal blur pass mirrors vertical correctlySampling, normalization, and use of
TexelSize.xfor horizontal offsets are consistent with the vertical pass; behavior and brightness should match as intended.src/Menu/BackgroundBlur.cpp (1)
498-505: The file referenced in this review (src/Menu/BackgroundBlur.cpp) cannot be found in the repository.The repository contains only shader files in the
package/Shaders/Menu/directory. The C++ file mentioned in the review comment does not exist at the pathsrc/Menu/BackgroundBlur.cpp. Additionally, a search forcurrentIntensityacross the entire repository returns zero results, indicating this symbol is not defined or used anywhere in the codebase.This suggests the review comment may be referencing code from a different branch, a previous state of the PR, or an incomplete repository state. Please verify that the correct branch is checked out and that the PR includes the expected C++ source files at
src/Menu/BackgroundBlur.cpp.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Menu/BackgroundBlur.cpp (2)
338-343: Prefer RAII (winrt::com_ptr) for local COM objects to future‑proof against early returns.
sourceSRV,originalRTV,originalDSV, andoriginalRSare managed manually withRelease(). Today, there are no early returns between creation and cleanup for the success path inPerformBlur, but any future changes that introduce new exit points could re‑introduce leaks.Given that globals in this file already use
winrt::com_ptr, consider switching these locals towinrt::com_ptras well and dropping the manualRelease()calls. This aligns with the existing style and makes the function safer to evolve.Also applies to: 384-389, 451-456, 465-472
345-473: Fix IA input layout mismatch and preserve caller scissor state.The blur passes use a full‑screen triangle (
Draw(3, 0)) with a VS that likely only consumesSV_VertexID, but we never clear ImGui’s POSITION/UV/COLOR input layout or vertex buffers. With a mismatched input-layout/VS signature, D3D11 can fail the draw withE_INVALIDARGand the blur won’t render. We also overwrite the scissor rectangles and only partially restore them.Recommended changes:
- Capture and clear the IA input layout for the blur passes, then restore it:
- ID3D11RasterizerState* originalRS = nullptr; - context->RSGetState(&originalRS); + ID3D11RasterizerState* originalRS = nullptr; + context->RSGetState(&originalRS); + + ID3D11InputLayout* originalInputLayout = nullptr; + context->IAGetInputLayout(&originalInputLayout); + context->IASetInputLayout(nullptr); @@ - // Restore state - context->OMSetRenderTargets(1, &originalRTV, originalDSV); + // Restore state + context->OMSetRenderTargets(1, &originalRTV, originalDSV); context->OMSetBlendState(nullptr, nullptr, 0xFFFFFFFF); context->PSSetShaderResources(0, 1, &nullSRV); context->RSSetState(originalRS); - context->RSSetScissorRects(0, nullptr); + context->RSSetScissorRects(0, nullptr); + context->IASetInputLayout(originalInputLayout); @@ - if (originalRS) - originalRS->Release(); + if (originalRS) + originalRS->Release(); + if (originalInputLayout) + originalInputLayout->Release();Optionally, you can also clear IA vertex buffers during the blur passes (
IASetVertexBufferswithnullptrs) and restore them afterward for extra robustness.
- Scissor rectangles:
Right now we set a single scissor rect for the blur and then call
RSSetScissorRects(0, nullptr)on restore. If the caller had scissoring enabled with custom rectangles, we’ll discard that configuration. If upstream rendering relies on scissor, consider capturing the existing rects withRSGetScissorRectsand restoring them instead of hard‑resetting to zero rects.
🧹 Nitpick comments (5)
src/Menu/BackgroundBlur.cpp (5)
9-17: Include headers forstd::mutexandstd::string_viewexplicitly.This file uses
std::mutex(forresourceMutex) andstd::string_view(in the window-name check) but does not include<mutex>/<string_view>here. Relying on transitive includes from other headers can break under different toolchains; consider adding the explicit standard headers to keep this TU self-contained.
21-23: Clarify downsampling comments vs. actual resolution factor.
DOWNSAMPLE_FACTORis set to 8 with a comment “eighth resolution”, but the texture comments say “quarter-res”. SincedsWidth = width / DOWNSAMPLE_FACTOR, these textures are actually at 1/8 resolution, not 1/4. Updating the comments to match the implementation will avoid confusion during future tuning.Also applies to: 41-52
434-455: Confirm whether the final composition should blur again or just upscale/copy.The final composition pass:
- Reuses
verticalPixelShader.- Uses the same blur constants (
sampleCount,texelSize,BLUR_INTENSITY) as the vertical blur pass.- Changes only the viewport and input SRV (
blurSRV2), relying on bilinear filtering for upscale.If the intent is just to sample the pre‑blurred downsampled texture once and blend it behind the window (no extra blur), this setup likely applies an unnecessary additional vertical blur during the upscale. In that case, you may want either:
- A dedicated “copy/upscale” PS, or
- Reusing the horizontal/vertical PS but resetting constants to a single sample (e.g.,
sampleCount = 1, zeroing offsets).If the extra blur is intentional, consider updating the comments to make it explicit that the final pass is another blur step, not a pure upscale.
507-520: Guardenabled/initializedwith the same mutex or atomics if toggled cross‑thread.
SetEnabled,GetEnabled, andIsEnabledread/writeenabled(andIsEnabledalso readsinitialized) withoutresourceMutex. If these flags can be flipped from threads other than the render thread, this is a data race in standard C++.If cross‑thread access is possible, either:
- Protect these reads/writes with the same mutex used for resources, or
- Make them
std::atomic<bool>.If everything runs strictly on the render thread, this is fine as is.
578-623: Avoid recomputing the full blur for every ImGui window.
RenderBackgroundBlurloops over all ImGui windows and callsPerformBlur(currentTexture, currentRTV, windowMin, windowMax)for each one. Each call toPerformBlur:
- Downsamples the entire source texture.
- Runs horizontal + vertical blur over the full downsampled area.
- Then composites only the scissored region for that window.
This is correct but O(N_windows) in full‑frame blur cost. For multiple blurred windows, it would be much cheaper to:
- Downsample + blur the full frame once per frame into a persistent blurred texture.
- In the loop, only do the final composition step (set scissor + blend from the cached blurred texture).
If performance becomes an issue with several blurred menus, refactoring towards a single blur per frame with per‑window composition will help.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu/BackgroundBlur.cpp(1 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:
src/Menu/BackgroundBlur.cpp
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/Menu/BackgroundBlur.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/Menu/BackgroundBlur.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
2 participants
Does nothing on its own, just here to split prs up easier.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.