fix(water): stop double-counting specular in LLF - #368
Conversation
Water's vanilla per-light specular loop and LightLimitFix's own clustered specular loop light the same surface from the same light list via separate additive render passes, doubling specular for any light both see. Gate the vanilla loop off under LIGHT_LIMIT_FIX, as Lighting.hlsl already does for its own vanilla loop. Doing that alone would silently drop specular from every shadow-casting light, since LLF's water loop `continue`s past Shadow-flagged lights instead of sampling their shadow. Mirror RunGrass.hlsl's shadow-sampling pattern so those lights still contribute. Based on upstream community-shaders#2403, which only did the first half; a maintainer review there flagged the narrower regression this commit also fixes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWater lighting excludes the vanilla specular loop when ChangesWater lighting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/Shaders/Water.hlsl (1)
1180-1183: 📐 Maintainability & Code Quality | 🔵 TrivialUse a conventional PR title and issue reference.
If the PR title is not already conventional, use
fix(water): avoid duplicate water specular.Add
Related tocommunity-shaders#2403`` if upstream PR#2403is the applicable linked record. Add `Fixes #...` when a regression issue exists.As per path instructions, use
type(scope): descriptiontitles and add appropriate GitHub keywords for bug fixes or 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 `@package/Shaders/Water.hlsl` around lines 1180 - 1183, Update the pull request metadata rather than the shader code: use the conventional title “fix(water): avoid duplicate water specular” and add “Related to `#2403`” when applicable; include a “Fixes #...” reference only if a regression issue exists.Source: Path instructions
🤖 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 `@package/Shaders/Water.hlsl`:
- Around line 1180-1183: Update the pull request metadata rather than the shader
code: use the conventional title “fix(water): avoid duplicate water specular”
and add “Related to `#2403`” when applicable; include a “Fixes #...” reference
only if a regression issue exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed4fa81a-ef58-4461-9b23-54fd68c876d5
📒 Files selected for processing (1)
package/Shaders/Water.hlsl
|
No actionable suggestions for changed features. |
alandtse
changed the title
GetShadowLightShadow reconstructs world position from this pixel's camera-relative position, which is the mirrored reflection camera during a reflection pass, not the main view -- sampling the shadow map there would look up the wrong location. Gate it on inWorld and !inReflection, matching Lighting.hlsl's equivalent call.
1 participant
Summary
Fixes water specular double-counting when Light Limit Fix is enabled, correcting a gap in an unmerged upstream fix.
Water's vanilla per-light specular loop and LLF's own clustered specular loop both light the same water surface from the same light list, via separate additive
BSRenderPasses -- any light both passes see gets its specular counted twice. Gating the vanilla loop off underLIGHT_LIMIT_FIX(mirroringLighting.hlsl's existing convention) fixes that.Doing only that would introduce a real regression: LLF's own clustered water loop
continues past anyShadow-flagged light instead of sampling its shadow, so once it becomes the sole water specular source, every shadow-casting point/spot light (torches, lanterns -- the nearest, most visually significant lights) would lose water specular entirely. This mirrorsRunGrass.hlsl's shadow-sampling pattern to sample the shadow instead of dropping the light.Background
Based on upstream community-shaders/skyrim-community-shaders#2403, which only gated the vanilla loop and has been sitting unmerged since May. A maintainer (
doodlum) leftCHANGES_REQUESTEDthere, correctly flagging that water renders as an extra pass and that the PR as written breaks specular on lights it silently drops. Verified against our fork's code (via two independent supervisor passes plus an adversarial review) before writing this: the additive multi-pass premise checks out (src/ShaderCache.cpp'sGetWaterShaderDefinesbakes light count into the compiled technique, andLightLimitFix::Hooks::BSWaterShader_SetupGeometryfires perBSRenderPass, not per water surface per frame), and the actual mechanism behind the regression is theLightFlags::ShadowcontinueatWater.hlsl's LLF block, not a portal/room-index issue -- so this PR fixes that specific gap rather than importing community-shaders#2403 as-is.Testing
hlslkit-compileagainstWater.hlsl's full permutation set: 672 SE variants + 696 VR variants, 0 errors, 0 new warnings on both.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Fk5GzLkFvm9Z7KsHFKS51t
Summary by CodeRabbit