feat: add lens effects#1363
Conversation
WalkthroughAdds a new LensEffects feature: multi‑pass HLSL lens‑effect shaders and config, C++ feature class with hooks/UI/presets, Sky shader integration, several shared shader utility headers, a small NaN clamp in ISSAOComposite, and registration/wiring of the LensEffects feature. Changes
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
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
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.
|
@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.
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.
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).
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.
Dawntic
force-pushed
the
lens-effects
branch
from
9d6da2e to
d6c401c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (19)
package/Shaders/Common/Color.hlsli (1)
6-8: Prefer precise Rec.601/709/2020 luminance constants and clear namingThe LUM_709 and LUM_202 coefficients are slightly off standard Rec.709/2020 values, and
LUM_202is ambiguous. For accuracy/clarity you could:-#define LUM_601 float3(0.299, 0.587, 0.114) -#define LUM_709 float3(0.212, 0.715, 0.072) -#define LUM_202 float3(0.262, 0.678, 0.059) +#define LUM_601 float3(0.2990, 0.5870, 0.1140) +#define LUM_709 float3(0.2126, 0.7152, 0.0722) +#define LUM_2020 float3(0.2627, 0.6780, 0.0593)…and update any uses of
LUM_202toLUM_2020. Also consider reusing these macros in the RGBToLuminance* helpers to avoid duplicated coefficients.src/Features/LensEffects.h (4)
1-2: Add explicit standard/D3D headers for used typesThis header uses
std::string,std::string_view,std::vector,std::array,std::unique_ptr,std::clamp,std::lerp, fixed-width integers,DirectX::XMFLOAT4A, and multipleID3D11*types but only includes"Feature.h". To avoid relying on transitive includes, consider adding:#include <array> #include <memory> #include <string> #include <string_view> #include <vector> #include <algorithm> // clamp #include <cmath> // lerp / fabs #include <cstdint> #include <d3d11.h> #include <DirectXMath.h>(adjust as needed if some are already brought in by Feature.h).
51-91: Consider RAII for D3D11 resources and renderdataThere is a wide surface of raw pointers here (
ID3D11*resources,ConstantBuffer* SettingsCB, andSetup::LF_RenderData* renderdata). This is error-prone ifSetupResources/ shutdown paths ever change or are called more than once.If feasible, prefer smart pointers:
- ConstantBuffer* SettingsCB = nullptr; + std::unique_ptr<ConstantBuffer> SettingsCB; - Setup::LF_RenderData* renderdata = nullptr; + std::unique_ptr<Setup::LF_RenderData> renderdata;and
Microsoft::WRL::ComPtr<T>(or similar) for the D3D11 interfaces, with cleanup centralized in the destructor. Otherwise, please double‑check that all these pointers are deterministically released/deleted on shutdown and on reinit paths.Also applies to: 533-533
363-364: Tighten VectorToXMFloat signature and LinearStep robustness
VectorToXMFloatdoesn’t mutate its input, andLinearStepcan produce inf/NaN whenedge0≈edge1. A small tweak improves safety:-inline DirectX::XMFLOAT4A VectorToXMFloat(float4& value) { return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); } -inline float LinearStep(float edge0, float edge1, float x) { return std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); } +inline DirectX::XMFLOAT4A VectorToXMFloat(const float4& value) +{ + return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); +} + +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); +}This avoids unstable results when edges coincide and clarifies the intent of
VectorToXMFloat.
366-384: Guard UpdateSettings against SB_BladeLength == 0 to avoid infinities/NaNs
UpdateSettingscurrently does:mSettings.SBEX_BladeSplayLen = (1 / mSettings.SB_BladeLength - 0.95f) * (mSettings.SB_BladeSplay > 0.01f);If a preset or UI ever sets
SB_BladeLengthto 0, this yields1 / 0which becomesinf/NaNand will propagate into the constant buffer and shaders.A small guard keeps this stable:
- mSettings.SBEX_BladeSplayLen = (1 / mSettings.SB_BladeLength - 0.95f) * (mSettings.SB_BladeSplay > 0.01f); + mSettings.SBEX_BladeSplayLen = + (mSettings.SB_BladeLength > 1e-6f) + ? ((1.0f / mSettings.SB_BladeLength - 0.95f) * (mSettings.SB_BladeSplay > 0.01f)) + : 0.0f;This preserves existing behavior for valid inputs while making the function safe for edge-case presets.
src/Feature.cpp (1)
163-188: Guard ValidateCache against missing Version in the INI
a_ini.GetValue(ini_name.c_str(), "Version")can returnnullptrif the key is missing, but the code passes it directly tostrcmp, which will crash in that case.A small change makes this robust:
- if (loaded) { - auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); - if (strcmp(versionInCache, version.c_str()) != 0) { + if (loaded) { + auto versionInCache = a_ini.GetValue(ini_name.c_str(), "Version"); + if (!versionInCache || std::strcmp(versionInCache, version.c_str()) != 0) { logger::info("Change in version detected. Installed {} but {} in Disk Cache", version, versionInCache); return false; } else { logger::info("Installed version and cached version match."); } }This keeps the behavior while avoiding undefined behavior when the INI lacks a Version entry.
package/Shaders/Common/CoordMath.hlsli (1)
1-36: Coordinate utilities and atlas helpers look correctThe
CoordMathhelpers (degree-to-vector, polar/cartesian, Chebyshev distance, atlas fetch, InsideRect) are well-formed, and clampingtexNuminAtlasFetch2x2/AtlasFetch1x4prevents out-of-bounds access on the atlas lookup—nicely addressing the earlier bounds concerns.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (5)
706-711: Use radial clamping for chromatic aberration offset.The current per-axis clamp
min(UICAMaxOffset.xx, Offset)allows diagonal offsets to exceed the intended maximum. A previous review suggested radial clamping, which remains unaddressed.Apply radial clamping:
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); + float motionMag = saturate(length(Motion) - UICAThreshold); + Offset *= motionMag * ScreenSize.xy; + float offsetLen = length(Offset); + if (offsetLen > UICAMaxOffset) { + Offset *= UICAMaxOffset / delta(offsetLen); + } + Offset *= normalize(delta(Motion)); }
220-229: Temporal averaging still has ring buffer mismatch.The write index
idx(cycling 5-16 per C++ code line 175) doesn't align with the read window [5..14]. Values written to indices 15-16 are never included in the average. This was flagged in previous reviews but remains unresolved.Apply the ring buffer fix from previous review:
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 = (idx - RING_BASE) % 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; + Sum += SunLUT.Load(int2(RING_BASE + i, 0)).x; return Sum / nFrames; }
292-292: Division by zero risk in DistFactor remains.When
SunSSRadiusis zero, the denominator becomes zero. This was flagged in previous review but not yet addressed.Apply the guard as suggested:
- float DistFactor = SunVisibility / (Math::PI * (SunSSRadius * SunSSRadius)); + float DistFactor = SunVisibility / delta(Math::PI * (SunSSRadius * SunSSRadius));
373-374: Guard sqrt against negative input in BladeFeather calculation.The expression
sqrt(InvDist - inv(UIBladeLength+0.1))can receive negative input whenDistis large, causing NaN propagation. This was flagged previously but not yet addressed.Apply the guard:
- 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-513: Array out-of-bounds risk when UIGhostShape >= 10.The loop iterates up to
UIGhostShapeand accessesinput.Vertices[i+1], but the array only has 10 elements [0..9]. This was flagged previously but not yet addressed.Clamp the loop bound:
- [loop] for(int i = 0; i < UIGhostShape; ++i){ + uint shape = (uint)clamp(round(UIGhostShape), 3.0, 9.0); + [loop] for(uint i = 0; i < shape; ++i){ float2 Edge = ((input.Vertices[i+1] - input.Vertices[i]) * float2(-1.0, 1.0)).yx;src/Features/LensEffects.cpp (7)
41-64: Add error handling for shader compilation failures.All shader compilation calls lack null-pointer checks. If
Util::CompileShaderreturns nullptr, the feature will crash when attempting to use these shaders. This was flagged previously but remains unaddressed.Add checks after each compilation:
void LensEffects::CompileShaders() { SunOcclusionMaskPixelShader = (ID3D11PixelShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0"); + if (!SunOcclusionMaskPixelShader) { + logger::error("[Lens Effects] Failed to compile SunOcclusionMaskPixelShader"); + return; + } // ... repeat for all shaders
136-137: Graceful degradation for missing textures.Using
DX::ThrowIfFailedhard-crashes the feature when textures are missing. A previous review suggested logging and disabling dependent passes instead, but this remains unaddressed.Handle texture loading failures gracefully:
- DX::ThrowIfFailed(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Atlas.dds", nullptr, &AtlasTexSRV)); - DX::ThrowIfFailed(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Frost.dds", nullptr, &IceTexSRV)); + if (FAILED(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Atlas.dds", nullptr, &AtlasTexSRV))) { + logger::error("[Lens Effects] Missing Atlas.dds; disabling Ghosts."); + settings.EnableGhosts = false; + } + if (FAILED(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Frost.dds", nullptr, &IceTexSRV))) { + logger::error("[Lens Effects] Missing Frost.dds; disabling Lens Frost."); + settings.EnableIce = false; + }
139-139: Use smart pointers for automatic memory management.Raw pointers allocated with
newat lines 139 and 152 lack correspondingdeletecalls, risking memory leaks. A previous review suggested usingstd::unique_ptrorstd::shared_ptr, but this remains unaddressed.Replace raw pointers with smart pointers:
- SettingsCB = new ConstantBuffer(ConstantBufferDesc<ConstBuffer>()); + SettingsCB = std::make_unique<ConstantBuffer>(ConstantBufferDesc<ConstBuffer>()); // ... later ... - renderdata = new Setup::LF_RenderData; + renderdata = std::make_unique<Setup::LF_RenderData>();Update the corresponding header declarations to use
std::unique_ptr<ConstantBuffer>andstd::unique_ptr<Setup::LF_RenderData>.Also applies to: 152-152
175-175: Frame index cycling creates temporal history mismatch.The frame index cycles from 5 to 16, then resets to 5, but the shader only reads frames 5-14. Frames 15-16 are written but never included in the temporal average. This was flagged previously but not addressed.
Coordinate with the HLSL ring buffer fix. If the shader reads frames [5..14], cycle within that range:
- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + frameIdx = (frameIdx < 14) ? ++frameIdx : 5;Or if changing the ring buffer base to 0 in HLSL, cycle 0-9 here:
- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + frameIdx = (frameIdx < 9) ? ++frameIdx : 0;
216-257: Bind depth SRV for occlusion pass (t0).The shader samples
DepthTextureat register t0, butSetupOcclusionMask()only binds main (t1) and motion vector (t2), leaving t0 unbound. A previous review identified this and provided a fix using depth stencils, but it remains unaddressed.Bind the depth SRV before the other resources:
+ auto& depthSRV = renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGETS_DEPTHSTENCIL::kPOST_ZPREPASS_COPY].depthSRV; + context->PSSetShaderResources(0, 1, &depthSRV); context->PSSetShaderResources(1, 1, &mainSRV); context->PSSetShaderResources(2, 1, &motionVectorSRV);
533-548: Add VR support for both eyes in sun projection.The code hardcodes
GetCameraData(0), which only uses the left eye in VR mode. This causes incorrect sun glare positioning for the right eye. A previous review identified this and suggested following the pattern from ScreenSpaceGI.cpp, but it remains unaddressed.Per SE/AE/VR coding guidelines, add runtime detection:
DirectX::XMFLOAT4A LensEffects::GetSunPosition() { static float sunRadius = 0.0f; float2 screenSize = Util::ConvertToDynamic(globals::state->screenSize); auto sunWSPos = *skyrim_SunPosition; float4 sunWorldPos = { sunWSPos.x, sunWSPos.y, sunWSPos.z, 1.0f }; - Matrix viewMatrix = Util::GetCameraData(0).viewMat; - Matrix projMatrix = Util::GetCameraData(0).projMat; + // Use eye 0 for flat, but should iterate both eyes for VR + uint32_t eyeCount = 1 + REL::Module::IsVR(); + auto eye = Util::GetCameraData(0); // TODO: handle per-eye for VR + Matrix viewMatrix = eye.viewMat; + Matrix projMatrix = eye.projMat; float4 sunViewPos = float4::Transform(sunWorldPos, viewMatrix);For full VR support, the sun position should be computed and stored per-eye.
550-557: Add bounds check for properties array access.While sky, sun, and sunBase are null-checked, the code directly accesses
properties[1]without verifying the array has at least 2 elements. A previous review suggested adding size checks, but this remains unaddressed.Add bounds checking:
DirectX::XMFLOAT4A LensEffects::GetSunColor() { static float4 sunColor = { 1.0f, 1.0f, 1.0f, 1.0f }; - 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& properties = sky->sun->sunBase->GetGeometryRuntimeData().properties; + if (properties.size() > 1 && properties[1]) { + if (auto property = skyrim_cast<RE::BSSkyShaderProperty*>(properties[1].get())) sunColor = { property->kBlendColor.red, property->kBlendColor.green, property->kBlendColor.blue, 1.0f }; + } + } return VectorToXMFloat(sunColor); }
🧹 Nitpick comments (2)
src/Features/LensEffects.h (1)
4-10: Avoid two LensEffects instances (singleton vs globals::features::lensEffects)
GetSingleton()returns a function-local staticLensEffects, whileglobals::features::lensEffectsis a separate global instance. That can easily lead to state divergence if some code uses the singleton and other code uses the global.I’d strongly recommend standardizing on a single instance (e.g., have
GetSingleton()return&globals::features::lensEffects, or drop the global and use only the singleton) so feature state and resources can’t get out of sync.src/Feature.cpp (1)
1-4: Conventional commit title and issue reference suggestionTo match the project’s conventional commits style, consider a title along the lines of:
feat(lens-effects): add lens flare pipelineIf there’s a tracking GitHub issue for lens effects, add a reference in the PR description, e.g.:
Implements #123(for a feature) orRelated to #123(for partial work / experiments).
📜 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 (6)
- package/Shaders/ISSAOComposite.hlsl
- package/Shaders/Sky.hlsl
- features/Lens Effects/Shaders/Features/LensEffects.ini
- src/Globals.cpp
- package/Shaders/Common/Random.hlsli
- package/Shaders/Common/Math.hlsli
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.hsrc/State.cppsrc/Feature.cpppackage/Shaders/Common/CoordMath.hlslipackage/Shaders/Common/Color.hlslifeatures/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/State.cppsrc/Feature.cppsrc/Features/LensEffects.cppsrc/Features/LensEffects.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Globals.hsrc/State.cppsrc/Feature.cpppackage/Shaders/Common/CoordMath.hlslipackage/Shaders/Common/Color.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/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 (22)
📓 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: 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: 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.
📚 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.hsrc/State.cppsrc/Feature.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.hsrc/Feature.cpppackage/Shaders/Common/Color.hlslifeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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/State.cppsrc/Feature.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/Feature.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.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/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cppsrc/Features/LensEffects.h
📚 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:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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-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:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 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 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/LensEffects.hlslsrc/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:
features/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/Features/LensEffects.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
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.
Applied to files:
src/Features/LensEffects.cpp
📚 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-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-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-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-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
📚 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:
src/Features/LensEffects.h
⏰ 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)
package/Shaders/Common/Color.hlsli (1)
44-82: Chrominance and HSV helpers look solid
RGBToChrominance,RGBtoHSV, andHSVtoRGBfollow standard formulations, include an epsilon to avoid division by zero in hue/saturation, and fit well with the existing Color utilities.src/Globals.h (1)
12-12: LensEffects correctly wired into globals::featuresThe forward declaration and
extern LensEffects lensEffects;follow the same pattern as other features, so LensEffects can participate inglobals::featuresandFeature::GetFeatureList()as expected.Also applies to: 84-85
src/State.cpp (1)
10-10: LensEffects override hook placement looks good
lensEffects.CheckOverride()is gated onlensEffects.loadedand invoked only when the shader cache is enabled, after terrain helpers and beforetruePBR->SetShaderResouces(context). This matches how other features integrate intoState::Draw()and keeps the override localized to the intended path.Also applies to: 28-44
src/Feature.cpp (1)
201-233: LensEffects correctly added to the feature rosterAdding
&globals::features::lensEffectstoFeature::GetFeatureList()ensures the new feature participates in loading, saving, cache validation, and UI like the others. Just confirm thatFeatureVersions::FEATURE_MINIMAL_VERSIONSalso has an entry for"LensEffects"so version checks and error messages stay consistent.features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (3)
188-201: Well-structured helper utilities.The constants and helper functions provide good safety mechanisms (delta for division protection) and consistent naming. The wrapping of existing utility functions maintains code clarity.
822-834: Verify linear color space handling for Ice texture blending.The Ice texture is sampled and directly multiplied with
input.Color. If the DDS texture is gamma-encoded (typical), this blending happens in gamma space, which can produce incorrect results under linear lighting.Based on learnings
Verify whether the Ice texture and color inputs need gamma-to-linear conversions before multiplication:
float3 Texture = IceTexture.Sample(Point_Sampler, input.TexCoord.xy).xyz; + // If texture is gamma-encoded: + Texture = Color::GammaToLinear(Texture); + // If input.Color is gamma-encoded: + float3 color = Color::GammaToLinear(input.Color); float FadeFactor = LinearStep(inv(SnowPrecipValue), 1.0, saturate(Dist-0.35)); FadeFactor *= LinearStep(0.0, 0.1, SnowPrecipValue); - float3 Color = Texture * input.Color * FadeFactor; + float3 Color = Texture * color * FadeFactor; + Color = Color::LinearToGamma(Color); return float4(Color, 0.0);
83-183: Register usage follows established codebase convention—no issues found.The script results confirm that register slots (b1–b2, s10–s13, u0–u1) are used consistently with the established convention in this codebase: hardcoded shader resource numbers are standard practice rather than named constants. These registers are scoped to specific feature shaders and rendering passes that don't execute simultaneously, so there are no conflicts. The Lens Effects shader's register allocation is appropriate.
src/Features/LensEffects.cpp (2)
382-415: UAV binding corrected to use fixed slot 7.The code now correctly binds the occlusion UAV at slot 7 with proper guards for when too many RTVs are bound, addressing previous review concerns. The resource reference cleanup is also properly handled.
634-647: Clarify unusedresultvariable in CheckRenderCondition.The function computes
resultfromfunc()but always returnstrue, with warnings suppressed. This suggests either incomplete implementation or dead code.Please clarify the intent:
- If
resultshould be returned, remove the warning suppression and return it- If
resultis intentionally unused (e.g., callingfunc()for side effects), add a comment explaining why- If this is temporary scaffolding, consider using
(void)result;instead of pragma suppression#pragma warning(push) #pragma warning(disable: 4189) bool LensEffects::Hooks::LensFlare_CheckRenderCondition::thunk(void* shader, RE::NiCamera* camera, uint64_t unk) { auto& lens = globals::features::lensEffects; if (!lens.gFlareShader && shader) lens.gFlareShader = shader; bool result = func(shader, camera, unk); + // TODO: Should this return result instead of always true? return true; } #pragma warning(pop)
Dawntic
force-pushed
the
lens-effects
branch
from
d6c401c to
f119a6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (24)
src/Features/LensEffects.cpp (8)
139-139: Suggest using smart pointer for SettingsCB.Raw
newwithout explicitdeleterisks memory leaks. The header declares this as a raw pointer, so a full fix requires changing the header declaration tostd::unique_ptr<ConstantBuffer>.For now, ensure the destructor or cleanup code releases this pointer. Better: update the header declaration and use:
SettingsCB = std::make_unique<ConstantBuffer>(ConstantBufferDesc<ConstBuffer>());
152-152: Suggest using smart pointer for renderdata.Similar to SettingsCB, this raw allocation is never explicitly deleted. The header should declare
std::unique_ptr<Setup::LF_RenderData> renderdata;and use:renderdata = std::make_unique<Setup::LF_RenderData>();
41-64: Critical: Add error handling for shader compilation failures.The shader compilation calls don't verify that
Util::CompileShaderreturns non-null pointers. If any shader fails to compile, the null pointers will cause crashes when used in rendering passes.Add null checks after each compilation:
void LensEffects::CompileShaders() { - SunOcclusionMaskPixelShader = (ID3D11PixelShader*)Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0"); + auto shader = Util::CompileShader(L"Data\\Shaders\\LensEffects\\LensEffects.hlsl", { { "OCCLUSION_PIXEL_SHADER", "" } }, "ps_5_0"); + if (!shader) { + logger::error("[Lens Effects] Failed to compile SunOcclusionMaskPixelShader"); + return; + } + SunOcclusionMaskPixelShader = static_cast<ID3D11PixelShader*>(shader);Apply similar checks for all 13 shader compilations.
136-138: Major: Graceful texture-load failure handling needed.
DX::ThrowIfFailedwill hard-crash the feature if Atlas.dds or Frost.dds are missing. Per past review, log the error and disable the dependent passes instead.- DX::ThrowIfFailed(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Atlas.dds", nullptr, &AtlasTexSRV)); - DX::ThrowIfFailed(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Frost.dds", nullptr, &IceTexSRV)); + if (FAILED(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Atlas.dds", nullptr, &AtlasTexSRV))) { + logger::error("[Lens Effects] Missing Atlas.dds; disabling Ghosts."); + settings.EnableGhosts = false; + } + if (FAILED(DirectX::CreateDDSTextureFromFile(device, L"Data\\Shaders\\LensEffects\\Textures\\Frost.dds", nullptr, &IceTexSRV))) { + logger::error("[Lens Effects] Missing Frost.dds; disabling Lens Frost."); + settings.EnableIce = false; + }Based on learnings
175-175: Critical: Frame index cycling mismatches shader temporal window.The frame index cycles from 5 to 16 then back to 5, but the HLSL shader's temporal average reads frames 5-14 (10 frames). This creates a mismatch where frames 15-16 are written but never read, and the history never covers all written slots consistently.
Change the cycle to match the shader's 10-frame window:
- frameIdx = (frameIdx < 16) ? ++frameIdx : 5; + frameIdx = (frameIdx < 14) ? ++frameIdx : 5;This aligns the CPU-side indexing with the shader's ring buffer (frames 5-14).
216-257: Critical: Depth SRV not bound for occlusion pass.The pixel shader samples
DepthTextureat register t0, butSetupOcclusionMask()only binds mainSRV (t1) and motionVectorSRV (t2). Sampling from an unbound texture produces undefined results.Per past review, bind the depth SRV explicitly:
+ auto& depthSRV = globals::game::renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGETS_DEPTHSTENCIL::kPOST_ZPREPASS_COPY].depthSRV; + context->PSSetShaderResources(0, 1, &depthSRV); context->PSSetShaderResources(1, 1, &mainSRV); context->PSSetShaderResources(2, 1, &motionVectorSRV);This uses the post-Z-prepass depth copy, consistent with other occlusion calculations in the codebase.
Based on learnings
396-406: Critical: Bind occlusion UAV at fixed slot 7 to match HLSL register.The code binds the UAV at slot
numRTs, but the HLSL shader declaresRWTexture2D<...> SunLUT : register(u7);. This mismatch causes the UAV to bind at the wrong slot.+ constexpr UINT kUAVSlot = 7; + if (numRTs > kUAVSlot) { + logger::warn("[Lens Effects] {} RTVs bound; skipping occlusion UAV bind at u{}.", numRTs, kUAVSlot); + overrideShader = false; + return; + } if (!useCloudLUT) { - context->OMSetRenderTargetsAndUnorderedAccessViews(numRTs, rtvs, dsv, numRTs, 1, &SunOcclusionUAV, nullptr); + context->OMSetRenderTargetsAndUnorderedAccessViews(numRTs, rtvs, dsv, kUAVSlot, 1, &SunOcclusionUAV, nullptr); } else { - context->OMSetRenderTargetsAndUnorderedAccessViews(numRTs, rtvs, dsv, numRTs, 1, &SunOcclusionUAV_AT, nullptr); + context->OMSetRenderTargetsAndUnorderedAccessViews(numRTs, rtvs, dsv, kUAVSlot, 1, &SunOcclusionUAV_AT, nullptr); context->PSSetShaderResources(30, 1, &SunOcclusionSRV); useCloudLUT = false; }Based on learnings
540-546: Major: Sun projection hardcoded to left eye breaks VR.
GetCameraData(0)always uses the left eye's matrices. In VR, this causes incorrect sun positioning for the right eye.Follow the pattern in
src/Features/ScreenSpaceGI.cpplines 613-614:+ for (int eyeIndex = 0; eyeIndex < (1 + REL::Module::IsVR()); ++eyeIndex) { + auto eye = Util::GetCameraData(eyeIndex); - Matrix viewMatrix = Util::GetCameraData(0).viewMat; - Matrix projMatrix = Util::GetCameraData(0).projMat; + Matrix viewMatrix = eye.viewMat; + Matrix projMatrix = eye.projMat; - float4 sunViewPos = float4::Transform(sunWorldPos, viewMatrix); - float SunSSRad = (*skyrim_SunGlareScale * SunScale) * (projMatrix._22 / sunViewPos.z) * (screenSize.y * 0.5f); - sunViewPos.w = sunRadius = (SunSSRad > 10.0f && SunSSRad < 100.0f) ? SunSSRad : sunRadius; + float4 sunViewPos = float4::Transform(sunWorldPos, viewMatrix); + float SunSSRad = (*skyrim_SunGlareScale * SunScale) * (projMatrix._22 / sunViewPos.z) * (screenSize.y * 0.5f); + sunViewPos.w = sunRadius = (SunSSRad > 10.0f && SunSSRad < 100.0f) ? SunSSRad : sunRadius; + // Store or apply per-eye results as needed + }Based on learnings
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (6)
706-711: Suggest radial clamping for chromatic aberration offset.
min(UICAMaxOffset.xx, Offset)clamps each axis independently, allowing diagonal offsets to exceedUICAMaxOffset(e.g.,(0.003, 0.003)has length0.00424 > 0.003).Use radial clamping:
float2 Offset = UICAIntensity * (1.0 + 5 * (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); + float len = length(Offset); + float scale = (len > 1e-6) ? min(1.0, UICAMaxOffset / len) : 1.0; + Offset = Offset * scale * ScreenSize.xy; }
292-292: Critical: Guard division by zero in DistFactor calculation.
SunSSRadiuscan be 0, causing division by zero and producing INF/NaN that propagates through subsequent calculations.Wrap the denominator with a safety check:
- float DistFactor = SunVisibility / (Math::PI * (SunSSRadius * SunSSRadius)); + float DistFactor = SunVisibility / max(Math::PI * (SunSSRadius * SunSSRadius), EPSILON_DIVISION);
373-374: Critical: Guard sqrt against negative input to prevent NaNs.
sqrt(InvDist - inv(UIBladeLength+0.1))can receive a negative argument whenInvDist < inv(UIBladeLength+0.1), producing NaN that propagates throughdelta().Clamp the argument before taking the square root:
- 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-513: Critical: Clamp UIGhostShape to prevent out-of-bounds array access.
input.Verticeshas 10 elements [0..9]. IfUIGhostShape >= 10, the loop accessesinput.Vertices[i+1]at index 10, which is out of bounds.Clamp the loop count and use uint for safety:
- [loop] for(int i = 0; i < UIGhostShape; ++i){ + uint shape = (uint)clamp(round(UIGhostShape), 3.0, 9.0); + [loop] for(uint i = 0; i < shape; ++i){ float2 Edge = ((input.Vertices[i+1] - input.Vertices[i]) * float2(-1.0, 1.0)).yx;
762-784: Major: Add linear color space handling in Sun Glare pass.The shader performs color operations in gamma space. Per project learnings, image-space post-processing shaders must convert to linear space for calculations, then back to gamma for output.
Wrap color operations with conversions:
float Mask = Glow * smoothstep(0.0, UISunGlareFade, InvDist); - float3 Color = UISunGlareColor.xyz * Mask * Depth * input.SunInt.y; + float3 Color = Color::GammaToLinear(UISunGlareColor.xyz); + Color = Color * Mask * Depth * input.SunInt.y; + Color = Color::LinearToGamma(Color); return float4(Color, 1.0);Based on learnings
222-229: Temporal averaging depends on frame index fix in C++.The shader writes to
SunLUT[int2(idx, 0)]and reads from indices 5-14. This assumesFrame(passed asidx) is always in the range 5-14. However, the C++ code at line 175 cyclesframeIdxfrom 5 to 16, which doesn't match.This temporal averaging will work correctly only if the C++ frame index cycling is fixed to match the 10-frame window (5-14). Ensure the C++ fix is applied:
// In src/Features/LensEffects.cpp line 175: frameIdx = (frameIdx < 14) ? ++frameIdx : 5;Otherwise, frames 15-16 will be written but never read by the averaging loop.
src/Features/LensEffects.h (10)
1-3: Suggest adding explicit includes for types used in the header.The header uses
std::array,std::unique_ptr,std::string, DirectX types, and Windows types but only includesFeature.h. This relies on transitive includes.Add explicit includes:
#pragma once #include "Feature.h" +#include <array> +#include <memory> +#include <string> +#include <string_view> +#include <d3d11.h> +#include <DirectXMath.h> #include <vector>This makes dependencies explicit and reduces fragility.
51-90: Consider smart pointers for D3D11 resources to prevent leaks.Raw
ID3D11*pointers risk memory leaks if not explicitly released. The codebase usesComPtrin some places for automatic lifetime management.Consider changing to:
Microsoft::WRL::ComPtr<ID3D11BlendState> BlendState[2]; Microsoft::WRL::ComPtr<ID3D11RasterizerState> Raster; // ... etc for other D3D11 resourcesOr ensure a destructor properly releases all resources. Based on learnings, the codebase uses raw pointers with manual management, so this is a lower priority refactor.
363-364: Harden LinearStep and make VectorToXMFloat const-correct.
LinearStepdivides by(edge1 - edge0)without checking for zero.VectorToXMFloattakes a non-const reference unnecessarily.- inline DirectX::XMFLOAT4A VectorToXMFloat(float4& value) { return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); } - inline float LinearStep(float edge0, float edge1, float x) { return std::clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); } + inline DirectX::XMFLOAT4A VectorToXMFloat(const float4& value) { return DirectX::XMFLOAT4A(value.x, value.y, value.z, value.w); } + 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); + }
428-432: Suggest encapsulating passesdone for better design.
passesdoneis a public mutable member that could be accidentally modified. Consider making it private with accessor methods.- public: int passesdone = 0; + public: + void IncrementPassesDone() { passesdone++; } + void ResetPassesDone() { passesdone = 0; } + int GetPassesDone() const { return passesdone; } Shaders::Enum GetDesc() const { return (Shaders::Enum)shaderdesc; } - int PassesRemaining() const { return (int)numpasses - (int)passesdone; } + int PassesRemaining() const { return (int)numpasses - passesdone; }
97-97: Critical: Initialize skyrim_SunVisble to prevent undefined behavior.The boolean member is read by hook code but never initialized, causing undefined behavior if read before being set.
- bool skyrim_SunVisble; + bool skyrim_SunVisble = false;
120-120: Critical: Initialize SunScale before use in GetSunPosition.
SunScaleis multiplied inGetSunPosition()but starts uninitialized, feeding garbage into radius calculations until the sun pass sets it.- float SunScale; + float SunScale = 0.0f;
374-374: Critical: Guard against division by zero in UpdateSettings.If
mSettings.SB_BladeLengthis zero, this line causes undefined behavior.- mSettings.SBEX_BladeSplayLen = (1 / mSettings.SB_BladeLength - 0.95f) * (mSettings.SB_BladeSplay > 0.01f); + mSettings.SBEX_BladeSplayLen = (mSettings.SB_BladeLength > 1e-6f) + ? ((1.0f / mSettings.SB_BladeLength - 0.95f) * (mSettings.SB_BladeSplay > 0.01f)) + : 0.0f;
516-525: Critical: Fix UpdateCurrentEffect to handle empty lists and stop after last pass.The scheduler unconditionally reads
RenderPassList[0](undefined behavior if empty) and returns the last effect indefinitely after reaching the end.Shaders::Enum UpdateCurrentEffect() { - auto desc = RenderPassList[0]->GetDesc(); - if (currentPass < RenderPassList.size()) { - desc = RenderPassList[currentPass]->GetDesc(); - GetEffect(desc).passesdone++; - currentPass++; - } - return desc; + if (RenderPassList.empty() || currentPass >= RenderPassList.size()) { + return Shaders::Bypass; + } + auto desc = RenderPassList[currentPass]->GetDesc(); + GetEffect(desc).passesdone++; + ++currentPass; + return desc; }
533-533: Major: Use smart pointer for renderdata to prevent memory leak.
renderdatais allocated withnewinSetupResources()but never deleted, causing a memory leak.Update the header:
- Setup::LF_RenderData* renderdata = nullptr; + std::unique_ptr<Setup::LF_RenderData> renderdata;And in the implementation:
renderdata = std::make_unique<Setup::LF_RenderData>();
564-568: Critical: Passing address of stack variable causes use-after-free risk.The thunk passes
¤t(address of local variable) tofunc(). After the thunk returns, this pointer becomes invalid, potentially causing crashes or memory corruption if the callee stores it.Use a static sentinel or pass the original argument:
struct LensFlare_AssignTexture { static void thunk(void* previous, uint64_t current) { + static uint64_t zero = 0; current = 0; - func(previous = ¤t, current); + func(&zero, current); } static inline REL::Relocation<decltype(thunk)> func; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl(1 hunks)src/Features/LensEffects.cpp(1 hunks)src/Features/LensEffects.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.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/Features/LensEffects.cppsrc/Features/LensEffects.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlslsrc/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 (20)
📓 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: 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: 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.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/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-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/Features/LensEffects.cppfeatures/Lens Effects/Shaders/LensEffects/LensEffects.hlsl
📚 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.hlslsrc/Features/LensEffects.h
📚 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-06-24T07:17:36.604Z
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.
Applied to files:
src/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
🔇 Additional comments (2)
src/Features/LensEffects.cpp (1)
553-555: Bounds checking for properties array access cannot be verified — manual review required.The verification scripts could not locate
src/Features/LensEffects.cppor access the codebase to confirm whether:
- The
propertiesvector is guaranteed to have at least 2 elements whensunBaseis valid- A size check already exists elsewhere in the code path
- The guards at lines 553-554 are sufficient
Please manually verify that accessing
properties[1]is safe:
- Confirm whether the
propertiesvector is guaranteed to have size ≥ 2 afterGetGeometryRuntimeData()- If not guaranteed, add a size check before accessing
properties[1]- Review the shader property initialization to understand the contract
features/Lens Effects/Shaders/LensEffects/LensEffects.hlsl (1)
83-182: No register conflicts identified; register pattern follows established codebase convention.The shader declares
cbuffer SunFlareat b2 (line 173) andcbuffer CB2also at b2 (line 210), but they are in separate conditional compilation blocks (#ifdef OCCLUSION_PIXEL_SHADER), so they never compile together. The sampler registers s10–s13 (lines 179–182) are local to this shader and do not conflict with identical register assignments in other shader files (cross-shader register reuse is standard HLSL practice).
|
@doodlum are you good with this? |
There was a problem hiding this comment.
Sun glare just adds an extremely white circle to the sky for me.
CA has nothing to do with motion, it shouldn't be on only in motion. It's also only one component on lens simulation, it should really be mixed with things like lens distortion and grain.
The starburst effect has aliasing, at least when upscaling is on.
The starburst effects and related ones look strange.
The lens flare effect seems to look worse than what you can get in some weather mods anyway, and I don't think it automatically disables the game one? It's unsure why we are replacing the game one which should work just fine.
The lens frost effect is very blue. Frost is not blue, and it contrasts poorly with snow areas depending on the weather mod being used. Those types of UI effects should really receive lighting, it would look pretty cool then.
It would make more sense to remove features that either vanilla already does or are never present in other games. That way the only settings exposed are for things people understand and don't want to just turn off. As someone that loves camera emulation, I don't think I'd want to use any of the effects provided in this package. I really think this type of package of effects should focus on camera emulation, mostly for screen-archers, otherwise it should just be included in jiaye's post processing. i.e. distortion, CA, motion blur, camera profiles, grain.
Alternatively, we have things that are not present in ENB PP. Things like underwater effects, depth of field, sun rays. Those could be included in this package as an offering that can exist between jiaye pp and enb pp that run before post-processing.
It's also unclear why this is separate to jiaye pp, and what this feature intends to do. Because if it was intending to match a camera lens, something like the post processing volume in UE5, it doesn't achieve it. So regardless this PR needs an explanation of what the purpose actually is and if there's plans for follow up PRs.
If you look at something like PRT, it specifically separates camera emulation from color grading. If ENB/jiaye PP can do the color grading part, this part should do the camera emulation part IMO.
|
@Dawntic are you giving up on this PR? If so, go ahead and close. |
|
Closing given how old this is without a response on continuing. |
5 participants
Summary by CodeRabbit