chore(sync): merge upstream/dev water + UI fixes (080afc0)#135
Conversation
📝 WalkthroughWalkthroughRefactors caustics sampling control flow in the water shader, adds render-target cleanup and two TAA water-history targets, removes legacy Water1/Water2 hook thunks, and gates custom cursor drawing by theme and loaded-cursor count. ChangesWater Effects Pipeline Modernization
Custom Cursor Rendering Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 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. |
There was a problem hiding this comment.
Pull request overview
Syncs three upstream fix: commits onto the fork’s v1.7.0-rc.1 baseline, addressing (1) a custom-cursor alt-tab crash that can occur when this fork ships no cursor PNGs, (2) water render-target lifetime/format handling to fix blending and a VRAM leak, and (3) shader warning cleanup to keep shader-validation warning-free.
Changes:
- Guard custom cursor rendering so it only runs when enabled and at least one cursor texture is loaded.
- Move WATER_1/WATER_2 format enforcement to
Deferred::SetupResources()and release existing RT resources before recreating to fix a VRAM leak. - Restructure caustics shader code to silence X4000 “potentially uninitialized” warnings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Menu/OverlayRenderer.cpp | Adds a guard to avoid drawing the custom cursor when no cursor textures are loaded. |
| src/Hooks.cpp | Removes Water1/Water2 RT creation thunks; updates hook installation block comments accordingly. |
| src/Deferred.cpp | Releases existing RT COM resources before recreating; enforces RGBA16F for WATER_1/WATER_2 history RTs. |
| features/Water Effects/Shaders/WaterEffects/WaterCaustics.hlsli | Refactors conditional assignments to avoid uninitialized-variable warnings. |
|
✅ A pre-release build is available for this PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Deferred.cpp (2)
142-144: 🏗️ Heavy liftAdd a user toggle for water history RT allocation.
These two RGBA16F history targets add persistent VRAM cost; expose an on/off or quality-tier toggle so users on lower-end GPUs can trade quality for stability/perf.
As per coding guidelines, "Flag potential performance impact issues for graphics features and suggest user toggles for performance-sensitive features".
🤖 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/Deferred.cpp` around lines 142 - 144, Add a user-configurable toggle to control allocation of the two RGBA16F water history render targets (created by calls to SetupRenderTarget for RE::RENDER_TARGETS::kWATER_1 and kWATER_2) so users on low-end GPUs can disable them; implement a new setting (e.g., bool UseWaterHistory or a quality enum) in the renderer/settings subsystem with a sensible default enabled, read that setting where Deferred.cpp constructs targets and only call SetupRenderTarget for kWATER_1 and kWATER_2 when the flag is true, and ensure the setting is exposed to user config/GUI and documented in defaults so VRAM-sensitive users can toggle it.Source: Coding guidelines
1-1: ⚡ Quick winTighten commit title/body to conventional-commit + explicit issue keywords.
Suggested title:
fix(sync): adopt upstream 1.7.0 water/ui fixes
Suggested PR body refs:Fixescommunity-shaders#2488, `Fixes `#2487,Fixescommunity-shaders#2490``.As per coding guidelines, include conventional commit titles and explicit issue-reference keywords when applicable.
🤖 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/Deferred.cpp` at line 1, Update the commit title and PR body to follow conventional-commit format and include explicit issue keywords: change the commit title to "fix(sync): adopt upstream 1.7.0 water/ui fixes" and update the PR description to include the issue references "Fixes `#2488`", "Fixes `#2487`", and "Fixes `#2490`"; ensure the repo/git commit message and the pull request description are edited accordingly so CI/issue trackers pick up the conventional commit type and the explicit "Fixes" references.Source: Coding guidelines
🤖 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/Deferred.cpp`:
- Around line 142-144: Add a user-configurable toggle to control allocation of
the two RGBA16F water history render targets (created by calls to
SetupRenderTarget for RE::RENDER_TARGETS::kWATER_1 and kWATER_2) so users on
low-end GPUs can disable them; implement a new setting (e.g., bool
UseWaterHistory or a quality enum) in the renderer/settings subsystem with a
sensible default enabled, read that setting where Deferred.cpp constructs
targets and only call SetupRenderTarget for kWATER_1 and kWATER_2 when the flag
is true, and ensure the setting is exposed to user config/GUI and documented in
defaults so VRAM-sensitive users can toggle it.
- Line 1: Update the commit title and PR body to follow conventional-commit
format and include explicit issue keywords: change the commit title to
"fix(sync): adopt upstream 1.7.0 water/ui fixes" and update the PR description
to include the issue references "Fixes `#2488`", "Fixes `#2487`", and "Fixes `#2490`";
ensure the repo/git commit message and the pull request description are edited
accordingly so CI/issue trackers pick up the conventional commit type and the
explicit "Fixes" references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11c5e3bd-1c8a-470d-ad57-8ef4711fd7ba
📒 Files selected for processing (4)
features/Water Effects/Shaders/WaterEffects/WaterCaustics.hlslisrc/Deferred.cppsrc/Hooks.cppsrc/Menu/OverlayRenderer.cpp
💤 Files with no reviewable changes (1)
- src/Hooks.cpp
Adopts the 3 upstream Community Shaders dev fixes landed since v1.7.0-rc.1: - community-shaders#2488 water-caustics X4000 warnings - community-shaders#2487 alt-tab CTD with no cursor png (fork ships none) - community-shaders#2490 water blending + VRAM texture leak (Water RT format-forcing moved to Deferred::SetupResources; keep-VR: Water hooks' VR offsets retire with them, Precip/Reflections keep theirs) Merged (not cherry-picked) to preserve upstream commit ancestry for clean future syncs. # Conflicts: # src/Hooks.cpp
alandtse
force-pushed
the
sync/upstream-1.7.0-fixes
branch
from
63a5304 to
1f35376
Compare
alandtse
changed the title
Cherry-picks the 3 upstream Community Shaders
devfixes landed since our v1.7.0-rc.1 sync (#121,b74dedf96). All are upstreamfix:commits; history was clean/linear.Commits
fix(water-caustics): X4000 warnings(shader-only,WaterCaustics.hlsli). Silences X4000 "potentially uninitialized" warnings; keeps our--max-warnings 0shader-validation aligned with upstream. Clean cherry-pick.fix(UI): CTD when alt-tabbing with no cursor png(OverlayRenderer.cpp, +1 guard). Only draws the custom cursor whenUseCustomCursor && CursorLoader::GetLoadedCount() > 0. Directly relevant to this fork — we intentionally ship no cursor/logo png, so this prevents an alt-tab CTD that can hit us specifically. Auto-merged over the chore(i18n): wrap fork feature & menu DrawSettings strings #127 i18n change in this file.fix: fix water blending and vram texture leak(Hooks.cpp−,Deferred.cpp+). See keep-VR note below.community-shaders#2490 — keep-VR conflict resolution (the only conflict)
It's a moved hook, resolved to preserve VR:
CreateRenderTarget_Water1/Water2creation-hooks (which force-setkR16G16B16A16_FLOATat RT creation and leak a texture each time) and instead sets the format inDeferred::SetupResourcesviaSetupRenderTarget(RE::RENDER_TARGETS::kWATER_1/kWATER_2, …R16G16B16A16_FLOAT…), which now releases the existing RTV/SRV/UAV/texture before recreating (the VRAM-leak fix).GetRuntimeData().renderTargets[]— CommonLib resolves the VR member offset internally — so SE/AE/VR are all covered with no raw addresses. The removed hooks' VR offsets (0x12C2/0x12D8) retire with the hooks (no orphaning).0x1917/0xCD2/0xD13) and the fork'sperfMode.InstallCreateRTThunks(). No orphanedWater1/Water2references remain.Verification
BuildRelease ALL— green.cpp_tests— 307/307.extract-i18n --check— en.json in sync (no string changes).Pre-merge note
community-shaders#2490 touches the VR water render targets. The addressing is now runtime-resolved (enum, not offsets), so it should be VR-correct by construction — but a quick VR water smoke is worth doing before merge given the fork is the VR maintainer.
Release impact
Titled
fix:(patch) — these are user-facing bug fixes (alt-tab CTD, VRAM leak, water blending) that warrant a changelog entry. Retitle tochore(sync):if you'd rather they not bump.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements