fix(unified water): stabilize LOD water during transitions#2391
Open
FIocker wants to merge 1 commit into
Open
Conversation
Keep generated Unified Water tiles under the feature-managed LOD parent after terrain attach instead of routing them through vanilla LOD-water ownership. This keeps the later cleanup around duplicate water entries and safer block rebuilding, while avoiding state churn that can leave close water and generated LOD water fighting after cell and worldspace transitions. Restore the child-worldspace cull retry path so already-attached LOD blocks are culled once their replacement cells finish attaching. Also keep water shader planes refreshed before draw, preserve uncovered water subtrees when rebuilding a terrain block, and ignore placed water overlays when pruning duplicate water-system objects.
Contributor
📝 WalkthroughWalkthroughUnifiedWater refactors water culling to defer child-worldspace transitions using atomic state, grid-aware cell testing, and terrain-block attachment rewrites. New hooks ( ChangesDeferred Culling and Worldspace Transitions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.21.0)OpenGrep fatal error (exit code 2): [00.14][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/UnifiedWater.h 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 425-485: The whitelist currently keys by BSTriShape*
(s_placedWaterWhitelist) causing stale entries and memory-growth; change the
whitelist to key by the reference identity returned by FindRefForWaterGeometry
(e.g., store RE::TESObjectREFR* or better RE::FormID) and update
IsPlacedWaterWhitelisted and TryWhitelistPlacedWaterOverlay to use that
ref/formID instead of the raw shape pointer; specifically: change
s_placedWaterWhitelist's type, have TryWhitelistPlacedWaterOverlay call
FindRefForWaterGeometry and insert the ref's identifier (after
IsPlacedWaterCandidate check) under s_placedWaterMutex, and have
IsPlacedWaterWhitelisted lookup by the same ref identifier (or return false when
no ref found); keep IsPlacedWaterCandidate and FindRefForWaterGeometry as-is and
adjust locking/unlocking points accordingly to prevent the stale-pointer and
monotonic-growth issues.
- Around line 554-568: The deferred cull flag pendingChildWsCull is only cleared
when CullAllWaterLODParents(...) returns true, causing
TryCompleteDeferredChildWorldspaceCull to spin forever; modify
TryCompleteDeferredChildWorldspaceCull (and/or CullAllWaterLODParents) to also
clear pendingChildWsCull when there are no remaining in-grid candidates or when
a transition timeout expires: add a check after calling CullAllWaterLODParents
to detect "nothing left to cull" (e.g., zero returned candidates or an exposed
boolean from CullAllWaterLODParents) and store(false) on pendingChildWsCull, and
additionally track/reset a transitionCullFrames counter (decrement per call and
clear pendingChildWsCull when it reaches zero) so the pending state is cleared
after the transitionCullFrames timeout even if CullAllWaterLODParents never
returns success; use currentPlayerWorldSpace, cachedTes and gWaterLOD as
existing context to locate the logic to change.
- Around line 993-1059: Preserve-only water blocks never set
block->waterAttached so they hit the early return and never get reattached;
after calling PreserveUncoveredWaterSubtrees (and/or before the early return
that checks attaching and block->waterAttached) detect if any rescued.root or
any rescued.shapes contains a valid shape+form and, if so, set
block->waterAttached = true (and ensure attaching remains true) so the
subsequent loop that re-registers preserved shapes and reattaches block->water
under uw.gWaterLOD runs; update the logic around PreserveUncoveredWaterSubtrees,
preserved, block->waterAttached and the early return accordingly.
- Around line 802-803: The detour registrations for TES_SetWorldSpace and
TES_DestroySkyCell use REL::RelocationID pairs but do not provide VR relocations
while SupportsVR() returns true, so the hooks will fail on VR; fix by either (A)
switching these registrations to use REL::VariantID and supply the VR offsets
(matching the pattern used in Hooks.cpp and WetnessEffects.cpp) for
TES_SetWorldSpace and TES_DestroySkyCell, or (B) guard the detour_thunk calls
behind a runtime check that skips them when running in VR (or disable
SupportsVR() if full VR support is not provided). Update the calls referencing
TES_SetWorldSpace, TES_DestroySkyCell, and any related installation logic to use
VariantID or conditional installation so VR runs do not attempt to install
non-existent relocations.
🪄 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: ccfe0ba6-1537-4fb2-bb52-8c1d0bffa6cb
📒 Files selected for processing (2)
src/Features/UnifiedWater.cppsrc/Features/UnifiedWater.h
Comment on lines
+425
to
485
| static std::shared_mutex s_placedWaterMutex; | ||
| static std::unordered_set<RE::BSTriShape*> s_placedWaterWhitelist; | ||
|
|
||
| if (!attaching || !block->waterAttached) | ||
| static bool IsTemporaryFormID(RE::FormID formID) | ||
| { | ||
| return (formID & 0xFF000000) == 0xFF000000; | ||
| } | ||
|
|
||
| static bool IsPlacedWaterCandidate(const RE::TESObjectREFR* ref) | ||
| { | ||
| const auto base = ref ? ref->GetBaseObject() : nullptr; | ||
| if (!base) | ||
| return false; | ||
|
|
||
| for (auto& [shape, instruction] : built) { | ||
| AddLODWater(waterSystem, shape, worldSpace, *gWaterLOD, block->water); | ||
| // Procedural close water can resolve through FindReferenceFor3D as an Activator | ||
| // with temporary FF... forms. Those are real replacement water surfaces and must | ||
| // still be allowed to cull UW LOD, otherwise interior/exterior flicker returns. | ||
| if (IsTemporaryFormID(ref->formID) || IsTemporaryFormID(base->formID)) | ||
| return false; | ||
|
|
||
| if (const auto prop = shape->GetGeometryRuntimeData().shaderProperty.get(); prop && prop->GetRTTI() == globals::rtti::BSWaterShaderPropertyRTTI.get()) { | ||
| const auto waterShaderProp = static_cast<RE::BSWaterShaderProperty*>(prop); | ||
| REX::EnumSet waterFlags = static_cast<RE::BSWaterShaderProperty::WaterFlag>(0b10000100); | ||
| waterFlags |= RE::BSWaterShaderProperty::WaterFlag::kUseCubemapReflections; | ||
| waterFlags |= RE::BSWaterShaderProperty::WaterFlag::kUseReflections; | ||
| if (instruction->form.ptr->flags.any(RE::TESWaterForm::Flag::kEnableFlowmap)) | ||
| waterFlags |= RE::BSWaterShaderProperty::WaterFlag::kEnableFlowmap; | ||
| if (instruction->form.ptr->flags.any(RE::TESWaterForm::Flag::kBlendNormals)) | ||
| waterFlags |= RE::BSWaterShaderProperty::WaterFlag::kBlendNormals; | ||
| waterShaderProp->waterFlags = waterFlags; | ||
| } | ||
| const auto formType = base->GetFormType(); | ||
| return formType == RE::FormType::Activator || formType == RE::FormType::Static; | ||
| } | ||
|
|
||
| static bool IsPlacedWaterWhitelisted(const RE::BSTriShape* shape) | ||
| { | ||
| if (!shape) | ||
| return false; | ||
|
|
||
| // Vanilla AddLODWater routes through TESWaterSystem::AddWater and attaches | ||
| // the water parent to gWaterLOD. Use the matching vanilla LOD remove wrapper | ||
| // to unwind the water-system side state, then reattach the parent once below. | ||
| RemoveLODWater(waterSystem, shape, *gWaterLOD); | ||
| std::shared_lock lock(s_placedWaterMutex); | ||
| return s_placedWaterWhitelist.contains(const_cast<RE::BSTriShape*>(shape)); | ||
| } | ||
|
|
||
| static RE::TESObjectREFR* FindRefForWaterGeometry(RE::NiAVObject* object) | ||
| { | ||
| for (auto current = object; current; current = current->parent) { | ||
| if (const auto ref = RE::TESObjectREFR::FindReferenceFor3D(current)) | ||
| return ref; | ||
| } | ||
|
|
||
| RemoveDuplicateWaterSystemObjects(waterSystem, *gWaterLOD); | ||
| DetachAllChildOccurrences(*gWaterLOD, block->water); | ||
| (*gWaterLOD)->AttachChild(block->water, true); | ||
| waterSystem->Enable(); | ||
| return nullptr; | ||
| } | ||
|
|
||
| static bool TryWhitelistPlacedWaterOverlay(RE::BSTriShape* shape, const char* source) | ||
| { | ||
| if (!shape) | ||
| return false; | ||
|
|
||
| if (IsPlacedWaterWhitelisted(shape)) | ||
| return true; | ||
|
|
||
| const auto ref = FindRefForWaterGeometry(shape); | ||
| if (!ref || !IsPlacedWaterCandidate(ref)) | ||
| return false; | ||
|
|
||
| (void)source; | ||
| std::unique_lock lock(s_placedWaterMutex); | ||
| s_placedWaterWhitelist.insert(shape); | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Key the placed-water whitelist by ref identity, not BSTriShape*.
s_placedWaterWhitelist only ever inserts raw shape pointers and never removes them. After enough streaming churn, allocator reuse can make an unrelated water shape inherit an old whitelist entry, which will incorrectly skip duplicate pruning and HasRegisteredWaterObjectInCell() replacement checks. The set also grows monotonically for the lifetime of the process.
🤖 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 425 - 485, The whitelist
currently keys by BSTriShape* (s_placedWaterWhitelist) causing stale entries and
memory-growth; change the whitelist to key by the reference identity returned by
FindRefForWaterGeometry (e.g., store RE::TESObjectREFR* or better RE::FormID)
and update IsPlacedWaterWhitelisted and TryWhitelistPlacedWaterOverlay to use
that ref/formID instead of the raw shape pointer; specifically: change
s_placedWaterWhitelist's type, have TryWhitelistPlacedWaterOverlay call
FindRefForWaterGeometry and insert the ref's identifier (after
IsPlacedWaterCandidate check) under s_placedWaterMutex, and have
IsPlacedWaterWhitelisted lookup by the same ref identifier (or return false when
no ref found); keep IsPlacedWaterCandidate and FindRefForWaterGeometry as-is and
adjust locking/unlocking points accordingly to prevent the stale-pointer and
monotonic-growth issues.
Comment on lines
+554
to
+568
| void UnifiedWater::TryCompleteDeferredChildWorldspaceCull(RE::TES* tes) | ||
| { | ||
| if (!pendingChildWsCull.load(std::memory_order_acquire) || | ||
| !IsChildWorldSpace(currentPlayerWorldSpace.load(std::memory_order_acquire)) || | ||
| !gWaterLOD || !*gWaterLOD) | ||
| return; | ||
|
|
||
| if (!tes) | ||
| tes = cachedTes.load(std::memory_order_acquire); | ||
| if (!tes || !tes->gridCells) | ||
| return; | ||
|
|
||
| if (CullAllWaterLODParents(*gWaterLOD, tes)) | ||
| pendingChildWsCull.store(false, std::memory_order_release); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t leave deferred child-worldspace culling latched forever.
pendingChildWsCull only resets when CullAllWaterLODParents() reports success. In the “nothing stale left to cull” case, that never happens, so TryCompleteDeferredChildWorldspaceCull() keeps walking every LOD parent on each displacement update for the rest of the child-worldspace session. Please tie completion to transitionCullFrames or clear the pending state once no in-grid candidates remain.
🤖 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 554 - 568, The deferred cull flag
pendingChildWsCull is only cleared when CullAllWaterLODParents(...) returns
true, causing TryCompleteDeferredChildWorldspaceCull to spin forever; modify
TryCompleteDeferredChildWorldspaceCull (and/or CullAllWaterLODParents) to also
clear pendingChildWsCull when there are no remaining in-grid candidates or when
a transition timeout expires: add a check after calling CullAllWaterLODParents
to detect "nothing left to cull" (e.g., zero returned candidates or an exposed
boolean from CullAllWaterLODParents) and store(false) on pendingChildWsCull, and
additionally track/reset a transitionCullFrames counter (decrement per call and
clear pendingChildWsCull when it reaches zero) so the pending state is cleared
after the transitionCullFrames timeout even if CullAllWaterLODParents never
returns success; use currentPlayerWorldSpace, cachedTes and gWaterLOD as
existing context to locate the logic to change.
Comment on lines
+802
to
+803
| stl::detour_thunk<TES_SetWorldSpace>(REL::RelocationID(13170, 13315)); | ||
| stl::detour_thunk<TES_DestroySkyCell>(REL::RelocationID(20029, 20463)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'UnifiedWater worldspace hook sites:\n'
rg -n -C2 'TES_SetWorldSpace|TES_DestroySkyCell' src/Features/UnifiedWater.h src/Features/UnifiedWater.cpp
printf '\nExisting VR-aware relocation patterns in the repo:\n'
rg -n -C2 'VariantID|RelocateMember|Module::IsVR|Offset::get\(\)' srcRepository: community-shaders/skyrim-community-shaders
Length of output: 28919
🏁 Script executed:
# Check SupportsVR for UnifiedWater
rg -n 'class UnifiedWater|SupportsVR' src/Features/UnifiedWater.h src/Features/UnifiedWater.cpp | head -30
# Look for VR relocation IDs for these functions
rg -n 'TES_SetWorldSpace|TES_DestroySkyCell' src --type-list | head -20
# Find similar detour patterns with VariantID for VR
rg -n 'stl::detour_thunk.*VariantID' srcRepository: community-shaders/skyrim-community-shaders
Length of output: 578
🏁 Script executed:
# Check if there are other VR-aware detours in Features
rg -n 'stl::detour_thunk.*VariantID|stl::detour_thunk.*IsVR' src/FeaturesRepository: community-shaders/skyrim-community-shaders
Length of output: 68
🏁 Script executed:
# Look at the full UnifiedWater class definition including SupportsVR
sed -n '1,100p' src/Features/UnifiedWater.h | cat -nRepository: community-shaders/skyrim-community-shaders
Length of output: 3947
🏁 Script executed:
# Search for VR address library patterns or relocation data
find . -name "*.json" -o -name "*offset*" -o -name "*relocation*" 2>/dev/null | head -20
# Check if there are conditional detours based on IsVR in UnifiedWater or similar features
rg -n 'IsVR.*detour|detour.*IsVR' src/Features
# Look at how other features handle worldspace-related hooks with VR
rg -n 'TESWorldSpace|worldSpace' src/Features --type cpp | head -20Repository: community-shaders/skyrim-community-shaders
Length of output: 2533
🏁 Script executed:
# Search across the entire codebase for these specific function names to see if there are VR variants
rg -n 'SetWorldSpace|DestroySkyCell' src -A 3 -B 1 | head -50Repository: community-shaders/skyrim-community-shaders
Length of output: 2329
🏁 Script executed:
# Check if there's conditional installation or if hooks should branch by IsVR
sed -n '800,830p' src/Features/UnifiedWater.cpp | cat -n
# Look for address library or VR-specific IDs in dependencies
find . -name "*.cpp" -o -name "*.h" | xargs grep -l "address.*library\|AddressLibrary" | head -5Repository: community-shaders/skyrim-community-shaders
Length of output: 2015
🏁 Script executed:
# Check the broader context - are these hooks supposed to be installed conditionally?
sed -n '795,810p' src/Features/UnifiedWater.cpp | cat -n
# Search for similar patterns where RelocationID is used for hooks that support VR
rg -n 'SupportsVR.*true' src/Features/*.h -A 50 | grep -A 20 'PostPostLoad\|detour' | head -40Repository: community-shaders/skyrim-community-shaders
Length of output: 3646
🏁 Script executed:
# Look for address library version checking or VR ID patterns
rg -n 'VariantID|VariantOffset' src --type cpp -B 2 -A 2 | head -60
# Check if there are any hooks that conditionally install based on IsVR
rg -n 'if.*IsVR.*detour|if.*IsVR.*write_thunk' src --type cppRepository: community-shaders/skyrim-community-shaders
Length of output: 922
🏁 Script executed:
# Search for how the codebase typically handles detours with VR - look for patterns
sed -n '920,940p' src/Features/UnifiedWater.cpp | cat -n
# Check if there are similar worldspace/TES hooks elsewhere with VR handling
rg -n 'TES.*thunk|worldSpace.*thunk' src/Features --type cpp -B 5 -A 5 | head -50Repository: community-shaders/skyrim-community-shaders
Length of output: 4180
🏁 Script executed:
# Check if there are conditional VR branches or workarounds for these hooks elsewhere
rg -n 'TES_SetWorldSpace|TES_DestroySkyCell' src --type cpp -B 5 -A 5
# Look for out-of-band VR handling in UnifiedWater
grep -n 'IsVR\|VariantID' src/Features/UnifiedWater.cpp src/Features/UnifiedWater.hRepository: community-shaders/skyrim-community-shaders
Length of output: 4343
🏁 Script executed:
# Check if there's any setup code checking for VR before these detours
sed -n '790,850p' src/Features/UnifiedWater.cpp | cat -nRepository: community-shaders/skyrim-community-shaders
Length of output: 3787
Add VR relocation handling for the new worldspace hooks or disable VR support.
Lines 802-803 register detours with REL::RelocationID(SE, AE) pairs, but SupportsVR() returns true. These hooks will fail to install on VR since no VR address IDs are provided. The codebase supports REL::VariantID with VR offsets (e.g., Hooks.cpp:928, WetnessEffects.cpp:234), but this feature's worldspace hooks lack them. Either provide VR relocation IDs using VariantID, conditionally skip these hooks on VR if they're non-essential, or update SupportsVR() to reflect actual VR compatibility.
🤖 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 802 - 803, The detour
registrations for TES_SetWorldSpace and TES_DestroySkyCell use REL::RelocationID
pairs but do not provide VR relocations while SupportsVR() returns true, so the
hooks will fail on VR; fix by either (A) switching these registrations to use
REL::VariantID and supply the VR offsets (matching the pattern used in Hooks.cpp
and WetnessEffects.cpp) for TES_SetWorldSpace and TES_DestroySkyCell, or (B)
guard the detour_thunk calls behind a runtime check that skips them when running
in VR (or disable SupportsVR() if full VR support is not provided). Update the
calls referencing TES_SetWorldSpace, TES_DestroySkyCell, and any related
installation logic to use VariantID or conditional installation so VR runs do
not attempt to install non-existent relocations.
Comment on lines
+993
to
+1059
| PreserveUncoveredWaterSubtrees(water, waterSystem, *instructions, worldSpace, preserved); | ||
| ClearWaterNodeChildren(water, waterSystem); | ||
| block->waterAttached = false; | ||
| for (auto& rescued : preserved) { | ||
| if (rescued.root) { | ||
| water->AttachChild(rescued.root.get(), true); | ||
| SetSubtreeAppCulled(rescued.root.get(), false); | ||
| } | ||
| } | ||
|
|
||
| attaching = true; | ||
|
|
||
| for (auto& instruction : *instructions) { | ||
| if (!instruction.form.ptr) | ||
| continue; | ||
|
|
||
| RE::NiCloningProcess cloningProcess; | ||
|
|
||
| const auto targetShape = lodLevel > 4 || uw.settings.UseOptimisedMeshes ? uw.optimisedWaterMesh : uw.waterMesh; | ||
| RE::BSTriShape* shape = targetShape->CreateClone(cloningProcess)->AsTriShape(); | ||
|
|
||
| const auto posX = (instruction.x - node->baseCellX) * 4096.0f + instruction.size * 2048.0f; | ||
| const auto posY = (instruction.y - node->baseCellY) * 4096.0f + instruction.size * 2048.0f; | ||
| shape->local.scale = static_cast<float>(instruction.size); | ||
| shape->local.translate = { posX, posY, instruction.waterHeight }; | ||
|
|
||
| water->AttachChild(shape, true); | ||
| built.emplace_back(shape, &instruction); | ||
|
|
||
| block->waterAttached = true; | ||
| } | ||
| } | ||
|
|
||
| func(block); | ||
|
|
||
| if (!attaching || !block->waterAttached) | ||
| return; | ||
|
|
||
| for (auto& [shape, instruction] : built) { | ||
| waterSystem->AddWater(shape, instruction->form.ptr, instruction->waterHeight, nullptr, true, false); | ||
|
|
||
| ApplyUnifiedWaterFlags(shape, instruction->form.ptr); | ||
|
|
||
| // Remove from WaterSystem, will manage it ourselves | ||
| if (!waterSystem->waterObjects.empty()) { | ||
| waterSystem->waterObjects.pop_back(); | ||
| } | ||
| } | ||
| for (auto& rescued : preserved) { | ||
| if (rescued.root) | ||
| SetSubtreeAppCulled(rescued.root.get(), false); | ||
|
|
||
| for (auto& preservedShape : rescued.shapes) { | ||
| if (!preservedShape.shape || !preservedShape.form) | ||
| continue; | ||
|
|
||
| waterSystem->AddWater(preservedShape.shape.get(), preservedShape.form, preservedShape.waterHeight, nullptr, true, false); | ||
| ApplyUnifiedWaterFlags(preservedShape.shape.get(), preservedShape.form); | ||
| preservedShape.shape->SetAppCulled(false); | ||
| } | ||
| } | ||
|
|
||
| RemoveDuplicateWaterSystemObjects(waterSystem, *uw.gWaterLOD); | ||
| if (block && block->water) | ||
| DetachAllChildOccurrences(*uw.gWaterLOD, block->water); | ||
| (*uw.gWaterLOD)->AttachChild(block->water, true); | ||
| waterSystem->Enable(); |
Contributor
There was a problem hiding this comment.
Preserved-only water blocks still fall through the attach path.
block->waterAttached is only flipped when a new generated tile is built. If PreserveUncoveredWaterSubtrees() found rescueable water but every instruction.form.ptr is null, the early return at Line 1028 skips re-registering those preserved shapes and never reattaches block->water under gWaterLOD, so the preservation path still drops the block.
Suggested fix
func(block);
- if (!attaching || !block->waterAttached)
+ const bool hasManagedWater = !built.empty() || !preserved.empty();
+ block->waterAttached = hasManagedWater;
+ if (!attaching || !hasManagedWater)
return;🤖 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 993 - 1059, Preserve-only water
blocks never set block->waterAttached so they hit the early return and never get
reattached; after calling PreserveUncoveredWaterSubtrees (and/or before the
early return that checks attaching and block->waterAttached) detect if any
rescued.root or any rescued.shapes contains a valid shape+form and, if so, set
block->waterAttached = true (and ensure attaching remains true) so the
subsequent loop that re-registers preserved shapes and reattaches block->water
under uw.gWaterLOD runs; update the logic around PreserveUncoveredWaterSubtrees,
preserved, block->waterAttached and the early return accordingly.
|
✅ A pre-release build is available for this PR: |
Contributor
|
@FIocker please review the suggestions Coderabbit made, and action any that are valid, or for those that are invalid, you can respond with why, then mark the thread as resolved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These rewrites attempt to make Unified Water a little more conservative about how generated LOD water is attached, culled, and cleaned up during cell/worldspace transitions
The main goal is to avoid cases where close water and generated LOD water briefly disagree about ownership after streaming, which can show up as flickering water, missing LOD water, or water disappearing after entering/leaving interiors. One such example the combination of UW and using the Frogs mod from Mihail's Monsters and Animals series.
Changes:
This keeps the appearance closer to the earlier Unified Water builds behavior while carrying forward the useful stability work from the newer implementation (newer UW builds had tiling issues between UW water and LOD water)
Testing:
Remaining issues found during testing:
Summary by CodeRabbit