fix(unified-water): water lod z-fighting#2019
Conversation
|
No actionable suggestions for changed features. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds child-worldspace detection and deferred completion for water LOD-parent culling: new atomic cached TES/worldspace state, grid-cell–driven cull helpers, lifecycle thunks to set/clear deferred culls, and retry sites to complete deferred culls across thread boundaries. Changes
Sequence Diagram(s)mermaid Player->>TES: TES_SetWorldSpace::thunk (player worldspace change) Note over TerrainBlock,WaterShader,WaterSys: deferred cull completion points Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SkrubbySkrubInAShrub
force-pushed
the
uw-fix-2
branch
from
7a69178 to
689989b
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 21-54: CullWaterParentByGridCells currently returns true as soon
as it finds any attached child which causes pendingChildWsCull to be cleared too
early; instead, make the function determine whether the deferred pass is truly
complete by scanning all children and only clearing pendingChildWsCull when no
children remain that are inside the grid but currently unattached (i.e.,
children that could flip to kAttached in subsequent frames). Modify
CullWaterParentByGridCells (and the call sites that set/clear
pendingChildWsCull) to: 1) iterate all children and track two booleans —
foundAttachedCell (existing behavior) and hasPotentiallyAttachableChild (child
within grid bounds but not currently attached); 2) set child->SetAppCulled(cull)
as before; and 3) return a value that only clears pendingChildWsCull when
foundAttachedCell is true AND hasPotentiallyAttachableChild is false (so
BGSTerrainNode_UpdateWaterMeshSubVisibility or later attachments can still
trigger another pass).
- Around line 394-395: DataLoaded() may return before uw.waterCache is
allocated, so update the hooks that call uw.waterCache->SetCurrentWorldSpace
(and the other calls around the same area) to check for a valid pointer first;
in the functions handling worldspace transitions (the hooks that call
SetCurrentWorldSpace and the related calls at the later block), guard with if
(!uw.waterCache) { /* gracefully disable/return */ } and either early-return or
disable/unregister the UnifiedWater feature so a missing/invalid water mesh
doesn't cause a null-deref; reference the uw.waterCache pointer and the
DataLoaded() control flow when locating the hook code to apply this guard.
- Around line 594-600: The render-thread branch that accesses tes->gridCells and
*gWaterLOD (guarded by pendingChildWsCull, currentPlayerWorldSpace, gWaterLOD,
cachedTes and calling CullAllWaterLODParents inside BSWaterShader_SetupGeometry)
must not walk or read the TES grid or NiNode tree; move this deferred
child-world cull off the render thread and perform it on the game thread using
the existing retry point in TESWaterSystem_UpdateDisplacementMeshPosition::thunk
(or schedule a task that runs there), so do not dereference cachedTes->gridCells
or *gWaterLOD on the render thread—instead mark the work and enqueue/trigger the
game-thread worker (or run CullAllWaterLODParents from the thunk) and clear
pendingChildWsCull only after the game-thread cull completes; also avoid racing
with BGSTerrainBlock_Attach/Detach by using the game-thread context for all
scene-graph reads.
🪄 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
Run ID: 49080823-3975-4056-891a-2404f58ef704
📒 Files selected for processing (2)
src/Features/UnifiedWater.cppsrc/Features/UnifiedWater.h
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/Features/UnifiedWater.cpp (3)
579-594: Consider extracting the per-shape cull logic to reduce duplication.This block duplicates the cell-state lookup and cull logic from
CullWaterParentByGridCells. While the context is slightly different (iteratingbuiltpairs vs. node children), a small helper could unify the cell-lookup-and-cull pattern.♻️ Sketch of a shared helper
// Near the top with other static helpers static bool ShouldCullAtCell(RE::TES* tes, int32_t cellX, int32_t cellY) { if (!tes || !tes->gridCells) return false; const auto& gridCells = tes->gridCells; const int32_t offsetX = tes->currentGridX - static_cast<int32_t>(gridCells->length >> 1); const int32_t offsetY = tes->currentGridY - static_cast<int32_t>(gridCells->length >> 1); const int32_t length = static_cast<int32_t>(gridCells->length); const int32_t ix = cellX - offsetX; const int32_t iy = cellY - offsetY; if (ix >= 0 && iy >= 0 && ix < length && iy < length) { if (const auto cell = gridCells->GetCell(ix, iy); cell && cell->cellState.any( RE::TESObjectCELL::CellState::kAttached, static_cast<RE::TESObjectCELL::CellState>(6))) return true; } return false; }Then use it in both
CullWaterParentByGridCellsand theBGSTerrainBlock_Attachloop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/UnifiedWater.cpp` around lines 579 - 594, Extract the repeated grid-cell culling logic into a static helper (e.g. ShouldCullAtCell) that accepts a RE::TES* tes and cell coordinates (cellX, cellY) and returns a bool; move the offset/length/gridCells checks and the cell->cellState.any(...) check into that helper, then replace the inline logic in the BGSTerrainBlock_Attach loop (the built iteration using ix/iy and shape->SetAppCulled) and in CullWaterParentByGridCells to call ShouldCullAtCell(tes, instruction->x, instruction->y) (or the appropriate node child coordinates) and pass its result to SetAppCulled, preserving the existing condition semantics and null checks.
55-57: Document the magic cell state value.
static_cast<RE::TESObjectCELL::CellState>(6)is used here and again at line 590, but without explanation. Consider adding a brief comment or defining a named constant for clarity.📝 Suggested documentation
if (x >= 0 && y >= 0 && x < length && y < length) { const auto cell = gridCells->GetCell(x, y); + // CellState::kAttached (5) or state 6 (post-attach loaded) indicate the cell has loaded water if (cell && cell->cellState.any( RE::TESObjectCELL::CellState::kAttached, static_cast<RE::TESObjectCELL::CellState>(6))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/UnifiedWater.cpp` around lines 55 - 57, Replace the unexplained magic cast static_cast<RE::TESObjectCELL::CellState>(6) used inside the cell->cellState.any check with a named constant or enum alias (e.g., kSomeCellState or kCellStateXYZ) and add a brief comment explaining what that state represents; update both occurrences (the check in UnifiedWater.cpp and the other at the later occurrence around line 590) to use the new constant so the meaning is clear instead of using the raw value 6 while keeping the condition logic unchanged.
622-627: Consider adding a brief comment explaining the acceptable data race ongridCells.The underlying TES and scene-graph data accessed in
TryCompleteDeferredChildWorldspaceCullare not protected beyond the atomic pointer handoff. WhilecachedTesis correctly synchronized with acquire-release semantics,gridCellsand cell state can be modified concurrently by game-thread hooks (BGSTerrainBlock_Attach/Detach). This appears intentional: the deferred cull mechanism withpendingChildWsCullretries on subsequent frames, and a stale cell-state read simply causes incorrect culling for one frame. Adding a comment explaining why this race is benign would clarify the design for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/UnifiedWater.cpp` around lines 622 - 627, Add a short explanatory comment above the fallback deferred cull block that notes the accepted data race: explain that TryCompleteDeferredChildWorldspaceCull is invoked with globals::features::unifiedWater and the atomic pointer cachedTes (loaded with memory_order_acquire), but gridCells and cell state may be concurrently modified by game-thread hooks (BGSTerrainBlock_Attach/Detach); state the that this is intentional because pendingChildWsCull will retry on subsequent frames and a stale read only affects culling for a single frame, so no additional synchronization is required. Mention the relevant symbols cachedTes, gridCells, TryCompleteDeferredChildWorldspaceCull, and pendingChildWsCull in the comment to guide future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 579-594: Extract the repeated grid-cell culling logic into a
static helper (e.g. ShouldCullAtCell) that accepts a RE::TES* tes and cell
coordinates (cellX, cellY) and returns a bool; move the offset/length/gridCells
checks and the cell->cellState.any(...) check into that helper, then replace the
inline logic in the BGSTerrainBlock_Attach loop (the built iteration using ix/iy
and shape->SetAppCulled) and in CullWaterParentByGridCells to call
ShouldCullAtCell(tes, instruction->x, instruction->y) (or the appropriate node
child coordinates) and pass its result to SetAppCulled, preserving the existing
condition semantics and null checks.
- Around line 55-57: Replace the unexplained magic cast
static_cast<RE::TESObjectCELL::CellState>(6) used inside the cell->cellState.any
check with a named constant or enum alias (e.g., kSomeCellState or
kCellStateXYZ) and add a brief comment explaining what that state represents;
update both occurrences (the check in UnifiedWater.cpp and the other at the
later occurrence around line 590) to use the new constant so the meaning is
clear instead of using the raw value 6 while keeping the condition logic
unchanged.
- Around line 622-627: Add a short explanatory comment above the fallback
deferred cull block that notes the accepted data race: explain that
TryCompleteDeferredChildWorldspaceCull is invoked with
globals::features::unifiedWater and the atomic pointer cachedTes (loaded with
memory_order_acquire), but gridCells and cell state may be concurrently modified
by game-thread hooks (BGSTerrainBlock_Attach/Detach); state the that this is
intentional because pendingChildWsCull will retry on subsequent frames and a
stale read only affects culling for a single frame, so no additional
synchronization is required. Mention the relevant symbols cachedTes, gridCells,
TryCompleteDeferredChildWorldspaceCull, and pendingChildWsCull in the comment to
guide future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d57451f-098c-455b-b198-ec135692c8b5
📒 Files selected for processing (2)
src/Features/UnifiedWater.cppsrc/Features/UnifiedWater.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/UnifiedWater.h
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Features/UnifiedWater.cpp (1)
622-629:⚠️ Potential issue | 🟠 MajorData race accessing
tes->gridCellsfrom render thread.While the code comments acknowledge staleness, the actual concern is a data race:
cachedTes.load()is atomic, but dereferencingtes->gridCellsand iterating over grid cells inTryCompleteDeferredChildWorldspaceCullis not synchronized. The game thread can concurrently modifygridCellsviaBGSTerrainBlock_Attach/Detach, leading to undefined behavior.If the render-thread fallback is needed for responsiveness, consider:
- Moving the deferred cull entirely to game-thread hooks (you already have two game-thread retry points at lines 502 and 689)
- Or using a mutex/spinlock around grid access
Additionally, line 627 introduces
uwwhich is redundant withsingletonalready defined at line 621.♻️ Minimal fix: remove render-thread fallback and rely on game-thread retries
- // Render-thread fallback for deferred child-worldspace cull completion. - // cachedTes/grid state can be stale while the game thread mutates terrain state. - // pendingChildWsCull keeps retrying, so stale reads only delay culling for a frame. - { - auto& uw = globals::features::unifiedWater; - uw.TryCompleteDeferredChildWorldspaceCull(singleton.cachedTes.load(std::memory_order_acquire)); - } + // Deferred child-worldspace cull is handled by game-thread hooks + // (BGSTerrainBlock_Attach, TESWaterSystem_UpdateDisplacementMeshPosition).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c1d5385-4dc7-46d9-a5ed-d372cf105589
📒 Files selected for processing (2)
src/Features/UnifiedWater.cppsrc/Features/UnifiedWater.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/UnifiedWater.h
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
See if we should use global.
|
the AI comment can't be fully addressed because any change I try breaks the fix. |
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
♻️ Duplicate comments (1)
src/Features/UnifiedWater.cpp (1)
624-627:⚠️ Potential issue | 🔴 CriticalKeep deferred child-world culling off the render thread.
TryCompleteDeferredChildWorldspaceCull()walkstes->gridCellsand mutates thegWaterLODtree. Calling it fromBSWaterShader_SetupGeometrystill racesBGSTerrainBlock_Attach/Detach; the atomics here only publish pointer values, they do not make either structure or lifetime thread-safe.🧵 Safer direction
- // Render-thread fallback for deferred child-worldspace cull completion. - // cachedTes/grid state can be stale while the game thread mutates terrain state. - // pendingChildWsCull keeps retrying, so stale reads only delay culling for a frame. - uw.TryCompleteDeferredChildWorldspaceCull(uw.cachedTes.load(std::memory_order_acquire)); + // Keep deferred child-world culling on the existing game-thread retry points + // in BGSTerrainBlock_Attach and TESWaterSystem_UpdateDisplacementMeshPosition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/UnifiedWater.cpp` around lines 624 - 627, The call to TryCompleteDeferredChildWorldspaceCull from BSWaterShader_SetupGeometry is unsafe because TryCompleteDeferredChildWorldspaceCull walks tes->gridCells and mutates gWaterLOD while BGSTerrainBlock_Attach/Detach run on the game thread; remove or gate the direct call (the uw.TryCompleteDeferredChildWorldspaceCull(...) invocation that passes uw.cachedTes.load(...)) so no traversal/mutation runs on the render thread; instead set a render-to-game signal (e.g., set pendingChildWsCull or enqueue a task) and ensure the actual TryCompleteDeferredChildWorldspaceCull invocation executes on the game thread or via the existing retry mechanism managed by the game loop, keeping only atomic pointer publishes on the render thread and moving all grid traversal/mutation to the game-thread handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 501-503: The game-thread retry paths (e.g., the call through
TryCompleteDeferredChildWorldspaceCull and the other retry blocks around the
commented ranges) are still reading globals::game::tes during the
TES_SetWorldSpace::thunk transition; change those retry closures to capture and
use the stored cachedTes (the value saved in TES_SetWorldSpace::thunk before
func() is invoked) instead of reading globals::game::tes so the immediate
cull/update logic runs against the correct worldspace; specifically, update the
retry lambdas/posted tasks invoked by TryCompleteDeferredChildWorldspaceCull and
the similar retry code at the other noted locations (around 588-595 and 684-687)
to accept/use cachedTes (or pass it in) rather than referencing
globals::game::tes.
---
Duplicate comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 624-627: The call to TryCompleteDeferredChildWorldspaceCull from
BSWaterShader_SetupGeometry is unsafe because
TryCompleteDeferredChildWorldspaceCull walks tes->gridCells and mutates
gWaterLOD while BGSTerrainBlock_Attach/Detach run on the game thread; remove or
gate the direct call (the uw.TryCompleteDeferredChildWorldspaceCull(...)
invocation that passes uw.cachedTes.load(...)) so no traversal/mutation runs on
the render thread; instead set a render-to-game signal (e.g., set
pendingChildWsCull or enqueue a task) and ensure the actual
TryCompleteDeferredChildWorldspaceCull invocation executes on the game thread or
via the existing retry mechanism managed by the game loop, keeping only atomic
pointer publishes on the render thread and moving all grid traversal/mutation to
the game-thread handler.
🪄 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
Run ID: dbc75277-af19-4cec-bded-afb52129c23f
📒 Files selected for processing (3)
src/Features/UnifiedWater.cppsrc/Globals.cppsrc/Globals.h
|
Is it because the Singleton is null when grabbed by globals? I believe you had mentioned that was also part of the issue with UW. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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)
213-220:⚠️ Potential issue | 🟡 MinorDuplicate declaration of
tes.
extern RE::TES* tes;appears on both Line 213 and Line 220. This is likely a merge artifact and should be removed.🔧 Proposed fix
extern RE::TES* tes; extern RE::TESWaterSystem* waterSystem; 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;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.h` around lines 213 - 220, Remove the duplicate extern declaration of the global pointer symbol `tes` in the Globals.h declarations block: locate the two lines declaring `extern RE::TES* tes;` and delete the redundant one so only a single `extern RE::TES* tes;` remains; verify surrounding externs (`waterSystem`, `isVR`, `memoryManager`, `iniSettingCollection`, `iniPrefSettingCollection`, `gameSettingCollection`) are unchanged and the header still compiles.
🧹 Nitpick comments (1)
src/Features/UnifiedWater.cpp (1)
19-20: Document the magic CellState value.The engine transition state
6is undocumented. Consider adding a brief comment explaining where this value comes from (e.g., reverse engineering notes or RE library reference) to aid future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/UnifiedWater.cpp` around lines 19 - 20, Add a short explanatory comment next to kTransitionAttachedCellState that documents that the magic value 6 corresponds to the engine's "transition/attached" CellState (RE::TESObjectCELL::CellState) used during cell transitioning, and note the provenance (e.g., discovered via reverse-engineering the game engine or confirmed from the RE library/reference) so future maintainers know why the static_cast to that enum uses 6.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Globals.h`:
- Around line 213-220: Remove the duplicate extern declaration of the global
pointer symbol `tes` in the Globals.h declarations block: locate the two lines
declaring `extern RE::TES* tes;` and delete the redundant one so only a single
`extern RE::TES* tes;` remains; verify surrounding externs (`waterSystem`,
`isVR`, `memoryManager`, `iniSettingCollection`, `iniPrefSettingCollection`,
`gameSettingCollection`) are unchanged and the header still compiles.
---
Nitpick comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 19-20: Add a short explanatory comment next to
kTransitionAttachedCellState that documents that the magic value 6 corresponds
to the engine's "transition/attached" CellState (RE::TESObjectCELL::CellState)
used during cell transitioning, and note the provenance (e.g., discovered via
reverse-engineering the game engine or confirmed from the RE library/reference)
so future maintainers know why the static_cast to that enum uses 6.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8c0a1df-e146-48ee-9eb9-9a6b8600ebe5
📒 Files selected for processing (3)
src/Features/UnifiedWater.cppsrc/Globals.cppsrc/Globals.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Globals.cpp
seems to work now after some rejigging |
(cherry picked from commit 3a0dc42)
2 participants
fix for the z-fighting in child worldspaces between the water lod and loaded water cells.
Summary by CodeRabbit
Performance Improvements
Bug Fixes