feat(water): BRDF sun specular#1635
Conversation
WalkthroughAdded a new WaterBRDF.hlsli shader module implementing a Cook‑Torrance microfacet BRDF for water specular, and updated Water.hlsl to include this module and call WaterBRDF::GetSunSpecular instead of its inline sun‑specular code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package/Shaders/Water/WaterBRDF.hlsli (2)
37-51: Consider a higher minimum roughness to prevent numerical precision issues.The minimum perceptual roughness of 0.001 results in α = 0.000001. This is significantly lower than Filament's recommended 0.045 minimum. While the code guards against division by zero, extremely low roughness values can cause:
- Numerical precision artifacts on some GPUs
- Very sharp specular spikes that may alias
Since the comment acknowledges Filament's recommendation, consider using a value closer to 0.01-0.02 for perceptual roughness (α = 0.0001-0.0004) which still produces sharp water reflections while avoiding edge cases.
// Clamp minimum roughness to prevent numerical issues // Filament recommends 0.045 for fp32, but we use slightly lower for water // Water can be extremely smooth, but not perfectly mirror-like - perceptualRoughness = max(perceptualRoughness, 0.001f); + perceptualRoughness = max(perceptualRoughness, 0.02f);
175-194: Minor redundancy in NdotL calculation.
L,V, andNdotLare computed twice—once inSpecularBRDFand again here. For a hot path like sun specular, consider inlining the BRDF computation directly inGetSunSpecularor returning additional outputs fromSpecularBRDF.That said, the current implementation prioritizes readability and modularity, which is reasonable for an initial implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Shaders/Water.hlsl(2 hunks)package/Shaders/Water/WaterBRDF.hlsli(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Water/WaterBRDF.hlslipackage/Shaders/Water.hlsl
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Water/WaterBRDF.hlslipackage/Shaders/Water.hlsl
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water/WaterBRDF.hlslipackage/Shaders/Water.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
package/Shaders/Water.hlsl (2)
573-574: LGTM!The include is correctly placed within the appropriate conditional compilation block, ensuring the BRDF module is only compiled when needed for the water specular path.
953-953: LGTM!The refactored call cleanly delegates sun specular computation to the new BRDF module. Parameters map correctly:
VarAmounts.xfor specular power andDeepColor.wfor turbidity/scatter factor.package/Shaders/Water/WaterBRDF.hlsli (7)
1-5: LGTM!Proper include guards and namespace encapsulation.
24-30: LGTM!The F0 value of 0.02 is physically correct for water (IOR 1.33). The energy boost is well-documented as an artistic compensation for real-world wave statistics and HDR limitations.
59-73: LGTM!Correct GGX NDF implementation following Filament's optimized formulation. The algebraic refactoring maintains numerical stability by avoiding explicit division by terms that could approach zero.
82-97: LGTM!Correct height-correlated Smith-GGX visibility with proper division-by-zero protection. The combined visibility term efficiently incorporates both geometric shadowing and the standard 4·NdotV·NdotL denominator.
105-125: LGTM!Correct Schlick Fresnel approximation. The algebraic rearrangement
f + f0 * (1 - f)is equivalent to the standard formF0 + (1 - F0) * (1 - u)^5and follows Filament's optimization.
135-163: LGTM!Correct Cook-Torrance BRDF composition with proper view direction convention (negation of incoming direction), appropriate clamping for numerical stability, and early exit optimization for below-horizon lighting.
195-197: LGTM!Clean namespace and include guard closure with helpful trailing comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
package/Shaders/Water/WaterBRDF.hlsli (3)
1-31: Header guard, namespace, and physical constants look solid (PR metadata also fine)Header guard, namespace scoping, and the chosen F0 value for water (0.02 from IOR≈1.33) all look correct, and the extra energy boost constant is clearly documented for artistic tuning. The PR title
feat(water): BRDF sun specularalready follows conventional-commit style; if there is a tracking issue, consider adding something likeImplements #123orRelated to #123in the PR description to tie this feature back to an issue.
32-51: GuardspecPoweragainst invalid negative valuesThe Blinn/Phong→roughness mapping assumes
specPower >= 0. If bad input ever makes it here (e.g.specPower < -2),sqrt(2/(specPower+2))can go NaN beforesaturate. A tiny clamp will make this more robust without changing valid cases.float SpecularPowerToRoughness(float specPower) { + // Guard against invalid inputs to avoid NaNs from sqrt + specPower = max(specPower, 0.0f); + // Convert specular power to perceptual roughness // ... float perceptualRoughness = saturate(sqrt(2.0f / (specPower + 2.0f))); @@ // Square for roughness (α in Filament terminology) return perceptualRoughness * perceptualRoughness; }
167-186: TidyGetSunSpecularand confirm linear‑space inputsThere are a couple of easy cleanups and one pipeline check here:
float3 V = -viewDirection;is unused and can be dropped.- You already compute
NdotLinsideSpecularBRDF; if this ends up as a hot path, consider refactoringSpecularBRDFto optionally returnNdotLso you don’t recompute the dot, though it’s a micro‑optimization.- Per existing work on linear lighting in this repo, please ensure
sunColor(and the output ofGetSunSpecular) are in linear space before tonemapping/compositing, so this physically‑based BRDF behaves as intended. Based on learnings, this matches how other image‑space shaders handle color in this project.A minimal code tidy would be:
float3 GetSunSpecular(float3 normal, float3 viewDirection, float4 sunDir, float3 sunColor, float specPower, float turbidity) { // Calculate BRDF float3 specularBRDF = SpecularBRDF(normal, viewDirection, sunDir.xyz, specPower); // Calculate NdotL for energy conservation float3 L = sunDir.xyz; - float3 V = -viewDirection; float NdotL = saturate(dot(normal, L)); @@ return sunSpecular; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Water/WaterBRDF.hlsli(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Water/WaterBRDF.hlsli
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Water/WaterBRDF.hlsli
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Water/WaterBRDF.hlsli
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (3)
package/Shaders/Water/WaterBRDF.hlsli (3)
59-89: GGX NDF and Smith visibility implementations match standard formulations
D_GGXandV_SmithGGXCorrelatedare consistent with the common GGX microfacet NDF and height‑correlated Smith visibility (withroughnessinterpreted as α), and themax(..., 1e-6f)guard inVavoids numerical blow‑ups at grazing angles. No changes needed here.
97-117: Fresnel implementation is correct and efficient for dielectric water
F_Schlickcorrectly implements the Schlick approximation with F0 and F90=1 and uses the optimized formf + f0 * (1 - f), which is algebraically equivalent toF0 + (1 - F0) * (1 - u)^5. This is appropriate for a dielectric water surface.
127-155: Verify direction conventions forviewDirectionandlightDirection
SpecularBRDFassumesviewDirectionis opposite of the usual V (you buildV = -viewDirection) and thatlightDirectionalready points from surface to light. This is fine as long as the call sites match the documented convention; otherwise N·V / N·L will be flipped and the lobe will move incorrectly. It might be worth double‑checking theWater.hlslcall to ensureviewDirectionandsunDir.xyzare passed with these exact meanings (and normalized) to avoid subtle BRDF shape issues.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
think too much duplicate to BRDF.hlsli
There was a problem hiding this comment.
functions were changed somewhat based on optimisations from that Filament page, but I can adapt to BRDF.hlsli stuff specifically
|
Did you write the code based on reading the principles or was there code from the material? If code, what's the license? |
its just code from the material, they only have math in that link. |
|
I don't understand your response. Did they provide code you copied? If so what is the license. |
|
Sorry yeah my response was dumb idk lol No, if you look at the link it's a maths resource basically. They have an engine as well elsewhere but I didn't look at any code in it. I coded it from the maths in the resource, plus a little bit of extra less physically accurate multipliers for the sake of appearance And the second part was saying that I will move this to a separate water feature and still support vanilla instead, as currently this was a full replacement. |
|
Please close or mark as a draft if you don't need this PR. We have limited reviewer time so PRs that are not ready for review should be clearly marked. |
3 participants
more accurate sun specular model based on https://google.github.io/filament/Filament.md.html
would look better with real HDR, can't get it much "brighter" with current tonemapping.
(Not attached to unified water in any way but used for screenshots for the better distant normals)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.