fix: water screen space flicker#1931
Conversation
Undoes the inverse behavior of water TAA until a better method can be found and implemented for HDR.
📝 WalkthroughWalkthroughA single line in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/Upscaling.cpp (1)
1013-1026: Suggest adding a scope to the PR title and an issue reference.Per the conventional commits guideline the title should carry a scope, e.g.:
fix(water): disable water TAA when upscaling is activeAdditionally, if there is a tracked issue for the water TAA / HDR artefact, linking it (e.g.
Fixes #<issue>) in the PR description would aid triage and auto-closure.As per coding guidelines:
type(scope): description(50 char limit, lowercase, no period), and issue references should use GitHub close keywords for bug fixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1013 - 1026, The PR title lacks a scope and an issue reference; update the PR title to follow conventional commits (type(scope): description) — e.g., change to "fix(water): disable water TAA when upscaling is active" — and add a GitHub issue close keyword in the PR description referencing the tracked issue (e.g., "Fixes #<issue>") so the change in Upscaling::ConfigureTAA (affecting BSImagespaceShaderISTemporalAA and the enableWaterTAA flag) is scoped and the bug/artefact ticket is linked for auto-closure and easier triage.
🤖 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/Upscaling.cpp`:
- Around line 1013-1026: The PR title lacks a scope and an issue reference;
update the PR title to follow conventional commits (type(scope): description) —
e.g., change to "fix(water): disable water TAA when upscaling is active" — and
add a GitHub issue close keyword in the PR description referencing the tracked
issue (e.g., "Fixes #<issue>") so the change in Upscaling::ConfigureTAA
(affecting BSImagespaceShaderISTemporalAA and the enableWaterTAA flag) is scoped
and the bug/artefact ticket is linked for auto-closure and easier triage.
|
✅ A pre-release build is available for this PR: |
|
This should have been pred as a revert with an actual git revert on the commit in question. |
|
@jiayev notice the revert here. We should discuss the pros and cons so we don't end up ping ponging. |
jiayev
left a comment
There was a problem hiding this comment.
simply revert will bring back the old water clamping issue. it's against our hdr rendering purpose.
either:
- upgrade two water blend texture to 64bit then restore this
- improve water shader to remove water flicker.
always seek for fundamental fix.
The water clamping fix should be part of the HDR branch then knowing there will be water flicker once merged. |
|
A better solution would be to tie the clamping fix to the HDR feature so once enabled at boot it applies the fix, users without HDR don't have to endure flickering water reflections. |
it's about hdr rendering, not hdr output. |
Undoes the inverse behavior of water TAA until a better method can be found and implemented for HDR.
Summary by CodeRabbit