fix(water): fix water blending (ghosting) and LOD gaps#2440
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUn‑premultiplies sampled waterHistory before blending and writes premultiplied final color with coverage; changes LOD‑4 attached‑cell culling to use the cell's kHasWater flag; adds an imagespace vfunc hook that may clear RTV slot 1 before chaining to the original ISWaterBlend render function. ChangesWater Rendering Pipeline Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/UnifiedWater.cpp (1)
484-487: 💤 Low valueDocument/replace the
CellStatemagic number (6) (src/Features/UnifiedWater.cpp:484-487)
static_cast<RE::TESObjectCELL::CellState>(6)isn’t backed by anyRE::TESObjectCELL::CellStatedefinition in this repo, so the meaning of6can’t be checked here. Replace it with a named constant (or versioned mapping + comment) explaining whichCellStateit corresponds to and why it’s required for SE/AE/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 484 - 487, Replace the magic literal static_cast<RE::TESObjectCELL::CellState>(6) used inside the gridCells->GetCell(...) check with a named constant or versioned mapping (e.g., constexpr auto CellState_kAttachedAlternative or a function MapCellStateForRuntime()) and update the cell->cellState.any(...) call to use that named symbol; add a short comment documenting which runtime/version (SE/AE/VR) this value maps to and why it’s required for compatibility, or implement a small mapping helper that returns the correct RE::TESObjectCELL::CellState for the current runtime and use that helper in place of the literal.
🤖 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.
Nitpick comments:
In `@src/Features/UnifiedWater.cpp`:
- Around line 484-487: Replace the magic literal
static_cast<RE::TESObjectCELL::CellState>(6) used inside the
gridCells->GetCell(...) check with a named constant or versioned mapping (e.g.,
constexpr auto CellState_kAttachedAlternative or a function
MapCellStateForRuntime()) and update the cell->cellState.any(...) call to use
that named symbol; add a short comment documenting which runtime/version
(SE/AE/VR) this value maps to and why it’s required for compatibility, or
implement a small mapping helper that returns the correct
RE::TESObjectCELL::CellState for the current runtime and use that helper in
place of the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8db8b00d-e75b-48bf-b6ef-abdd853f051c
📒 Files selected for processing (3)
package/Shaders/ISWaterBlend.hlslsrc/Features/UnifiedWater.cppsrc/Hooks.cpp
FIocker
force-pushed
the
fix/water-blend-ghosting-lod-gaps-20260530
branch
from
75b5d08 to
3350784
Compare
|
✅ A pre-release build is available for this PR: |
Rationale: discarded non-water pixels can leave stale water history behind, and sampling cleared transparent history without premultiplication creates dark edge artifacts. Loaded dry cells can also hide LOD water even when no active water replaces it. Implementation: clear the ISWaterBlend history render target before the blend pass, store water history premultiplied by coverage and un-premultiply it when sampled, and cull Unified Water LOD parents only when the attached grid cell actually has water while keeping deferred child-world cull completion accurate. Acknowledgement: Manual port of community-shaders#2440 by FIocker. Co-authored-by: FIocker <4461558+FIocker@users.noreply.github.com>
3 participants
Fixes water edge ghosting and dark outlines caused by stale water blend history, and prevents LOD water gaps that became visible once the history blending was cleaned up
The water blend pass discards non-water pixels, which means the history target can keep old coverage/color in those pixels. This clears the water history render target once per frame before the blend pass while keeping the discard in place, so the main scene is not overwritten
Since the cleared history is transparent black, water history is now stored premultiplied by coverage and un-premultiplied when sampled. This prevents bilinear filtering against cleared pixels from creating dark outlines along moving water edges
Also only culls Unified Water LOD tiles for attached cells that actually have water. Dry loaded cells do not create active water, so hiding their LOD tile could leave visible gaps
Summary by CodeRabbit