feat: Snow Cover #1369
feat: Snow Cover #1369
Conversation
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Globals.h (1)
54-90: Optional: normalize feature extern order (alphabetical) for consistency.Prior reviews asked for alphabetical ordering in this block; consider reordering to reduce future merge conflicts.
🧹 Nitpick comments (7)
features/Snow Cover/Shaders/SnowCover/RiftenWorld.json (4)
12-12: Normalize path separators for MapTexture.You used "Shaders\SnowCover\Skyrim". Other configs often use forward slashes. For consistency and fewer escape sequences, prefer "Shaders/SnowCover/Skyrim" unless the loader specifically requires backslashes.
- "MapTexture": "Shaders\\SnowCover\\Skyrim", + "MapTexture": "Shaders/SnowCover/Skyrim",
19-21: Month bounds: confirm indices and seasonal intent for Riften.MaxSummerMonth = 6 and MaxWinterMonth = 0. If months are 0–11, this sets winter to only month 0. Confirm this aligns with desired climate; otherwise consider a broader winter window (e.g., 10–2) or document rationale.
21-21: Snowing/Melting speeds both 1.0 — ensure transitions aren’t too abrupt/subtle.If other worlds bias melting > snowing or vice‑versa, consider tuning to match Riften’s climate feel.
Also applies to: 27-27
4-6: Precision nit: trim float literals for readability.Values like 0.019999999552965164 and 0.996880292892456 can be shortened (e.g., 0.02, 0.9969) without material impact and reduce churn in diffs.
Example:
- "MainSpec": 0.019999999552965164, + "MainSpec": 0.02, - "AltSpec": 0.019999999552965164, + "AltSpec": 0.02, - "MainTint": [1.0, 0.996880292892456, 0.996880292892456, 1.0], + "MainTint": [1.0, 0.9969, 0.9969, 1.0], - "AltTint": [1.0, 1.0, 1.0, 0.6078431606292725], + "AltTint": [1.0, 1.0, 1.0, 0.6078],Also applies to: 15-17, 22-22
package/Shaders/DistantTree.hlsl (1)
229-241: Minor HLSL fixes in foliage block.
- Use
uintoutputs forGetDimensionsto match the intrinsic signature.- Clamp computed skylight.
- float rx; - float ry; - TexDiffuse.GetDimensions(rx, ry); - skylight = 1 - TexDiffuse.Sample(SampDiffuse, input.TexCoord.xy - float2(0, 2. / ry)).a; + uint rx, ry; + TexDiffuse.GetDimensions(rx, ry); + skylight = saturate(1.0 - TexDiffuse.Sample(SampDiffuse, input.TexCoord.xy - float2(0.0, 2.0 / (float)ry)).a);Avoid recomputing CameraView mul; cache once and reuse its
.zfor depth and later code paths.- SnowCover::ApplySnowFoliage(baseColor.xyz, normal, input.WorldPosition.xyz + FrameBuffer::CameraPosAdjust[eyeIndex].xyz, skylight, mul(FrameBuffer::CameraView[eyeIndex], float4(input.WorldPosition.xyz, 1)).z); + float3 viewPosCached = mul(FrameBuffer::CameraView[eyeIndex], float4(input.WorldPosition.xyz, 1)).xyz; + SnowCover::ApplySnowFoliage(baseColor.xyz, normal, input.WorldPosition.xyz + FrameBuffer::CameraPosAdjust[eyeIndex].xyz, skylight, viewPosCached.z);src/Features/SnowCover.h (2)
12-25: Fix constant name typos (maintainability).Several constant names are misspelled:
DEFAULT_FOLIGE_EFFECT_OFFSET→DEFAULT_FOLIAGE_EFFECT_OFFSET, andDEFULAT_*→DEFAULT_*. These propagate into member names (mapZscale) and can confuse future maintainers.- static constexpr float DEFAULT_FOLIGE_EFFECT_OFFSET = -2048.0; + static constexpr float DEFAULT_FOLIAGE_EFFECT_OFFSET = -2048.0; @@ - static constexpr float DEFULAT_MAIN_SPEC = 0.02; - static constexpr float DEFULAT_ALT_SPEC = 0.02; - static constexpr float DEFULAT_MAP_ZSCALE = 75000.0f; - static constexpr float DEFULAT_GLINT_1 = 1.2f; - static constexpr float DEFULAT_GLINT_2 = 33.f; - static constexpr float DEFULAT_GLINT_3 = .15f; - static constexpr float DEFULAT_GLINT_4 = 2.f; + static constexpr float DEFAULT_MAIN_SPEC = 0.02f; + static constexpr float DEFAULT_ALT_SPEC = 0.02f; + static constexpr float DEFAULT_MAP_ZSCALE = 75000.0f; + static constexpr float DEFAULT_GLINT_1 = 1.2f; + static constexpr float DEFAULT_GLINT_2 = 33.0f; + static constexpr float DEFAULT_GLINT_3 = 0.15f; + static constexpr float DEFAULT_GLINT_4 = 2.0f;Remember to update uses (e.g.,
FoliageHeightOffset,mapZscale, glint defaults).
56-85: WorldSettings defaults: keep naming consistent with constants.
FoliageHeightOffsetreferencesDEFAULT_FOLIGE_EFFECT_OFFSETandmapZscalereferences a misspelled default. After renaming constants, align these initializers. Also consider PascalCase consistently or snake_case consistently; the file mixes both (mapZscalevsPeakMainAngle).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
features/Snow Cover/Shaders/SnowCover/MarkarthWorld.json(1 hunks)features/Snow Cover/Shaders/SnowCover/RiftenWorld.json(1 hunks)features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli(1 hunks)features/Snow Cover/Shaders/SnowCover/SolitudeWorld.json(1 hunks)features/Snow Cover/Shaders/SnowCover/Tamriel.json(1 hunks)features/Snow Cover/Shaders/SnowCover/WhiterunWorld.json(1 hunks)features/Snow Cover/Shaders/SnowCover/WindhelmWorld.json(1 hunks)package/Shaders/Common/Permutation.hlsli(1 hunks)package/Shaders/Common/SharedData.hlsli(3 hunks)package/Shaders/DistantTree.hlsl(2 hunks)package/Shaders/Lighting.hlsl(8 hunks)package/Shaders/RunGrass.hlsl(2 hunks)src/Feature.cpp(2 hunks)src/FeatureBuffer.cpp(2 hunks)src/Features/SnowCover.cpp(1 hunks)src/Features/SnowCover.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/State.h(2 hunks)src/Utils/UI.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- features/Snow Cover/Shaders/SnowCover/Tamriel.json
🚧 Files skipped from review as they are similar to previous changes (9)
- features/Snow Cover/Shaders/SnowCover/WhiterunWorld.json
- package/Shaders/Common/Permutation.hlsli
- features/Snow Cover/Shaders/SnowCover/SolitudeWorld.json
- src/Utils/UI.cpp
- features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli
- src/Features/SnowCover.cpp
- features/Snow Cover/Shaders/SnowCover/MarkarthWorld.json
- features/Snow Cover/Shaders/SnowCover/WindhelmWorld.json
- package/Shaders/RunGrass.hlsl
🧰 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/State.hpackage/Shaders/Lighting.hlslpackage/Shaders/DistantTree.hlslsrc/Globals.hsrc/FeatureBuffer.cppsrc/Feature.cpppackage/Shaders/Common/SharedData.hlslisrc/Features/SnowCover.hsrc/Globals.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/State.hsrc/Globals.hsrc/FeatureBuffer.cppsrc/Feature.cppsrc/Features/SnowCover.hsrc/Globals.cpp
🧠 Learnings (10)
📓 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: ThePagi
PR: doodlum/skyrim-community-shaders#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.
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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.
📚 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/Lighting.hlslpackage/Shaders/DistantTree.hlsl
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlslpackage/Shaders/DistantTree.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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 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/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Lighting.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:
package/Shaders/Lighting.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:
package/Shaders/Lighting.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.hsrc/Feature.cppsrc/Globals.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/Globals.cpp
🪛 GitHub Actions: Build Community Shaders and addons
src/Globals.h
[error] 64-64: Compilation failed: C2059 syntax error: '<<' in src/Globals.h:64 during MSVC build (cmake --build ALL).
🔇 Additional comments (18)
features/Snow Cover/Shaders/SnowCover/RiftenWorld.json (4)
18-25: Angle thresholds look plausible; confirm intended ordering and semantics.MinAngle < PeakMainAngle < MaxAngle and PeakAltAngle = 1.0. Verify these match shader expectations (e.g., dot(N,Up) vs. angle in radians, inclusive bounds). Misinterpretation could swap main/alt coverage on steep vs. flat surfaces.
13-13: Height offsets: validate with in‑game sampling.FoliageHeightOffset = -3000, WinterHeightOffset = -20000, SummerHeightOffset = 20000. Sanity‑check these against Tamriel/nearby worlds to avoid sudden coverage cliffs at cell borders.
Also applies to: 29-30
1-3: Tint toggles: intentional to affect grass but not trees.AffectGrassTint = 1, AffectTreeTint = 0 — calling out as an intentional artistic choice for Riften’s look. No action needed if by design.
If trees should pick up edge snow tinting similar to other worlds, flip AffectTreeTint to 1.
10-11: Confirm MapMin/MapMax ordering (Y inverted)MapMin Y = 208896.0 > MapMax Y = -176128.0 in features/Snow Cover/Shaders/SnowCover/RiftenWorld.json (lines 10–11); if the loader expects min≤max per axis this will invert/distort UV projection or cull the region.
- "MapMax": [253952.0, -176128.0], - "MapMin": [-233472.0, 208896.0], + "MapMin": [-233472.0, -176128.0], + "MapMax": [253952.0, 208896.0],Runner’s repo-wide check returned no output — re-run the checker (ensure paths with spaces are handled) to find other inverted configs.
src/FeatureBuffer.cpp (2)
13-13: SnowCover wiring include — LGTM.Header inclusion aligns with later use. No issues.
39-53: Feature buffer packing order changed — ensure it exactly matches HLSL FeatureData
- The byte layout produced by _GetFeatureBufferData(...) must exactly match the GPU FeatureData struct (SharedData.hlsli); reordering or switching .settings/GetCommonBufferData() will corrupt downstream fields.
- I could not find package/Shaders/Common/SharedData.hlsli or src/FeatureBuffer.cpp in this checkout — provide those file paths or paste the FeatureData struct and the FeatureBuffer packing snippet so I can re-verify.
- Replace any type‑punning writes into (unsigned char*) buffers with std::memcpy(data + offset, &feat_datas, sizeof(feat_datas)); offset += sizeof(feat_datas); to avoid undefined behavior.
src/Feature.cpp (2)
22-22: SnowCover include — LGTM.Header inclusion aligns with globals and buffer usage.
203-230: Fix pointer bitwise-AND in feature list (src/Feature.cpp:203-230)&globals::features::upscaling & globals::features::volumetricLighting performs a bitwise AND on pointers — split into two entries.
- &globals::features::upscaling & globals::features::volumetricLighting, + &globals::features::upscaling, + &globals::features::volumetricLighting,Search found only HLSL usages of SnowCover and no FeatureVersions header or SnowCover C++ symbols in the repo; confirm SnowCover is present in FeatureVersions::FEATURE_MINIMAL_VERSIONS and that SnowCover::SupportsVR() exists.
src/Globals.cpp (2)
21-21: SnowCover include — LGTM.Matches new global declaration below.
56-79: Globals feature instances — verify types and externs.SnowCover snowCover{} addition and reorder look fine; I couldn't locate src/Globals.h or the feature headers in the checked-out workspace — confirm that src/Globals.h (or the repo's Globals header) declares:
extern SnowCover snowCover;
and that InteriorSunShadows is declared in the header you include (e.g. src/Features/InteriorSun.h).src/State.h (2)
183-186: Including VertexShaderDescriptor in equality is correct — audit hashes/memcmpGood addition. The provided repo search produced no hits for custom hashes or memcmp-based comparisons of PermutationCB; manually verify there are no std::hash specializations, unordered_map keys, or memcmp comparisons that must be updated to include VertexShaderDescriptor. Location: src/State.h (lines 183–186)
153-155: NoSnow bit added — HLSL uses it; confirm C++ definition & reserve next bit for UI.
- package/Shaders/Lighting.hlsl:2250 checks Permutation::ExtraShaderDescriptor & Permutation::ExtraFlags::NoSnow — ensure ExtraShaderDescriptors::NoSnow = 1 << 6 in C++ matches that bit and any serialization/UI that inspects this mask.
- I could not locate src/State.h or the C++ enum in this checkout; verify the C++ definition exists and that 1 << 7 is unused in both C++ and HLSL before adding InventoryUI (InventoryUI = 1 << 7).
package/Shaders/DistantTree.hlsl (2)
181-189: SNOW_COVER gating is clean and avoids macro collisions. Undefining SNOW/PROJECTED_UV/SPARKLE and aliasing SampColorSampler prevent conflicts; BASIC_SNOW_COVER is intentionally defined in package/Shaders/DistantTree.hlsl:185 and package/Shaders/RunGrass.hlsl:455.
225-228: Normal from ddx/ddy — single normal path here; verify winding.Normal is computed once as
-normalize(cross(ddx_coarse(input.WorldPosition.xyz), ddy_coarse(input.WorldPosition.xyz)))(lines 225–227) and encoded viaFrameBuffer::WorldToView(...)->GBuffer::EncodeNormal(line 274). There is no alternate forward/deferred normal in this shader, so the “both paths” part of the original comment is incorrect — still confirm the leading-produces the intended winding/lighting (compare to previous commit or other shaders).Likely an incorrect or invalid review comment.
package/Shaders/Lighting.hlsl (1)
1019-1024: Vertex-normal flip exclusion looks correct.Excluding LOD alpha-tested soft-lighting permutations from the double-sided normal fix prevents artifacts on ultra trees.
If you want to replace the ultra-tree compound condition elsewhere, confirm
ExtraFlags::IsTreealso applies to object LOD trees before changing it.package/Shaders/Common/SharedData.hlsli (2)
140-175: SnowCoverSettings layout looks CB-friendly; flags correctly use uint.Good use of uint for booleans and padding to maintain 16-byte alignment across C++/HLSL. No register pressure added (still in b6).
Please confirm FeatureBuffer packing on the C++ side matches this exact field order (especially the two pad blocks and trailing uint3), to avoid subtle misbinds.
234-251: Reordered FeatureData entries may destabilize other features.Reordering entries (e.g., moving
cloudShadowsSettingsand insertingsnowCoverSettingsearlier) changes offsets for every consumer. If the C++ side updated the same order for all features, you're fine; otherwise, consider appending new blocks to the end to minimize blast radius.I can provide a quick map of sizeof/offsets from the C++ structs to cross-check against this HLSL layout if helpful.
src/Features/SnowCover.h (1)
133-158: Seasonal altitude math: OK; follows author’s intent.Using game calendar with fractional month is fine. Header uses
/ 60.0; the .cpp’s/ 61.0note was intentional per author—just ensure both places are consistent with the chosen behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Common/SharedData.hlsli (1)
39-49: Ensure C++ side binding for ExtendedMaterialSettings
Search found no references to eitherCPMSettingsorExtendedMaterialSettingsin the C++ codebase. You must add or update the CPU-side code that populates the shader constant buffer to use the newExtendedMaterialSettingsstruct name and verify its memory layout matches the HLSL definition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package/Shaders/Common/SharedData.hlsli(3 hunks)src/Feature.cpp(2 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Globals.hsrc/Globals.cppsrc/Feature.cpppackage/Shaders/Common/SharedData.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Globals.hsrc/Globals.cppsrc/Feature.cpp
🧠 Learnings (1)
📚 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
⏰ 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 (7)
src/Feature.cpp (1)
22-22: LGTM: SnowCover header included.The include follows the established pattern for feature headers and is necessary for the SnowCover integration.
src/Globals.cpp (2)
21-21: LGTM: SnowCover header included.The include is necessary for the SnowCover feature registration and follows the established pattern.
56-78: LGTM: Feature instances properly registered.The new feature instances (ExtendedTranslucency, PerformanceOverlay, SnowCover, Upscaling) and reordered declarations follow the established registration pattern in the globals::features namespace. Default initialization is appropriate.
Based on learnings.
src/Globals.h (2)
6-28: LGTM: Forward declarations properly added.All forward declarations for the new and reordered features (ExtendedTranslucency, LODBlending, LightLimitFix, PerformanceOverlay, Skylighting, SnowCover, TerrainVariation, Upscaling, VolumetricLighting) are necessary and follow the established pattern.
59-81: LGTM: Extern declarations consistent with definitions.All extern declarations for the new and reordered features match their definitions in Globals.cpp. No duplicates or merge conflict artifacts detected.
package/Shaders/Common/SharedData.hlsli (2)
234-250: Critical: Confirm CPU-side FeatureData struct and buffer update logic matches HLSL cbuffer layout.
Ensure the C++ definition (or generated code) and buffer update calls reflect the new field order and added settings from SharedData.hlsli.
140-175: Verify C++ SnowCoverSettings definition and 16-byte alignment
Ensure the HLSL struct at package/Shaders/Common/SharedData.hlsli (lines 140–175) is mirrored exactly in C++—same field order, types and padding—and add a static_assert(sizeof(SnowCoverSettings) % 16 == 0) to validate GPU constant buffer layout.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
package/Shaders/RunGrass.hlsl (1)
454-454: Use Math::PI; remove local PI macroPI is already provided in Common/Math.hlsli with higher precision. Drop the local macro and reference Math::PI.
-# define PI 3.1415927package/Shaders/Lighting.hlsl (1)
2229-2235: GetDimensions type mismatch — use uint and castMatch DXC/FXC semantics.
- float rx; - float ry; - TexColorSampler.GetDimensions(rx, ry); + uint rx, ry; + TexColorSampler.GetDimensions(rx, ry); @@ - snowOcclusion = 1 - TexColorSampler.Sample(SampColorSampler, uv - float2(0, 2. / ry)).a; + snowOcclusion = 1.0 - TexColorSampler.Sample(SampColorSampler, uv - float2(0.0, 2.0 / float(ry))).a;
🧹 Nitpick comments (6)
src/Globals.h (1)
64-66: Consider alphabetical ordering for consistency.The extern declarations
lodBlendingandlightLimitFixare not in alphabetical order. For consistency with the rest of the file and based on past review feedback, consider reordering them alphabetically (lightLimitFix before lodBlending).Apply this diff to reorder alphabetically:
- extern LODBlending lodBlending; extern LightLimitFix lightLimitFix; + extern LODBlending lodBlending; - extern LightLimitFix lightLimitFix; extern PerformanceOverlay performanceOverlay;package/Shaders/Common/SharedData.hlsli (1)
140-176: Consider consistent naming convention.The
SnowCoverSettingsstruct mixes PascalCase (e.g.,EnableSnowCover,AffectGrassTint) and camelCase (e.g.,peakMainAngle,mapZscale) for member names. For better maintainability, consider using a consistent naming convention throughout the struct.src/Utils/UI.cpp (4)
278-286: Restore previous ImGui window font scale (avoid clobbering nested scales)Currently resets to 1.0f, which can break callers that changed scale earlier. Save/restore the previous window scale instead.
- // Apply scale if needed - if (scale != 1.0f) { - ImGui::SetWindowFontScale(scale); - } + // Apply scale if needed, preserving previous scale + float _prevScale = 1.0f; + if (scale != 1.0f) { + ImGuiWindow* _w = ImGui::GetCurrentWindow(); + _prevScale = _w->FontWindowScale; + ImGui::SetWindowFontScale(scale); + } @@ - // Restore original scale if needed - if (scale != 1.0f) - ImGui::SetWindowFontScale(1.0f); + // Restore previous window scale if changed + if (scale != 1.0f) { + ImGui::SetWindowFontScale(_prevScale); + }
315-323: Same issue here: don’t hard-reset font scale to 1.0fPreserve and restore the previous window font scale.
- ImGui::PushStyleVar(ImGuiStyleVar_ItemSpacing, ImVec2(0, 0)); - ImGui::SetWindowFontScale(textScale); + ImGui::PushStyleVar(ImGuiStyleVar_ItemSpacing, ImVec2(0, 0)); + float _prevScale = 1.0f; + { + ImGuiWindow* _w = ImGui::GetCurrentWindow(); + _prevScale = _w->FontWindowScale; + } + ImGui::SetWindowFontScale(textScale); @@ - ImGui::SetWindowFontScale(1.0f); + ImGui::SetWindowFontScale(_prevScale);
79-86: Guard against null/empty filename before logging/IOAvoid logging a null C-string and calling stbi_load with null/empty paths.
// Validate input parameters - if (!device || !out_srv) { + if (!device || !out_srv) { logger::warn("LoadTextureFromFile: Invalid parameters - device: {}, out_srv: {}", device ? "valid" : "null", out_srv ? "valid" : "null"); return false; } + if (!filename || filename[0] == '\0') { + logger::warn("LoadTextureFromFile: Invalid filename (null/empty)"); + *out_srv = nullptr; + return false; + }
715-718: Avoid UB with tolower on signed char; use unsigned char wrapper::tolower on ‘char’ is UB for negative values. Use std::tolower with unsigned char.
- std::transform(shortName.begin(), shortName.end(), shortName.begin(), ::tolower); - std::transform(displayName.begin(), displayName.end(), displayName.begin(), ::tolower); - std::transform(query.begin(), query.end(), query.begin(), ::tolower); + auto _toLower = [](unsigned char c) { return static_cast<char>(std::tolower(c)); }; + std::transform(shortName.begin(), shortName.end(), shortName.begin(), _toLower); + std::transform(displayName.begin(), displayName.end(), displayName.begin(), _toLower); + std::transform(query.begin(), query.end(), query.begin(), _toLower);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package/Shaders/Common/SharedData.hlsli(3 hunks)package/Shaders/DistantTree.hlsl(2 hunks)package/Shaders/Lighting.hlsl(8 hunks)package/Shaders/RunGrass.hlsl(2 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/Utils/UI.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/DistantTree.hlslsrc/Utils/UI.cppsrc/Globals.hpackage/Shaders/RunGrass.hlslpackage/Shaders/Lighting.hlslsrc/Globals.cpppackage/Shaders/Common/SharedData.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Utils/UI.cppsrc/Globals.hsrc/Globals.cpp
🧠 Learnings (2)
📓 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-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlsl
🔇 Additional comments (13)
package/Shaders/DistantTree.hlsl (2)
185-192: Good SNOW_COVER gating/integrationHeader hygiene looks correct: SNOW/PROJECTED_UV/SPARKLE are sanitized; include path is consistent. No issues.
229-232: Geometric normal orientation — please verifyUsing -normalize(cross(ddx, ddy)) centralizes normal calc, but the sign convention must match other foliage/distant tree passes. Please sanity-check against sun lighting and shadows for flipped highlights.
package/Shaders/RunGrass.hlsl (1)
455-462: SnowCover include/gating looks correctMacro hygiene matches pattern used elsewhere; no issues.
src/Globals.cpp (1)
21-21: Feature wiring OK; confirm UI/initialization orderingSnowCover/ExtendedTranslucency additions and reordering look fine. Please verify menu ordering/init side-effects are intended (some code reads features sequentially).
Also applies to: 55-77
package/Shaders/Lighting.hlsl (4)
30-38: Pragmatic SNOW_COVER gating for charactersUndefining SNOW_COVER for SKINNED/SKIN/EYE/HAIR/!EXTENDED_MATERIALS is sensible to avoid unintended overlays.
1866-1877: LOD land noise skip when snow active — good guardSkipping LOD noise darkening when SnowCover is enabled avoids double darkening. Looks good.
3046-3049: Nice touch: vertexColor blended by snowFactorThis preserves tint while adding snow — safe and visually coherent.
2228-2237: Incorrect assumption about RIM_LIGHTING in ultra-tree detection
No occurrence ofdefined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)exists in Lighting.hlsl—historically the guard has never includedRIM_LIGHTING. This change retains existing behavior; only addRIM_LIGHTINGif you need to narrow the match.Likely an incorrect or invalid review comment.
src/Globals.h (1)
6-28: LGTM! Forward declarations are well-organized.The new forward declarations for ExtendedTranslucency, PerformanceOverlay, SnowCover, TerrainVariation, Upscaling, and VolumetricLighting follow a logical alphabetical ordering, making the codebase easier to navigate.
package/Shaders/Common/SharedData.hlsli (2)
39-39: LGTM! Descriptive struct rename.Renaming
CPMSettingstoExtendedMaterialSettingsimproves code clarity and is consistently applied in the cbuffer declaration at line 238.
246-246: Verify cbuffer placement does not break existing features.Based on past review discussions, placing new feature settings in the middle of the cbuffer (rather than at the end) can potentially break other features if there are padding or alignment issues. While the struct appears properly aligned, please confirm that this placement of
snowCoverSettingshas been tested with all existing features to avoid unexpected issues.Based on learnings from past reviews about cbuffer ordering concerns.
src/Utils/UI.cpp (2)
1196-1196: LGTM: newline at EOFThanks for fixing the trailing newline/brace formatting. No behavioral changes.
17-27: Unable to locate UI.cpp; please verify includes
I couldn’t findsrc/Utils/UI.cpp—confirm the correct file path and ensure you add#include <array>forstd::arrayusage and#include <cctype>if you refactor to usestd::tolower.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/Features/SnowCover.cpp (1)
217-217: Remove commented-out dead code.Delete this commented function rather than leaving it in the source.
-//void SnowCover::Draw(const RE::BSShader*, const uint32_t){}As per coding guidelines
🧹 Nitpick comments (5)
src/Features/SnowCover.cpp (5)
113-113: Replace fixed buffer with imgui_stdlib::InputText.Using a fixed 128-byte buffer with
ImGui::InputTextis error-prone. Useimgui_stdlib::InputTextwithstd::stringinstead.-ImGui::InputText("Map texture", mapbuf, 128); +ImGui::InputText("Map texture", &map_tex);Then remove the manual buffer copy at line 134.
As per coding guidelines
154-154: Replace fixed buffer with imgui_stdlib::InputText.Using a fixed 128-byte buffer with
ImGui::InputTextis error-prone. Useimgui_stdlib::InputTextwithstd::stringinstead.-ImGui::InputText("Main texture", tbuf, 128); +ImGui::InputText("Main texture", &main_tex);Then remove the manual string assignment at line 158.
As per coding guidelines
159-159: Replace fixed buffer with imgui_stdlib::InputText.Using a fixed 128-byte buffer with
ImGui::InputTextis error-prone. Useimgui_stdlib::InputTextwithstd::stringinstead.-ImGui::InputText("Alt texture", altbuf, 128); +ImGui::InputText("Alt texture", &alt_tex);Then remove the manual string assignment at line 163.
As per coding guidelines
422-427: Consider using ComPtr for safer COM resource management.Raw pointer management with manual
Release()calls is error-prone. Consider usingMicrosoft::WRL::ComPtror similar RAII wrapper for the texture views.Example:
std::array<ComPtr<ID3D11ShaderResourceView>, 7> views; // No manual Release() needed - automatic cleanupThis prevents resource leaks and double-free issues.
As per coding guidelines
15-19: Centralize safe string-copy helperNo existing safe string-copy function was found;
copyStringis acceptable here, but consider creating astring_utilsmodule to house it for future reuse.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SnowCover.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SnowCover.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SnowCover.cpp
🧠 Learnings (4)
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.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/SnowCover.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/SnowCover.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
|
Please review rabbit suggestions and resolve once reviewed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/Features/SnowCover.cpp (8)
113-113: Consider using ImGui::InputText from imgui_stdlib.Instead of managing char buffers manually, consider using
ImGui::InputTextwithstd::stringfromimgui_stdlib.hfor cleaner code and automatic memory management.Based on learnings
204-208: Simplify formatted text output.
ImGui::Textsupports formatting directly viaImGui::Text("Month: %.2f", perFrame.Month). Usingfmt::formatand.c_str()adds unnecessary overhead.Based on learnings
345-347: Add explicit file operation success checks.While the try-catch block handles
std::system_error, explicitly checkingfile.is_open()before writing andfile.good()after writing provides better error detection and clearer diagnostics.As per coding guidelines
auto path = (Util::PathHelpers::GetFeatureShaderPath("SnowCover") / curr_worldspace).replace_extension(std::filesystem::path(".json")); std::ofstream file(path); + if (!file.is_open()) { + logger::error("[Snow Cover] Failed to open file for writing: {}", path.generic_string()); + return; + } file << config.dump(4); + if (!file.good()) { + logger::error("[Snow Cover] Failed to write config to file: {}", path.generic_string()); + }
27-27: Define magic numbers as named constants.The altitude range values (-20000.0f, 20000.0f) and other magic numbers (buffer sizes 128/256, clamp range -2.0f/2.0f) should be defined as named constants in the header file for maintainability.
Based on learnings
217-217: Remove commented-out dead code.The commented-out
Drawfunction should be removed rather than left in the codebase.Based on learnings
228-245: Potential stale snowingDensity when weather data unavailable.When
EnableSnowCoveris true butprecipitationDatais null (lines 231-235),snowingDensityis never set and may retain a stale value from previous frames. InitializesnowingDensity = 0.0fat the start of theEnableSnowCoverblock to ensure clean state.As per coding guidelines
bool snowing = false; bool raining = false; if (wsettings.EnableSnowCover) { + snowingDensity = 0.0f; if (auto sky = RE::Sky::GetSingleton()) {
469-472: Critical: Pointer aliasing causes double-free on subsequent Reload().When alt textures are not provided,
views[3-5]are aliased toviews[0-2]without incrementing reference counts. On the nextReload()call, lines 423-427 and 445-450 will Release the same texture views twice, causing double-free.As per coding guidelines
Solution: Increment reference count when aliasing:
} else { views[3] = views[0]; + views[3]->AddRef(); views[4] = views[1]; + views[4]->AddRef(); views[5] = views[2]; + views[5]->AddRef(); }Alternative solution: Track aliases and skip Release:
Maintain a
std::set<size_t>of aliased indices and check before calling Release.
546-546: Critical: Potential null pointer dereference in animation check.Line 546 calls
userData->GetObjectReference()->IsBoundAnimObject()without verifying thatGetObjectReference()returns a non-null pointer. This can cause a null pointer dereference.As per coding guidelines
- if ((a_pass->geometry->HasAnimation() || (userData && (userData->GetObjectReference()->IsBoundAnimObject() || userData->CanBeMoved()))) && !whitelist.contains(FormIdParser::fnv_hash(name))) { + auto objRef = userData ? userData->GetObjectReference() : nullptr; + if ((a_pass->geometry->HasAnimation() || (userData && ((objRef && objRef->IsBoundAnimObject()) || userData->CanBeMoved()))) && !whitelist.contains(FormIdParser::fnv_hash(name))) {
🧹 Nitpick comments (1)
src/Features/SnowCover.cpp (1)
15-19: copyString is unique—consider moving to a shared utilities module. No existing string-copy helper was found in Util.h or other utility files; relocate this helper to a common utilities header for discoverability and future reuse.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SnowCover.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SnowCover.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SnowCover.cpp
🧠 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-05T18:13:03.123Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.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/SnowCover.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/SnowCover.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 (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (4)
src/Features/SnowCover.cpp (4)
10-13: LGTM!The JSON serialization macro correctly includes both
EnableExpensiveFoliageandSnowHeightOffsetfields for complete UserSettings serialization.
276-281: LGTM!The initialization logic correctly calls
Reload()and sets up the initiallastHourvalue.
283-296: LGTM!The worldspace retrieval logic correctly handles both interior cells and worldspaces, returning the appropriate editor ID.
514-531: LGTM!The serialization and reset functions are correctly implemented. The empty
Reset()appears intentional as resource cleanup happens elsewhere.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/Features/SnowCover.cpp (7)
32-32: Define altitude range constants in the header.The hardcoded altitude range values
-20000.0fand20000.0fappear multiple times. Define these as named constants in SnowCover.h for maintainability.Based on learnings
Add to SnowCover.h:
static constexpr float ALTITUDE_RANGE_MIN = -20000.0f; static constexpr float ALTITUDE_RANGE_MAX = 20000.0f;Then update these lines:
- ImGui::SliderFloat("Snow Offset", &settings.SnowHeightOffset, -20000.0f, 20000.0f); + ImGui::SliderFloat("Snow Offset", &settings.SnowHeightOffset, ALTITUDE_RANGE_MIN, ALTITUDE_RANGE_MAX);Also applies to: 98-98, 102-102
222-222: Remove commented dead code.Delete the commented-out function declaration for better code hygiene.
-//void SnowCover::Draw(const RE::BSShader*, const uint32_t){}
515-515: Define texture slot as a named constant.The shader resource slot number
38should be defined as a named constant (e.g.,SNOW_TEXTURE_SLOT) in the header file for clarity and maintainability.Based on learnings
Add to SnowCover.h:
static constexpr uint32_t SNOW_TEXTURE_SLOT = 38;Then update:
- context->PSSetShaderResources(38, (uint)views.size(), views.data()); + context->PSSetShaderResources(SNOW_TEXTURE_SLOT, (uint)views.size(), views.data());
233-250: Initialize snowingDensity to prevent stale data.When
EnableSnowCoveris true but weather/precipitation data is unavailable (lines 234-246),snowingDensityis never set and may retain a stale value from previous frames.As per coding guidelines
Apply this diff:
SnowCover::PerFrame SnowCover::GetCommonBufferData() { Reload(); + snowingDensity = 0.0f; static float delta = 0;
348-356: Add file open and write success checks.The code doesn't verify that the file opened successfully before writing or check if the write operation succeeded.
As per coding guidelines
Apply this diff:
try { auto curr_worldspace = GetWorldspace(); auto path = (Util::PathHelpers::GetFeatureShaderPath("SnowCover") / curr_worldspace).replace_extension(std::filesystem::path(".json")); std::ofstream file(path); + if (!file.is_open()) { + logger::error("[Snow Cover] Failed to open file for writing: {}", path.generic_string()); + return; + } file << config.dump(4); + if (!file.good()) { + logger::error("[Snow Cover] Failed to write config to file: {}", path.generic_string()); + } } catch (const std::system_error& e) {
458-478: Pointer aliasing risk with texture view assignment.Lines 461, 466, 471, 475-477: Assigning
views[3] = views[0](and similar) creates pointer aliasing. If these views are Released independently later (e.g., lines 427-432, 450-455, 480-481), this causes a double-free.As per coding guidelines
Consider one of these approaches:
- AddRef when aliasing:
views[3] = views[0]; views[3]->AddRef();- Track aliases: Use a boolean map to skip Release on aliased views
- Duplicate textures: Create distinct view objects for defaults
Apply approach 1 (simplest):
if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}.dds texture: {}", alt_tex.generic_string(), hr); views[3] = views[0]; + if (views[3]) views[3]->AddRef(); } hr = DirectX::CreateDDSTextureFromFile(device, context, (data_path / alt_tex).concat("_n.dds").native().c_str(), nullptr, &views.at(4)); if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}_n.dds texture: {}", alt_tex.generic_string(), hr); views[4] = views[1]; + if (views[4]) views[4]->AddRef(); } hr = DirectX::CreateDDSTextureFromFile(device, context, (data_path / alt_tex).concat("_rmaos.dds").native().c_str(), nullptr, &views.at(5)); if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}_rmaos.dds texture: {}", alt_tex.generic_string(), hr); views[5] = views[2]; + if (views[5]) views[5]->AddRef(); } } else { views[3] = views[0]; + if (views[3]) views[3]->AddRef(); views[4] = views[1]; + if (views[4]) views[4]->AddRef(); views[5] = views[2]; + if (views[5]) views[5]->AddRef(); }
546-568: Potential null pointer dereference in animation check.Line 551:
GetObjectReference()may return null, but the code callsIsBoundAnimObject()without checking.As per coding guidelines
Apply this diff:
- if ((a_pass->geometry->HasAnimation() || (userData && (userData->GetObjectReference()->IsBoundAnimObject() || userData->CanBeMoved()))) && !whitelist.contains(FormIdParser::fnv_hash(name))) { + auto objRef = userData ? userData->GetObjectReference() : nullptr; + if ((a_pass->geometry->HasAnimation() || (userData && ((objRef && objRef->IsBoundAnimObject()) || userData->CanBeMoved()))) && !whitelist.contains(FormIdParser::fnv_hash(name))) {
🧹 Nitpick comments (1)
src/Features/SnowCover.h (1)
12-12: Fix typos in constant names.Line 12:
DEFAULT_FOLIGE_EFFECT_OFFSETshould beDEFAULT_FOLIAGE_EFFECT_OFFSET
Lines 18-19:DEFULAT_MAIN_SPECandDEFULAT_ALT_SPECshould beDEFAULT_MAIN_SPECandDEFAULT_ALT_SPECApply this diff:
- static constexpr float DEFAULT_FOLIGE_EFFECT_OFFSET = -2048.0; // color foliage earlier/later than snow appears + static constexpr float DEFAULT_FOLIAGE_EFFECT_OFFSET = -2048.0; // color foliage earlier/later than snow appears - static constexpr float DEFULAT_MAIN_SPEC = 0.02; // specular for main material - static constexpr float DEFULAT_ALT_SPEC = 0.02; // specular for alt material + static constexpr float DEFAULT_MAIN_SPEC = 0.02; // specular for main material + static constexpr float DEFAULT_ALT_SPEC = 0.02; // specular for alt materialAlso update references in lines 60, 68-69:
- float FoliageHeightOffset = DEFAULT_FOLIGE_EFFECT_OFFSET; + float FoliageHeightOffset = DEFAULT_FOLIAGE_EFFECT_OFFSET; - float MainSpec = DEFULAT_MAIN_SPEC; - float AltSpec = DEFULAT_ALT_SPEC; + float MainSpec = DEFAULT_MAIN_SPEC; + float AltSpec = DEFAULT_ALT_SPEC;Also applies to: 18-19
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Snow Cover/Shaders/SnowCover/whitelist.txt(1 hunks)src/Features/SnowCover.cpp(1 hunks)src/Features/SnowCover.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- features/Snow Cover/Shaders/SnowCover/whitelist.txt
🧰 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/SnowCover.hsrc/Features/SnowCover.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SnowCover.hsrc/Features/SnowCover.cpp
🧠 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-05T18:13:03.123Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.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/SnowCover.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/SnowCover.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). (4)
- 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)
- GitHub Check: Feature Version Audit (PR)
🔇 Additional comments (7)
src/Features/SnowCover.h (3)
134-159: LGTM! Robust seasonal altitude calculation.The method correctly handles null calendar with a fallback value and implements proper seasonal interpolation between summer and winter heights.
176-193: LGTM! Proper hook installation pattern.The vtable patching follows the project's standard pattern using REL::Relocation for SE/AE/VR runtime compatibility.
101-175: LGTM! Well-structured feature class interface.The public API is comprehensive and properly exposes all necessary controls for the SnowCover feature, including settings, resource management, and lifecycle hooks.
src/Features/SnowCover.cpp (4)
1-20: LGTM! Safe string copy implementation.The
copyStringhelper correctly usesstrncpywith proper null-termination to prevent buffer overflows.
281-301: LGTM! Proper worldspace/cell detection.The
GetWorldspaceutility correctly handles null checks for TES singleton and provides appropriate fallbacks for both interior cells and exterior worldspaces.
359-390: LGTM! Robust file loading with proper error handling.The file opening logic correctly checks for existence and open success, with appropriate error logging and early returns.
523-536: LGTM! Clean serialization implementation.The Load/Save methods correctly delegate to the JSON macro, and RestoreDefaultSettings properly resets state.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
package/Shaders/RunGrass.hlsl (1)
560-570: Fix GetDimensions type mismatch.
GetDimensionswritesuintbutrxandryare declared asfloat, which can break compilation with DXC/FXC.Apply this diff to fix the type mismatch:
- float rx; - float ry; - TexBaseSampler.GetDimensions(rx, ry); + uint rx, ry; + TexBaseSampler.GetDimensions(rx, ry); @@ - snowOcclusion = max(snowOcclusion, 1 - TexBaseSampler.SampleBias(SampBaseSampler, float2(input.TexCoord.x, input.TexCoord.y * 0.5 - 0.5 / ry), SharedData::MipBias).a); + snowOcclusion = max(snowOcclusion, 1.0 - TexBaseSampler.SampleBias(SampBaseSampler, float2(input.TexCoord.x, input.TexCoord.y * 0.5 - 0.5 / float(ry)), SharedData::MipBias).a); @@ - snowOcclusion = max(snowOcclusion, 1 - TexBaseSampler.SampleBias(SampBaseSampler, input.TexCoord.xy - float2(0, 1. / ry), SharedData::MipBias).a); + snowOcclusion = max(snowOcclusion, 1.0 - TexBaseSampler.SampleBias(SampBaseSampler, input.TexCoord.xy - float2(0.0, 1.0 / float(ry)), SharedData::MipBias).a);package/Shaders/Lighting.hlsl (2)
2221-2226: Avoid implicit bool→float cast.At line 2225,
inWorld(bool) is assigned directly tosnowOcclusion(float), causing an implicit cast.Apply this diff to make the cast explicit:
-# else - float snowOcclusion = inWorld; -# endif +# else + float snowOcclusion = inWorld ? 1.0 : 0.0; +# endif
2228-2236: Fix GetDimensions type mismatch.
GetDimensionswritesuintbutrxandryare declared asfloat, which can break compilation with DXC/FXC.Apply this diff to fix the type mismatch:
- float rx; - float ry; - TexColorSampler.GetDimensions(rx, ry); + uint rx, ry; + TexColorSampler.GetDimensions(rx, ry); @@ - snowOcclusion = 1 - TexColorSampler.Sample(SampColorSampler, uv - float2(0, 2. / ry)).a; + snowOcclusion = 1.0 - TexColorSampler.Sample(SampColorSampler, uv - float2(0.0, 2.0 / float(ry))).a;src/Features/SnowCover.cpp (4)
118-118: Use ImGui::InputText from imgui_stdlib for type safety.The current approach uses raw char buffers with
ImGui::InputText. Consider usingimgui_stdlib.h'sInputTextoverload that acceptsstd::string*directly, eliminating buffer management and size constraints.Based on learnings
209-213: Remove redundant fmt::format wrapper.
ImGui::Text()already supports format strings directly viaImGui::Text("%s", fmt::format(...).c_str())pattern, or you can useImGui::Text()with format specifiers. The current pattern adds unnecessary string construction.- ImGui::Text(fmt::format("Month: {}", perFrame.Month).c_str()); - ImGui::Text(fmt::format("TimeSnowing: {}", perFrame.TimeSnowing).c_str()); - ImGui::Text(fmt::format("SnowingDensity: {}", perFrame.SnowingDensity).c_str()); + ImGui::Text("Month: %.2f", perFrame.Month); + ImGui::Text("TimeSnowing: %.2f", perFrame.TimeSnowing); + ImGui::Text("SnowingDensity: %.2f", perFrame.SnowingDensity);Based on learnings
222-222: Remove commented-out dead code.This commented function serves no purpose and should be deleted to keep the codebase clean.
Based on learnings
460-473: Missing AddRef() on fallback texture aliasing causes double-free risk.When texture loading fails at lines 462, 467, and 472, the code assigns
views[3/4/5] = views[0/1/2]without callingAddRef(). Later, whenviewsare released (lines 451-456, 484-485, and presumably in cleanup), this creates a double-free condition.Lines 476-481 correctly call
AddRef()before aliasing. Apply the same pattern to the error fallback cases:if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}.dds texture: {}", alt_tex.generic_string(), hr); + views[0]->AddRef(); views[3] = views[0]; } hr = DirectX::CreateDDSTextureFromFile(device, context, (data_path / alt_tex).concat("_n.dds").native().c_str(), nullptr, &views.at(4)); if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}_n.dds texture: {}", alt_tex.generic_string(), hr); + views[1]->AddRef(); views[4] = views[1]; } hr = DirectX::CreateDDSTextureFromFile(device, context, (data_path / alt_tex).concat("_rmaos.dds").native().c_str(), nullptr, &views.at(5)); if (hr != S_OK) { logger::warn("Snow Cover: Error loading {}_rmaos.dds texture: {}", alt_tex.generic_string(), hr); + views[2]->AddRef(); views[5] = views[2]; }As per coding guidelines
🧹 Nitpick comments (2)
package/Shaders/Lighting.hlsl (1)
2272-2274: Remove confusing comment.The comment "why is glossiness not just a float anyway?" is misleading because
glossinessis already a float scalar in this context. Consider removing it to avoid confusion.Apply this diff to remove the comment:
- snowFactor = SnowCover::ApplySnow(snowedColor, snowNormal, glossiness, shininess, disp, adjustedWorldPos, snowOcclusion, input.WorldPosition.z - waterHeight, viewPosition.z, uv - uvOriginal); - // why is glossiness not just a float anyway? + snowFactor = SnowCover::ApplySnow(snowedColor, snowNormal, glossiness, shininess, disp, adjustedWorldPos, snowOcclusion, input.WorldPosition.z - waterHeight, viewPosition.z, uv - uvOriginal);src/Features/SnowCover.cpp (1)
16-20: Move copyString to utility module or use existing helper.This helper duplicates functionality that likely exists in the codebase's utility module. Per review feedback, custom string utilities should be placed in utils rather than feature-specific files.
Consider checking if
Utilalready provides a similar function, or move this toUtilif it's needed project-wide.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package/Shaders/Lighting.hlsl(14 hunks)package/Shaders/RunGrass.hlsl(2 hunks)src/Features/SnowCover.cpp(1 hunks)src/Features/SnowCover.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/SnowCover.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Lighting.hlslsrc/Features/SnowCover.cpppackage/Shaders/RunGrass.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/SnowCover.cpp
🧠 Learnings (9)
📓 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-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 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/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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/SnowCover.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/SnowCover.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/SnowCover.cpp
📚 Learning: 2025-10-02T14:20:33.417Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.417Z
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/SnowCover.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
package/Shaders/RunGrass.hlsl (1)
454-460: LGTM!The SNOW_COVER scaffolding correctly undefines conflicting macros and includes the SnowCover feature for grass shading.
package/Shaders/Lighting.hlsl (8)
30-39: LGTM!The SNOW_COVER gating correctly disables the feature for incompatible shader paths and enables GLINT for TRUE_PBR+SNOW_COVER.
967-970: LGTM!The SnowCover.hlsli inclusion is properly guarded and SNOW is undefined to prevent conflicts with the new SNOW_COVER feature.
1062-1066: LGTM!The
sh0initialization values (0 for LANDSCAPE, 0.5 for non-LANDSCAPE) are appropriate for their respective displacement contexts.
1410-1410: LGTM!The fallback RMAOS assignments correctly compute roughness as
1 - glossinessfor non-PBR landscape tiles.Also applies to: 1491-1491, 1571-1571, 1651-1651, 1732-1732, 1812-1812
1866-1876: LGTM!The SNOW_COVER guard correctly prevents the LOD noise multiplier from being applied to
baseColorwhen snow cover is enabled, since snow will override it later.
2294-2299: LGTM!The glint precomputation is properly guarded by the GLINT define and only executes when
GlintLogMicrofacetDensity > 1.1, which is appropriate.
3045-3047: LGTM!The vertex color adjustment correctly blends toward white based on
snowFactor, which is appropriate for snow-covered surfaces.
1015-1015: Ultra tree detection remains fragile.The compound condition
!(defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(SOFT_LIGHTING))for ultra trees is fragile and could break with shader permutation changes. However, this is necessary because ultra trees have no dedicated define, andExtraFlags::IsTreemay not apply to them (based on learnings).Based on learnings
src/Features/SnowCover.cpp (1)
550-572: LGTM! Null-safety checks are correctly implemented.The conditional at line 555 properly guards
GetObjectReference()before callingIsBoundAnimObject()using short-circuit evaluation. The animation/havok object detection and whitelist/blacklist logic are correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package/Shaders/Lighting.hlsl (1)
2250-2250: Consider simplifying nested max() for readability.The nested
max()calls for computingdispin LANDSCAPE mode are functionally correct but could be more readable. Consider introducing a helper or loop if HLSL permits, or using intermediate variables.Current:
float disp = sh0 * max(displacementParams[0].HeightScale * input.LandBlendWeights1.x, max(...));Alternative (if supported):
float maxHeight = displacementParams[0].HeightScale * input.LandBlendWeights1.x; maxHeight = max(maxHeight, displacementParams[1].HeightScale * input.LandBlendWeights1.y); maxHeight = max(maxHeight, displacementParams[2].HeightScale * input.LandBlendWeights1.z); maxHeight = max(maxHeight, displacementParams[3].HeightScale * input.LandBlendWeights1.w); maxHeight = max(maxHeight, displacementParams[4].HeightScale * input.LandBlendWeights2.x); maxHeight = max(maxHeight, displacementParams[5].HeightScale * input.LandBlendWeights2.y); float disp = sh0 * maxHeight;This improves readability without changing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/Lighting.hlsl
🧠 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-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 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/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Lighting.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (8)
package/Shaders/Lighting.hlsl (8)
30-39: LGTM: Preprocessor gating is correct.The SNOW_COVER feature is appropriately disabled for character-related shaders (SKINNED, SKIN, EYE, HAIR) and when EXTENDED_MATERIALS is not available. The conditional GLINT definition under TRUE_PBR for snow materials is reasonable.
967-970: LGTM: Conditional include is correct.Undefining
SNOWwhenSNOW_COVERis defined prevents conflicts between the legacy snow system and the new SnowCover feature. The conditional include ofSnowCover.hlsliis appropriate.
1062-1066: LGTM: sh0 initialization is explicit and correct.The explicit initialization of
sh0to0for LANDSCAPE and0.5for non-LANDSCAPE paths prepares for subsequent parallax/displacement calculations. The different values are intentional for the different rendering paths.
1410-1410: LGTM: Fallback RMAOS assignments are correct.The fallback RMAOS values for non-PBR landscape tiles are correctly computed. The pattern
float4(1 - glossiness, 0, 1, 0)appropriately sets roughness from glossiness, with zero metallic, full AO, and zero specular. The scalarglossinessis correctly multiplied by the weight and the float4 vector.Also applies to: 1491-1491, 1571-1571, 1651-1651, 1732-1732, 1812-1812
1870-1873: LGTM: Snow cover correctly bypasses LOD noise.The
SNOW_COVERguard ensures that when snow is active, the LOD land noise multiplier doesn't affectbaseColor, allowing the snow system to fully control the appearance. This is the correct behavior.
2241-2289: LGTM: Snow application logic is comprehensive.The main snow cover implementation correctly:
- Gates snow application on
EnableSnowCoverand appropriate shader flags- Computes displacement from parallax/height data for different rendering paths
- Applies snow to color, normal, and material properties via
SnowCover::ApplySnow/ApplySnowPBR- Conditionally affects foliage tint when
AffectTreeTintis enabled- Blends snow normals with surface normals
- Adjusts LOD land colors and fade factors when snow is present
The conditional branches correctly handle TRUE_PBR, LANDSCAPE, EMAT, LODLANDNOISE, and LOD_LAND_BLEND permutations.
2293-2298: LGTM: GLINT precomputation is correctly positioned.The GLINT precomputation is correctly placed after the SNOW_COVER block, ensuring that snow-modified glint parameters are used. The conditional check on
GlintLogMicrofacetDensity > 1.1appropriately skips precomputation when glint is disabled or negligible.
3044-3046: LGTM: Snow vertex color adjustment is correct.The vertex color adjustment
vertexColor.rgb = snowFactor + (1 - snowFactor) * vertexColor.rgbcorrectly brightens the vertex color toward white based on the snow coverage factor. This is equivalent tolerp(vertexColor.rgb, 1.0, snowFactor)and is the expected behavior for snow-covered surfaces.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Lighting.hlsl (1)
1411-1814: Avoid double-multiplying landscape blend weights in fallback RMA.Inside each
LandTile*PBR == 0branch we now multiply by the blend weight twice: once explicitly (input.LandBlendWeights*.? * …) and again when adding toblendedRMAOS(* weight). That squares the layer contribution, so mixed tiles report far too little roughness/occlusion. Drop the explicit weight factor in the RMAOS initialisers (same fix applies to all six layers).- landRMAOS1 = input.LandBlendWeights1.x * float4(1 - glossiness, 0, 1, 0); + landRMAOS1 = float4(1 - glossiness, 0, 1, 0);(Repeat for
landRMAOS2,landRMAOS3,landRMAOS4,landRMAOS5, andlandRMAOS6.)
♻️ Duplicate comments (1)
package/Shaders/Lighting.hlsl (1)
2229-2235: Use uint outputs with GetDimensions to keep the shader compiling.
Texture2D::GetDimensionswritesuints. Declaringrx/ryasfloatmis-matches the signature and fails under DXC/FXC. Declare them asuint(oruint2) and cast when you need a float divisor.- float rx; - float ry; - TexColorSampler.GetDimensions(rx, ry); + uint rx; + uint ry; + TexColorSampler.GetDimensions(rx, ry); @@ - snowOcclusion = 1 - TexColorSampler.Sample(SampColorSampler, uv - float2(0, 2. / ry)).a; + snowOcclusion = 1 - TexColorSampler.Sample(SampColorSampler, uv - float2(0.0, 2.0 / float(ry))).a;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli(1 hunks)package/Shaders/Lighting.hlsl(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
features/Snow Cover/Shaders/SnowCover/SnowCover.hlslipackage/Shaders/Lighting.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli
🧠 Learnings (5)
📓 Common learnings
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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.
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/Snow Cover/Shaders/SnowCover/SnowCover.hlsli
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 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/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Lighting.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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Utils/FormIdParser.h (1)
8-13: Complete the documentation for all utility methods.The previous review requested comments for these methods. Documentation has been added for
parseHexFile(line 9) andparseTriNameFile(line 11), buttrim(line 8) andfnv_hash(line 13) still lack explanatory comments. For consistency and maintainability, all public methods should be documented.Apply this diff to add the missing documentation:
+ // Removes leading and trailing whitespace from a string. static std::string trim(const std::string& str); // Parses a text file of numbers in hexadecimal format into a set. One number per line. A # symbol can be used for one-line comments. static std::unordered_set<std::uint32_t> parseHexFile(const std::filesystem::path&); // Parses a text file of any line of text without surrounding whitespace into a set. Used to parse Trishape node names. A # symbol can be used for one-line comments. static std::unordered_set<std::uint64_t> parseTriNameFile(const std::filesystem::path&); + // Computes a 64-bit FNV-1a hash of the given null-terminated string. static std::uint64_t fnv_hash(const char* key);Based on the previous review feedback.
package/Shaders/Lighting.hlsl (1)
1015-1019: Duplicate ultra-tree detection pattern — centralize.This compound define guard repeats elsewhere. Consider a single helper macro/flag (e.g., IS_ULTRA_TREE) to avoid drift and ease future changes. Reference the similar condition around the snow occlusion block.
🧹 Nitpick comments (3)
package/Shaders/Lighting.hlsl (1)
2218-2236: Ultra-tree occlusion sampling: consider stricter types and casts.Prefer uint2 for GetDimensions and explicit casts in UV math for DXC portability. Also consider hoisting the alpha-threshold (0.001) to a named const for reuse.
- float rx; - float ry; - TexColorSampler.GetDimensions(rx, ry); + uint rx, ry; + TexColorSampler.GetDimensions(rx, ry); @@ - snowOcclusion = 1 - TexColorSampler.Sample(SampColorSampler, uv - float2(0, 2. / ry)).a; + snowOcclusion = 1.0 - TexColorSampler.Sample(SampColorSampler, uv - float2(0.0, 2.0 / float(ry))).a;features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli (2)
31-38: Height/environment multipliers: OK; consider naming/units clarity.Works as written. Minor: consider renaming height_tresh -> height_thresh and documenting units/scales (mapZscale, BlendSmoothness).
Also applies to: 40-43
147-174: Non-PBR path OK; constants are magic — consider centralizing.The 25*500 factor for shininess is opaque. Consider moving such scalars into SharedData::snowCoverSettings for easier tuning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli(1 hunks)package/Shaders/Common/Color.hlsli(1 hunks)package/Shaders/Lighting.hlsl(13 hunks)src/Utils/FormIdParser.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:
package/Shaders/Common/Color.hlslifeatures/Snow Cover/Shaders/SnowCover/SnowCover.hlslipackage/Shaders/Lighting.hlslsrc/Utils/FormIdParser.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/Snow Cover/Shaders/SnowCover/SnowCover.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Utils/FormIdParser.h
🧠 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:
features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 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/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Lighting.hlsl
🧬 Code graph analysis (1)
src/Utils/FormIdParser.h (1)
src/Utils/FormIdParser.cpp (4)
parseHexFile(16-51)parseHexFile(16-16)parseTriNameFile(53-77)parseTriNameFile(53-53)
🪛 Clang (14.0.6)
src/Utils/FormIdParser.h
[error] 3-3: 'string' 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 (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 (9)
package/Shaders/Common/Color.hlsli (1)
68-76: HSV utilities look correct; good addition.Formulas match established patterns; guards avoid div-by-zero when V=0. No issues spotted.
Also applies to: 77-81, 82-103
package/Shaders/Lighting.hlsl (3)
30-38: Snow Cover gating is sensible.Undefining SNOW_COVER for SKINNED/SKIN/EYE/HAIR or when EXTENDED_MATERIALS is off prevents accidental enablement on unsupported paths.
2271-2281: Glossiness scalar usage fixed — good.Passing glossiness as a float and removing swizzles resolves the earlier compile-time/type issue.
3045-3047: Vertex color snow tint blend is fine.Simple, branch-free blend; matches feature intent.
features/Snow Cover/Shaders/SnowCover/SnowCover.hlsli (5)
19-25: Normal reorientation implementation looks correct.Matches reoriented normal mapping for s=(0,0,1). No issues.
45-55: Foliage HSV tinting: consistent with new Color utilities.Logic reads clean; no correctness concerns.
79-108: ApplySnowBase math looks sound after clamp fix.The corrected clamp argument order prevents underflow/overflow in weatherMult. Early-out threshold is appropriate.
110-144: PBR path integration is coherent; glint params blended correctly.F0/roughness/metallic/AO and glint fields are lerped by mult; good separation of main vs alt sets.
9-16: Texture register assignments verified as collision-free.Verification confirms that t38–t44 registers are used exclusively by SnowCover and do not collide with any other features or Common modules. The assignments are safe.
| { | ||
| landRMAOS1 = input.LandBlendWeights1.x * float4(1 - glossiness.x, 0, 1, 0); | ||
| landRMAOS1 = input.LandBlendWeights1.x * float4(1 - glossiness, 0, 1, 0); | ||
| } |
There was a problem hiding this comment.
Fix double application of layer weights in PBR terrain RMAOS fallback.
In the non-PBR-tile fallback branches you multiply by the layer weight inside landRMAOSN and again when accumulating, effectively squaring weights. Drop the inner factor.
Apply this diff:
- landRMAOS1 = input.LandBlendWeights1.x * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS1 = float4(1 - glossiness, 0, 1, 0);
@@
- landRMAOS2 = input.LandBlendWeights1.y * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS2 = float4(1 - glossiness, 0, 1, 0);
@@
- landRMAOS3 = input.LandBlendWeights1.z * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS3 = float4(1 - glossiness, 0, 1, 0);
@@
- landRMAOS4 = input.LandBlendWeights1.w * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS4 = float4(1 - glossiness, 0, 1, 0);
@@
- landRMAOS5 = input.LandBlendWeights2.x * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS5 = float4(1 - glossiness, 0, 1, 0);
@@
- landRMAOS6 = input.LandBlendWeights2.y * float4(1 - glossiness, 0, 1, 0);
+ landRMAOS6 = float4(1 - glossiness, 0, 1, 0);Also applies to: 1491-1492, 1571-1572, 1651-1652, 1732-1733, 1812-1813
🤖 Prompt for AI Agents
In package/Shaders/Lighting.hlsl around lines 1409-1411 (and similarly at
1491-1492, 1571-1572, 1651-1652, 1732-1733, 1812-1813), the non-PBR-tile
fallback branches multiply layer weights twice by applying the layer weight
inside the landRMAOSN assignment and again during accumulation; remove the inner
multiplication so landRMAOSN is assigned without the layer weight (leave the
accumulation step to apply the weight once) for each of these occurrences.
| #include <string> | ||
| #include <unordered_set> |
There was a problem hiding this comment.
Missing required header: <filesystem>.
The methods parseHexFile and parseTriNameFile on lines 10 and 12 use std::filesystem::path, but <filesystem> is not included. This will cause compilation failure.
Apply this diff to add the missing include:
#include <string>
#include <unordered_set>
+#include <filesystem>Note: The static analysis error about the missing 'string' file is a false positive from the analysis environment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <string> | |
| #include <unordered_set> | |
| #include <string> | |
| #include <unordered_set> | |
| #include <filesystem> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 3-3: 'string' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In src/Utils/FormIdParser.h around lines 3 to 4, the header is missing the
required <filesystem> include used by parseHexFile and parseTriNameFile which
reference std::filesystem::path; add #include <filesystem> alongside the
existing includes (e.g., after <string>) so the file compiles successfully with
std::filesystem types available.
4 participants
A feature handling seasonal snow cover, also usable for ash and similar materials that cover large areas. The feature blends a snow texture into the original rendered textures based on the configured criteria. The snow textures are always projected from the top (with parallax shifting the uv to avoid lines on vertical surfaces) and foliage optionally has snow only around the edges.
Summary by CodeRabbit
New Features
Documentation
Chores