fix(water): remove water lights specular when llf on - #2403
Conversation
📝 WalkthroughWalkthroughThe Water.hlsl pixel shader now conditionally skips the per-specular-light accumulation loop when ChangesWater Shader Specular Light Compilation Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. |
jiayev
changed the title
![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)
1149-1160: ⚡ Quick winPolish PR metadata for clarity and traceability
Suggested commit title (≤50 chars):
fix(water): skip legacy specular loop with llf
If this fixes a tracked bug, add an issue keyword in the PR description, e.g.Fixes #<id>.As per coding guidelines,
**/*: "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... [and] Issue References ..."🤖 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 1149 - 1160, Update the PR metadata: set the commit/PR title to "fix(water): skip legacy specular loop with llf" and add an issue reference (e.g. "Fixes #<id>") in the PR description if this change resolves a tracked bug; no code changes are required in the LIGHT_LIMIT_FIX/NUM_SPECULAR_LIGHTS specular loop (the HLSL block with LightPos, PosAdjust, input.WPosition, FresnelRI, etc.)—just update the commit message and PR description to follow the Conventional Commits guideline and include the issue keyword for traceability.
🤖 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 1149-1160: Update the PR metadata: set the commit/PR title to
"fix(water): skip legacy specular loop with llf" and add an issue reference
(e.g. "Fixes #<id>") in the PR description if this change resolves a tracked
bug; no code changes are required in the LIGHT_LIMIT_FIX/NUM_SPECULAR_LIGHTS
specular loop (the HLSL block with LightPos, PosAdjust, input.WPosition,
FresnelRI, etc.)—just update the commit message and PR description to follow the
Conventional Commits guideline and include the issue keyword for traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0830b024-1479-4e1b-b03e-30bebfe56855
📒 Files selected for processing (1)
package/Shaders/Water.hlsl
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
pretty sure water is an extra pass. also water does not support portal strict lights, so this breaks portal strict lights on water. also check water-specific lights flag and how it's used
* fix(water): stop double-counting specular under LLF 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. * fix(water): skip shadow lookup in reflections 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.
2 participants
Summary by CodeRabbit