feat(ibl): apply ibl on fog, pre-apply lambertian and change default settings#1390
Conversation
WalkthroughAdds FogAmount and PreserveFogLuminance to IBL settings (C++ and HLSL), exposes them in UI/serialization, implements ImageBasedLighting::GetFogIBLColor(...) in the IBL shader, and conditionally overrides fog color in several pixel shaders when diffuse IBL is enabled and the scene is not interior. Changes
Sequence Diagram(s)sequenceDiagram
participant PS as Pixel Shader
participant SD as SharedData
participant IBL as ImageBasedLighting
PS->>PS: fogColor = input.FogParam.xyz
PS->>SD: read EnableDiffuseIBL, InInterior
alt diffuse IBL enabled && not interior
PS->>IBL: GetFogIBLColor(fogColor)
IBL-->>PS: iblFogColor
PS->>PS: fogColor = iblFogColor
end
PS->>PS: finalColor = lerp(litColor, fogColor, fogFactor)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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: 1
🧹 Nitpick comments (4)
src/Features/IBL.cpp (1)
23-32: UI label clarity (nit)"Affects Fog Amount" is a bit ambiguous. Consider a clearer label like "Fog Amount (IBL Blend)" and a brief tooltip describing the blend with IBL diffuse ambient.
- ImGui::SliderFloat("Affects Fog Amount", &settings.FogAmount, 0.0f, 1.0f, "%.2f"); + ImGui::SliderFloat("Fog Amount (IBL Blend)", &settings.FogAmount, 0.0f, 1.0f, "%.2f"); + if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::Text("Blends original fog color with IBL-derived ambient color.\n" + "0 = vanilla fog, 1 = full IBL influence."); + }package/Shaders/Lighting.hlsl (1)
3115-3123: Fog IBL override hook is correct; verify color space consistencyRuntime gating matches other IBL usage (EnableDiffuseIBL && !InInterior). Ensure GetFogIBLColor returns fogColor in the same color space used here (gamma), as the final lerp is in gamma space post LinearToGamma.
If GetFogIBLColor computes linear, wrap with Color::LinearToGamma.
package/Shaders/ISSAOComposite.hlsl (1)
177-182: APPLY_FOG: check linear/gamma handling with IBL fogISSAOComposite applies AO in linear space and returns to gamma before fog. The fog lerp uses gamma-space values. Ensure GetFogIBLColor outputs gamma-space fogColor to stay consistent; otherwise wrap with Color::LinearToGamma.
package/Shaders/Water.hlsl (1)
1178-1181: Refraction fog path: consistent IBL overrideGood to see parity with the other fog path. Confirm color space and consider saturating FogAmount at use-site in GetFogIBLColor as a safety net (see suggested fix in IBL.hlsli).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
features/IBL/Shaders/IBL/IBL.hlsli(2 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)package/Shaders/Effect.hlsl(1 hunks)package/Shaders/ISSAOComposite.hlsl(2 hunks)package/Shaders/Lighting.hlsl(1 hunks)package/Shaders/Water.hlsl(3 hunks)src/Features/IBL.cpp(2 hunks)src/Features/IBL.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/Lighting.hlslpackage/Shaders/Effect.hlslpackage/Shaders/ISSAOComposite.hlslfeatures/IBL/Shaders/IBL/IBL.hlslipackage/Shaders/Water.hlsl
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
src/Features/IBL.h
⏰ 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 (8)
src/Features/IBL.h (1)
50-53: Verify shader struct parity and add static_assert
The CPU‐sideSettingsstruct insrc/Features/IBL.hnow includes a newFogAmountfield and trims padding, preserving its 32-byte size. Please:
- Add a compile-time guard in
src/Features/IBL.h:static_assert(sizeof(Settings) == 32, "IBL::Settings must remain 32 bytes to match shader cbuffer layout");- Locate your GPU constant‐buffer definition (e.g. the
IBLSettingsstruct in your HLSL include) and ensure it has:struct IBLSettings { uint EnableDiffuseIBL; float DiffuseIBLScale; float DALCAmount; float IBLSaturation; float FogAmount; uint SampleUnderHorizonFromDynCube; // padding to 32 bytes as needed };- Confirm the field order, total byte size, and 16-byte alignment on the GPU side.
src/Features/IBL.cpp (1)
14-15: Serialization updated correctlyIncluding FogAmount in NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT keeps persistence in sync. No issues.
package/Shaders/Common/SharedData.hlsli (1)
184-187: Manual verification required: CPU and GPU IBLSettings definitions not foundI wasn’t able to locate the C++
IBL::Settingsstruct (e.g. insrc/Features/IBL.h) nor the HLSLIBLSettingsstruct inSharedData.hlsliunderpackage/Shaders/Common. Please confirm that:
- The GPU-side
IBLSettingsstruct fields and ordering exactly mirror the CPU-sideIBL::Settingsstruct.- The overall size remains a 32-byte footprint in the cbuffer.
package/Shaders/Effect.hlsl (1)
796-803: Good integration; confirm fog color spaceThe gating and use of GetFogIBLColor mirror Lighting.hlsl. As the blend is done in gamma space here, ensure GetFogIBLColor returns gamma-space fogColor to avoid subtle shifts.
package/Shaders/ISSAOComposite.hlsl (1)
128-131: Header include is appropriate and safely guardedIncluding IBL.hlsli under defined(IBL) is correct and keeps dependencies explicit.
package/Shaders/Water.hlsl (3)
987-989: IBL header inclusion OKGuarded include aligns with conditional usage. No issues.
1147-1154: VC path: IBL-adjusted fog is correctly gatedUsing GetFogIBLColor prior to fog lerp is correct. Same color-space caveat: blend is in gamma space; ensure GetFogIBLColor returns gamma-space fogColor.
1163-1172: Non-VC path: preFogColor override is placed correctlyApplying IBL to preFogColor before fog lerp keeps the logic intact. Same note on fog color space as above.
|
✅ A pre-release build is available for this PR: |
| ImGui::SliderFloat("Diffuse IBL Scale", &settings.DiffuseIBLScale, 0.0f, 10.0f, "%.2f"); | ||
| ImGui::SliderFloat("Diffuse IBL Saturation", &settings.IBLSaturation, 0.0f, 2.0f, "%.2f"); | ||
| ImGui::SliderFloat("DALC Amount", &settings.DALCAmount, 0.0f, 1.0f, "%.2f"); | ||
| ImGui::SliderFloat("Affects Fog Amount", &settings.FogAmount, 0.0f, 1.0f, "%.2f"); |
|
Overall I feel like this isn't really necessary when it should be on weather authors |
jiayev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Features/IBL.cpp (1)
36-39: Clamp FogAmount on load to harden against malformed configsEven though the UI constrains 0–1, JSON may contain out-of-range values. Clamp on load.
void IBL::LoadSettings(json& o_json) { settings = o_json; + settings.FogAmount = std::clamp(settings.FogAmount, 0.0f, 1.0f); }Add once per TU:
#include <algorithm>
♻️ Duplicate comments (1)
features/IBL/Shaders/IBL/IBL.hlsli (1)
27-41: Fix undefined SH sample direction; clamp FogAmount and guard luminance divisionSampling SH with float3(0,0,0) is undefined and will bias/zero the result. Also, FogAmount should be clamped defensively, and luminance preservation should use an epsilon to avoid wild scaling.
Apply:
- float3 GetFogIBLColor(float3 fogColor) + float3 GetFogIBLColor(float3 fogColor) { - float3 directionalAmbientColor = max(0, mul(SharedData::DirectionalAmbient, float4(float3(0, 0, 0), 1.0))).xyz; - float3 iblColor = directionalAmbientColor * SharedData::iblSettings.DALCAmount + Color::Saturation(GetDiffuseIBL(float3(0, 0, 0)), SharedData::iblSettings.IBLSaturation) * SharedData::iblSettings.DiffuseIBLScale; + // Use a stable, normalized world-up direction for a representative fog IBL sample + const float3 up = float3(0, 0, 1); + float3 directionalAmbientColor = max(0, mul(SharedData::DirectionalAmbient, float4(up, 1.0))).xyz; + float3 iblColor = directionalAmbientColor * SharedData::iblSettings.DALCAmount + + Color::Saturation(GetDiffuseIBL(up), SharedData::iblSettings.IBLSaturation) * SharedData::iblSettings.DiffuseIBLScale; if (SharedData::iblSettings.PreserveFogLuminance) { const float fogLuminance = Color::RGBToLuminance2(fogColor); const float iblLuminance = Color::RGBToLuminance2(iblColor); - if (iblLuminance > 0) { + const float eps = 1e-4; + if (iblLuminance > eps) { const float scale = fogLuminance / iblLuminance; iblColor *= scale; } else { iblColor = fogColor; } } - return lerp(fogColor, iblColor, SharedData::iblSettings.FogAmount); + return lerp(fogColor, iblColor, saturate(SharedData::iblSettings.FogAmount)); }If your convention uses +Z down, swap up to float3(0, 0, -1).
🧹 Nitpick comments (2)
package/Shaders/Common/SharedData.hlsli (1)
181-188: IBLSettings CPU/GPU layout looks consistent; add a brief “keep in sync” noteOrder and sizes (4 uints + 4 floats = 32 bytes) match the C++ Settings struct. Good job keeping packing aligned to 16 bytes. Consider adding a short comment that this layout must stay in lockstep with src/Features/IBL.h::IBL::Settings to prevent silent CB misbinds in future edits.
src/Features/IBL.cpp (1)
24-26: Add a tooltip for Fog Mix to clarify behaviorA tiny UX touch helps users understand the blend. Suggest:
- ImGui::SliderFloat("Fog Mix", &settings.FogAmount, 0.0f, 1.0f, "%.2f"); + ImGui::SliderFloat("Fog Mix", &settings.FogAmount, 0.0f, 1.0f, "%.2f"); + if (auto _tt = Util::HoverTooltipWrapper()) { + ImGui::TextUnformatted("Blends original fog color with IBL.\n0 = original fog, 1 = fully IBL-driven.\nTip: keep moderate values for natural results."); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/IBL/Shaders/IBL/IBL.hlsli(2 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/IBL.cpp(1 hunks)src/Features/IBL.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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/IBL/Shaders/IBL/IBL.hlsli
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
src/Features/IBL.h
⏰ 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). (4)
- GitHub Check: Comment on PR with audit results
- 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 (2)
features/IBL/Shaders/IBL/IBL.hlsli (1)
24-25: Ensure Single 1/π Application in Diffuse IBLI scanned all GetDiffuseIBL() call sites (RunGrass.hlsl, Effect.hlsl, AmbientCompositeCS.hlsl, DistantTree.hlsl, Lighting.hlsl) and found no other explicit
/ Math::PI. The only division lives in:
- features/IBL/Shaders/IBL/IBL.hlsli
return float3(colorR, colorG, colorB) / Math::PI;Before merging, please verify that your downstream diffuse BRDF (e.g. Lambert) or any decode step does not also apply a 1/π factor—otherwise the result will be under-lit.
src/Features/IBL.cpp (1)
11-16: Serialization order updated — backward compat OK; goodFields are explicitly named and default-initialized, so existing configs load safely. Thanks for including FogAmount and PreserveFogLuminance here.
| uint PreserveFogLuminance = 0; | ||
| uint pad; | ||
| float DiffuseIBLScale = 1.0f; | ||
| float DALCAmount = 0.33f; | ||
| float IBLSaturation = 1.0f; | ||
| float FogAmount = 0.0f; | ||
| } settings; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize pad and guard layout with a static_assert
Pad currently has no in-class initializer; while RestoreDefaultSettings zeroes it, it may be indeterminate before that call. Initialize it to 0 and add a static_assert to lock in the intended size/alignment.
uint PreserveFogLuminance = 0;
- uint pad;
+ uint pad = 0;
float DiffuseIBLScale = 1.0f;
float DALCAmount = 0.33f;
float IBLSaturation = 1.0f;
float FogAmount = 0.0f;
} settings;Outside this hunk, consider adding:
static_assert(sizeof(Settings) == 32, "IBL::Settings size must match HLSL IBLSettings packing");
static_assert(alignof(Settings) == 16, "IBL::Settings must remain 16-byte aligned");🤖 Prompt for AI Agents
In src/Features/IBL.h around lines 48 to 54, the 'pad' member is not in-class
initialized and can be indeterminate prior to RestoreDefaultSettings; initialize
'pad' to 0 in the declaration and add static_asserts to enforce layout: check
sizeof(Settings) equals the expected packed size and alignof(Settings) equals 16
to match HLSL packing. Ensure the static_assert messages clearly reference
"IBL::Settings size must match HLSL IBLSettings packing" and "IBL::Settings must
remain 16-byte aligned".
3 participants
This pull request enhances the integration of Image-Based Lighting (IBL) with fog rendering in the shader pipeline. The main change is the introduction of a new
FogAmountparameter and associated logic to blend IBL effects into fog color, making fog appearance more physically accurate and responsive to lighting settings. These changes are implemented across multiple shaders and exposed in the IBL settings UI.IBL and Fog Integration:
FogAmountparameter to theIBLSettingsstruct, allowing control over how much IBL affects fog color. This is exposed in the user interface as "Affects Fog Amount". [1] [2] [3] [4]GetFogIBLColorfunction inIBL.hlslito compute a fog color that blends the original fog color with IBL contributions, controlled by the newFogAmountparameter. [1] [2]Shader Modifications for Fog Rendering:
Effect.hlsl,Lighting.hlsl,ISSAOComposite.hlsl,Water.hlsl) to useGetFogIBLColorwhen IBL is enabled and not in interior scenes, ensuring consistent fog-IBL blending throughout the rendering pipeline. [1] [2] [3] [4] [5]Codebase and Dependency Updates:
IBL/IBL.hlsliin additional shader files to support the new fog-IBL blending logic. [1] [2]IBLSettingsto account for the new parameter. [1] [2]These changes collectively improve the realism and configurability of fog in scenes using IBL, allowing for more nuanced and dynamic atmospheric effects.
Summary by CodeRabbit