fix(pbr): conditional env map normalisation#1533
Conversation
|
✅ A pre-release build is available for this PR: |
WalkthroughIntroduces Color::ConditionalSpecularNormalization in Color.hlsli, adds EPSILON_AMBIENT to Math.hlsli, and adjusts whitespace in DeferredCompositeCS.hlsl. The new function conditionally normalizes specular irradiance based on ambient checks, ratio, logarithmic blend, normalization, and brightness compensation. No other logic changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Shader Caller
participant Color as Color::ConditionalSpecularNormalization
Note over Caller,Color: Conditional specular normalization flow (new)
Caller->>Color: (specularIrradiance, specularLuminance, ambientSpecular)
activate Color
rect rgba(230,240,255,0.5)
Note right of Color: Ambient safety check
Color-->>Color: if ambient <= EPSILON_AMBIENT → return input
end
rect rgba(235,255,235,0.5)
Note right of Color: Compute ratio & blend
Color-->>Color: ratio = specular/ambient (unscaled)
Color-->>Color: blend = saturate(log2(max(ratio,1)) * 0.1)
end
rect rgba(255,245,230,0.6)
Note right of Color: Normalize & blend
Color-->>Color: normalized = specular / max(luminance, tiny)
Color-->>Color: out = lerp(specular, normalized, blend)
Color-->>Color: out *= 1.33
end
Color-->>Caller: out
deactivate Color
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli(4 hunks)package/Shaders/Common/Color.hlsli(1 hunks)package/Shaders/DeferredCompositeCS.hlsl(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/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/Common/Color.hlslipackage/Shaders/DeferredCompositeCS.hlsl
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/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
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.
📚 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.hlslipackage/Shaders/Common/Color.hlslipackage/Shaders/DeferredCompositeCS.hlsl
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package/Shaders/Common/Color.hlsli (2)
119-122: Avoid a hard cutoff at near‑black ambient (fade instead of early return).The early return at AMBIENT_EPSILON introduces a discrete change if ambient crosses the threshold. Prefer a soft fade so normalization influence smoothly goes to zero as ambient dims.
Apply this minimal change:
- // Safety check for essentially black ambient lighting - if (directionalAmbientColorSpecular < AMBIENT_EPSILON) { - return specularIrradiance; // Don't normalize if ambient is essentially black - } + // Soft fade-in of normalization influence as ambient rises from black + // 0 at 0 ambient → 1 by ~AMBIENT_EPSILON + float ambientWeight = saturate(directionalAmbientColorSpecular / (AMBIENT_EPSILON)); @@ - return lerp(specularIrradiance, normalizedSpecular, blendFactor); + return lerp(specularIrradiance, normalizedSpecular, blendFactor * ambientWeight);Also applies to: 131-131
112-118: Clarify expected color space and standardize luminance source (add overload).Callers may compute luminance with different weights; also, inputs should be linear-space per our pipeline. Add a brief doc note and an overload that computes luminance internally to reduce footguns. Based on learnings.
Apply inline doc near the signature:
- // Conditional specular irradiance normalization to fix PBR blue tint issues + // Conditional specular irradiance normalization to fix PBR blue tint issues. + // Expects all inputs in linear color space. + // specularIrradianceLuminance should be derived via Color::RGBToLuminance in linear space.Add an overload (outside this hunk):
// Convenience overload: derives luminance internally (linear space expected). float3 ConditionalSpecularNormalization(float3 specularIrradiance, float directionalAmbientColorSpecular) { float lum = RGBToLuminance(specularIrradiance); return ConditionalSpecularNormalization(specularIrradiance, lum, directionalAmbientColorSpecular); }Please confirm all call sites feed linear-space values (no gamma) into this helper. Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Common/Color.hlsli(1 hunks)
🧰 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/Color.hlsli
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
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.
📚 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:
package/Shaders/Common/Color.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 (2)
package/Shaders/Common/Color.hlsli (2)
124-132: Good centralization + smooth ramp; removes the >2.0 jump.The helper consolidates behavior and replaces the hard threshold with a gradual blend (log2 ramp ≈ 1→4), which should prevent spec energy pops. Looks good.
71-73: Confirm Color.hlsli path & constant presence
I couldn’t locatepackage/Shaders/Common/Color.hlslior theReflectionNormalisationScaledefinition—please verify the correct file path/extension and ensure the constant exists before renaming.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
1 participant
after/before
Summary by CodeRabbit