refactor: unify multiple EnvBRDF calls#1412
Conversation
WalkthroughAdds a unified BRDF environment approximation API Changes
Sequence Diagram(s)sequenceDiagram
participant Shader as Shader (PBR / Hair / DynamicCubemaps / Wetness)
participant BRDF as BRDF::EnvBRDF
participant Hirvonen as EnvBRDFApproxHirvonen
participant Lazarov as EnvBRDFApproxLazarov
Shader->>BRDF: EnvBRDF(roughness, NdotV)
alt ENV_BRDF_HIRVONEN defined
BRDF->>Hirvonen: compute approximation
Hirvonen-->>BRDF: float2(k1,k0)
else
BRDF->>Lazarov: compute approximation
Lazarov-->>BRDF: float2(k1,k0)
end
BRDF-->>Shader: float2(k1,k0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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/Common/BRDF.hlsli (2)
230-235: Hirvonen rational terms: guard denominators to avoid NaNs at extreme inputsAt very low/high roughness or grazing angles, the rational fits’ denominators can approach zero. Add a small epsilon guard to prevent NaNs/infs propagating through the frame.
Apply this diff:
- float k0 = dot(float2(1.0, a), mul(m0, float2(1.0, c))); - k0 /= dot(float3(1.0, a, a3), mul(m1, float3(1.0, c, c3))); - float k1 = dot(float2(1.0, a), mul(m2, float2(1.0, c))); - k1 /= dot(float3(1.0, a, a3), mul(m3, float3(1.0, c2, c3))); + float k0_num = dot(float2(1.0, a), mul(m0, float2(1.0, c))); + float k0_den = dot(float3(1.0, a, a3), mul(m1, float3(1.0, c, c3))); + float k1_num = dot(float2(1.0, a), mul(m2, float2(1.0, c))); + float k1_den = dot(float3(1.0, a, a3), mul(m3, float3(1.0, c2, c3))); + float k0 = k0_num / max(k0_den, EPSILON_DIVISION); + float k1 = k1_num / max(k1_den, EPSILON_DIVISION);
215-217: Clarify return semantics (A/B mapping) for maintainabilityPlease document that the function returns float2(A, B) to be consumed as F0*A + B. This avoids confusion across callsites and between Lazarov vs Hirvonen fits.
Apply this diff:
- float2 EnvBRDFApproxHirvonen(float roughness, float NdotV) + // Returns float2(A, B) such that preintegrated env BRDF ≈ F0 * A + B + float2 EnvBRDFApproxHirvonen(float roughness, float NdotV)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli(2 hunks)features/Hair Specular/Shaders/Hair/Hair.hlsli(1 hunks)features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli(2 hunks)package/Shaders/Common/BRDF.hlsli(1 hunks)package/Shaders/Common/PBR.hlsli(4 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:
features/Hair Specular/Shaders/Hair/Hair.hlslipackage/Shaders/Common/BRDF.hlslifeatures/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlslipackage/Shaders/Common/PBR.hlslifeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Hair Specular/Shaders/Hair/Hair.hlslifeatures/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlslifeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#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:
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.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: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (8)
package/Shaders/Common/BRDF.hlsli (1)
238-245: No legacy BRDF helpers remainRan pattern searches across all shader files and found zero occurrences of EnvBRDFApproxLazarov or EnvBRDFApproxWater. Migration off the old helpers is complete—ready to merge.
features/Hair Specular/Shaders/Hair/Hair.hlsli (1)
258-260: Good switch to unified BRDF APIReplacing EnvBRDFApproxLazarov with BRDF::EnvBRDFApprox aligns hair with the new centralized implementation and compile-time selection. No behavioral issues spotted around the call.
package/Shaders/Common/PBR.hlsli (2)
337-337: Direct-light energy compensation now follows unified BRDF approxThe change to BRDF::EnvBRDFApprox keeps the F0*A + B contract intact; downstream scaling remains valid. Looks good.
435-436: Consistent migration of indirect/coat/wetness lookups to unified APIAll affected callsites now use BRDF::EnvBRDFApprox with the same F0*A + B usage pattern. This de-duplicates logic and centralizes the approximation choice. Good change.
Also applies to: 445-447, 487-488
features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli (2)
1-1: Include addition is correctIncluding Common/BRDF.hlsli enables access to the centralized env BRDF functions without introducing register conflicts. Good.
150-150: Unified specular BRDF in wetness pathSwitching to BRDF::EnvBRDFApprox and keeping the local S term for roughness-dependent Fresnel preserves the F0*A + B pattern. Change looks correct.
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (2)
1-1: Correct dependency inclusionAdding BRDF.hlsli provides the unified EnvBRDFApprox without altering existing resource bindings. Looks good.
80-80: Correct API migration for env BRDFUsing BRDF::EnvBRDFApprox(roughness, NoV) here aligns the dynamic cubemap path with the centralized implementation and the compile-time Hirvonen/Lazarov switch.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package/Shaders/Common/BRDF.hlsli (2)
215-237: Hirvonen approx: add domain clamps and denom guards to prevent NaNs at edgesInputs outside [0,1] or near-zero denominators can cause instability. Clamp inputs and guard the rational function denominators.
Apply this diff:
float2 EnvBRDFApproxHirvonen(float roughness, float NdotV) { const float2x2 m0 = float2x2(0.99044, -1.28514, 1.29678, -0.755907); const float3x3 m1 = float3x3(1, 2.92338, 59.4188, 20.3225, -27.0302, 222.592, 121.563, 626.13, 316.627); const float2x2 m2 = float2x2(0.0365463, 3.32707, 9.0632, -9.04756); const float3x3 m3 = float3x3(1, 3.59685, -1.36772, 9.04401, -16.3174, 9.22949, 5.56589, 19.7886, -20.2123); + // Clamp inputs to expected domains for numerical stability + roughness = saturate(roughness); + NdotV = saturate(NdotV); + float a = roughness * roughness; float a2 = a * a; float a3 = a * a2; float c = NdotV; float c2 = c * c; float c3 = c * c2; - float k0 = dot(float2(1.0, a), mul(m0, float2(1.0, c))); - k0 /= dot(float3(1.0, a, a3), mul(m1, float3(1.0, c, c3))); - float k1 = dot(float2(1.0, a), mul(m2, float2(1.0, c))); - k1 /= dot(float3(1.0, a, a3), mul(m3, float3(1.0, c2, c3))); + float k0_num = dot(float2(1.0, a), mul(m0, float2(1.0, c))); + float k0_den = dot(float3(1.0, a, a3), mul(m1, float3(1.0, c, c3))); + float k0 = k0_num / max(k0_den, EPSILON_DIVISION); + float k1_num = dot(float2(1.0, a), mul(m2, float2(1.0, c))); + float k1_den = dot(float3(1.0, a, a3), mul(m3, float3(1.0, c2, c3))); + float k1 = k1_num / max(k1_den, EPSILON_DIVISION); return float2(k1, k0); }
238-245: Tighten preprocessor style and document return semantics of EnvBRDFCosmetic but safer across toolchains: remove spaces after ‘#’. Also add a short comment clarifying that the return is (A, B) with spec ≈ F0*A + B.
Apply this diff:
-float2 EnvBRDF(float roughness, float NdotV) -{ -# if defined(ENV_BRDF_HIRVONEN) - return EnvBRDFApproxHirvonen(roughness, NdotV); -# else - return EnvBRDFApproxLazarov(roughness, NdotV); -# endif -} +// Returns float2(A, B) such that the integrated specular term is approximated as: +// spec ≈ F0 * A + B +// Inputs are expected in [0,1]: roughness ∈ [0,1], NdotV ∈ [0,1]. +float2 EnvBRDF(float roughness, float NdotV) +{ +#if defined(ENV_BRDF_HIRVONEN) + return EnvBRDFApproxHirvonen(roughness, NdotV); +#else + return EnvBRDFApproxLazarov(roughness, NdotV); +#endif +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli(2 hunks)features/Hair Specular/Shaders/Hair/Hair.hlsli(1 hunks)features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli(2 hunks)package/Shaders/Common/BRDF.hlsli(1 hunks)package/Shaders/Common/PBR.hlsli(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
- features/Hair Specular/Shaders/Hair/Hair.hlsli
- package/Shaders/Common/PBR.hlsli
- features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/Common/BRDF.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 (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (1)
package/Shaders/Common/BRDF.hlsli (1)
215-237: No EnvBRDF call-sites found – AB ordering is safeI searched every
.hlsl,.hlsli,.fxand.fxhfile for:
- any call to
EnvBRDF(…)- any
EnvBRDFApprox*definitions outsidepackage/Shaders/Common/BRDF.hlsliResults:
- Zero matches for
EnvBRDF(- No stray
EnvBRDFApprox*definitionsSince this function isn’t referenced elsewhere, there’s no risk of swapping the scale (AB.x) and bias (AB.y) terms at call sites.
|
As described this should have been a feat or even two PRs. This isn't just a refactor. This is a feat right? |
Not really I guess, the added one is just a heavier substitute of what we already have (Lazarov) that does the same thing slightly better. It's more of a refactor that they got unified under same EnvBRDF() for easier use. Just more modular. |
|
I guess the issue is the commit title has two things in it. What is most important in this commit? Like if we're looking at this one month later, what do we need to know about this commit? Imagine you're trying to understand the full history and really don't want to open the commit to see what changed. |
jiayev
changed the title
|
@alandtse ok i've also retitled this one. |
3 participants
This pull request refactors how the environment BRDF approximation is handled across multiple shader files, centralizing the logic in
BRDF.hlsliand introducing a new, more accurate approximation method. The main change is the replacement of hardcoded and duplicated BRDF approximation functions with a unifiedBRDF::EnvBRDFApproxfunction, which now supports both the existing Lazarov method and the new Hirvonen method via a compile-time switch. This improves maintainability, consistency, and enables future extensibility.BRDF approximation refactor and centralization:
BRDF::EnvBRDFApproxandBRDF::EnvBRDFApproxHirvonenfunctions topackage/Shaders/Common/BRDF.hlsli, allowing selection between Lazarov and Hirvonen methods based on theENV_BRDF_HIRVONENmacro.features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsliandfeatures/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli, and replaced their usage with calls to the centralizedBRDF::EnvBRDFApprox. [1] [2]Shader code consistency and usage updates:
DynamicCubemaps,WetnessEffects,Hair, andPBRnamespaces to call the new unifiedBRDF::EnvBRDFApproxfunction, removing references to the oldEnvBRDFApproxLazarovand local variants. [1] [2] [3] [4] [5] [6] [7]Code maintainability improvements:
#include "Common/BRDF.hlsli"to affected shader files to ensure access to the centralized BRDF functions. [1] [2]These changes make the codebase easier to maintain and extend, while also enabling the use of more accurate BRDF approximations for specular reflection.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes