feat: unified water #1634
feat: unified water #1634
Conversation
|
Warning Rate limit exceeded@davo0411 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new UnifiedWater overlay feature with flowmap generation/loading, async disk/runtime WaterCache, engine hooks for water rendering and mesh attachment, shader VS_OUTPUT and flowmap sampling updates, DirectX texture I/O helpers, world-to-cell utilities, and new globals/RTTI entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Game as Game Engine
participant UW as UnifiedWater
participant FM as Flowmap
participant WC as WaterCache
participant FS as FileSystem
participant D3D as D3D Utils
Game->>UW: DataLoaded()
activate UW
UW->>UW: LoadOrderChanged()
UW->>FM: LoadOrGenerateFlowmap(useMips)
activate FM
FM->>FS: Check for Tamriel-Flowmap DDS
alt DDS exists
FM->>FS: Load DDS -> create NiSourceTexture
else missing
FM->>D3D: Compose tiles, generate mips, create texture
FM->>FS: Save Tamriel-Flowmap.<w>.<h>.<ox>.<oy>.dds
end
FM-->>UW: Flowmap ready (texture + dims)
deactivate FM
UW->>WC: LoadOrGenerateCaches()
activate WC
WC->>FS: Check for per-world-space disk caches
alt Disk caches exist
WC->>WC: Load DiskCache -> TryBuildRuntimeCache()
else
WC->>WC: GenerateCaches() async (extract cell data, generate instructions)
WC->>FS: Write DiskCache files
WC->>WC: TryBuildRuntimeCache()
end
WC-->>UW: Caches ready
deactivate WC
UW->>Game: PostPostLoad() -> install hooks/detours
UW-->>Game: Hooks installed
Game->>UW: Render-time shader param request
activate UW
UW->>FM: TryGetFlowmap()
FM-->>UW: Provide texture, width, height, offsets
UW->>Game: Populate shader globals (flowmap SRV, cell offsets, sizes)
deactivate UW
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
src/Utils/Game.cpp (1)
315-325: LGTM! Consider documenting the cell size constant.The
WorldToCellimplementations correctly convert world coordinates to cell indices usingfloor(), which properly handles negative coordinates. The 3D overload delegation is clean.Minor suggestion: Consider extracting
4096.0fto a named constant (e.g.,constexpr float CELL_SIZE = 4096.0f;) for clarity and maintainability, similar to the constants innamespace Units.src/Features/UnifiedWater/Flowmap.cpp (2)
255-258: Hardcoded tile dimensions may cause silent failures.The validation requires exactly 64x64 tiles with exactly 6 mip levels. If flow textures have different dimensions (e.g., higher resolution), they're silently skipped with only a warning. Consider making the expected dimensions configurable or documenting this constraint.
315-319: Null check ondeferredCtxis redundant.At line 316,
deferredCtxis checked for null, but if it were null, the function would have returned at line 159-162. The check is harmless but unnecessary.- if (deferredCtx && FAILED(deferredCtx->FinishCommandList(FALSE, commandList.put()))) { + if (FAILED(deferredCtx->FinishCommandList(FALSE, commandList.put()))) {src/Features/UnifiedWater/WaterCache.cpp (1)
52-59: Potential signed/unsigned comparison issue.At line 58,
lodIndexis compared againstinstructions.size()which returnssize_t. WhenlodIndexis negative (which is checked), it's fine, but mixing signed comparison with unsigned container size can be error-prone.- const auto lodIndex = std::countr_zero(static_cast<uint32_t>(lodLevel)) - 2; - if (lodIndex < 0 || lodIndex >= instructions.size()) + const int lodIndex = std::countr_zero(static_cast<uint32_t>(lodLevel)) - 2; + if (lodIndex < 0 || static_cast<size_t>(lodIndex) >= instructions.size()) return nullptr;src/Features/UnifiedWater.h (1)
105-106: Ownership semantics of raw pointers unclear.
flowmapandwaterCacheare raw pointers. It's unclear who owns these objects and when they're allocated/deallocated. Consider using smart pointers for clearer ownership semantics.- Flowmap* flowmap = nullptr; - WaterCache* waterCache = nullptr; + std::unique_ptr<Flowmap> flowmap; + std::unique_ptr<WaterCache> waterCache;src/Features/UnifiedWater/WaterCache.h (1)
7-21: Union type punning requires care.The
Formunion stores either auint32_t idor aRE::TESWaterForm* ptr. Reading from a different member than was last written is technically undefined behavior in C++, though it works in practice. Consider usingstd::variantor a tagged union for type safety.src/Features/UnifiedWater.cpp (6)
50-54: Add parentheses for clarity in complex boolean expressions.The operator precedence is correct, but explicit parentheses improve readability and prevent future maintenance errors.
- if (!waterCache || !waterCache->IsBuildRunning() && !waterCache->HasBuildFailed()) + if (!waterCache || (!waterCache->IsBuildRunning() && !waterCache->HasBuildFailed())) return; const auto shaderCache = globals::shaderCache; - const float vOffset = shaderCache->IsCompiling() || shaderCache->GetFailedTasks() > 0 && !shaderCache->IsHideErrors() ? 120.0f : 0.0f; + const float vOffset = (shaderCache->IsCompiling() || (shaderCache->GetFailedTasks() > 0 && !shaderCache->IsHideErrors())) ? 120.0f : 0.0f;
118-119: Rawnewallocations without RAII; use smart pointers to prevent leaks.The
flowmapandwaterCacheare never deleted. Consider usingstd::unique_ptras class members for automatic lifetime management.- flowmap = new Flowmap(); - waterCache = new WaterCache(); + flowmap = std::make_unique<Flowmap>(); + waterCache = std::make_unique<WaterCache>();This requires changing the member declarations in the header to
std::unique_ptr<Flowmap>andstd::unique_ptr<WaterCache>, and updating accessor calls throughout.
181-186: Inconsistent file handling and missing write verification.Line 177 explicitly calls
close()but lines 182-185 do not. Additionally, there's no verification that the write succeeded.if (hash != existingHash) { std::ofstream file(path, std::ios::binary | std::ios::trunc); if (file.is_open()) { file.write(reinterpret_cast<const char*>(&hash), sizeof(hash)); + if (!file.good()) { + logger::warn("[Unified Water] Failed to write load order hash"); + } + file.close(); } }
321-322: Magic number for cell state should be documented or use a named constant.
static_cast<RE::TESObjectCELL::CellState>(6)is unclear. Add a comment explaining what state 6 represents, or define a named constant.- if (const auto cell = gridCells->GetCell(x, y); cell && cell->cellState.any(RE::TESObjectCELL::CellState::kAttached, static_cast<RE::TESObjectCELL::CellState>(6))) + // CellState 6 represents a transitional attached state + constexpr auto kTransitionalAttached = static_cast<RE::TESObjectCELL::CellState>(6); + if (const auto cell = gridCells->GetCell(x, y); cell && cell->cellState.any(RE::TESObjectCELL::CellState::kAttached, kTransitionalAttached))
396-397: Magic number for water flags should be documented.
0b10000100sets specific bits inWaterFlagbut it's unclear which flags these represent without cross-referencing the enum definition.- REX::EnumSet waterFlags = static_cast<RE::BSWaterShaderProperty::WaterFlag>(0b10000100); + // Bit 2: Unknown, Bit 7: Unknown - base flags for LOD water + REX::EnumSet waterFlags = static_cast<RE::BSWaterShaderProperty::WaterFlag>(0b10000100);Or better, use named flags if available in the enum definition.
414-432: Save-null-restore pattern is intentional but fragile.The pattern of saving
block->water, setting it to null before callingfunc(), then restoring it prevents the original function from processing the water. This works but is fragile iffunc()behavior changes.Consider adding a brief comment explaining the intent:
void UnifiedWater::BGSTerrainBlock_Detach::thunk(RE::BGSTerrainBlock* block) { + // Temporarily null out water to prevent original func from processing it const auto water = block->water; block->water = nullptr; func(block); block->water = water;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
features/Unified Water/Shaders/Features/UnifiedWater.ini(1 hunks)package/Shaders/Water.hlsl(12 hunks)src/Feature.cpp(2 hunks)src/Features/UnifiedWater.cpp(1 hunks)src/Features/UnifiedWater.h(1 hunks)src/Features/UnifiedWater/Flowmap.cpp(1 hunks)src/Features/UnifiedWater/Flowmap.h(1 hunks)src/Features/UnifiedWater/WaterCache.cpp(1 hunks)src/Features/UnifiedWater/WaterCache.h(1 hunks)src/Globals.cpp(6 hunks)src/Globals.h(4 hunks)src/ShaderCache.h(1 hunks)src/Utils/D3D.cpp(2 hunks)src/Utils/D3D.h(2 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
features/Unified Water/Shaders/Features/UnifiedWater.inisrc/Globals.hsrc/Feature.cppsrc/Utils/D3D.cppsrc/ShaderCache.hsrc/Utils/D3D.hsrc/Features/UnifiedWater/Flowmap.cppsrc/Globals.cppsrc/Utils/Game.cpppackage/Shaders/Water.hlslsrc/Features/UnifiedWater.hsrc/Features/UnifiedWater/Flowmap.hsrc/Features/UnifiedWater/WaterCache.cppsrc/Utils/Game.hsrc/Features/UnifiedWater/WaterCache.hsrc/Features/UnifiedWater.cpp
**/*.{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/Feature.cppsrc/Utils/D3D.cppsrc/ShaderCache.hsrc/Utils/D3D.hsrc/Features/UnifiedWater/Flowmap.cppsrc/Globals.cppsrc/Utils/Game.cpppackage/Shaders/Water.hlslsrc/Features/UnifiedWater.hsrc/Features/UnifiedWater/Flowmap.hsrc/Features/UnifiedWater/WaterCache.cppsrc/Utils/Game.hsrc/Features/UnifiedWater/WaterCache.hsrc/Features/UnifiedWater.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/Globals.hsrc/Feature.cppsrc/Utils/D3D.cppsrc/ShaderCache.hsrc/Utils/D3D.hsrc/Features/UnifiedWater/Flowmap.cppsrc/Globals.cppsrc/Utils/Game.cppsrc/Features/UnifiedWater.hsrc/Features/UnifiedWater/Flowmap.hsrc/Features/UnifiedWater/WaterCache.cppsrc/Utils/Game.hsrc/Features/UnifiedWater/WaterCache.hsrc/Features/UnifiedWater.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Globals.hsrc/Feature.cppsrc/Globals.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
src/Globals.hsrc/Globals.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/Feature.cppsrc/Globals.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
src/ShaderCache.hpackage/Shaders/Water.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
src/ShaderCache.hpackage/Shaders/Water.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
src/ShaderCache.h
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
src/ShaderCache.h
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Water.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 (27)
src/ShaderCache.h (1)
548-569: LGTM! Helpful inline documentation.The hex value comments improve readability for debugging and matching against game shader descriptors.
features/Unified Water/Shaders/Features/UnifiedWater.ini (1)
1-2: LGTM! Standard feature INI configuration.The file follows the established pattern for feature versioning. The version format
1-0-0will be correctly parsed byFeature::Load().src/Feature.cpp (1)
28-28: LGTM! Proper feature registration.The UnifiedWater feature is correctly included and registered in the feature list following the established pattern. As per coding guidelines, new features should be registered in the
globals::featuresnamespace, which is correctly done here.Also applies to: 231-232
package/Shaders/Water.hlsl (6)
83-85: Good architectural change: fog computation moved to pixel shader.For
UNIFIED_WATER, removing the vertex-interpolatedFogParamand computing fog per-pixel (lines 1196-1202, 1222-1228) provides more accurate results, especially for large water surfaces where vertex density may be low.
93-93: LGTM! Non-square flowmap dimension support.Expanding
TexCoord4fromfloattofloat2correctly supports non-square flowmap dimensions, which is necessary for the unified water LOD system.Also applies to: 287-287
541-551: LGTM! Well-implemented flowmap mip-level calculation.The manual derivative-based mip calculation with clamping (0-5) and flow vector scaling provides proper LOD handling for flowmap normals, preventing aliasing at distance while maintaining visual consistency.
218-238: LGTM! UV coordinate handling for unified water cells.The
cellShiftcalculation andscaledUVapproach properly handle multi-cell water tiling, ensuring consistent texture mapping across the unified water surface.
631-637: LGTM! Consistent blending for unified water.Using both
TexCoord4.xycomponents and forcingdistanceBlendFactor = 1.0ensures consistent visual appearance across the entire unified water surface, avoiding distance-based artifacts on large water bodies.Also applies to: 1047-1050
1196-1209: Verify fog exponent consistency.The pixel shader fog calculation uses
FresnelRI.yas the exponent (line 1200), while the vertex shader usesNormalsScale.w(line 194). Ensure these values are equivalent or intentionally different for the unified water path.src/Globals.h (2)
23-23: LGTM! Proper global feature declaration.The
UnifiedWaterforward declaration and extern follow the established pattern for feature registration in theglobals::featuresnamespace. Based on coding guidelines, this is the correct approach for registering new features.Also applies to: 75-75
208-208: LGTM! Supporting globals for unified water.The additions of
RE::TES* tes,BSWaterShaderPropertyRTTI, andNiSourceTextureRTTIprovide necessary infrastructure for the unified water feature's world-space operations and runtime type identification.Also applies to: 235-235, 239-239
src/Globals.cpp (2)
71-71: LGTM!The
UnifiedWaterfeature registration follows the established pattern for feature instances inglobals::features. Based on learnings, this correctly registers the new feature in the globals::features namespace.
124-128: LGTM!The RTTI relocations for
BSWaterShaderPropertyRTTIandNiSourceTextureRTTIfollow the existing declaration and initialization patterns used by other shader property RTTIs.src/Utils/Game.h (1)
173-175: LGTM!The
WorldToCelldeclarations correctly match their implementations and provide clean overloads for both 2D and 3D point types.src/Utils/D3D.h (2)
4-4: Verify necessity of winrt/base.h include.This header brings in Windows Runtime C++ support. If it's only needed for the new texture I/O functions in D3D.cpp, consider moving it to the implementation file to reduce header dependencies.
31-32: LGTM!The texture I/O function declarations are well-designed with appropriate parameters for DirectX texture save/load operations.
src/Utils/D3D.cpp (1)
410-472: LGTM! Robust atomic file write implementation.The
SaveTextureToFilefunction has solid error handling:
- Proper input validation
- Directory creation with error handling
- Atomic write pattern using temporary file and rename
- Cleanup of temporary file on failure
src/Features/UnifiedWater/Flowmap.cpp (1)
6-13: LGTM!The null-check chain correctly validates the texture and its associated resources before returning.
src/Features/UnifiedWater/Flowmap.h (1)
1-29: LGTM!The header provides a clean interface with proper const-correctness on getters and sensible default initialization for all members.
src/Features/UnifiedWater/WaterCache.cpp (2)
250-267: Monitor thread continues after potential early exit fromGenerateCaches.If
GenerateCachesreturnstruebut then the caller doesn't wait, the monitor thread (async.monitor) continues running. This is fine, but thejthreaddestructor will request stop and join. Ensure this doesn't block main thread unexpectedly on feature unload.
689-722: LGTM!The
ReadMinLandHeightDatafunction correctly handles endianness and iterates through the height data to find the minimum land height.src/Features/UnifiedWater/WaterCache.h (2)
109-157: LGTM!The
BuildProgressstruct provides a clean thread-safe progress tracking mechanism with appropriate atomic operations and memory ordering.
162-164: File does not exist in repository.The file path
src/Features/UnifiedWater/WaterCache.hdoes not exist in this repository. A search of the entire codebase shows noWaterCache.hfile, and there is noUnifiedWaterfeature directory. Water-related content is located only in shader files withinpackage/Shaders/. This review comment cannot be verified against the codebase.Likely an incorrect or invalid review comment.
src/Features/UnifiedWater.h (1)
98-98: This review comment references non-existent code in an incorrect repository context.The file
src/Features/UnifiedWater.hdoes not exist in this repository. The doodlum/skyrim-community-shaders repository is a shader codebase containing feature directories with HLSL/GLSL shaders, not C++ plugin code. There is noSupportsVR()method or C++ implementation to verify. The review comment appears to be misapplied to this repository.Likely an incorrect or invalid review comment.
src/Features/UnifiedWater.cpp (3)
203-242: LGTM!The hook installation and platform-specific patching follow established patterns in the codebase. The use of
REL::RelocationIDwithREL::Relocateproperly handles SE/AE differences.
244-267: LGTM with note on the hash-in-pointer hack.The approach of temporarily storing the texture name hash in the
normalTexture1pointer slot is documented and serves a valid purpose (ensuring material uniqueness). The hash is properly zeroed out inComputeCRC32::thunkbefore the original function runs.
434-479: LGTM!The shader setup thunks properly guard against null
flowmapand include helpful comments explaining the texture coordinate calculations and how they counter operations in the originalSetupGeometry.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Features/UnifiedWater.h (3)
12-21: Polish user‑facing feature summary and avoid over‑absolute claimsThe summary is a big improvement over the previous TODO, but it still leans on engine jargon (“LOD0”, “LOD mismatch”) and makes a very strong “Completely and fundamentally resolves” claim that might be hard to guarantee across modded setups.
Consider slightly softening the language, avoiding engine‑specific acronyms, and adding a fifth bullet that mentions runtime compatibility / limitations, per the earlier review note. For example:
- return { - "Unified Water provides a comprehensive fix to water LOD mismatch by replacing distant water tiles with LOD0 (Close Water).", - { "Unifies distant and close water appearance, streamlining all lighting visuals.", - "Completely and fundamentally resolves water LOD mismatch issues.", - "Provides background systems for water geometry rendering, allowing more advanced water effects.", - "Improves vanilla performance by using optimized water meshes for distant water." } - }; + return { + "Unified Water replaces distant water tiles with full-quality water to fix seams and improve visual consistency.", + { "Matches distant water to nearby water to remove visible transitions and seams.", + "Replaces engine-driven water LOD tiles with full-quality water surfaces where possible.", + "Provides background systems for water geometry and flowmaps used by advanced water effects.", + "Includes optional optimised meshes that can improve performance on vanilla-style setups.", + "Designed for all supported runtimes; heavily modded water overhauls may still need compatibility patches." } + };Adjust wording as needed to best fit what you’ve seen in testing, but keeping it non‑technical and not over‑promising will help set correct expectations in the UI.
24-29: Optional: align config naming and UI wording for “optimised/optimized” meshes
Settings::UseOptimisedMeshesuses British spelling while the summary bullet currently says “optimized water meshes”. Not a bug, but you may want to standardize on one spelling (either “optimized” or “optimised”) in both the setting name (if still changeable) and the user‑facing summary text to avoid small polish inconsistencies.
1-116: PR metadata: suggest conventional commit title and issue referencesTo align with the project’s guidelines:
Suggested conventional commit title (shorter, conventional‑style):
feat(water): add unified water systemIf this PR fixes a specific water LOD / distant water issue, consider adding one of the following to the PR description:
Fixes #<issue>for a bug this definitively resolvesImplements #<issue>orAddresses #<issue>if it’s a feature tracked elsewhereRelated to #<issue>if it’s part of a broader effortThis will help keep the history and issue tracking clean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/UnifiedWater.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/UnifiedWater.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/UnifiedWater.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/UnifiedWater.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (2)
src/Features/UnifiedWater.h (2)
31-83: Hook declarations look consistent; ensure SE/AE/VR coverage in implementationThe thunk +
REL::Relocation<decltype(thunk)> func;pattern here is consistent with the rest of the project, and the chosen hook points (water system init, terrain attach/detach, displacement mesh updates, etc.) match the unified water goals.Since this feature is enabled for VR (see
SupportsVR()), please double‑check in the.cppthat:
- All these relocations are properly resolved for SE, AE, and VR (no missing IDs / wrong signatures on VR), and
- Failure to resolve any of them degrades gracefully (e.g., disabling UnifiedWater), rather than hard‑crashing.
No changes needed in the header, just confirming that the implementation follows the usual runtime‑detection/error‑handling patterns.
97-97: ConfirmSupportsVR()reflects current level of VR testingDeclaring
SupportsVR()astrueis great if all hooks and shaders are fully validated on VR, but if VR support is still experimental or partially untested, you might want to:
- Temporarily return
falseuntil testing is complete, or- Mark the feature as “experimental in VR” in the UI summary text.
Please confirm this matches the actual state of VR support for UnifiedWater before merging.
|
✅ A pre-release build is available for this PR: |
|
|
||
| void WorldToCell(const RE::NiPoint3& worldPos, int32_t& x, int32_t& y); | ||
|
|
||
| constexpr uint32_t FCC(const char s[4]) noexcept |
There was a problem hiding this comment.
What does FCC do or mean?
There was a problem hiding this comment.
FCC = Four character code, i assume sic is using it here instead of CELL or WRLD as the FCC has no runtime cost compared to using string comparison
There was a problem hiding this comment.
Needs to be in the code.
There was a problem hiding this comment.
Needs to be in the code.
There was a problem hiding this comment.
Yeah, it's four character code. The plugin files use four bytes to represent a name of each section, this just allows those four bytes to be represented as a human readable string in the codebase, but it compiles down to a simple integer comparison.
There was a problem hiding this comment.
Add a doxygen comment so we know 5 months from now.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/Features/UnifiedWater.cpp (2)
106-118: Unsafe pointer chain can crash; address the TODO comments.Per coding guidelines, provide complete solutions instead of TODO placeholders. The chained calls can dereference null or access empty containers, which will crash the game.
- // TODO error check this properly - const auto waterShape = nif->GetChildren().front()->AsNode()->GetChildren().front()->AsTriShape(); - waterMesh = RE::NiPointer(waterShape); + if (nif->GetChildren().empty()) { + logger::error("[Unified Water] Water mesh has no children"); + return; + } + const auto firstChild = nif->GetChildren().front(); + if (!firstChild || !firstChild->AsNode() || firstChild->AsNode()->GetChildren().empty()) { + logger::error("[Unified Water] Water mesh structure is invalid"); + return; + } + const auto waterShape = firstChild->AsNode()->GetChildren().front()->AsTriShape(); + if (!waterShape) { + logger::error("[Unified Water] Water mesh is not a tri-shape"); + return; + } + waterMesh = RE::NiPointer(waterShape); logger::debug("[Unified Water] Water mesh loaded"); if (const auto error = RE::BSModelDB::Demand("meshes\\water\\optimisedwatermesh.nif", nif, args); error != RE::BSResource::ErrorCode::kNone) { logger::error("[Unified Water] Failed to load optimised water mesh"); return; } - // TODO error check this properly - const auto optimisedWaterShape = nif->GetChildren().front()->AsNode()->GetChildren().front()->AsTriShape(); + if (nif->GetChildren().empty()) { + logger::error("[Unified Water] Optimised water mesh has no children"); + return; + } + const auto optFirstChild = nif->GetChildren().front(); + if (!optFirstChild || !optFirstChild->AsNode() || optFirstChild->AsNode()->GetChildren().empty()) { + logger::error("[Unified Water] Optimised water mesh structure is invalid"); + return; + } + const auto optimisedWaterShape = optFirstChild->AsNode()->GetChildren().front()->AsTriShape(); + if (!optimisedWaterShape) { + logger::error("[Unified Water] Optimised water mesh is not a tri-shape"); + return; + }
137-139: Blocking spin-wait on the main thread may freeze the game during loading.This busy-wait loop blocks the calling thread until the cache build completes. If
DataLoadedis called on the main thread, the game will be unresponsive.Consider either:
- Making this non-blocking and handling incomplete caches gracefully
- Moving the wait to a background thread with a callback
- Using a condition variable instead of polling
- while (waterCache->IsBuildRunning()) { - std::this_thread::sleep_for(100ms); - } + // If cache build is still running, log and continue - cache will be available when ready + if (waterCache->IsBuildRunning()) { + logger::info("[Unified Water] Cache build in progress, will complete asynchronously"); + }Alternatively, if blocking is truly required, ensure this runs off the main thread.
🧹 Nitpick comments (6)
src/Features/UnifiedWater.cpp (6)
120-121: Potential memory leak with rawnew.
flowmapandwaterCacheare allocated with rawnewbut no corresponding cleanup is visible in this file. IfUnifiedWateris a singleton or long-lived object, this may be acceptable, but prefer RAII patterns.Consider using
std::unique_ptrfor automatic cleanup:- flowmap = new Flowmap(); - waterCache = new WaterCache(); + flowmap = std::make_unique<Flowmap>(); + waterCache = std::make_unique<WaterCache>();This requires updating the member declarations in the header to use
std::unique_ptr<Flowmap>andstd::unique_ptr<WaterCache>.
176-180: Add error handling for file I/O operations.The code doesn't verify that the full 8 bytes were read successfully. A short read would leave
existingHashpartially initialized.uint64_t existingHash = 0; if (fs::exists(path)) { std::ifstream file(path, std::ios::binary); - if (file.is_open()) { + if (file.is_open() && file.read(reinterpret_cast<char*>(&existingHash), sizeof(existingHash))) { - file.read(reinterpret_cast<char*>(&existingHash), sizeof(existingHash)); + if (file.gcount() != sizeof(existingHash)) { + logger::warn("[Unified Water] Incomplete hash file, treating as changed"); + existingHash = 0; + } - file.close(); + } else { + logger::warn("[Unified Water] Failed to read hash file"); } }
184-187: Verify file write success.Writing the hash file could fail due to permissions or disk space issues, but the failure is silently ignored.
if (hash != existingHash) { std::ofstream file(path, std::ios::binary | std::ios::trunc); - if (file.is_open()) { + if (file.is_open() && file.write(reinterpret_cast<const char*>(&hash), sizeof(hash))) { - file.write(reinterpret_cast<const char*>(&hash), sizeof(hash)); + // Successfully written + } else { + logger::warn("[Unified Water] Failed to write hash file"); } }
205-244: Consider error handling for hook installation.The hook installation calls don't check for failures. If a hook fails to install (e.g., due to version mismatches or memory protection), the feature may silently malfunction.
While the
stl::detour_thunkand related functions may internally handle errors, consider wrapping critical hooks with try-catch or validation to provide better diagnostics:try { stl::detour_thunk<TES_SetWorldSpace>(REL::RelocationID(13170, 13315)); // ... other hooks logger::info("[Unified Water] Installed hooks"); } catch (const std::exception& e) { logger::error("[Unified Water] Failed to install hooks: {}", e.what()); throw; }
362-365: Consider logging at debug level.The warning at line 362 may spam logs in normal gameplay if water instructions are legitimately missing for certain chunks. Consider lowering to debug level or adding a rate-limiter.
- logger::warn("[Unified Water] No instructions found for {} chunk at {}, {}", worldSpace->GetFormEditorID(), node->x, node->y); + logger::debug("[Unified Water] No instructions found for {} chunk at {}, {}", worldSpace->GetFormEditorID(), node->x, node->y);
373-373: Clarify complex ternary condition.The condition
lodLevel > 4 || singleton.settings.UseOptimisedMeshesmay benefit from explicit parentheses or extraction to a named variable for readability.- const auto targetShape = lodLevel > 4 || singleton.settings.UseOptimisedMeshes ? singleton.optimisedWaterMesh : singleton.waterMesh; + const bool useOptimisedMesh = (lodLevel > 4) || singleton.settings.UseOptimisedMeshes; + const auto targetShape = useOptimisedMesh ? singleton.optimisedWaterMesh : singleton.waterMesh;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/UnifiedWater.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/UnifiedWater.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/UnifiedWater.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/UnifiedWater.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
⏰ 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 (5)
src/Features/UnifiedWater.cpp (5)
7-24: LGTM!The settings serialization and management code is clean and uses the nlohmann JSON library idiomatically.
37-47: LGTM!The ImGui tree node is properly balanced with
TreePop()at line 46, addressing the previous review concern.
426-429: LGTM: Safe child detachment loop.The loop correctly detaches all children by decrementing from
size()to zero. The checkcount > 0before--countensures no underflow.
436-462: LGTM: Proper RTTI validation before cast.The code correctly validates the RTTI before performing a
static_castat both line 446 and earlier at line 396. This follows safe casting practices.
323-323: Replace the hardcoded magic number with a named constant or add clarifying documentation.The code uses
static_cast<RE::TESObjectCELL::CellState>(6)which represents an undocumented flag without a named constant in CommonLibSSE. OnlyCellState::kAttached = 7is formally defined; value 6 is a magic number. Consider either:
- Defining a local named constant if this represents a known Skyrim cell state (e.g.,
kLoadedor similar)- Adding a comment explaining what bit 6 represents
- Refactoring to use only documented enum values if possible
⛔ Skipped due to learnings
Learnt from: ThePagi Repo: doodlum/skyrim-community-shaders PR: 1369 File: src/Features/SnowCover.cpp:515-515 Timestamp: 2025-10-02T14:20:33.454Z Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
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: 1
🧹 Nitpick comments (4)
package/Shaders/Water.hlsl (4)
218-238: Consider adding comments to explain cell coordinate transformations.The
UNIFIED_WATERtexture coordinate calculations introduce a cell-shift system that's not immediately obvious. While the implementation appears correct, adding brief comments explaining the coordinate system would improve maintainability.For example:
+// Calculate cell shift for LOD water tile alignment float2 cellShift = float2(floor(ObjectUV.z * 0.5), floor((ObjectUV.z - 1.0) * 0.5)); +// Scale and offset UVs to handle cross-cell boundaries float2 scaledUV = input.TexCoord0.xy * ObjectUV.z - cellShift;
1218-1224: Consider documenting FresnelRI.y usage for fog calculation.The
UNIFIED_WATERfog computation usesFresnelRI.yas the fog power exponent (line 1222). While this allows artistic control by reusing an existing parameter, it's not immediately obvious that the fresnel parameter affects fog falloff.Consider adding a brief comment or using a more descriptive alias if this parameter is specifically tuned for unified water fog.
1244-1250: Consider extracting duplicated fog calculation.The fog computation for
UNIFIED_WATERis duplicated between the VC path (lines 1218-1224) and non-VC path (lines 1244-1250). Consider extracting this into a helper function to reduce maintenance burden.float CalculateUnifiedWaterFog(float distance, float4 fogParam, float fogFarMax, float fogPower) { return min(fogFarMax, pow(saturate(distance * fogParam.y - fogParam.x), fogPower)); }Then call it as:
float fogDistanceFactor = CalculateUnifiedWaterFog(input.WPosition.w, FogParam, FogFarColor.w, FresnelRI.y);
1-1300: Consider updating PR title to follow conventional commits format.Per the coding guidelines, the PR title should follow the conventional commits format:
- Current: "feat: unified water"
- Suggested: "feat(water): add unified water lod system"
The suggested format provides better clarity about the scope (water system) and includes a more descriptive summary. The 50-character limit is met with this suggestion.
Based on learnings, this ensures consistent PR titles across the repository.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Water.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:
package/Shaders/Water.hlsl
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Water.hlsl
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water.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 (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (9)
package/Shaders/Water.hlsl (9)
193-197: LGTM: Fog computation correctly moved to pixel shader.The fog calculation is properly gated to compute in the vertex shader only when
UNIFIED_WATERis not defined. This allows per-pixel fog computation for the unified water feature, providing higher visual quality at the cost of additional pixel shader work.
287-307: LGTM: Texture coordinate calculations align with structure changes.The
UNIFIED_WATERpaths correctly use 2D dimensions (ObjectUV.xy) instead of the vanilla 1D approach (ObjectUV.xx), which aligns with theTexCoord4type change fromfloattofloat2. This enables non-square flowmap textures.Also applies to: 309-309
653-657: LGTM: Flowmap dimensions handling is consistent.The conditional logic correctly extracts 2D dimensions for
UNIFIED_WATER(supporting non-square textures) and falls back to square dimensions for vanilla paths.
661-665: LGTM: Flowmap normal sampling uses varied parameters for blending.The multiple
GetFlowmapNormalcalls with different UV shifts, multipliers, and offsets create variation that's blended together. The specific values appear to be artistically tuned.
987-1005: LGTM: Skylighting integration for exterior water refractions.The skylighting integration correctly applies to exterior water (not interior as the AI summary suggests). This enhances the visual quality of underwater refractions by sampling skylighting at interpolated positions between the water surface and refracted geometry.
The color space conversions are properly handled with
GammaToLinearandLinearToGammawrappers.Note: The AI summary describing this as "interior-scene skylight integration" is incorrect.
1282-1282: LGTM: Added saturate for robust color output.Adding
saturate()to the final shader output ensures the color values are clamped to the valid [0, 1] range, preventing potential issues with out-of-range values in downstream processing.
83-85: No breaking changes to shader interface in VS_OUTPUT struct.The code snippet shows conditional compilation logic, not structural changes:
TexCoord4is alreadyfloat2on line 93 (not changed fromfloat)FogParamis conditionally removed whenUNIFIED_WATERis defined, with fog computation moved to the pixel shader using theFogParamcbuffer instead (lines 1222–1223)These are intentional variant behaviors controlled by compile-time flags. Water.hlsl's VS_OUTPUT struct is not referenced by other shaders; each shader manages its own vertex output structure independently.
1069-1072: Unified water correctly maintains full normal detail at all distances. ThedistanceBlendFactor = 1.0foverride is intentional: unified water uses a single LOD0 tile, eliminating the need to fade normal quality with distance. This differs from vanilla water's multi-LOD approach where normals fade out at distance to match geometry simplification. The design is consistent with unified water's special handling throughout the shader (UV mapping, geometry simplification, and fog removal), so no change is needed here.
563-574: The manual mip level calculation and flow vector scaling are intentionally coupled for correctness.The code uses
SampleLevelwith explicit mip calculation rather thanSampleBiasbecause the flow vector magnitude must be scaled by the same mip level to prevent animation artifacts at different texture scales. Theddx/ddyderivatives inform the mip selection, andexp2(-mipLevel)scales both the flow vector and UV adjustments proportionally, ensuring visual consistency across the texture hierarchy. This pattern is necessary in branching code where automatic derivatives cannot be reliably computed.The mip level clamp to 5 (range [0, 5]) assumes a standard texture mip hierarchy of 6 levels and is consistent with similar calculations in the codebase (see
ScreenSpaceGI/gi.cs.hlsl). This is reasonable for typical flowmap texture dimensions.The function's existing documentation could clarify this architecture: consider adding a note explaining that
mipScalemust match the sampled mip level to maintain flow animation coherence.
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Comments need to be resolved and @sicsix needs to approve this since it's his code.
Was there a test build with this?
Also if we need to release, who's handling the mod page? We already have unreleased mods for @sicsix that we get regular questions about. If @sicsix doesn't want to release, let's figure out how we handle it in the future. We are actively being forked and unreleased features are likely to get released by random people if we don't do something.
|
Yes, there is a test build like 2 builds ago in cs-testing. Whilst writing the code from scratch would have been outside my abilities, I believe I understand it enough to provide the support in his stead and I will handle the mod page. |
|
I'm away for a month now and won't be able to do any testing but will have time to do blind coding if necessary. |
I will handle mod page. Yet to discuss specific numbers with @sicsix but likely 90/10 DP split or something with him being able to manage the page as well. I will do art/page writeup. I would like to get this onto nexus sometime January/Feb before I start work up again. I will discuss privately with @sicsix in relation to existing features (interior sun) that aren't released. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Features/UnifiedWater/WaterCache.cpp (3)
227-248: Captured raw pointereditorIDmay become dangling during async execution.The lambda at line 235 captures
editorIDby value, which is aconst char*pointing into game memory (worldSpace->editorID.c_str()). If the worldspace or its editor ID string is modified or freed during async execution, this will cause undefined behavior.Apply this diff to capture a safe copy of the editor ID:
- const char* editorID = worldSpace && worldSpace->editorID.c_str() ? worldSpace->editorID.c_str() : ""; - if (!worldSpace || !editorID || !*editorID) { + std::string editorIDStr = worldSpace && worldSpace->editorID.c_str() ? worldSpace->editorID.c_str() : ""; + if (!worldSpace || editorIDStr.empty()) { logger::warn("[Unified Water] [Cache] WorldSpace has no EditorID - skipping"); buildProgress.Done(true); continue; } - async.pool->push_task([this, worldSpace, editorID] { + async.pool->push_task([this, worldSpace, editorID = std::move(editorIDStr)] {
363-368: Potential null pointer dereference forworldSpace->worldWater.At line 365,
worldSpace->worldWater->formIDis accessed without checking ifworldWateris null. This will crash for custom worldspaces that don't have a default world water defined.Apply this diff to add a null check:
if (waterHeight > landHeight && fabs(waterHeight) < 50000.0f) { - if (!formID) + if (!formID && worldSpace->worldWater) formID = worldSpace->worldWater->formID; // Use default world water if no water form set } else formID = 0;
627-677: Resource leak and null pointer dereference inTryGetCellData.Multiple critical issues:
Line 651:
file->GetRuntimeFormID(outFormID)is called without checking iffileis null. When the first loop (lines 642-649) exhausts all files without finding a match,filebecomes null at line 648's final iteration, causing a crash.Lines 637, 648, 661: Each call to
Duplicate()returns a new pointer, but when the loop continues without breaking, the previous pointer is overwritten without cleanup, causing resource leaks.Restructure to track and clean up duplicated files:
bool WaterCache::TryGetCellData(RE::TESWorldSpace* worldSpace, RE::TESFileArray* files, const int32_t x, const int32_t y, RE::FormID& outFormID, float& outWaterHeight, float& outLandHeight, bool resolveFormID) { outFormID = 0; outWaterHeight = FLT_MAX; outLandHeight = FLT_MAX; const auto size = static_cast<int32_t>(files->size()); const auto arrayData = files->data(); int32_t fileIndex = size - 1; - RE::TESFile* file = arrayData[fileIndex]->Duplicate(); + RE::TESFile* file = nullptr; bool foundWaterData = false; bool foundLandData = false; // Search through the files in reverse load order to find the cell and read the water height and waterForm FormID do { + file = arrayData[fileIndex]->Duplicate(); if (file && file->SeekCell(worldSpace, x, y)) { ReadWaterData(file, outWaterHeight, outFormID); foundWaterData = true; break; } - file = --fileIndex >= 0 ? arrayData[fileIndex]->Duplicate() : nullptr; + --fileIndex; } while (fileIndex >= 0); - if (resolveFormID && outFormID) + if (resolveFormID && outFormID && file) outFormID = file->GetRuntimeFormID(outFormID); // Continue searching from the previous file to find the original record for the cell, this always has the landscape data - extract land height do { + file = fileIndex >= 0 ? arrayData[fileIndex]->Duplicate() : nullptr; if (file && file->SeekCell(worldSpace, x, y) && file->SeekLandscapeForCurrentCell()) { ReadMinLandHeightData(file, outLandHeight); foundLandData = true; break; } - file = --fileIndex >= 0 ? arrayData[fileIndex]->Duplicate() : nullptr; + --fileIndex; } while (fileIndex >= 0); if (!foundWaterData || !foundLandData) { outFormID = 0; outWaterHeight = FLT_MIN; outLandHeight = 0; return false; } if (outWaterHeight == FLT_MAX) outWaterHeight = worldSpace->defaultWaterHeight; if (outLandHeight == FLT_MAX) outLandHeight = worldSpace->defaultLandHeight; return foundWaterData && foundLandData; }package/Shaders/Water.hlsl (1)
1326-1335: Same fog exponent concern as VC path.The non-VC path also uses
FresnelRI.yas the fog power exponent (line 1333), which should be consistent with the VC path concern noted above.
🧹 Nitpick comments (1)
package/Shaders/Water.hlsl (1)
786-804: Code duplication for LOD water normal blending.The LOD path now duplicates the non-LOD normal blending logic (lines 761-784) to achieve unified water appearance. While this creates ~20 lines of near-identical code, the preprocessor complexity with
WATER_PARALLAXandFLOWMAPbranches makes abstraction difficult.Consider extracting the shared normal blending logic into a helper function or macro if future changes require synchronizing both paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Shaders/Water.hlsl(14 hunks)src/Features/UnifiedWater/WaterCache.cpp(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/Water.hlslsrc/Features/UnifiedWater/WaterCache.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Water.hlslsrc/Features/UnifiedWater/WaterCache.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/UnifiedWater/WaterCache.cpp
🧠 Learnings (10)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
package/Shaders/Water.hlslsrc/Features/UnifiedWater/WaterCache.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (27)
src/Features/UnifiedWater/WaterCache.cpp (16)
5-40: LGTM!The parent world resolution logic and atomic cache map access are correctly implemented with proper memory ordering.
42-50: LGTM!The function safely delegates to the runtime cache after ensuring the world space is set.
52-75: LGTM!The LOD index calculation and bounds checking are correctly implemented with appropriate validation at each step.
77-102: LGTM!The developer utility correctly generates a precache for Tamriel with appropriate bounds and error handling.
104-112: LGTM!The fallback logic from loading to regenerating caches is straightforward and correct.
114-141: LGTM!The cache regeneration logic properly uses error-code-based filesystem operations for robust error handling.
143-196: LGTM!The cache loading logic correctly uses atomic operations for thread-safe cache map updates and properly restores the current cache context.
272-290: LGTM!The precache building logic correctly populates height data for all cells within the world space bounds.
405-544: LGTM!The instruction generation algorithm correctly groups contiguous cells with matching properties into larger squares, with appropriate bounds checking and processed-cell tracking.
546-598: LGTM!The runtime cache construction properly resolves form pointers, validates them, and ensures water forms are initialized before use.
600-619: LGTM!The world space validation logic correctly resolves parent worlds, filters by water presence, and deduplicates entries.
621-625: LGTM!The LOD coordinate calculation correctly handles both positive and negative coordinates with proper floor division.
679-694: LGTM!The water data reading correctly handles endianness conversion using
std::bit_castand byte swapping.
696-729: LGTM!The VHGT parsing correctly handles endianness and computes the minimum land height from the offset and delta array.
731-751: LGTM!The templated write function correctly serializes the header and vector data with appropriate error handling.
753-782: LGTM!The templated read function correctly deserializes the cache file with proper header validation and error handling.
package/Shaders/Water.hlsl (11)
83-94: LGTM: VS_OUTPUT struct changes are consistent with UNIFIED_WATER feature.The conditional exclusion of
FogParamwhenUNIFIED_WATERis defined aligns with moving fog computation to the pixel shader. TheTexCoord4change fromfloattofloat2correctly accommodates storing both dimensions fromObjectUV.xy.
192-197: LGTM: Fog calculation correctly gated for UNIFIED_WATER.When
UNIFIED_WATERis defined, fog computation is deferred to the pixel shader, so omitting the vertex shader fog calculation is correct and consistent with theVS_OUTPUTstruct changes.
218-238: LGTM: UV coordinate handling for UNIFIED_WATER.The
cellShiftandscaledUVcalculations provide proper handling for multi-cell water tiles in the unified water system.
265-287: LGTM: Texture coordinate handling for UNIFIED_WATER paths.The wading and flowmap texture coordinate calculations correctly use the new scaled UV system for
UNIFIED_WATER, while preserving the original paths. TheTexCoord4assignment ofObjectUV.xyproperly stores flowmap dimensions for pixel shader consumption.
608-618: LGTM: Improved mip level calculation for flowmap normals.The new mip-clamped sampling approach with flow vector scaling is a well-designed improvement that preserves detail at distance while preventing over-blurring. Clamping the mip level to
[0, 5]and compensating the flow vector withexp2(-mipLevel)maintains visual consistency across viewing distances.
1072-1090: LGTM: Skylighting integration for exterior water refractions.The interior guard correctly prevents skylighting from affecting indoor water. The skylighting diffuse contribution to refraction color improves outdoor water lighting fidelity.
1154-1157: LGTM: Clean introduction ofdistanceBlendFactor.The
distanceBlendFactorvariable provides a clean control point for unified water behavior. Setting it to1.0whenUNIFIED_WATERis defined ensures consistent water appearance at all distances.
1367-1367: LGTM: Defensive saturate on final color output.Adding
saturate()to the final color output is good practice to ensure values are clamped to[0, 1]before being written to the render target, preventing potential overflow artifacts from cumulative lighting calculations.
949-959: Minor preprocessor formatting adjustments.These are whitespace alignment changes with no functional impact.
1201-1201: Consistent use ofdistanceBlendFactor.Using
distanceBlendFactorfor the normal calculation ensures unified water appearance at all distances whenUNIFIED_WATERis defined.
265-285: Previous undefined variable issue appears resolved.The past review flagged undefined variables (
normalizedWaveDirVS,waveDirForwardVS, etc.) in this region. The current code no longer references these variables, indicating the issue has been addressed.
davo0411
moved this from Priority Issues
to In review
in Community Shaders Development Tracker
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Globals.h (1)
208-215: Remove the duplicateglobals::game::tesdeclaration.
extern RE::TES* tes;is declared twice in the same namespace block (Line 208 and Line 214). It won’t typically break compilation, but it’s easy to accidentally make them diverge later and it adds noise.Proposed fix
namespace game { extern RE::BSGraphics::RendererShadowState* shadowState; extern RE::BSGraphics::State* graphicsState; extern RE::BSGraphics::Renderer* renderer; extern RE::BSShaderManager::State* smState; extern RE::TES* tes; extern bool isVR; extern RE::MemoryManager* memoryManager; extern RE::INISettingCollection* iniSettingCollection; extern RE::INIPrefSettingCollection* iniPrefSettingCollection; extern RE::GameSettingCollection* gameSettingCollection; - extern RE::TES* tes; extern float* cameraNear; extern float* cameraFar; extern float* deltaTime; extern RE::BSUtilityShader* utilityShader; extern RE::Sky* sky; extern RE::UI* ui; }
🧹 Nitpick comments (1)
package/Shaders/Water.hlsl (1)
609-618: Mip selection viaddx/ddy(uv)is fragile unlessuvis in normalized texture space. Consider using a texture-driven LOD helper.If
uvis not normalized to the sampled texture (or is scaled by world/cell factors),log2(max(dot(dx,dx), dot(dy,dy)))can drift and cause over/under-blurring across different flowmap resolutions.If shader model/support allows, a more robust option is to let the hardware compute LOD based on the final sampling UV (or compute in texel space using actual dimensions).
(Also: the new
scaledFlowVectorapproach is non-obvious—worth verifying it doesn’t unintentionally slow flow animation at distance beyond what you want.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package/Shaders/Water.hlslsrc/Feature.cppsrc/Globals.cppsrc/Globals.hsrc/ShaderCache.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ShaderCache.h
- src/Globals.cpp
- src/Feature.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Globals.hpackage/Shaders/Water.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/Globals.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Globals.hpackage/Shaders/Water.hlsl
🧠 Learnings (13)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Globals.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
src/Globals.h
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
package/Shaders/Water.hlsl
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
package/Shaders/Water.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: Run Shader Unit Tests
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (9)
src/Globals.h (2)
24-24: UnifiedWater global registration looks consistent with the feature pattern.Adding
struct UnifiedWater;and exposingglobals::features::unifiedWatermatches the established “register new features inglobals::features” convention. Based on learnings, this is the right place to wire it.Also applies to: 78-78
241-246: The RTTI relocationsBSWaterShaderPropertyRTTIandNiSourceTextureRTTIare already properly initialized insrc/Globals.cpp::ReInit()(lines 186-187) using the project's runtime relocation pattern (REL::Relocation<const RE::NiRTTI*>). All usages in the codebase include appropriate null checks and error handling before casting (e.g.,src/Features/UnifiedWater/Flowmap.cpp:127includes logging and graceful failure). No action required.package/Shaders/Water.hlsl (7)
193-197: Confirm fog-distance math is in the intended space (clip vs view) and matches prior behavior.
fogDistanceFactoruseslength(worldViewPos.xyz)whereworldViewPosis derived fromWorldViewProj. If that’s truly clip-space here, fog will be projection-dependent. If it’s actually view-space in this shader’s convention, it’s fine—but worth double-checking because this factor drives both fog color lerp and alpha.
218-238: UNIFIED_WATER UV remap: please sanity-check edge cases forObjectUV.zandObjectUV.xtoggling.The
scaledUV/cellShiftpath changes how scrolling is computed whenObjectUV.xis set. This is the kind of change that can look correct for most cells but misalign on cell boundaries (especially with negative coords / wrap). I’d specifically validate:
- transitions across adjacent tiles (no seams),
- behavior when
ObjectUV.zis 0/1 (or other sentinel values),- VR eye consistency (since these values are per-geometry packed).
265-287:TexCoord4asObjectUV.xy(dimensions) is good, but guard against zero/invalid dims.UNIFIED_WATER now divides by
ObjectUV.xy/dims. If any path can produce 0 (or denorm) dimensions, you’ll get INF/NaN UVs that can cascade into derivatives and mip selection.If the data contract guarantees non-zero, documenting that expectation near the
ObjectUVpacking site (C++ mesh/geom setup) would help prevent future regressions.
1053-1072: Skylighting refraction diffuse multiply: verify color space expectations ofColor::Water(...).You’re doing
GammaToLinear(refractionDiffuseColor) * refractionDiffuseColorSkylightthenLinearToGamma(...), which is correct ifrefractionDiffuseColoris gamma-encoded at that point. IfColor::Water()is already linear (or conditionally linear under linear-lighting permutations), this could become a double conversion.
1136-1139:distanceBlendFactor = 1for UNIFIED_WATER: confirm this is intentional for all effects (normals/specular/fog blending).This forces the “near” water path everywhere Unified Water is enabled (since it’s passed into
GetWaterNormaland used in specularFraction lerps). If the intent is “always LOD0 behavior,” this matches—but it’s a broad lever, so it’s worth confirming it doesn’t break expected distance fadeouts (especially with wetness/debug effects).Also applies to: 1183-1183
1283-1329: Unified-water fog path is now PS-computed: please check continuity vs non-unified fog and parameter consistency.You’re now computing
fogDistanceFactorin PS underUNIFIED_WATERusinginput.WPosition.w,FogParam, andFresnelRI.y, whereas the non-unified path uses VS-computed fog (andNormalsScale.was the exponent). If this is meant to match the old look, verify:
- same fog curve as before (or consciously updated),
- no discontinuity toggling Unified Water on/off,
- no interior leakage (you already gate some interior paths, but this fog is applied broadly).
81-87: Verify allinput.FogParamusages are gated under!defined(UNIFIED_WATER)to avoid hard-fail compilation.This approach removes
FogParamfrom the VS→PS interface whenUNIFIED_WATERis defined to support a different fog model, but will fail compilation if anyinput.FogParamusage is outside!defined(UNIFIED_WATER)guards. Confirm all accesses are properly protected.
|
@sicsix please formally approve this pr |
4 participants
adds a comprehensive water LOD fix, replacing distant water tiles with LOD0 water.
Completely rewritten flow map systems.
99% is @sicsix 's work, however I implemented a few shader/crash fixes.
Unchanged .cpp from older test builds.
Summary by CodeRabbit
New Features
Utilities
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.