fix(unified-water): cell transition flicker

[Dawntic]

Fixes flicker after interior -> exterior transition by patching a 2nd path which iterates the root LOD multibound causing materials to be initialized twice.

This PR reverts #1998, #2019, #2353, #2367 and supersedes PR #2391.

Summary by CodeRabbit

• Chores
  • Refactored internal water rendering system for improved stability and efficiency.
  • Optimized water mesh management and attachment handling during world transitions.

[coderabbitai]

📝 Walkthrough

Walkthrough

UnifiedWater refactors water attachment and shader computation by introducing worldspace change hooks, inlining terrain block water generation, updating grid-based culling logic, removing a redundant global waterSystem pointer, and adjusting shader uniform and displacement calculations to use singleton flowmap state.

Changes

Unified Water Attachment and Shader Lifecycle

Layer / File(s)	Summary
Header declarations for new worldspace hooks src/Features/UnifiedWater.h, src/Features/UnifiedWater.cpp	UnifiedWater.h adds nested thunk-relocation structs for TES_SetWorldSpace and TES_DestroySkyCell, removes the BuildWaterForBlock helper declaration, and removes the unused vector include. The cpp file repositions the Settings macro in the edited header region.
Hook installation and worldspace tracking thunks src/Features/UnifiedWater.cpp	PostPostLoad installs detours for TES_SetWorldSpace and TES_DestroySkyCell using updated instruction sequences. The new thunks call original functions and update waterCache's current worldspace to track world transitions.
Water attachment and detachment lifecycle src/Features/UnifiedWater.cpp	BGSTerrainBlock_Attach::thunk inlines water generation—detaching existing water, cloning LOD meshes per cache, configuring shader flags, adding water objects, and attaching to gWaterLOD. BGSTerrainBlock_Detach::thunk detaches and clears water children and gWaterLOD references. BGSTerrainNode_UpdateWaterMeshSubVisibility::thunk performs direct grid-cell-based culling per water child.
Shader uniform and displacement calculation updates src/Features/UnifiedWater.cpp	BSWaterShader_SetupGeometry::thunk sources flowX/flowY/cellX/cellY from singleton.flowmap offsets/sizes. TESWaterSystem_UpdateDisplacementMeshPosition::thunk uses singleton locals, early-returns if no flowmap exists, and computes gDisplacementCellTexCoordOffset from current displacement position and flowmap state.
Remove redundant globals::game::waterSystem src/Globals.h, src/Globals.cpp	Removes the globals::game::waterSystem singleton pointer declaration and definition, and removes GetSingleton() initializations from ReInit() and OnDataLoaded().

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• community-shaders/skyrim-community-shaders#1763: Overlaps on UnifiedWater.cpp water-generation call sites and waterCache instruction lookups used in BGSTerrainBlock_Attach.
• community-shaders/skyrim-community-shaders#2019: Coordinates the same runtime hooks (TES_SetWorldSpace, TES_DestroySkyCell, UpdateWaterMeshSubVisibility, displacement updates) for water LOD and worldspace culling.
• community-shaders/skyrim-community-shaders#1809: Complements the shader uniform computation changes by adjusting HLSL scroll/normal scrolling logic in Water.hlsl that uses the flowX/flowY/cellX/cellY inputs.

Suggested reviewers

• alandtse
• sicsix
• SkrubbySkrubInAShrub

Poem

🐰 A water thunk blooms bright and new,
The meshes dance—attach, detach, renew!
Grid cells aligned with worldspace grace,
No globals linger in this space.
Flow and displacement—shaders sing with delight! 💦

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: addressing cell transition flicker in the unified-water system through comprehensive refactoring of water mesh handling and worldspace management.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

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/Features/UnifiedWater.cpp (1)

243-267: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don't install these hooks until Unified Water finishes initializing.

DataLoaded() can return before waterCache and the mesh pointers are created, but PostPostLoad() still registers the new worldspace/attach hooks. The next worldspace change or terrain attach then dereferences null feature state instead of failing closed. Please gate hook registration behind successful initialization, or early-out each thunk to func(...) when Unified Water is unavailable.

As per coding guidelines, src/**/*.{h,cpp}: "Include proper resource management and graceful degradation error handling in all DirectX code to prevent crashes from malformed configurations".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Features/UnifiedWater.cpp` around lines 243 - 267, PostPostLoad currently
installs detours (TES_SetWorldSpace, TES_DestroySkyCell,
BGSTerrainBlock_Attach/Detach, BGSTerrainNode_UpdateWaterMeshSubVisibility,
TESWaterSystem_InitializeWater_SetWaterShaderMaterialParams,
BSWaterShaderMaterial_ComputeCRC32) before UnifiedWater has finished creating
waterCache and mesh pointers, causing null derefs; fix by gating hook
registration in UnifiedWater::PostPostLoad behind a successful initialization
check (e.g., DataLoaded() && waterCache && mesh pointers or an IsInitialized()
method) and only call stl::detour_thunk/write_thunk_call/write_vfunc when that
check passes, or alternatively add an early-return guard in each thunk
(BGSTerrainBlock_Attach, BGSTerrainBlock_Detach, TES_SetWorldSpace,
BGSTerrainNode_UpdateWaterMeshSubVisibility, etc.) that falls back to calling
the original func when UnifiedWater is unavailable; ensure both approaches are
used where appropriate so hooks are safe if initialization hasn't completed.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 389-394: The loop is mutating water->GetChildren() while
range-iterating it, which can invalidate iteration; instead first copy the child
pointers into a separate container and then iterate that copy to call
waterSystem->RemoveWater(child.get()) and water->DetachChild(child.get()).
Locate the block that currently iterates over water->GetChildren(), build a
temporary vector (or list) of the child shared_ptr/raw pointers, then perform
removal/detach using that temporary collection to avoid mutating the original
list during iteration.

---

Outside diff comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 243-267: PostPostLoad currently installs detours
(TES_SetWorldSpace, TES_DestroySkyCell, BGSTerrainBlock_Attach/Detach,
BGSTerrainNode_UpdateWaterMeshSubVisibility,
TESWaterSystem_InitializeWater_SetWaterShaderMaterialParams,
BSWaterShaderMaterial_ComputeCRC32) before UnifiedWater has finished creating
waterCache and mesh pointers, causing null derefs; fix by gating hook
registration in UnifiedWater::PostPostLoad behind a successful initialization
check (e.g., DataLoaded() && waterCache && mesh pointers or an IsInitialized()
method) and only call stl::detour_thunk/write_thunk_call/write_vfunc when that
check passes, or alternatively add an early-return guard in each thunk
(BGSTerrainBlock_Attach, BGSTerrainBlock_Detach, TES_SetWorldSpace,
BGSTerrainNode_UpdateWaterMeshSubVisibility, etc.) that falls back to calling
the original func when UnifiedWater is unavailable; ensure both approaches are
used where appropriate so hooks are safe if initialization hasn't completed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d097637d-9359-4d94-9664-512adb7861f2

📥 Commits

Reviewing files that changed from the base of the PR and between 19483b3 and 875c512.

📒 Files selected for processing (4)

• src/Features/UnifiedWater.cpp
• src/Features/UnifiedWater.h
• src/Globals.cpp
• src/Globals.h

💤 Files with no reviewable changes (2)

• src/Globals.h
• src/Globals.cpp

[github-actions]

✅ A pre-release build is available for this PR:
Download