feat: add lens effects #1363
feat: add lens effects #1363
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Feature wiring (C++ globals & registration)src/Feature.cpp, src/Globals.h, src/Globals.cpp, src/State.cpp |
Add and expose a global LensEffects feature, include its header, register it in the feature list, and invoke lensEffects.CheckOverride() from State::Draw() when loaded. |
LensEffects feature (header + implementation)src/Features/LensEffects.h, src/Features/LensEffects.cpp |
New LensEffects class and API: settings/presets/JSON IO; ConstBuffer and GPU update; D3D11 resources, samplers, UAV/SRV/texture pointers; shader compilation and resource setup; per-pass pipelines (Occlusion, Burst, SunGlare, LensGlare, Halo, Ghosts, CA, Ice); weather/sun handling; hook/thunk declarations and Install(); ImGui UI and preset management. |
LensEffects shader module & configfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl, features/Lens Effects/Shaders/Features/LensEffects.ini |
Add a large multi-pass HLSL module (IO structs, cbuffers, samplers, textures/UAVs, macros, many VS/PS entry points) and a new LensEffects.ini containing [Info] Version = 1-0-0. |
Sky shader integrationpackage/Shaders/Sky.hlsl |
Under #if defined(LENS_EFFECTS) && defined(DEFERRED) add LensEffects resource declarations (RWTexture/Texture variants for CLOUDS/DITHER) and PS logic to load/write/atomically update lens-effect parameters. |
Shared shader utilities additionspackage/Shaders/Common/Color.hlsli, package/Shaders/Common/Math.hlsli, package/Shaders/Common/Random.hlsli, package/Shaders/Common/CoordMath.hlsli |
Add luminance macros and RGB/HSV/chrominance helpers; vector min/max/sum, ufmod, nRoot, sincos2, LinearStep/smootherstep/MapRange/Diffraction; RandomSH overloads; CoordMath utilities (angle/vector conversions, polar/cartesian, atlas fetch helpers, rect test). |
ISSAO composite tweakpackage/Shaders/ISSAOComposite.hlsl |
Add a NaN clamp for sourceColor.x using a bitmask test to set NaN to 0.0 after sampling. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Engine as Engine
participant State as State::Draw
participant LensFX as LensEffects (C++)
participant GPU as GPU
participant SkyPS as Sky PS
Engine->>State: Render frame
State->>LensFX: if (loaded) CheckOverride()
LensFX->>LensFX: Update weather/sun, build per-pass list
LensFX->>GPU: Create/Bind CBs, SRVs, UAVs, Samplers
GPU->>GPU: Execute per-pass shaders (Occlusion -> Burst -> SunGlare -> LensGlare -> Halo -> Ghosts -> CA/Ice)
alt Deferred Sky path (LENS_EFFECTS && DEFERRED)
SkyPS->>GPU: read/write LensEffects UAV/SRV (params/counters, atomics)
end
GPU-->>Engine: Composited frame
sequenceDiagram
autonumber
participant LensFX as LensEffects (C++)
participant HLSL as LensEffects.hlsl
participant Resources as D3D11
LensFX->>Resources: Create SunLUT, textures, SRVs/UAVs, samplers, Settings CB
LensFX->>HLSL: Bind SettingsCB and resources, set defines
HLSL->>HLSL: Run multi-pass VS/PS with per-pass IO and write to UAV/SRVs
HLSL-->>Resources: Persist occlusion/flare outputs
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Areas to focus:
- Hook/thunk installation and relocation/detour safety in
LensEffects::Hooks::Install. - Consistency of resource bindings (register slots, UAV/SRV types) between HLSL and the C++ resource setup.
- Lifetime and thread-safety of D3D11 resources, UAV writes, and atomic operations invoked from Sky shader.
- Shader define gating (CLOUDS / DITHER / DEFERRED / LENS_EFFECTS) correctness across code paths.
- JSON preset IO and ImGui UI bindings for settings persistence.
- Hook/thunk installation and relocation/detour safety in
Possibly related PRs
- perf: reduce overhead of hot functions #1290 — touches
State::Draw; related due to insertedlensEffects.CheckOverride()call. - feat: add Weather Picker #1167 — similar feature registration changes in globals/feature list.
- feat: drs based upscaling #1443 — related edits touching feature registration/bootstrap paths.
Suggested reviewers
- doodlum
- alandtse
- jiayev
Poem
I nibble pixels, chase the sun's bright beam,
Starbursts sparkle, ghosts in silver seam,
Halos and frost in tiny arcs I weave,
I hop through shaders — magic I conceive. 🐇✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 1.75% 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 'feat: add lens effects' clearly and concisely describes the main change—introducing a new lens effects feature to the codebase. |
✨ 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
package/Shaders/Sky.hlsl (1)
171-178: Well-structured lens effects integration.The conditional texture declarations and pixel shader modifications are properly implemented. The use of atomic operations for cloud pixel counting is appropriate.
Consider adding a comment documenting the expected layout of the LensEffects texture (e.g., what data is stored at coordinates (3,0) for SunParams and (2,0) for color).
Also applies to: 267-278
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli (1)
120-122: Consider documenting FastAtan approximation accuracy.The FastAtan function uses a polynomial approximation that may have reduced accuracy at extreme values.
Add a comment documenting the expected accuracy range and error bounds of this approximation, such as:
// FastAtan: Polynomial approximation of atan(x) // Accuracy: ±0.01 radians for |x| < 1.0 float FastAtan(float x){ return x * (-0.1784f * abs(x) - 0.0663f * x * x + 1.0301f); }src/Features/LensEffects.h (6)
5-11: Consider completing the singleton pattern implementation.While the current implementation works, consider making the constructor private and deleting copy/move operations to prevent accidental instantiation or copying.
Add after line 11:
private: LensEffects() = default; ~LensEffects() = default; LensEffects(const LensEffects&) = delete; LensEffects& operator=(const LensEffects&) = delete; LensEffects(LensEffects&&) = delete; LensEffects& operator=(LensEffects&&) = delete; public:
18-18: Remove unnecessary trailing comment.The trailing
//comment serves no purpose.- virtual inline bool SupportsVR() override { return false; }; // + virtual inline bool SupportsVR() override { return false; };
216-216: Initialize padding array.For consistency and to avoid potential issues, initialize the padding array.
- float _pad[2]; + float _pad[2] = {};
339-358: Consider extracting magic numbers to named constants.The function contains several magic numbers that would benefit from being named constants for better maintainability.
Add constants before the function:
private: static constexpr float BLADE_SPLAY_THRESHOLD = 0.01f; static constexpr float BLADE_WIDTH_SCALE = 0.25f; static constexpr float BLADE_TAPER_OFFSET = 80.0f; static constexpr float BLADE_LENGTH_FACTOR = 0.95f; static constexpr float SG_SCALE_FACTOR = 0.5f; static constexpr float GH_SCALE_FACTOR = 0.5f; static constexpr float HL_LINE_WIDTH_SCALE = 0.1f; static constexpr float RAYS_WIDTH_MIN = 11.0f; static constexpr float RAYS_WIDTH_MAX = 7.0f;
520-527: Document the intentional override behavior.The function captures the original result but always returns true. Add a comment explaining this intentional behavior.
static bool thunk(void* shader, RE::NiCamera* camera, uint64_t unk) { + // Intentionally always return true to force rendering regardless of original condition bool result = func(shader, camera, unk); return true; }
570-570: Add newline at end of file.Files should end with a newline character.
src/Features/LensEffects.cpp (1)
1019-1025: Improve error handling for file operations.While the code catches filesystem exceptions, it continues execution which might leave the system in an inconsistent state.
Consider returning early or throwing after logging the error to prevent partial initialization:
} catch (const std::filesystem::filesystem_error& e) { logger::error("[Lens Effects] Filesystem error when creating {}: {}", customSettingsPath, e.what()); + throw std::runtime_error("Failed to create settings file"); } catch (const std::exception& e) { logger::error("[Lens Effects] Unexpected error when creating {}: {}", customSettingsPath, e.what()); + throw std::runtime_error("Failed to create settings file"); }features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (3)
204-204: Document the purpose of pragma warning directives.The code disables warning 3206 but doesn't explain why. Consider adding a comment explaining what warning is being suppressed and why it's safe to ignore.
+// Disable warning about race condition in UAV access - handled by temporal averaging #pragma warning(disable:3206)Also applies to: 224-224
540-549: Consider early exit optimization in edge detection loop.The edge detection loop continues even after finding edges that would clip the pixel. Consider breaking early when a clipping condition is met.
[loop] for(int i = 0; i < UIGhostShape; ++i){ float2 Edge = (input.Vertices[i+1] - input.Vertices[i]) * float2(-1.0, 1.0); [unroll] for(int j=0; j<3; ++j){ float Local = dot(Edge.yx, input.Vertices[i] - GhostOffset[j]); float EdgeDist = lerp(Local, Ghost[j], inv(Local * Local) * UIGhostRoundness); Ghost[j] = min(Ghost[j], EdgeDist); } + if (max3(Ghost) <= 0) break; }
922-933: Incomplete rain shader implementation.The rain pixel shader returns zero values without any implementation. This appears to be a placeholder.
Would you like me to help implement the rain effect shader or create an issue to track this TODO?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (11)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
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.
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: isreflectionsraytracing.hlsl and isworldmap.hlsl in the skyrim-community-shaders repository are imag...
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/Globals.hpackage/Shaders/ISSAOComposite.hlslsrc/Feature.cppsrc/Globals.cppsrc/State.cpppackage/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslisrc/Features/LensEffects.hsrc/Features/LensEffects.cpp
📚 Learning: in the skyrim-community-shaders project, boolean flags in c++ structs that interface with hlsl shade...
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:
src/Globals.hpackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: default parameter values are supported in the hlsl compiler used by the skyrim-community-shaders pro...
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:
src/Globals.hpackage/Shaders/ISSAOComposite.hlslsrc/Feature.cppsrc/Globals.cppsrc/State.cpppackage/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: in the skyrim-community-shaders project, boolean flags in c++ structs that interface with hlsl shade...
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:
src/Globals.hpackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: in the skyrim-community-shaders project, simple scalar constants in hlsl shaders use #define (e.g., ...
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/ISSAOComposite.hlslpackage/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: in the skyrim community shaders project, the `smoothdrawcalls` array in the `state` class is declare...
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Applied to files:
package/Shaders/ISSAOComposite.hlslsrc/State.cpp
📚 Learning: in the skyrim-community-shaders repository, file deletion error handling improvements that replace e...
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.
Applied to files:
package/Shaders/ISSAOComposite.hlslpackage/Shaders/Sky.hlsl
📚 Learning: in the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabl...
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
package/Shaders/ISSAOComposite.hlslsrc/State.cpppackage/Shaders/Sky.hlsl
📚 Learning: in src/feature.cpp, when an obsolete feature's ini file is deleted, the feature should be silently d...
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
src/Feature.cpp
📚 Learning: when reviewing prs, always clarify the scope if there are multiple related features or dependencies....
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
package/Shaders/Sky.hlslsrc/Features/LensEffects.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 (26)
features/Lens Effects/Shaders/Features/LensEffects.ini (1)
1-2: LGTM! Clean configuration file structure.The INI file follows the expected format for feature configuration files in this codebase, with proper version specification that will be compatible with the feature loading system.
package/Shaders/ISSAOComposite.hlsl (1)
135-135: Good defensive programming practice.The NaN/infinity check for the red channel adds robustness to the shader by ensuring invalid floating-point values are sanitized. The bitwise implementation correctly identifies both positive and negative infinity/NaN values using IEEE 754 representation.
src/Feature.cpp (2)
16-16: LGTM! Proper feature header inclusion.The include follows the alphabetical ordering pattern used for other feature headers.
226-227: LGTM! Correct feature list integration.The LensEffects feature is properly added to the feature list, ensuring it participates in the engine's feature lifecycle management alongside other features.
src/Globals.cpp (2)
25-25: LGTM! Proper feature header inclusion.The include follows the alphabetical ordering pattern used for other feature headers.
85-85: LGTM! Correct global instance declaration.The global LensEffects instance follows the established pattern used by other features in the codebase, ensuring proper integration with the global state management system.
src/State.cpp (3)
11-11: LGTM! Proper feature header inclusion.The include is correctly placed alongside other feature headers.
28-28: LGTM! Correct local reference declaration.The local reference follows the established pattern used by other features for performance optimization in the Draw method.
42-44: LGTM! Proper feature integration in rendering pipeline.The conditional call to
CheckOverride()follows the established pattern used by other features, ensuring the lens effects system can participate in the rendering override process when the feature is loaded.src/Globals.h (1)
12-12: LGTM! Properly integrated into global declarations.The forward declaration and extern instance follow the established patterns and maintain alphabetical ordering.
Also applies to: 86-86
src/Features/LensEffects.h (9)
1-4: LGTM!Header guard and includes are appropriate.
41-83: Well-organized resource declarations.All Direct3D 11 resources are properly declared and initialized.
84-117: LGTM!Runtime state variables are properly declared and initialized.
118-123: LGTM!Weather disable array is properly declared. The double braces are valid C++ syntax for aggregate initialization.
225-296: Well-designed settings structure.The ColdSettings struct is properly organized with clear parameter grouping and initialization.
317-330: Properly aligned constant buffer structure.The constant buffer is correctly aligned for GPU usage with appropriate padding.
360-378: LGTM!The shader enum is well-organized with clear naming.
190-192: Use bool for boolean flags in HL settings.- uint HL_EnableExp = true; - uint HL_FlipExpOffset = false; + bool HL_EnableExp = true; + bool HL_FlipExpOffset = false;⛔ Skipped due to learnings
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.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.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.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.
140-164: Use bool instead of uint for boolean flags.For clarity and type safety, boolean enable flags should use
booltype.- uint SB_EnableBlades = true; + bool SB_EnableBlades = true;- uint SB_EnableRays = true; + bool SB_EnableRays = true;- uint GH_EnableClampOffset = true; + bool GH_EnableClampOffset = true;⛔ Skipped due to learnings
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.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.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.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.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.Learnt from: davo0411 PR: doodlum/skyrim-community-shaders#1070 File: src/State.cpp:79-83 Timestamp: 2025-05-30T11:44:15.542Z Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.src/Features/LensEffects.cpp (5)
9-41: LGTM! Well-structured settings serialization.The JSON serialization macros are properly configured for all settings structures with appropriate default handling.
173-208: Clean implementation of shader dispatch system.The frame-based update logic and function pointer dispatch table provide an efficient way to manage shader selection.
575-661: Well-structured rendering pipeline hooks.The hook implementations correctly intercept and modify shader selection based on sky object types and rendering conditions.
448-450: Verifyprecipstartdeclaration and initializationI wasn’t able to locate
LensEffects.cppor theprecipstartvariable in the codebase. Please confirm that:
- The correct file path (e.g.
src/Features/LensEffects.cpp) is used.precipstartis declared and given an initial value before any conditional use.
814-816: Verify array bounds for ghost passes indexingEnsure that
ghostpassescan never exceed the number of elements incoldSettings.GH_Params(or be negative), otherwiseGH_Params[i]will be out of range.• Locate where
ghostpassesis declared and see what its maximum value can be
• Inspect the type/size ofcoldSettings.GH_Params(fixed‐size array vs. dynamic container)
• Add a guard or clamp before the loop, for example:size_t count = std::min<size_t>(ghostpasses, coldSettings.GH_Params.size()); for (int i = int(count) - 1; i >= 0; --i) { // … }• Alternatively, assert at runtime:
assert(ghostpasses <= coldSettings.GH_Params.size());Please confirm these bounds checks are in place before merging.
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (2)
7-183: Well-structured shader inputs and constant buffer organization.The shader structures are properly defined with appropriate interpolation qualifiers. The constant buffer layout follows best practices with proper alignment considerations.
307-438: Excellent implementation of starburst effects.The shader efficiently simulates aperture blade effects with proper optimization through loop unrolling and conditional branching. The mathematical calculations for blade geometry and ray generation are well-implemented.
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Thanks. Doodlum or profjack may have more comments but see if you can see patterns we've adapted for other features. I did notice you're doing your own settings files which is an anti pattern but perhaps it's fine since you have a lot of settings. Please note doodlum may push back. The trend has been to remove settings and rely on sane presets.
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
Yeah the main reason for saving settings separately is for weather modders and others who might want to package their mods with custom lens settings, otherwise they would have to override people’s settings for all features which is not ideal imo. As for everything else, sounds reasonable, I’ll get onto it. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Features/LensEffects.cpp (2)
42-63: Still no null-check afterUtil::CompileShaderPrevious review already covered this – the calls still risk dereferencing nullptrs if a shader fails to compile.
Addif (!compiled) { log+early return }after each call or wrap with helper.
130-141: Replace rawnewwith smart pointers to avoid leaks
SettingsCBandrenderdataare heap-allocated but never freed. Usestd::unique_ptr(or store by value) so lifetime is automatic:-SettingsCB = new ConstantBuffer(ConstantBufferDesc<ConstBuffer>()); -renderdata = new Setup::LF_RenderData; +SettingsCB = std::make_unique<ConstantBuffer>(ConstantBufferDesc<ConstBuffer>()); +renderdata = std::make_unique<Setup::LF_RenderData>();src/Features/LensEffects.h (2)
387-391:passesdoneshould be private – breaks encapsulationField is still public despite prior feedback. Make it private and expose controlled accessors to avoid accidental misuse.
471-472: Add informative message to thrown exceptionThrowing
std::out_of_range("")gives no clue what went wrong. Provide context:-throw std::out_of_range(""); +throw std::out_of_range("LensEffects: effect descriptor not found");features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
252-254: Potential division-by-zero persists inGetWeatherFactor
CloudFactor / delta((Math::PI * (SunRadius * SunRadius)))can still hit zero ifSunRadiusis 0, anddelta()only guards the denominator ofx / y, not the outer division. Add a max() with small epsilon or early exit to guarantee non-zero divisor.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (11)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Feature.cpp
- src/State.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- features/Lens Effects/Shaders/Features/LensEffects.ini
- src/Globals.cpp
- package/Shaders/ISSAOComposite.hlsl
- src/Globals.h
- package/Shaders/Sky.hlsl
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
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.
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-29T00:27:42.717Z
Learning: When reviewing PRs, the committer's description should inform analysis of changes and may take precedence to conclusions when reasonable, but the choice of commit type should still be independently evaluated for compliance with conventional commits. Both committer intent and conventional commit standards should be considered to determine if the classification is defensible.
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.
📚 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/LensEffects.hfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
PR: doodlum/skyrim-community-shaders#1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-08-05T17:40:44.796Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.796Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslisrc/Features/LensEffects.cpp
📚 Learning: 2025-05-30T11:44:15.542Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-08-05T18:22:40.542Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.542Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslisrc/Features/LensEffects.cpp
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslisrc/Features/LensEffects.cpp
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-08-05T18:13:03.077Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.077Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-06-17T09:27:49.594Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
src/Features/LensEffects.cpp
🔇 Additional comments (1)
src/Features/LensEffects.cpp (1)
234-244: Blend-state creation path can still dereference nullptr
OMGetBlendStatedoesn’t return anHRESULT; you must validate that it actually wrote a pointer before callingGetDesc. If the device returnsnullptr,GetDescwill crash.context->OMGetBlendState(&BlendState[1], nullptr, nullptr); -if (BlendState[1]) { +if (BlendState[1]) { D3D11_BLEND_DESC SrcBlendDesc = {}; BlendState[1]->GetDesc(&SrcBlendDesc); @@ - globals::d3d::device->CreateBlendState(&SrcBlendDesc, &BlendState[0]); + DX::ThrowIfFailed( + globals::d3d::device->CreateBlendState(&SrcBlendDesc, &BlendState[0])); } else { + logger::error("[Lens Effects] Failed to retrieve current blend state"); + return; }Likely an incorrect or invalid review comment.
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
src/Features/LensEffects.cpp (3)
40-63: Missing nullptr checks after shader compilation
Util::CompileShadercan returnnullptr; subsequent use will crash. Guard each return or early-out on failure.
127-129: Texture loading errors ignored
CreateDDSTextureFromFilefailures are silently ignored; resulting null SRVs will AV later. Capture theHRESULTand bail or fall back.
130-141: Raw pointers leak – prefer smart pointers
SettingsCBandrenderdataare allocated withnewand never deleted. Switch tostd::unique_ptras done elsewhere in the codebase.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
203-206: Negative LUT index on first frame
SunLUT_AT[int2(idx-1,0)]underflows whenFrame == 0, writing outside UAV bounds (undefined behaviour). Clamp or wrap the index.src/Features/LensEffects.h (2)
365-390:passesdoneshould be private (duplicate of earlier feedback)
passesdoneis still public; encapsulate it and add accessors as previously suggested.
465-472: Provide a descriptivestd::out_of_rangemessage (duplicate)Throwing with an empty string hinders debugging. Supply a clear message.
🧹 Nitpick comments (4)
src/Feature.cpp (1)
226-228: Verify menu ordering and VR support for LensEffects in the feature list.
- Adding
lensEffectshere controls menu order. Confirm intended placement relative to other image-space/post-process features (e.g., near ScreenSpace* or VolumetricLighting) to keep UX consistent.- Make sure
LensEffects::SupportsVR()returns the correct value so the VR feature list filter behaves as expected.If you want, I can scan State/LensEffects to confirm SupportsVR and propose a minimal reorder to match the project’s grouping.
src/State.cpp (1)
28-29: Global alias introduced but never used
auto& lensEffects = globals::features::lensEffects;is created only to be referenced once two lines below, making the alias unnecessary.
Either drop the temporary or keep it and reuse it consistently for symmetry with the other feature refs.features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli (1)
70-80: Random helpers duplicate existing engine utilities
Random*functions duplicate identical hash-based randoms already inCommon/Glints/noisegen.cs.hlsl. Consider moving them toCommon/Random.hlsli(referenced byUtility.hlsl) and sharing one implementation.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
334-336: Aperture blade array may overrunThe loop unconditionally writes 16 elements (
0…15) intoApertureBladesbut the declaring struct only holds 16 slots and laterUIBladeVertscan be < 16. Access is safe, but the last write duplicates index 0 to 15 – unnecessary and risks confusion. Consider:[loop] for(uint i=0;i<UIBladeVerts && i<16;++i) …
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (11)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
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.
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-29T00:27:42.717Z
Learning: When reviewing PRs, the committer's description should inform analysis of changes and may take precedence to conclusions when reasonable, but the choice of commit type should still be independently evaluated for compliance with conventional commits. Both committer intent and conventional commit standards should be considered to determine if the classification is defensible.
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.
📚 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:
package/Shaders/ISSAOComposite.hlslsrc/Globals.cppsrc/State.cppsrc/Feature.cppsrc/Globals.hpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.hfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.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:
package/Shaders/ISSAOComposite.hlslpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 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:
package/Shaders/ISSAOComposite.hlslsrc/Globals.cppsrc/State.cppsrc/Feature.cppsrc/Globals.hpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.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:
package/Shaders/ISSAOComposite.hlslsrc/Globals.hpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.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:
package/Shaders/ISSAOComposite.hlslsrc/Globals.hpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-08-05T17:40:44.796Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.796Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
package/Shaders/ISSAOComposite.hlslsrc/Globals.cppsrc/State.cppsrc/Feature.cppsrc/Globals.hpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-05-30T11:44:15.542Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Applied to files:
package/Shaders/ISSAOComposite.hlslsrc/State.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
package/Shaders/ISSAOComposite.hlslpackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
package/Shaders/ISSAOComposite.hlslsrc/State.cpppackage/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-08-05T18:22:40.542Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.542Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Globals.cppsrc/Feature.cppsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-06-17T09:27:49.594Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
src/Feature.cppsrc/Features/LensEffects.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
package/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-08-05T18:13:03.077Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.077Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
package/Shaders/Sky.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
PR: doodlum/skyrim-community-shaders#1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
🔇 Additional comments (10)
features/Lens Effects/Shaders/Features/LensEffects.ini (1)
1-2: Ensure minimal-version registration for LensEffects in FeatureVersions.The Version string format
1-0-0matches the loader (hyphens converted to dots). Verify thatFeatureVersions::FEATURE_MINIMAL_VERSIONScontains an entry for"LensEffects"; otherwise Feature::Load will flag it as UNKNOWN and disable the feature.Example addition outside this file (for clarity):
// FeatureVersions.h / .cpp { "LensEffects", REL::Version(1, 0, 0) },src/Feature.cpp (1)
16-16: Include looks correct and follows existing pattern.No issues. Confirms LensEffects is visible to the feature registry.
src/Globals.h (2)
12-12: Forward declaration is fine and consistent with existing feature declarations.
86-87: Global instance declaration aligns with the pattern for other features.Ensure the corresponding definition exists in Globals.cpp (it does) and that LensEffects defers device-dependent work until after OnInit/ReInit.
src/Globals.cpp (2)
25-25: Include addition is correct.
85-86: Global instance definition matches header and existing features.Confirm that LensEffects does not perform heavy GPU initialization in its constructor; device/context are set up later in ReInit. Prefer initializing device-bound resources in your feature’s load/init hooks to avoid static init order pitfalls.
src/State.cpp (1)
42-44:CheckOverride()runs every draw-call – confirm cost
lensEffects.CheckOverride()is executed inside the hotState::Draw()path each time shader-cache is enabled. Ensure the method is O(1) (no dynamic allocations / STL search) or cache the decision per frame to avoid a perf cliff on scenes with thousands of draw calls.package/Shaders/Sky.hlsl (1)
171-178: Conflicting UAV type under mutually-exclusive defines
LensEffectsis declared asTexture2D<float4>whenCLOUDSis defined, but asRWTexture2D<float4>when it is not.
If a permutation compiles with bothLENS_EFFECTSandCLOUDSundefined the second declaration wins and compilation is fine; however if future permutations define neither (or both) flags the double definition will clash. Guard the two declarations with an#elif / #elsepair to guarantee exactly one is seen.⛔ Skipped due to learnings
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.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.Learnt from: ThePagi PR: doodlum/skyrim-community-shaders#1369 File: package/Shaders/Lighting.hlsl:0-0 Timestamp: 2025-08-05T17:40:44.796Z Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.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.Learnt from: ThePagi PR: doodlum/skyrim-community-shaders#1369 File: src/Features/SnowCover.cpp:277-293 Timestamp: 2025-08-05T18:22:40.542Z Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.Learnt from: alandtse PR: doodlum/skyrim-community-shaders#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: 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.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.src/Features/LensEffects.h (2)
221-234: Confirmfloat4typedef exists
std::array<float4, 10>will not compile unlessfloat4is an alias in a visible header. Verify the alias is defined; otherwise replace withDirectX::XMFLOAT4Aor a custom struct.
508-512: Unconditionally returningtruemay break render-condition logic
LensFlare_CheckRenderCondition::thunkforwards to the original function but discards its bool result and always returnstrue. Ensure this is intentional; it forces the engine to render even when the original test fails.
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
src/Features/LensEffects.h (4)
38-59: Prefer smart-pointer or RAII wrappers for D3D11 resourcesRaw
ID3D11*pointers risk leaks and unclear ownership. UseMicrosoft::WRL::ComPtr<>(or equivalent) and release them in the class destructor to guarantee deterministic cleanup.
387-387: Encapsulate passesdone member.The
passesdonemember should be private with appropriate accessor methods to maintain encapsulation.
465-472: Add descriptive exception message.The exception thrown when effect is not found should include a descriptive message.
525-529: Passing address of a stack variable into original function risks UAF
thunkresetscurrent = 0;and then callsfunc(previous = ¤t, current);.
¤trefers to a stack variable that becomes invalid once the thunk returns, potentially leaving the callee with a dangling pointer. Pass the original argument or a static sentinel instead.package/Shaders/Sky.hlsl (1)
267-277: Do not rely on exact float equality for screen-space Y
if(input.Position.y == SunParams.y)compares two FP values generated on different code-paths (rasteriser vs. texture load). This will almost never evaluate to true once the shader runs on real hardware due to sub-pixel variation, breaking the intended LUT write.
Use an integer pixel-coord comparison or an epsilon check.- if(input.Position.y == SunParams.y) + if(abs(input.Position.y - SunParams.y) < 0.5)src/Features/LensEffects.cpp (5)
41-63: Add error handling for shader compilation failures.The shader compilation doesn't check if
Util::CompileShaderreturns nullptr. If any shader fails to compile, the pointers will be null and could cause crashes when used later.
127-128: Add error handling for texture loading.
CreateDDSTextureFromFilecan fail if the texture files don't exist or are corrupted. The function should handle these failures gracefully.
130-130: Memory management concerns with raw pointers.Using
newwithout correspondingdeletecan cause memory leaks. Consider using smart pointers instead.Also applies to: 140-140
159-166: Frame index wrap jumps to 5 – off-by-one logic?After reaching 16 the counter resets to 5, never 0, producing uneven temporal history (frames 0-4 missing). Probably meant to reset to 0.
- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + frameIdx = (frameIdx < 16) ? ++frameIdx : 0;
234-244: Potential null pointer dereference when accessing BlendState.The code checks if
BlendState[1]is null but then immediately uses it afterOMGetBlendState. The retrieval could fail leaving it null.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (2)
203-211: Negative LUT index whenFrame == 0
GetTemporalAveragewrites toSunLUT_AT[int2(idx-1,0)]; whenFrameis0this underflows and reads/writes outside the texture. Clamp or wrap the index.- SunLUT_AT[int2(idx-1, 0)] = Value; + SunLUT_AT[int2(max(0, idx-1), 0)] = Value;
252-255: Potential division by zero in delta() calculation.The
delta()function likely computes a small value to prevent division by zero, but the denominator calculation could still result in zero ifCloudFactorequalsPI * (SunRadius * SunRadius).
🧹 Nitpick comments (2)
features/Lens Effects/Shaders/Features/LensEffects.ini (1)
2-2: Consider using standard semantic versioning format.The version format
1-0-0uses hyphens instead of the more conventional dots. Consider using1.0.0to align with semantic versioning standards commonly used across the project.-Version = 1-0-0 +Version = 1.0.0features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
180-181: Magic numbers should be defined as descriptive constants.The values
SUNCLIPandOCCLSAMPLESare defined but other magic numbers throughout the shader (like0.05,20,200at line 236, etc.) should also be constants for maintainability.#define SUNCLIP 0.05 #define OCCLSAMPLES 20 +#define SS_BOUNDARY_OFFSET 200 +#define MOTION_THRESHOLD 0.04 +#define TEMPORAL_FRAME_COUNT 10
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (11)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Globals.cpp
- src/State.cpp
- package/Shaders/ISSAOComposite.hlsl
- src/Globals.h
- src/Feature.cpp
- features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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-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:
package/Shaders/Sky.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
PR: doodlum/skyrim-community-shaders#1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
src/Features/LensEffects.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 (3)
src/Features/LensEffects.cpp (1)
525-533: Add null pointer guards for sun position access.The function dereferences
skyrim_SunPositionwithout checking if it's null. Additionally, be aware of VR eye index considerations.DirectX::XMFLOAT4A LensEffects::GetSunPosition() { + if (!skyrim_SunPosition || !skyrim_SunGlareScale) { + return DirectX::XMFLOAT4A(0.0f, 0.0f, 0.0f, 0.0f); + } auto sunWSPos = *skyrim_SunPosition;features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (2)
366-366: Verify sun intensity clipping logic is intentional.The code clips rendering when
input.SunInt.w - SUNCLIPis negative, which happens when sun intensity is below 0.05. This might cause visual popping.Is the abrupt cutoff at
SUNCLIP(0.05) intentional, or should there be a smooth fade instead to prevent visual artifacts when the sun intensity transitions across this threshold?Also applies to: 430-430
889-889: Remove hardcoded resolution values.While this was marked as addressed in commits, verify that the UV coordinate scaling no longer uses hardcoded resolution values.
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Feature.cpp (1)
178-181: Potential null-dereference in ValidateCache when Version is missing from cache.
versionInCachemay be nullptr; callingstrcmpwill crash. Fail-safe early or compare via string_view.Apply this fix:
- auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); - if (strcmp(versionInCache, version.c_str()) != 0) { + auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); + if (!versionInCache) { + logger::info("No cached version found for {}. Forcing cache invalidation.", ini_name); + return false; + } + if (std::string_view(versionInCache) != std::string_view(version)) { logger::info("Change in version detected. Installed {} but {} in Disk Cache", version, versionInCache); return false; } else { logger::info("Installed version and cached version match."); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Feature.cpp(2 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Globals.h
- src/Globals.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
⏰ 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 (1)
src/Feature.cpp (1)
226-228: Feature list additions LGTM; confirm ordering/initialization and VR gating.
Appending extendedTranslucency and lensEffects is fine. Validate that their relative order is intentional (init/hook/UI ordering), and that lensEffects.SupportsVR() reflects reality so VR builds filter it correctly when not in dev mode.If ordering depends on other features (e.g., sky/IBL), please confirm no regressions occur when lensEffects initializes after ibl/interiorSun/skylighting.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/Feature.cpp (1)
178-179: Critical null pointer vulnerability in ValidateCache.Line 179 has a potential null pointer dereference when
versionInCacheis null, which can occur if the INI file lacks a Version entry.Apply this diff to add null safety:
- if (strcmp(versionInCache, version.c_str()) != 0) { + if (versionInCache == nullptr || strcmp(versionInCache, version.c_str()) != 0) {
♻️ Duplicate comments (1)
package/Shaders/Sky.hlsl (1)
270-270: Fix floating-point equality comparison for reliable hardware execution.The exact float equality check
input.Position.y == SunParams.ywill almost never evaluate to true on real hardware due to sub-pixel variations between rasterizer-generated positions and texture-loaded values.Apply this fix to use epsilon-based comparison:
- if(input.Position.y == SunParams.y) + if(abs(input.Position.y - SunParams.y) < 0.5)
🧹 Nitpick comments (8)
package/Shaders/Common/Color.hlsli (2)
6-8: Consider consolidating redundant luminance constants.The new
LUM_601macro (line 6) duplicates the existingRGBToLuminance2function coefficients (line 26), andLUM_709is very close to the existingRGBToLuminancecoefficients (line 16). This creates maintenance overhead and potential confusion about which standard to use.Consider refactoring to use these macros consistently throughout the file:
float RGBToLuminance(float3 color) { - return dot(color, float3(0.2125, 0.7154, 0.0721)); + return dot(color, LUM_709); } float RGBToLuminance2(float3 color) { - return dot(color, float3(0.299, 0.587, 0.114)); + return dot(color, LUM_601); }
58-58: Consider using a named constant for the epsilon value.The magic number
1.0e-10for preventing division by zero could benefit from being a named constant for better readability and maintainability.+static const float DIVISION_EPSILON = 1.0e-10; + float3 RGBtoHSV(float3 c) { float4 K = float4(0.0, -1.0/3.0, 2.0/3.0, -1.0); float4 p = (c.g < c.b) ? float4(c.b, c.g, K.w, K.z) : float4(c.g, c.b, K.x, K.y); float4 q = (c.r < p.x) ? float4(p.x, p.y, p.z, c.r) : float4(c.r, p.y, p.z, p.x); - float d = q.x - min(q.w, q.y); float e = 1.0e-10; - return float3(abs(q.z + (q.w - q.y) / (6.0 * d + e)), d / (q.x + e), q.x); + float d = q.x - min(q.w, q.y); + return float3(abs(q.z + (q.w - q.y) / (6.0 * d + DIVISION_EPSILON)), d / (q.x + DIVISION_EPSILON), q.x); }package/Shaders/Sky.hlsl (2)
267-277: Consider adding bounds checking for texture access.While the lens effects logic is well-structured, consider adding bounds validation for the texture coordinate access patterns to prevent potential out-of-bounds reads/writes, especially for
LensEffects[int2(2,0)]andLensEffectsAT[int2(0,0)].Consider adding bounds validation:
#if defined(LENS_EFFECTS) && defined(DEFERRED) #if defined(DITHER) float4 SunParams = LensEffects.Load(int2(3,0)); - if(abs(input.Position.y - SunParams.y) < 0.5) + if(abs(input.Position.y - SunParams.y) < 0.5 && all(int2(2,0) >= 0)) LensEffects[int2(2,0)] = psout.Color; #elif defined(CLOUDS) float4 SunParams = LensEffects.Load(int3(3,0,0)); uint Out; - if(psout.Color.w > 0.5 && length(input.Position.xy - SunParams.xy) - SunParams.w <= 0.0) + if(psout.Color.w > 0.5 && length(input.Position.xy - SunParams.xy) - SunParams.w <= 0.0 && all(int2(0,0) >= 0)) InterlockedAdd(LensEffectsAT[int2(0,0)], 1, Out); #endif #endif
273-273: Improve variable declaration formatting for readability.The variable declaration
uint Out;on the same line as the SunParams load reduces readability. Consider separating the declarations.- float4 SunParams = LensEffects.Load(int3(3,0,0)); uint Out; + float4 SunParams = LensEffects.Load(int3(3,0,0)); + uint Out;package/Shaders/Common/Random.hlsli (1)
38-41: Fix code formatting and add documentation.The random functions have inconsistent formatting and lack documentation for their intended use cases.
Apply this diff to improve formatting and add documentation:
+ // Simple pseudo-random number generators for shader use + // Returns values in range [0, 1) float Random(float seed){ - return frac(sin(seed * 12.9898) * 43758.5453);} + return frac(sin(seed * 12.9898) * 43758.5453); + } + float Random(float2 coords){ - return frac(sin(dot(coords, float2(12.9898, 78.233))) * 43758.5453);} + return frac(sin(dot(coords, float2(12.9898, 78.233))) * 43758.5453); + }package/Shaders/Common/CoordMath.hlsli (3)
6-7: Consider code formatting and add input validation.The single-line function definitions are hard to read and could benefit from proper formatting. Additionally, consider adding bounds checking for the degrees parameter.
- float2 DegreesToVector(float degrees) {float2 outV; sincos(radians(degrees), outV.y, outV.x); return outV;} - float4 DegreesToVector(float2 degrees) {float4 outV; sincos(radians(degrees), outV.yw, outV.xz); return outV;} + float2 DegreesToVector(float degrees) + { + float2 outV; + sincos(radians(degrees), outV.y, outV.x); + return outV; + } + + float4 DegreesToVector(float2 degrees) + { + float4 outV; + sincos(radians(degrees), outV.yw, outV.xz); + return outV; + }
9-10: Consider formatting consistency.While the logic is correct for Chebyshev distance calculation, formatting consistency with the rest of the file would improve readability.
- float ChebyDistance(float2 Coords) {return max(abs(Coords.x), abs(Coords.y));} - float ChebyDistance(float2 PointA, float2 PointB) {float2 d = PointA - PointB; return max(abs(d.x), abs(d.y));} + float ChebyDistance(float2 Coords) + { + return max(abs(Coords.x), abs(Coords.y)); + } + + float ChebyDistance(float2 PointA, float2 PointB) + { + float2 d = PointA - PointB; + return max(abs(d.x), abs(d.y)); + }
22-24: Consider using more descriptive variable names.The variable name
poscould be more descriptive, such asatlasOffsetsortextureOffsets, to better convey its purpose.- static const float2 pos[4] = { + static const float2 atlasOffsets[4] = { float2(0.0, 0.0), float2(0.5, 0.0), float2(0.0, 0.5), float2(0.5, 0.5)}; - return mad(coords, 0.5, pos[texNum - 1]);} + return mad(coords, 0.5, atlasOffsets[texNum - 1]);}And similarly for AtlasFetch1x4:
- static const float2 pos[4] = { + static const float2 atlasOffsets[4] = { float2(0.0, 0.00), float2(0.0, 0.25), float2(0.0, 0.50), float2(0.0, 0.75)}; - return mad(coords, float2(1.0, 0.25), pos[texNum - 1]);} + return mad(coords, float2(1.0, 0.25), atlasOffsets[texNum - 1]);}Also applies to: 28-30
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (15)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- package/Shaders/ISSAOComposite.hlsl
- src/Globals.h
- features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
- src/Features/LensEffects.cpp
- src/Features/LensEffects.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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-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:
package/Shaders/Common/Color.hlslipackage/Shaders/Sky.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: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (21)
package/Shaders/Common/Color.hlsli (2)
29-32: LGTM! Chrominance calculation is mathematically correct.The implementation correctly calculates chrominance as the difference between the maximum and minimum RGB components, which represents the colorfulness relative to brightness.
53-67: RGB/HSV conversion functions look mathematically sound.The HSV conversion algorithms use standard formulations with proper handling of edge cases (epsilon value for division by zero). The implementations are efficient and appropriate for shader usage.
package/Shaders/Sky.hlsl (1)
171-178: LGTM! Resource declarations follow proper conditional compilation pattern.The lens effects resource declarations are appropriately gated behind feature flags and use different resource types based on the rendering path (CLOUDS vs non-CLOUDS). The resource bindings are consistent with the broader lens effects system.
features/Lens Effects/Shaders/Features/LensEffects.ini (1)
1-2: Confirm this INI file is for development purposes and aligns with PR objectives.This minimal INI file with only version information appears to be a placeholder or development artifact. According to the PR objectives, the contributor mentioned creating separate settings files for lens effects as an intentional design choice to allow weather modders and mod authors to package custom lens settings without overriding users' other feature settings.
However, the current INI only contains version metadata and no actual lens effect settings. If this is intentional for the initial implementation, consider adding a comment or documentation explaining the design rationale.
src/Feature.cpp (2)
16-16: LGTM on the include addition.The LensEffects header inclusion is correctly positioned alphabetically within the existing feature includes.
226-227: LGTM on feature list registration.The LensEffects feature is correctly added to the feature list following the established pattern. The placement maintains alphabetical ordering within the features array.
src/Globals.cpp (2)
25-25: LGTM on the include addition.The LensEffects header inclusion is correctly positioned alphabetically within the existing feature includes.
85-85: LGTM on the global instance definition.The LensEffects global instance follows the established pattern used by other features and is correctly placed within the globals::features namespace.
src/State.cpp (3)
11-11: LGTM on the include addition.The LensEffects header inclusion is correctly positioned within the existing feature includes.
28-28: LGTM on the local reference.The local reference to the LensEffects feature follows the established pattern used by other features in the Draw() method.
42-43: LGTM on the feature integration.The conditional check and method call for LensEffects follows the established pattern used by other features. The placement between terrain helper and truePBR resource setup is appropriate.
package/Shaders/Common/CoordMath.hlsli (5)
1-3: LGTM! Proper include guard implementation.The header guard follows standard naming conventions and prevents multiple inclusions.
4-5: LGTM! Well-organized namespace structure.The CoordMath namespace provides clear organization for coordinate utility functions.
12-15: LGTM! Proper handling of edge case.The CartesianToPolar function correctly handles the edge case where the radius is zero by setting the angle to zero, preventing undefined behavior.
17-19: LGTM! Efficient polar to cartesian conversion.The implementation efficiently uses sincos for simultaneous calculation of both components.
33-34: LGTM! Proper namespace and header guard closure.The namespace and header guard are properly closed.
package/Shaders/Common/Math.hlsli (5)
20-27: LGTM! Clean component-wise min/max implementations.The min/max functions are correctly implemented with proper chaining for efficiency.
28-29: LGTM! Straightforward vector sum functions.The sum functions correctly add all components together.
36-36: LGTM! Efficient use of intrinsic sincos function.The function correctly uses the HLSL intrinsic
sincosfor efficient simultaneous sine and cosine calculation.
47-57: LGTM! Correct implementation of smootherstep interpolation.The smootherstep functions correctly implement the 6t^5 - 15t^4 + 10t^3 polynomial for smoother interpolation than smoothstep.
31-32: Potential division by zero issue in ufmod functions.While
EPSILON_DIVISIONprevents division by zero, themax()operation should use the denominator before division, not justy.Apply this fix to ensure proper safe division:
- float ufmod(float x, float y) {return x - y * floor(x / max(y, EPSILON_DIVISION));} - float2 ufmod(float2 x, float2 y) {return x - y * floor(x / max(y, EPSILON_DIVISION));} + float ufmod(float x, float y) {return (y == 0) ? x : x - y * floor(x / y);} + float2 ufmod(float2 x, float2 y) {return (y == 0) ? x : x - y * floor(x / y);}Alternatively, if you want to keep the epsilon approach:
- float ufmod(float x, float y) {return x - y * floor(x / max(y, EPSILON_DIVISION));} - float2 ufmod(float2 x, float2 y) {return x - y * floor(x / max(y, EPSILON_DIVISION));} + float ufmod(float x, float y) {float divisor = max(abs(y), EPSILON_DIVISION) * sign(y); return x - y * floor(x / divisor);} + float2 ufmod(float2 x, float2 y) {float2 divisor = max(abs(y), EPSILON_DIVISION) * sign(y); return x - y * floor(x / divisor);}Likely an incorrect or invalid review comment.
Dawntic
force-pushed
the
lens-effects
branch
3 times, most recently
from
1216602 to
0bd3a3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (2)
202-221: Fix ring-buffer indexing to prevent negative index and make accumulation consistentTwo issues here:
- When Frame == 0,
idx-1underflows to -1 and writes out-of-bounds.- You write to
idx-1, but the sum reads fixed slots[5..14], so the just-written value is never included consistently. Use a stable ring with a fixed base offset.Apply this diff to make both integer and float paths safe and consistent:
-static const int nFrames = 10; +static const uint nFrames = 10u; +static const uint kRingStart = 5u; // base slot for temporal ring -uint GetTemporalAverage(uint Value, uint idx){ - SunLUT_AT[int2(idx-1, 0)] = Value; +uint GetTemporalAverage(uint Value, uint idx){ + const uint writeIdx = kRingStart + ((idx + nFrames - 1u) % nFrames); + SunLUT_AT[int2(writeIdx, 0)] = Value; - uint Sum = 0.0; - [unroll] for(int i=0; i < nFrames; ++i) - Sum += SunLUT_AT.Load(int2(5+i, 0)); + uint Sum = 0u; + [unroll] for(uint i = 0u; i < nFrames; ++i) + Sum += SunLUT_AT.Load(int2(kRingStart + i, 0)); return Sum / nFrames; } -float GetTemporalAverage(float Value, uint idx){ - SunLUT[int2(idx-1, 0)] = Value.xxxx; +float GetTemporalAverage(float Value, uint idx){ + const uint writeIdx = kRingStart + ((idx + nFrames - 1u) % nFrames); + SunLUT[int2(writeIdx, 0)] = Value.xxxx; float Sum = 0.0; - [unroll] for(int i=0; i < nFrames; ++i) - Sum += SunLUT.Load(int2(5+i, 0)).x; + [unroll] for(uint i = 0u; i < nFrames; ++i) + Sum += SunLUT.Load(int2(kRingStart + i, 0)).x; return Sum / nFrames; }
253-255: Guard against division by (near) zero when normalizing CloudFactorDenominator can collapse for small radii and produce instability despite
delta(...). Harden the path with an explicit epsilon.- CloudFactor = saturate(inv(CloudFactor / delta((Math::PI * (SunRadius * SunRadius))))); + const float denom = Math::PI * (SunRadius * SunRadius); + CloudFactor = saturate(inv(CloudFactor / max(delta(denom), 1e-5)));
🧹 Nitpick comments (4)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (4)
269-301: Gating occlusion work on a single pixel via SV_Position.x is brittleUsing
if(index < 1)withindex = (int)input.Position.xassumes a full-screen triangle and relies on raster coords; changes in viewport/scissor or MSAA could cause nondeterministic execution. Prefer a dedicated compute-like pass or gate on a constant system value (e.g., SV_DispatchThreadID in CS) or a known small mask draw.If this must remain in PS, consider additionally gating on
input.Position.y < 1andSV_SampleIndex == 0(if available) to reduce accidental multi-execution across samples.
404-425: Avoid potential NaNs in ray computation for zero-length normals
normalize(input.TexCoord.xy + BF4_Coords[i] * PixelSize)can hit zero vectors at center, andpow(saturate(InvDist), RandomRays * (7 / delta(Rays)))divides bydelta(Rays)which is fine, but ifInvDistis 0 and exponent is > 0,pow(0, >0)is 0; safe. The main edge is the normalize(0). Usemax(length(v), eps)style.- float2 Coords = normalize(input.TexCoord.xy + BF4_Coords[i] * PixelSize); + float2 dir = input.TexCoord.xy + BF4_Coords[i] * PixelSize; + float len = max(length(dir), 1e-6); + float2 Coords = dir / len;
486-505: Ghost CA displacement normalization can underflow
normalize(SunPosition - GhostPosition)can be zero when GhostPosition==SunPosition. Guard to avoid NaN CADispacement.- float2 CADispacement = normalize(SunPosition - GhostPosition) * float2(1.0, -1.0); + float2 deltaPos = SunPosition - GhostPosition; + float len = max(length(deltaPos), 1e-6); + float2 CADispacement = (deltaPos / len) * float2(1.0, -1.0);
879-884: Unreferenced MotionVector in ICE shader and unused import
MotionVectoris declared for the ICE shader path but not used; remove to keep the resource footprint lean.-Texture2D MotionVector : register(t2);
📜 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 sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (15)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Features/LensEffects.h
🚧 Files skipped from review as they are similar to previous changes (13)
- src/Globals.h
- src/Globals.cpp
- package/Shaders/Sky.hlsl
- src/Feature.cpp
- package/Shaders/Common/Random.hlsli
- features/Lens Effects/Shaders/Features/LensEffects.ini
- src/State.cpp
- package/Shaders/Common/CoordMath.hlsli
- package/Shaders/Common/Math.hlsli
- features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
- package/Shaders/ISSAOComposite.hlsl
- src/Features/LensEffects.cpp
- package/Shaders/Common/Color.hlsli
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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-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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
PR: doodlum/skyrim-community-shaders#1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 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/Lens Effects/Shaders/LensEffects/LensEffects.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 (2)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (2)
321-346: Starburst color/intensity mixing: consider linear-space operationsGlow/corona, blade masks, and
UIBurstColormixes are likely intended in linear space. If your pipeline is gamma-correct, ensureMain/UI colors are in linear when blended, or convert as needed.Can you confirm whether the lens effects chain runs in linear space in this project? If not, I can propose a minimal wrapper using the existing Color helpers to correct it.
229-230: Confirm Depth_Sampler ComparisonFunc (reversed‑Z vs. standard) and correctness of compareRef=1Found a local comparison sampler and three comparison samples, but I could not locate where the engine creates/binds the sampler state or where reversed‑Z / depth comparison is configured — so I cannot verify the comparison direction or that passing 1 as the compareRef is correct.
- Declaration found:
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl:178
- SamplerComparisonState Depth_Sampler : register(s13);
- SampleCmpLevelZero usages to check:
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl:229
- DepthTexture.SampleCmpLevelZero(Depth_Sampler, Coords, 1).x;
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl:373
- depth.SampleCmpLevelZero(Depth_Sampler, input.Position.xy / ScreenSize.xy, 1).x;
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl:641
- Depth.SampleCmpLevelZero(Depth_Sampler, input.Position.xy / ScreenSize.xy, 1).x;
Please verify in the engine/API code that binds the sampler at register s13:
- which ComparisonFunc is used (LESS/LE vs. GREATER/GE), and
- whether the renderer uses reversed‑Z;
and confirm that compareRef = 1 is the intended reference value for the depth encoding in DepthTexture (adjust the compareRef or comparison direction if needed).
features/Lens Effects/Shaders/LensEffects/LensEffectHelper.hlsli
Outdated
Show resolved
Hide resolved
|
@doodlum you good with merging this? |
|
i will test soon |
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
Tried using this. Got a black screen for 20 seconds or so, and none of the effects do anything at all.
Dawntic
force-pushed
the
lens-effects
branch
from
6cceabf to
7ad3e91
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
package/Shaders/Sky.hlsl (1)
269-272: Don’t compare floats for equality in screen space.Use an epsilon or integer pixel compare to make the LUT write robust.
Apply this diff:
- float4 SunParams = LensEffects.Load(int2(3,0)); - if(input.Position.y == SunParams.y) + float4 SunParams = LensEffects.Load(int2(3,0)); + if (abs(input.Position.y - SunParams.y) < 0.5) LensEffects[int2(2,0)] = psout.Color;src/Features/LensEffects.cpp (5)
248-258: Null-check OMGetBlendState result before GetDesc().Apply:
- if (!BlendState[1]) { - context->OMGetBlendState(&BlendState[1], nullptr, nullptr); - if (BlendState[1]) { + if (!BlendState[1]) { + ID3D11BlendState* tmp = nullptr; + context->OMGetBlendState(&tmp, nullptr, nullptr); + if (tmp) { + BlendState[1] = tmp; D3D11_BLEND_DESC SrcBlendDesc = {}; BlendState[1]->GetDesc(&SrcBlendDesc); auto& blendState = SrcBlendDesc.RenderTarget[0]; blendState.BlendEnable = TRUE; blendState.RenderTargetWriteMask = D3D11_COLOR_WRITE_ENABLE_ALL; globals::d3d::device->CreateBlendState(&SrcBlendDesc, &BlendState[0]); } }
171-179: frameIdx wrap bug (jumps to 5, skipping history).Apply:
- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + frameIdx = (frameIdx + 1) % 16;
551-555: Guard sun color property chain (avoid crash on null/short vector).Apply:
- if (auto sky = globals::game::sky; sky && sky->sun && sky->sun->sunBase) - if (auto property = skyrim_cast<RE::BSSkyShaderProperty*>(sky->sun->sunBase->GetGeometryRuntimeData().properties[1].get())) - sunColor = { property->kBlendColor.red, property->kBlendColor.green, property->kBlendColor.blue, 1.0f }; + if (auto sky = globals::game::sky; sky && sky->sun && sky->sun->sunBase) { + auto& props = sky->sun->sunBase->GetGeometryRuntimeData().properties; + if (props.size() > 1 && props[1]) { + if (auto property = skyrim_cast<RE::BSSkyShaderProperty*>(props[1].get())) { + sunColor = { property->kBlendColor.red, property->kBlendColor.green, property->kBlendColor.blue, 1.0f }; + } + } + }
42-63: Add hard-fail on shader compile errors (avoid null deref later).Apply this diff (pattern; extend to all compiles):
void LensEffects::CompileShaders() { - SunOcclusionMaskPixelShader = (ID3D11PixelShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0"); - ChromaticAberrationPixelShader = (ID3D11PixelShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "CHROMATIC_ABERRATION_PIXEL_SHADER", "" } }, "ps_5_0"); + SunOcclusionMaskPixelShader = static_cast<ID3D11PixelShader*>(Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0")); + ChromaticAberrationPixelShader = static_cast<ID3D11PixelShader*>(Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "CHROMATIC_ABERRATION_PIXEL_SHADER", "" } }, "ps_5_0")); BypassVertexShader = (ID3D11VertexShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "BYPASS_VERTEX_SHADER", "" } }, "vs_5_0"); @@ - IcePixelShader = (ID3D11PixelShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "ICE_PIXEL_SHADER", "" } }, "ps_5_0"); + IcePixelShader = static_cast<ID3D11PixelShader*>(Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "ICE_PIXEL_SHADER", "" } }, "ps_5_0"); + if (!(SunOcclusionMaskPixelShader && ChromaticAberrationPixelShader && BypassVertexShader && + BurstVertexShader && BurstPixelShader && GhostVertexShader && GhostPixelShader && + SunGlareVertexShader && SunGlarePixelShader && HaloVertexShader && HaloPixelShader && + LensGlareVertexShader && LensGlarePixelShader && IceVertexShader && IcePixelShader)) { + logger::error("[Lens Effects] Shader compilation failed; aborting CompileShaders()"); + return; + } }
137-148: Use RAII for engine-owned objects to prevent leaks.Apply this diff and update header types accordingly:
- SettingsCB = new ConstantBuffer(ConstantBufferDesc<ConstBuffer>()); + SettingsCB = std::make_unique<ConstantBuffer>(ConstantBufferDesc<ConstBuffer>()); @@ - renderdata = new Setup::LF_RenderData; + renderdata = std::make_unique<Setup::LF_RenderData>();(Header changes included below.)
src/Features/LensEffects.h (7)
1-2: Add explicit standard headers for used types (avoid transitive includes).Apply:
#pragma once #include "Feature.h" +#include <array> +#include <memory> +#include <string> +#include <string_view> +#include <algorithm> +#include <cmath> +#include <cstdint> +#include <d3d11.h> +#include <wrl/client.h>
312-327: Const-correctness and divide-by-zero guard in math helpers.Apply:
- virtual inline DirectX::XMFLOAT4A VectorToXMFloat(float4& value) { return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); } - virtual inline float LinearStep(float edge0, float edge1, float x) { return std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); } + virtual inline DirectX::XMFLOAT4A VectorToXMFloat(const float4& value) { return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); } + virtual inline float LinearStep(float edge0, float edge1, float x) + { + const float denom = (edge1 - edge0); + if (std::fabs(denom) <= 1e-6f) return x >= edge1 ? 1.0f : 0.0f; + return std::clamp((x - edge0) / denom, 0.0f, 1.0f); + }
389-405: Encapsulate passesdone state.Apply:
- int passesdone = 0; + private: + int passesdone = 0; + public: + void IncrementPassesDone() { ++passesdone; } + void ResetPassesDone() { passesdone = 0; } + int GetPassesDone() const { return passesdone; } @@ - int PassesLeft() const { return (int)numpasses - (int)passesdone; } + int PassesLeft() const { return (int)numpasses - passesdone; }Also update usages below (see next comments).
449-457: Fix pass scheduler: end cleanly and avoid last-pass stickiness.Apply:
- if (Passes[currentEffect]->PassesLeft() == 0) - if (currentEffect + 1 < Passes.size()) - currentEffect++; - - Passes[currentEffect]->passesdone++; - return (Passes[currentEffect]->IsActive()) ? Passes[currentEffect]->GetDesc() : Shaders::Bypass; + if (Passes[currentEffect]->PassesLeft() <= 0) { + if (currentEffect + 1 < Passes.size()) { + ++currentEffect; + } else { + return Shaders::Bypass; + } + } + Passes[currentEffect]->IncrementPassesDone(); + return Passes[currentEffect]->IsActive() ? Passes[currentEffect]->GetDesc() : Shaders::Bypass;
38-41: Use smart pointers for engine-owned buffers.Apply:
- ConstantBuffer* SettingsCB = nullptr; + std::unique_ptr<ConstantBuffer> SettingsCB;(Also update .cpp allocations accordingly.)
528-536: Don’t pass address of stack variable to func (dangling pointer).Apply:
- static void thunk(void* previous, uint64_t current) - { - current = 0; - func(previous = ¤t, current); - } + static void thunk(void* previous, uint64_t /*current*/) + { + static uint64_t s_zero = 0; + func(&s_zero, 0); + }
4-18: Initialize defaults to avoid UB and set safe enum default.Apply:
struct LensEffects : Feature { + LensEffects() noexcept + : settings(&stdSettings), shaderdesc(Shaders::Bypass), SunScale(0.0f) {}
🧹 Nitpick comments (5)
package/Shaders/Sky.hlsl (1)
273-276: Faster/safer inside-circle test (avoid sqrt and reduce FP error).Use squared distance ≤ r².
Apply this diff:
- float4 SunParams = LensEffects.Load(int3(3,0,0)); uint Out; - if(psout.Color.w > 0.5 && length(input.Position.xy - SunParams.xy) - SunParams.w <= 0.0) + float4 SunParams = LensEffects.Load(int3(3,0,0)); uint Out; + float2 d = input.Position.xy - SunParams.xy; + if (psout.Color.w > 0.5 && dot(d, d) <= SunParams.w * SunParams.w) InterlockedAdd(LensEffectsAT[int2(0,0)], 1, Out);src/Features/LensEffects.h (4)
79-90: Prefer initializing frameIdx to 0.Apply:
- uint frameIdx = 5; + uint frameIdx = 0;
100-107: SunScale is uninitialized; set to 0.Apply:
- float SunScale; + float SunScale = 0.0f;
459-466: Reset using accessor; keep lists in sync.Apply:
- for (auto& pass : Passes) pass->passesdone = 0; + for (auto& pass : Passes) pass->ResetPassesDone();
510-517: Suppressing C4189 needs justification; or remove dead local.Add a brief comment or remove the unused ‘result’ variable to avoid pragma.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (14)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Globals.cpp
- src/Feature.cpp
- package/Shaders/Common/CoordMath.hlsli
- src/Globals.h
- src/State.cpp
- package/Shaders/ISSAOComposite.hlsl
- features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
- package/Shaders/Common/Color.hlsli
- features/Lens Effects/Shaders/Features/LensEffects.ini
- package/Shaders/Common/Math.hlsli
- package/Shaders/Common/Random.hlsli
🧰 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/Features/LensEffects.cpppackage/Shaders/Sky.hlslsrc/Features/LensEffects.h
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/LensEffects.cppsrc/Features/LensEffects.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: 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.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
PR: doodlum/skyrim-community-shaders#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/Features/LensEffects.cpp
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
src/Features/LensEffects.cpp
📚 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/LensEffects.cpppackage/Shaders/Sky.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: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (2)
src/Features/LensEffects.h (1)
49-49: Confirm and remove unused SunTexSRV.
Declaration: src/Features/LensEffects.h:49. Repo search returned no matches; re-check with: rg -n --hidden -S '\bSunTexSRV\b' || git grep -n 'SunTexSRV'. If truly unused, remove the declaration.package/Shaders/Sky.hlsl (1)
171-178: t30/u7 bound only in Sky.hlsl — no collisions found. Scan of package/Shaders/** found register(t30)/register(u7) only at package/Shaders/Sky.hlsl:173,174,176; no other matches.
There was a problem hiding this comment.
Sun glare makes the sun darker, not brighter.
Starbust looks strange.
CA works weird, only seems to work in motion.
Lens glare seems to only apply at the bottom of the screen.
Sun halo does nothing
Lens Ice should be applied to UI buffer so it works with framegen.
Nothing is explained in the menus at all.
Ghosts seem fine, just a weird name for it.
Performance seems fine, maybe 3% hit on my 4080.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (19)
package/Shaders/Sky.hlsl (2)
171-178: Verify UAV binding at slot 7 in C++ code.The shader declares the UAV at register u7, but the C++ binding code must match. This was flagged in previous reviews.
270-270: Replace exact float equality with epsilon check.Comparing
input.Position.y == SunParams.yis unreliable due to floating-point precision. Use an epsilon-based check as suggested in previous reviews.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (4)
1-856: Verify linear color-space handling for post-processing.The shader samples and blends colors. Ensure gamma-to-linear conversions are applied consistently as flagged in previous reviews for the linear lighting system.
Based on learnings
220-229: Fix temporal averaging ring buffer and Frame==0 underflow.The temporal average functions have indexing issues that were flagged in previous reviews. Apply the suggested ring-buffer fix.
373-375: Guard sqrt against negative input to prevent NaNs.The sqrt calculation can receive negative values, as flagged in previous reviews. Clamp the input before sqrt.
505-514: Clamp UIGhostShape and use safe loop bounds.Loop can index out of bounds when UIGhostShape >= 10, as previously flagged. Apply the suggested fix.
src/Features/LensEffects.cpp (6)
139-139: Consider smart pointers for resource management.Raw pointers risk leaks. The previous review suggested using
std::unique_ptrfor SettingsCB and renderdata.Also applies to: 152-152
41-64: Add error handling for shader compilation.Shader compilation lacks null checks. Apply the error handling suggested in previous reviews.
136-137: Add error handling for texture loading.
CreateDDSTextureFromFilecan fail. Add HRESULT checking as previously suggested.
175-175: Frame index wraps to 5 instead of 0.The frame counter resets to 5 after reaching 16, skipping frames 0-4. Change to wrap to 0 for consistent temporal history.
396-403: Bind UAV at fixed slot 7 to match shader register.The UAV binding uses
numRTsas the slot, but the shader expects u7. Apply the fix from previous reviews to bind at slot 7 explicitly.
542-546: Add null guards for long pointer dereference chain.The chain
sky->sun->sunBase->GetGeometryRuntimeData().properties[1]needs guards as flagged previously.src/Features/LensEffects.h (3)
51-90: Consider using ComPtr for D3D11 resources.Raw D3D11 pointers risk leaks. Previous reviews suggested using
Microsoft::WRL::ComPtrfor automatic lifetime management.
363-364: Harden LinearStep and make VectorToXMFloat const-correct.
LinearStepcan divide by zero when edge0 == edge1. Also,VectorToXMFloatshould take a const reference. Apply fixes from previous reviews.
516-525: Fix pass scheduler to use <= and handle completion.The scheduler checks
== 0which prevents advancing past the last pass. Use<= 0and return Bypass when complete, as suggested previously.package/Shaders/ISSAOComposite.hlsl (1)
139-139: Sanitize all color channels and catch infinities, not just NaNs in red.Current check only sanitizes the red channel and uses
>which allows infinities to pass. Apply the fix from the previous review to zero any NaN/Inf in RGB channels.package/Shaders/Common/Math.hlsli (3)
63-71: Refactor: Diffraction sinc doesn't return 1.0 at zero.The current implementation uses
max(sinc, EPSILON_DIVISION)which doesn't correctly handle the mathematical limit lim(x→0) sin(x)/x = 1. When the argument is near zero, it should return 1.0 rather than a small fraction.Apply this improvement for mathematically correct sinc:
float Diffraction(float x, float Frequency, float Phase, float Amplitude){ - float sinc = PI * (x * Frequency + Phase); - sinc = sin(sinc) / max(sinc, EPSILON_DIVISION); + float arg = PI * (x * Frequency + Phase); + float sinc = (abs(arg) < EPSILON_DIVISION) ? 1.0 : sin(arg) / arg; return sinc * sinc * Amplitude;} float3 Diffraction(float3 x, float Frequency, float Phase, float Amplitude){ - float3 sinc = PI * (x * Frequency + Phase); - sinc = sin(sinc) / max(sinc, EPSILON_DIVISION); + float3 arg = PI * (x * Frequency + Phase); + float3 sinc = (abs(arg) < EPSILON_DIVISION) ? 1.0 : sin(arg) / arg; return sinc * sinc * Amplitude;}
39-46: Critical: LinearStep sign-flip breaks reversed ranges.Using
max((edge1 - edge0), EPSILON_DIVISION)flips the denominator sign whenedge1 < edge0, producing incorrect interpolation. This breaks reversed ranges (e.g., interpolating from 1.0 to 0.0).Apply this fix to preserve denominator sign:
float LinearStep(float edge0, float edge1, float x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float denom = edge1 - edge0; + denom = (denom >= 0 ? 1.0 : -1.0) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);} float2 LinearStep(float2 edge0, float2 edge1, float2 x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float2 denom = edge1 - edge0; + denom = sign(denom) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);} float3 LinearStep(float3 edge0, float3 edge1, float3 x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float3 denom = edge1 - edge0; + denom = sign(denom) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);}
48-49: Major: MapRange inherits the sign-flip issue.The same
max()pattern breaks reversed ranges inMapRange. WhenoldMax < oldMin, the denominator sign is incorrectly flipped.Apply this fix:
float MapRange(float x, float oldMin, float oldMax, float newMin, float newMax){ - return newMin + ((x - oldMin) / max(oldMax - oldMin, EPSILON_DIVISION)) * (newMax - newMin);} + float denom = oldMax - oldMin; + denom = (denom >= 0 ? 1.0 : -1.0) * max(abs(denom), EPSILON_DIVISION); + return newMin + ((x - oldMin) / denom) * (newMax - newMin);}
🧹 Nitpick comments (1)
package/Shaders/Common/Random.hlsli (1)
38-42: Reformat for consistency and add documentation.The function definitions use compressed single-line formatting that differs from the rest of the file. Additionally, there's no documentation explaining what "SH" stands for or when to use these simpler hash functions versus the more sophisticated ones (like
pcg,murmur3) already available in the file.Apply this diff to improve formatting and add documentation:
+ // Simple hash functions based on sine - fast but lower quality + // Use for non-critical randomness where performance matters float RandomSH(float seed) - return frac(sin(seed * 12.9898) * 43758.5453);} + { + return frac(sin(seed * 12.9898) * 43758.5453); + } + float RandomSH(float2 seed) - return frac(sin(dot(seed, float2(12.9898, 78.233))) * 43758.5453);} + { + return frac(sin(dot(seed, float2(12.9898, 78.233))) * 43758.5453); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (14)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Feature.cpp
- src/State.cpp
- package/Shaders/Common/Color.hlsli
- features/Lens Effects/Shaders/Features/LensEffects.ini
🧰 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:
package/Shaders/Common/Random.hlslipackage/Shaders/ISSAOComposite.hlslsrc/Globals.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Globals.hpackage/Shaders/Common/CoordMath.hlslisrc/Features/LensEffects.cpppackage/Shaders/Common/Math.hlslipackage/Shaders/Sky.hlslsrc/Features/LensEffects.h
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/Globals.cppsrc/Globals.hsrc/Features/LensEffects.cppsrc/Features/LensEffects.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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
🧠 Learnings (4)
📚 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:
package/Shaders/ISSAOComposite.hlslfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
PR: doodlum/skyrim-community-shaders#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Globals.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
PR: doodlum/skyrim-community-shaders#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/Features/LensEffects.cpppackage/Shaders/Sky.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
package/Shaders/Sky.hlsl
🧬 Code graph analysis (2)
src/Features/LensEffects.cpp (2)
src/State.cpp (3)
SetupResources(556-589)SetupResources(556-556)i(197-197)src/Features/LensEffects.h (3)
UpdateSettings(366-384)GetSingleton(6-10)LinearStep(364-364)
src/Features/LensEffects.h (2)
src/Features/RenderDoc.h (1)
SupportsVR(59-59)src/Features/LensEffects.cpp (66)
SetupResources(66-165)SetupResources(66-66)CompileShaders(41-64)CompileShaders(41-41)CheckOverride(167-179)CheckOverride(167-167)LookupShader(181-198)LookupShader(181-181)AppendOcclusionLUT(382-405)AppendOcclusionLUT(382-382)SetupOcclusionMask(216-257)SetupOcclusionMask(216-216)SetupBurstEffect(259-269)SetupBurstEffect(259-259)SetupSunGlareEffect(271-283)SetupSunGlareEffect(271-271)SetupLensGlareEffect(285-295)SetupLensGlareEffect(285-285)SetupHaloEffect(297-307)SetupHaloEffect(297-297)SetupGhostEffect(309-339)SetupGhostEffect(309-309)SetupIceEffect(359-380)SetupIceEffect(359-359)BypassShader(407-415)BypassShader(407-407)SetupCAEffect(341-357)SetupCAEffect(341-341)GetSunPosition(523-538)GetSunPosition(523-523)GetSunColor(540-547)GetSunColor(540-540)GetWeatherShader(417-477)GetWeatherShader(417-417)GetWeatherPrecip(479-489)GetWeatherPrecip(479-479)CheckWeatherChange(491-501)CheckWeatherChange(491-491)UpdateWeatherBasedDisable(503-521)UpdateWeatherBasedDisable(503-503)RefreshToggles(986-997)RefreshToggles(986-986)RestoreDefaultSettings(1010-1014)RestoreDefaultSettings(1010-1010)DrawSettings(665-984)DrawSettings(665-665)LoadSettings(999-1003)LoadSettings(999-999)SaveSettings(1005-1008)SaveSettings(1005-1005)UpdateBufferValues(200-214)UpdateBufferValues(200-200)thunk(549-584)thunk(549-549)thunk(586-594)thunk(586-586)thunk(596-612)thunk(596-596)thunk(614-622)thunk(614-614)thunk(626-636)thunk(626-626)thunk(639-651)thunk(639-639)thunk(653-663)thunk(653-653)
🪛 Clang (14.0.6)
src/Features/LensEffects.h
[error] 2-2: 'Feature.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). (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 (9)
src/Globals.h (1)
12-12: LGTM!Forward declaration and extern follow the established pattern for feature integration.
Also applies to: 84-84
src/Globals.cpp (1)
15-15: LGTM!Feature registration follows the established pattern for the globals::features namespace.
Based on learnings
Also applies to: 80-80
package/Shaders/Common/CoordMath.hlsli (1)
1-37: LGTM!Coordinate math utilities are well-structured with proper bounds checking on atlas fetch functions (lines 21, 28 use clamp to ensure valid indices).
src/Features/LensEffects.h (1)
17-17: Update VR support status or add implementation.
SupportsVR()returnsfalsewith a comment marker. Clarify whether VR support is planned or document why it's not supported.package/Shaders/Common/Math.hlsli (5)
21-30: LGTM! Clean vector reduction helpers.The min/max/sum helper functions are correctly implemented with efficient recursive patterns for multi-component vectors.
32-33: LGTM! Safe modulo implementation.The unsigned floating-point modulo functions correctly guard against division by zero using
max(y, EPSILON_DIVISION).
35-35: LGTM! nRoot guards against division by zero.The
n == 0guard addresses the previous review comment about division by zero. The simplified handling of negativex(clamping to zero) is acceptable for typical shader use cases.
37-37: LGTM! Convenient sincos wrapper.The
sincos2wrapper provides a cleaner return-value interface over the out-parameter version.
51-61: LGTM! Smootherstep polynomial is correctly implemented.The smootherstep functions correctly implement the 6t⁵ - 15t⁴ + 10t³ polynomial. Note that they depend on
LinearStepfor normalization, so fixing theLinearStepsign issue will also fix any inherited issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
src/Features/LensEffects.h (5)
1-3: Past review already identified missing headers for types used in this file (std::array, std::unique_ptr, std::string, etc.). That issue remains valid.
51-90: Past reviews identified that raw D3D11 pointers risk leaks and should useMicrosoft::WRL::ComPtr<>for automatic lifetime management. This remains a valid concern across all these resource declarations.
363-364: Past reviews identified:
VectorToXMFloatshould takeconst float4&instead of non-const referenceLinearStepneeds divide-by-zero protection when edge0 == edge1
428-428: Past review suggested encapsulating thepassesdonemember with private access and public getter/setter methods.
507-514: Past review noted thestd::out_of_rangeexception should include a descriptive message like "Effect descriptor not found in pass list".features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (5)
373-374: Past review identified thatsqrt(InvDist - inv(UIBladeLength+0.1))can receive a negative argument and produce NaN. The argument should be clamped to a non-negative value before calling sqrt.
505-506: Past review identified that the loop can indexinput.Vertices[i+1]out of bounds whenUIGhostShape >= 10. The loop iteration count should be clamped to prevent this.
706-711: Past review identified that the per-axis clampingmin(UICAMaxOffset.xx, Offset)allows diagonal offsets to exceed the intended maximum. Should use radial clamping instead.
762-784: Past review identified that the sun-glare pass samples and blends colors in gamma space without proper linear color space conversions. Based on learnings about image-space post-processing shaders in this repository, all color math should be performed in linear space with appropriate conversions.
220-229: Fix temporal average window mismatch.The current code writes temporal samples to
SunLUT[int2(idx, 0)]whereidxcycles through 5-16 (based onframeIdxin the C++ code), but only reads from indices 5-14 (nFrames = 10). This means the last two frames written (indices 15-16) are never included in the average, creating an inconsistent temporal window.Apply this fix to align the read window with the write cycle:
static const int nFrames = 10; +static const int RING_BASE = 5; float GetTemporalAverage(float Value, uint idx){ - SunLUT[int2(idx, 0)] = Value.xxxx; + uint ringIdx = RING_BASE + ((idx - RING_BASE) % nFrames); + SunLUT[int2(ringIdx, 0)] = Value.xxxx; float Sum = 0.0; [unroll] for(int i=0; i < nFrames; ++i) - Sum += SunLUT.Load(int2(5+i, 0)).x; + Sum += SunLUT.Load(int2(RING_BASE + i, 0)).x; return Sum / nFrames; }This ensures a proper ring buffer where the most recent 10 frames are always averaged.
src/Features/LensEffects.cpp (3)
41-64: Past review identified that shader compilation lacks error handling. IfUtil::CompileShaderreturns nullptr, using these pointers later will cause crashes.
139-152: Past review noted that raw pointers allocated withnew(SettingsCB and renderdata) should use smart pointers likestd::unique_ptrto prevent memory leaks.
550-557: Past review identified that the pointer chainsky->sun->sunBase->GetGeometryRuntimeData().properties[1]needs additional null/bounds guards to prevent crashes if the properties vector has fewer than 2 elements or ifproperties[1]is null.
🧹 Nitpick comments (5)
package/Shaders/Common/CoordMath.hlsli (2)
6-7: Consider adding inline documentation.The
DegreesToVectoroverloads are mathematically correct but lack documentation explaining the component ordering. For the float2 variant, clarifying that the float4 output is(cos(degrees.x), cos(degrees.y), sin(degrees.x), sin(degrees.y))would help future maintainers.
6-35: Consider adding function documentation.While the implementations are correct, inline documentation would improve maintainability. Consider adding brief comments explaining:
- Parameter meanings and valid ranges
- Return value semantics (e.g., angle ranges, component ordering)
- Atlas layout assumptions for
AtlasFetch*functions- Boundary inclusiveness for
InsideRectThis is particularly valuable for utility functions used across multiple shaders.
src/Features/LensEffects.cpp (3)
136-137: Error handling improved but consider logging on failure.The texture loading now uses
DX::ThrowIfFailedwhich will throw exceptions on failure. Consider adding a try-catch with logging to provide more context about which texture failed to load, especially since these are user-facing assets.
533-548: Consider adding null check for skyrim_SunPosition.The code dereferences
skyrim_SunPosition(line 537) without a null check. While this pointer is initialized inSetupResources(), adding a guard would make the code more defensive.DirectX::XMFLOAT4A LensEffects::GetSunPosition() { static float sunRadius = 0.0f; + if (!skyrim_SunPosition) { + logger::warn("[Lens Effects] skyrim_SunPosition is null"); + return DirectX::XMFLOAT4A(0.0f, 0.0f, 0.0f, sunRadius); + } float2 screenSize = Util::ConvertToDynamic(globals::state->screenSize); auto sunWSPos = *skyrim_SunPosition;
459-460: Consider null checks for singleton accessors.The code calls
RE::Calendar::GetSingleton()andRE::PlayerCharacter::GetSingleton()without null checks. While singletons typically don't return null in this codebase, adding defensive checks would make the code more robust, especially forGetParentCell()which could potentially return null.float time = RE::Calendar::GetSingleton()->GetCurrentGameTime(); - bool IsInside = RE::PlayerCharacter::GetSingleton()->GetParentCell()->IsInteriorCell(); + auto player = RE::PlayerCharacter::GetSingleton(); + auto cell = player ? player->GetParentCell() : nullptr; + bool IsInside = cell ? cell->IsInteriorCell() : false;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.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:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslpackage/Shaders/Common/CoordMath.hlslisrc/Features/LensEffects.cppsrc/Features/LensEffects.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/Lens Effects/Shaders/LensEffects/LensEffects.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/Features/LensEffects.cppsrc/Features/LensEffects.h
🧠 Learnings (2)
📚 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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
PR: doodlum/skyrim-community-shaders#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/Features/LensEffects.cpp
🧬 Code graph analysis (2)
src/Features/LensEffects.cpp (2)
src/State.cpp (3)
SetupResources(556-589)SetupResources(556-556)i(197-197)src/Features/LensEffects.h (3)
UpdateSettings(366-384)GetSingleton(6-10)LinearStep(364-364)
src/Features/LensEffects.h (2)
src/Features/RenderDoc.h (1)
SupportsVR(59-59)src/Features/LensEffects.cpp (66)
SetupResources(66-165)SetupResources(66-66)CompileShaders(41-64)CompileShaders(41-41)CheckOverride(167-179)CheckOverride(167-167)LookupShader(181-198)LookupShader(181-181)AppendOcclusionLUT(382-415)AppendOcclusionLUT(382-382)SetupOcclusionMask(216-257)SetupOcclusionMask(216-216)SetupBurstEffect(259-269)SetupBurstEffect(259-259)SetupSunGlareEffect(271-283)SetupSunGlareEffect(271-271)SetupLensGlareEffect(285-295)SetupLensGlareEffect(285-285)SetupHaloEffect(297-307)SetupHaloEffect(297-297)SetupGhostEffect(309-339)SetupGhostEffect(309-309)SetupIceEffect(359-380)SetupIceEffect(359-359)BypassShader(417-425)BypassShader(417-417)SetupCAEffect(341-357)SetupCAEffect(341-341)GetSunPosition(533-548)GetSunPosition(533-533)GetSunColor(550-557)GetSunColor(550-550)GetWeatherShader(427-487)GetWeatherShader(427-427)GetWeatherPrecip(489-499)GetWeatherPrecip(489-489)CheckWeatherChange(501-511)CheckWeatherChange(501-501)UpdateWeatherBasedDisable(513-531)UpdateWeatherBasedDisable(513-513)RefreshToggles(996-1007)RefreshToggles(996-996)RestoreDefaultSettings(1020-1024)RestoreDefaultSettings(1020-1020)DrawSettings(675-994)DrawSettings(675-675)LoadSettings(1009-1013)LoadSettings(1009-1009)SaveSettings(1015-1018)SaveSettings(1015-1015)UpdateBufferValues(200-214)UpdateBufferValues(200-200)thunk(559-594)thunk(559-559)thunk(596-604)thunk(596-596)thunk(606-622)thunk(606-606)thunk(624-632)thunk(624-624)thunk(636-646)thunk(636-636)thunk(649-661)thunk(649-649)thunk(663-673)thunk(663-663)
🪛 Clang (14.0.6)
src/Features/LensEffects.h
[error] 2-2: 'Feature.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (11)
package/Shaders/Common/CoordMath.hlsli (6)
1-5: LGTM: Clean include guard and namespace setup.The include guard and namespace declaration follow best practices for HLSL headers, preventing multiple inclusion and name collisions.
11-14: LGTM: Proper edge-case handling.The zero-radius check on line 13 correctly prevents undefined angle values at the origin. Note that
atan2returns angles in the range [-π, π].
16-18: LGTM: Efficient polar-to-Cartesian conversion.Correct implementation using
sincosfor optimal performance.
20-25: LGTM: Bounds checking implemented.The
clampon line 21 properly addresses the bounds-checking concern from previous reviews, ensuring safe array access.
27-32: LGTM: Bounds checking implemented.The
clampon line 28 properly addresses the bounds-checking concern from previous reviews, ensuring safe array access.
34-35: LGTM: Correct rectangle containment test.The implementation correctly checks inclusive boundaries (
rectMin <= p <= rectMax). The use ofstepfunctions is idiomatic for HLSL.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
1-7: LGTM! Includes are properly organized.The shader includes the necessary common utilities for frame buffer operations, math, coordinate transformations, random number generation, and color space handling.
src/Features/LensEffects.cpp (4)
1-40: LGTM! JSON serialization setup is well-structured.The NLOHMANN macros properly define serialization for all settings structures, enabling clean save/load functionality.
246-254: LGTM! Rasterizer state handling includes proper null checks.The code correctly checks if
Rasteris null before attempting to get and modify its description.
396-406: UAV binding logic looks correct.The code properly binds the occlusion UAV at slot 7 and guards against cases where there are too many RTVs (8 or more), which would conflict with the UAV slot. The logic aligns with the HLSL register declaration.
996-1024: LGTM! Settings management functions are clean and well-structured.The toggle refresh, load, save, and restore functions properly integrate with the render data system and maintain consistency between settings and active effects.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (12)
package/Shaders/Common/Math.hlsli (3)
63-71: Consider more robust sinc calculation in Diffraction.This was flagged in previous reviews. The current implementation uses
max(sinc, EPSILON_DIVISION)which guards against division by zero but doesn't properly handle the mathematical limit: sinc(0) = 1.For more robust behavior:
float Diffraction(float x, float Frequency, float Phase, float Amplitude){ - float sinc = PI * (x * Frequency + Phase); - sinc = sin(sinc) / max(sinc, EPSILON_DIVISION); + float arg = PI * (x * Frequency + Phase); + float sinc = (abs(arg) < EPSILON_DIVISION) ? 1.0 : sin(arg) / arg; return sinc * sinc * Amplitude;} float3 Diffraction(float3 x, float Frequency, float Phase, float Amplitude){ - float3 sinc = PI * (x * Frequency + Phase); - sinc = sin(sinc) / max(sinc, EPSILON_DIVISION); + float3 arg = PI * (x * Frequency + Phase); + float3 sinc = (abs(arg) < EPSILON_DIVISION) ? 1.0 : sin(arg) / arg; return sinc * sinc * Amplitude;}This explicitly handles the mathematical limit and avoids introducing small errors when the argument is near zero.
39-46: Critical: LinearStep still flips sign when edge1 < edge0.This issue was flagged in previous reviews but remains unfixed. When
edge1 < edge0, the denominator is negative, butmax((edge1 - edge0), EPSILON_DIVISION)clamps it to a positive value, inverting the interpolation direction.Apply this fix to preserve the sign:
float LinearStep(float edge0, float edge1, float x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float denom = edge1 - edge0; + denom = (denom >= 0 ? 1.0 : -1.0) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);} float2 LinearStep(float2 edge0, float2 edge1, float2 x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float2 denom = edge1 - edge0; + denom = sign(denom) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);} float3 LinearStep(float3 edge0, float3 edge1, float3 x){ - return saturate((x - edge0) / max((edge1 - edge0), EPSILON_DIVISION));} + float3 denom = edge1 - edge0; + denom = sign(denom) * max(abs(denom), EPSILON_DIVISION); + return saturate((x - edge0) / denom);}
48-49: Critical: MapRange inherits the LinearStep sign-flipping bug.This issue was flagged in previous reviews. When
oldMax < oldMin(reversed range),max(oldMax - oldMin, EPSILON_DIVISION)flips the sign, breaking reversed range mappings.Apply this fix:
float MapRange(float x, float oldMin, float oldMax, float newMin, float newMax){ - return newMin + ((x - oldMin) / max(oldMax - oldMin, EPSILON_DIVISION)) * (newMax - newMin);} + float denom = oldMax - oldMin; + denom = (denom >= 0 ? 1.0 : -1.0) * max(abs(denom), EPSILON_DIVISION); + return newMin + ((x - oldMin) / denom) * (newMax - newMin);}features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (4)
704-711: Clamp chromatic-aberration offset radially, not per-axis.min(UICAMaxOffset.xx, Offset) allows diagonal overshoot. Scale by length.
- if(UICAThreshold > 0.0){ + if(UICAThreshold > 0.0){ float2 Motion = MotionVector.Load(int3(input.Position.xy, 0)).xy - 1e-6; Offset *= saturate(length(Motion) - UICAThreshold); - Offset = min(UICAMaxOffset.xx, Offset) * ScreenSize.xy * normalize(Motion); + float2 dir = normalize(Motion); + float2 raw = Offset * ScreenSize.xy * dir; + float len = length(raw); + float scale = min(1.0, UICAMaxOffset / delta(len)); + Offset = raw * scale; }
222-229: Temporal averaging doesn’t update the history window that you read from.You always read [5..14] but write at index idx, so frames outside [5..14] never contribute. Use a ring buffer anchored to the read base.
-static const int nFrames = 10; +static const int nFrames = 10; +static const int RING_BASE = 5; -float GetTemporalAverage(float Value, uint idx){ - SunLUT[int2(idx, 0)] = Value.xxxx; +float GetTemporalAverage(float Value, uint idx){ + uint ringIdx = (idx + nFrames - 1) % nFrames; + SunLUT[int2(RING_BASE + ringIdx, 0)] = Value.xxxx; float Sum = 0.0; - [unroll] for(int i=0; i < nFrames; ++i) - Sum += SunLUT.Load(int2(5+i, 0)).x; + [unroll] for(int i=0; i < nFrames; ++i) + Sum += SunLUT.Load(int2(RING_BASE + i, 0)).x; return Sum / nFrames; }
373-375: sqrt can receive negative input → NaNs propagate.Clamp the argument before sqrt in BladeFeather.
- float BladeFeather = UIBladeFeather / delta(sqrt(InvDist - inv(UIBladeLength+0.1))); + float sqrtArg = max(0.0, InvDist - inv(UIBladeLength + 0.1)); + float BladeFeather = UIBladeFeather / delta(sqrt(sqrtArg));
505-514: Possible out-of-bounds on input.Vertices[i+1] when UIGhostShape ≥ 10.Clamp shape and use uint counter to keep i+1 in-range.
- [loop] for(int i = 0; i < UIGhostShape; ++i){ + uint shape = (uint)clamp(round(UIGhostShape), 3.0, 9.0); + [loop] for(uint i = 0u; i < shape; ++i){ float2 Edge = ((input.Vertices[i+1] - input.Vertices[i]) * float2(-1.0, 1.0)).yx;src/Feature.cpp (1)
181-186: Null-pointer risk: versionInCache may be null.Guard before strcmp to avoid crashes on missing/old caches.
- auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); - if (strcmp(versionInCache, version.c_str()) != 0) { + auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); + if (versionInCache == nullptr || strcmp(versionInCache, version.c_str()) != 0) {package/Shaders/Sky.hlsl (1)
269-276: Don’t compare floats for exact equality in screen space.
input.Position.y == SunParams.yis brittle. Use an epsilon/pixel window.- if(input.Position.y == SunParams.y) + if (abs(input.Position.y - SunParams.y) < 0.5) LensEffects[int2(2,0)] = psout.Color;src/Features/LensEffects.cpp (3)
41-64: Handle shader compilation failures.Check CompileShader return and bail/disable effect on failure to avoid null derefs later.
- SunOcclusionMaskPixelShader = (ID3D11PixelShader*)Util::CompileShader(...); + if (auto sh = Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0"); sh) { + SunOcclusionMaskPixelShader = static_cast<ID3D11PixelShader*>(sh); + } else { + logger::error("[Lens Effects] Failed to compile OCCLUSION_PIXEL_SHADER"); + overrideShader = false; return; + }Apply similar checks to all CompileShader calls in this function.
171-177: Frame index wrap introduces gaps vs. shader history.
frameIdx = (frameIdx < 16) ? ++frameIdx : 5;skips 0–4 and never cycles cleanly with the shader’s [5..14] window. Wrap 5..14 (or implement the same ring base as HLSL).- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + constexpr uint32_t kBase = 5, kCount = 10; + frameIdx = (frameIdx < (kBase + kCount)) ? frameIdx + 1 : kBase;
552-557: Null-guards on sky property chain.
properties[1]access can crash when vector shorter or element null. Add size and null checks before cast.- if (auto sky = globals::game::sky; sky && sky->sun && sky->sun->sunBase) - if (auto property = skyrim_cast<RE::BSSkyShaderProperty*>(sky->sun->sunBase->GetGeometryRuntimeData().properties[1].get())) + if (auto sky = globals::game::sky; sky && sky->sun && sky->sun->sunBase) { + auto& props = sky->sun->sunBase->GetGeometryRuntimeData().properties; + if (props.size() > 1 && props[1]) { + if (auto property = skyrim_cast<RE::BSSkyShaderProperty*>(props[1].get())) sunColor = { property->kBlendColor.red, property->kBlendColor.green, property->kBlendColor.blue, 1.0f }; + } + }
🧹 Nitpick comments (7)
package/Shaders/Common/Math.hlsli (1)
35-35: Consider handling odd roots of negative numbers.The current implementation clamps negative values to zero with
max(0, x), which prevents valid odd roots of negative numbers (e.g.,nRoot(-8, 3)should return-2but will return0).For more mathematically complete behavior, consider:
- float nRoot(float x, float n) {return (n == 0) ? 1.0 : pow(max(0, x), rcp(n));} + float nRoot(float x, float n) { + if (n == 0) return 1.0; + return (x >= 0) ? pow(x, rcp(n)) : -pow(-x, rcp(n)); + }However, if negative inputs are not expected in your use cases, the current implementation is acceptable.
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (3)
523-531: Do color math in linear space.Atlas sampling and blending are in gamma. Convert around the math to match linear-light pipeline.
Based on learnings- float3 Atlas = AtlasTexture.Sample(Linear_Sampler, input.AtlasCoords.xy).xyz; - Color += Atlas * input.AtlasCoords.z * inv(EdgeEffect) * 0.5; + float3 atlasGamma = AtlasTexture.Sample(Linear_Sampler, input.AtlasCoords.xy).xyz; + float3 atlasLin = Color::GammaToLinear(atlasGamma); + float3 colorLin = Color::GammaToLinear(Color); + colorLin += atlasLin * input.AtlasCoords.z * inv(EdgeEffect) * 0.5; + Color = Color::LinearToGamma(colorLin);Also applies to: 526-528
704-723: Chromatic aberration on Main should respect linear pipeline.Wrap Main.Load results with Gamma→Linear, then back to Linear→Gamma at return.
Based on learnings- float4 Aberration = Main.Load(int3(input.Position.xy, 0)); + float4 Aberration = float4(Color::GammaToLinear(Main.Load(int3(input.Position.xy, 0)).xyz), 1.0); ... - return Aberration; + return float4(Color::LinearToGamma(Aberration.xyz), Aberration.w);
244-252: Avoid magic threshold for SunVisibility.
if (SunVisibility < 100) return 0;is arbitrary and resolution‑dependent. Derive from disc area or expose a constant.- if(SunVisibility < 100) return 0; + const float kMinVisibleFrac = 0.01; // 1% of disc + float discArea = Math::PI * (SunSSRadius * SunSSRadius); + if (SunVisibility < discArea * kMinVisibleFrac) return 0;src/Features/LensEffects.cpp (1)
139-139: Prefer RAII for owned resources.Replace raw new with smart pointers to prevent leaks on early returns.
- SettingsCB = new ConstantBuffer(ConstantBufferDesc<ConstBuffer>()); + SettingsCB = std::make_unique<ConstantBuffer>(ConstantBufferDesc<ConstBuffer>()); - renderdata = new Setup::LF_RenderData; + renderdata = std::make_unique<Setup::LF_RenderData>();Adjust member types and call sites accordingly (use
.get()only where APIs require raw pointers).Also applies to: 152-153
src/Features/LensEffects.h (2)
17-17: Remove extraneous semicolon.The extra semicolon after the closing brace is unnecessary.
Apply this diff:
- virtual inline bool SupportsVR() override { return false; }; // + virtual inline bool SupportsVR() override { return false; }
348-348: Consider encapsulating thesettingsmember.The
settingsmember is publicly accessible, which reduces encapsulation. Consider making it private with appropriate accessor methods if you want to control access or add validation in the future.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (14)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- features/Lens Effects/Shaders/Features/LensEffects.ini
- src/Globals.cpp
- package/Shaders/ISSAOComposite.hlsl
- package/Shaders/Common/Random.hlsli
- package/Shaders/Common/CoordMath.hlsli
- src/State.cpp
- package/Shaders/Common/Color.hlsli
🧰 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/Globals.hpackage/Shaders/Sky.hlslpackage/Shaders/Common/Math.hlslisrc/Feature.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cppsrc/Features/LensEffects.h
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/Globals.hsrc/Feature.cppsrc/Features/LensEffects.cppsrc/Features/LensEffects.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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
🧠 Learnings (3)
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
PR: doodlum/skyrim-community-shaders#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/Sky.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
package/Shaders/Sky.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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
🧬 Code graph analysis (2)
src/Features/LensEffects.cpp (3)
src/State.cpp (3)
SetupResources(556-589)SetupResources(556-556)i(197-197)include/PCH.h (1)
ThrowIfFailed(148-153)src/Features/LensEffects.h (3)
UpdateSettings(366-384)GetSingleton(6-10)LinearStep(364-364)
src/Features/LensEffects.h (1)
src/Features/LensEffects.cpp (28)
SetupResources(66-165)SetupResources(66-66)CompileShaders(41-64)CompileShaders(41-41)GetSunPosition(533-548)GetSunPosition(533-533)GetSunColor(550-557)GetSunColor(550-550)GetWeatherShader(427-487)GetWeatherShader(427-427)GetWeatherPrecip(489-499)GetWeatherPrecip(489-489)UpdateBufferValues(200-214)UpdateBufferValues(200-200)thunk(559-594)thunk(559-559)thunk(596-604)thunk(596-596)thunk(606-622)thunk(606-606)thunk(624-632)thunk(624-624)thunk(636-646)thunk(636-636)thunk(649-661)thunk(649-649)thunk(663-673)thunk(663-663)
🪛 Clang (14.0.6)
src/Features/LensEffects.h
[error] 2-2: 'Feature.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). (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 (8)
package/Shaders/Common/Math.hlsli (4)
21-30: LGTM! Well-structured component helpers.The min/max/sum component helpers are correctly implemented with efficient recursive patterns. These utilities will be useful for vector operations throughout the shader codebase.
32-33: LGTM! Correct unsigned float modulo implementation.The
ufmodfunctions correctly implement unsigned float modulo with appropriate division-by-zero protection.
37-37: LGTM! Convenient wrapper for sincos intrinsic.The
sincos2function provides a clean vector-returning interface for the sincos intrinsic.
51-61: LGTM! Correct smootherstep implementation.The smootherstep functions correctly implement Ken Perlin's improved smoothstep polynomial (6t⁵ - 15t⁴ + 10t³). Note that these functions depend on
LinearStep, so fixing the sign-preservation issue inLinearStep(lines 39-46) will automatically correct any edge cases here.src/Feature.cpp (1)
232-233: Feature wiring LGTM.Including LensEffects in the feature list looks correct.
src/Globals.h (1)
12-12: Global declaration wiring LGTM.Forward declaration and extern for LensEffects are consistent.
Also applies to: 84-85
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
389-397: No issues found; the code is correct.A
float2overload ofRandom::RandomSHexists. At line 395,floor(Coords * RayWidth)produces afloat2argument that correctly matches thefloat2overload. The function returnsfrac(sin(dot(seed, float2(12.9898, 78.233))) * 43758.5453), which guarantees output in the [0,1] range sincefrac()returns fractional parts.Likely an incorrect or invalid review comment.
src/Features/LensEffects.cpp (1)
396-404: UAV slot mapping is correctly implemented.The C++ code binds
SunOcclusionUAVandSunOcclusionUAV_ATtoUAVStartSlot=7, which correctly maps to the HLSLregister(u7)declarations forLensEffectsATandLensEffectsin Sky.hlsl (lines 173 & 176). ThenumRTsparameter does not affect UAV slot resolution; UAV slots are independent of render target count. ThePSSetShaderResources(30, 1, &SunOcclusionSRV)call also correctly matches theregister(t30)bindings in the shader.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (3)
292-293: Guard DistFactor against a zero sun-radius.
SunSSRadiuscan be zero, so this divide produces INF/NaN and corrupts the LUT. Wrap the denominator indelta()(or equivalent epsilon clamp) before dividing.- float DistFactor = SunVisibility / (Math::PI * (SunSSRadius * SunSSRadius)); + float DistFactor = SunVisibility / delta(Math::PI * (SunSSRadius * SunSSRadius));
373-375: Prevent sqrt from seeing negative input.
InvDist - inv(UIBladeLength + 0.1)easily slips negative, so the sqrt emits NaN and the flare spikes blow up. Clamp the argument to ≥0 (or use delta) before the sqrt.- float BladeFeather = UIBladeFeather / delta(sqrt(InvDist - inv(UIBladeLength+0.1))); + float featherArg = max(0.0, InvDist - inv(UIBladeLength + 0.1)); + float BladeFeather = UIBladeFeather / delta(sqrt(featherArg));
222-229: Fix the temporal ring buffer to stay in-bounds.
Framefrom the constant buffer hits 16 every cycle, soSunLUT[int2(idx, 0)]writes past the 16-wide texture (valid slots 0–15). Because the read window is hard-coded to[5..14], the newest sample (slot 16) is also ignored. Wrap the index into the 10-slot window that the sum reads.static const int nFrames = 10; float GetTemporalAverage(float Value, uint idx){ - SunLUT[int2(idx, 0)] = Value.xxxx; + const uint ringBase = 5; + const uint ringIdx = (idx + nFrames - 1) % nFrames; + SunLUT[int2(ringBase + ringIdx, 0)] = Value.xxxx; float Sum = 0.0; - [unroll] for(int i=0; i < nFrames; ++i) - Sum += SunLUT.Load(int2(5+i, 0)).x; + [unroll] for(int i=0; i < nFrames; ++i) + Sum += SunLUT.Load(int2(ringBase + i, 0)).x; return Sum / nFrames; } float GetTemporalAverage(uint Value, uint idx){ - SunLUT_AT[int2(idx, 0)] = Value; + const uint ringBase = 5; + const uint ringIdx = (idx + nFrames - 1) % nFrames; + SunLUT_AT[int2(ringBase + ringIdx, 0)] = Value;Also applies to: 233-241
src/Features/LensEffects.cpp (2)
171-176: KeepframeIdxinside the 10-slot history window.The current increment reaches 16 before resetting, so the shader writes to slot 16 (out of range for the 16-wide LUT) and the newest sample is ignored. Wrap inside
[5..14]to match the shader fix.- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + constexpr uint32_t kRingBase = 5; + constexpr uint32_t kRingLength = 10; + frameIdx = kRingBase + ((frameIdx - kRingBase + 1) % kRingLength);
239-245: Bind the depth SRV that the shader samples.
DepthTexturesits att0in the pixel shader, but this setup only bindst1/t2. Unless the engine leaves a compatible depth SRV lingering, everyDepthTexture.SampleCmpLevelZerois undefined. Pull the post-Z-prepass depth SRV and bind it explicitly before the main/motion textures.- context->PSSetShaderResources(1, 1, &mainSRV); - context->PSSetShaderResources(2, 1, &motionVectorSRV); + auto& depthSRV = renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGETS_DEPTHSTENCIL::kPOST_ZPREPASS_COPY].SRV; + context->PSSetShaderResources(0, 1, &depthSRV); + context->PSSetShaderResources(1, 1, &mainSRV); + context->PSSetShaderResources(2, 1, &motionVectorSRV);src/Features/LensEffects.h (2)
97-97: Default the sun-visible flag.
skyrim_SunVisbleis read before hooks run; leaving it indeterminate risks bogus logic in the first frame. Initialize it tofalse.- bool skyrim_SunVisble; + bool skyrim_SunVisble = false;
563-569: Don’t pass a dangling stack pointer to the original thunk.
previous = ¤thands the callee a pointer into this stack frame; oncethunkreturns, the pointer dangles and any later dereference is UAF. Use a stable storage location instead.struct LensFlare_AssignTexture //remove lensflare texture init/assignment { static void thunk(void* previous, uint64_t current) { - current = 0; - func(previous = ¤t, current); + static uint64_t zero = 0; + func(&zero, 0); }package/Shaders/ISSAOComposite.hlsl (1)
139-139: Sanitize every RGB component and include infinities.Only the red channel is scrubbed, and the
>check lets ±INF survive. That leaves NaNs/Infs in G/B to contaminate later math. Zero all three channels and use>=so infinities are caught.- sourceColor.x = ((asuint(sourceColor.x) & 0x7fffffff) > 0x7f800000) ? 0.0 : sourceColor.x; + sourceColor.r = ((asuint(sourceColor.r) & 0x7fffffff) >= 0x7f800000) ? 0.0 : sourceColor.r; + sourceColor.g = ((asuint(sourceColor.g) & 0x7fffffff) >= 0x7f800000) ? 0.0 : sourceColor.g; + sourceColor.b = ((asuint(sourceColor.b) & 0x7fffffff) >= 0x7f800000) ? 0.0 : sourceColor.b;
🧹 Nitpick comments (1)
package/Shaders/Common/Random.hlsli (1)
38-41: Consider documenting the purpose and typical use case.The "SH" suffix in
RandomSHis unclear, and without documentation users won't know when to use these functions versus the more sophisticated hashes already available in this file (murmur3, pcg, etc.).Consider adding a brief comment explaining the purpose and performance/quality tradeoff:
+ // Lightweight sine-based hash functions for visual effects where quality/performance favors simplicity. + // For higher-quality randomness, prefer pcg* or murmur3 functions. float RandomSH(float seed) { return frac(sin(seed * 12.9898) * 43758.5453); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Lens Effects/Shaders/LensEffects/Textures/Atlas.ddsis excluded by!**/*.ddsfeatures/Lens Effects/Shaders/LensEffects/Textures/Frost.ddsis excluded by!**/*.dds
📒 Files selected for processing (14)
features/Lens Effects/Shaders/Features/LensEffects.ini(1 hunks)features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)package/Shaders/Common/Color.hlsli(3 hunks)package/Shaders/Common/CoordMath.hlsli(1 hunks)package/Shaders/Common/Math.hlsli(1 hunks)package/Shaders/Common/Random.hlsli(1 hunks)package/Shaders/ISSAOComposite.hlsl(1 hunks)package/Shaders/Sky.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- package/Shaders/Sky.hlsl
- src/Globals.h
- package/Shaders/Common/CoordMath.hlsli
- features/Lens Effects/Shaders/Features/LensEffects.ini
- src/State.cpp
- package/Shaders/Common/Math.hlsli
- src/Feature.cpp
🧰 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/Globals.cpppackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Common/Color.hlslipackage/Shaders/Common/Random.hlslisrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.h
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/Globals.cppsrc/Features/LensEffects.cppsrc/Features/LensEffects.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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
🧠 Learnings (21)
📓 Common learnings
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: 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: 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} : Place all feature shaders under features/YourFeature/Shaders/
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: 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.
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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
📚 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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Globals.cppsrc/Features/LensEffects.h
📚 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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/Globals.cpp
📚 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:
src/Globals.cpppackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Common/Color.hlslisrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.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/ISSAOComposite.hlslsrc/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.h
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
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.
Applied to files:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.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: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
src/Features/LensEffects.cppsrc/Features/LensEffects.h
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.h
📚 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} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-06-17T09:27:49.594Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
src/Features/LensEffects.cpp
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-06-09T22:27:55.011Z
Learnt from: soda3000
Repo: doodlum/skyrim-community-shaders PR: 1123
File: src/Menu.cpp:1407-1408
Timestamp: 2025-06-09T22:27:55.011Z
Learning: When checking for missing constants or definitions in C++ code, always verify both the source file (.cpp) and the corresponding header file (.h) before flagging missing definitions, as constants and static members are typically declared in header files.
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.h
📚 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 src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
src/Features/LensEffects.h
🧬 Code graph analysis (2)
src/Features/LensEffects.cpp (3)
src/State.cpp (3)
SetupResources(556-589)SetupResources(556-556)i(197-197)include/PCH.h (1)
ThrowIfFailed(148-153)src/Features/LensEffects.h (3)
UpdateSettings(366-384)GetSingleton(6-10)LinearStep(364-364)
src/Features/LensEffects.h (2)
src/Features/RenderDoc.h (1)
SupportsVR(59-59)src/Features/LensEffects.cpp (56)
SetupResources(66-165)SetupResources(66-66)CompileShaders(41-64)CompileShaders(41-41)CheckOverride(167-179)CheckOverride(167-167)LookupShader(181-198)LookupShader(181-181)AppendOcclusionLUT(382-415)AppendOcclusionLUT(382-382)SetupOcclusionMask(216-257)SetupOcclusionMask(216-216)SetupBurstEffect(259-269)SetupBurstEffect(259-259)SetupSunGlareEffect(271-283)SetupSunGlareEffect(271-271)SetupLensGlareEffect(285-295)SetupLensGlareEffect(285-285)SetupHaloEffect(297-307)SetupHaloEffect(297-297)SetupGhostEffect(309-339)SetupGhostEffect(309-309)SetupIceEffect(359-380)SetupIceEffect(359-359)BypassShader(417-425)BypassShader(417-417)SetupCAEffect(341-357)SetupCAEffect(341-341)GetSunPosition(533-548)GetSunPosition(533-533)GetSunColor(550-557)GetSunColor(550-550)GetWeatherShader(427-487)GetWeatherShader(427-427)GetWeatherPrecip(489-499)GetWeatherPrecip(489-489)CheckWeatherChange(501-511)CheckWeatherChange(501-501)UpdateWeatherBasedDisable(513-531)UpdateWeatherBasedDisable(513-513)UpdateBufferValues(200-214)UpdateBufferValues(200-200)thunk(559-594)thunk(559-559)thunk(596-604)thunk(596-596)thunk(606-622)thunk(606-606)thunk(624-632)thunk(624-624)thunk(636-646)thunk(636-636)thunk(649-661)thunk(649-649)thunk(663-673)thunk(663-663)
🪛 Clang (14.0.6)
src/Features/LensEffects.h
[error] 2-2: 'Feature.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). (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/Common/Random.hlsli (1)
38-41: Verify the choice of simple sine-based hashing.These functions use classic sine-based pseudo-random generation, which is simpler and faster but lower quality than the sophisticated options already available in this file (e.g.,
pcg,murmur3,perlinNoise). If the lens effects feature requires lightweight randomization for per-pixel operations, this is a reasonable tradeoff. However, if quality matters more than performance, consider using the existing higher-quality functions.package/Shaders/Common/Color.hlsli (3)
44-47: LGTM!The chrominance calculation (max - min) is mathematically correct and provides a measure of color saturation. The implementation is clean and efficient.
68-75: LGTM!The RGB to HSV conversion uses a standard branchless algorithm with proper epsilon protection against division by zero. The implementation is mathematically correct and efficient for GPU execution.
77-82: LGTM!The HSV to RGB conversion uses a standard algorithm with fractional hue handling and proper saturation. The implementation correctly inverts the RGBtoHSV function and is well-suited for shader execution.
5 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation