refactor(ibl): smooth blend dynamic cubemaps#1425
Conversation
WalkthroughReplaces horizon-based dynamic-cubemap gating with a continuous DynamicCubemapsAmount across C++ settings, shared cbuffer, and shaders. Diffuse IBL samples EnvReflectionsTexture for dynamic contribution and lerps with ReflectionTexture based on the amount; removes prior gamma override and horizon checks. Fog luminance uses Color::RGBToLuminance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Settings UI
participant CPU as IBL::Settings
participant CBuffer as SharedData::IBLSettings
participant Shader as DiffuseIBLCS
participant Output
User->>UI: Adjust DynamicCubemapsAmount (0.0..1.0)
UI->>CPU: set settings.DynamicCubemapsAmount
CPU->>CBuffer: upload updated iblSettings (new layout)
Shader->>CBuffer: read DynamicCubemapsAmount
Shader->>Shader: sample ReflectionTexture -> base
alt dcAmount <= 0.001
Shader->>Output: use base
else dcAmount >= 0.999
Shader->>Shader: sample EnvReflectionsTexture -> dynamic
Shader->>Output: use dynamic
else
Shader->>Shader: sample EnvReflectionsTexture -> dynamic
Shader->>Shader: color = lerp(base, dynamic, dcAmount)
Shader->>Output: write blended color
end
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 (4)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl (1)
42-49: Avoid double sampling when amount is 0 or 1; clamp amount in-shaderCurrent code always samples ReflectionTexture and may then sample EnvReflectionsTexture. When amount is 0 or 1 this is unnecessary work. Also, clamp the amount defensively to [0,1] in shader.
Apply:
- // Sample cubemap with optimized direction - float3 color = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; -#if defined(DYNAMIC_CUBEMAPS) - if (SharedData::iblSettings.DynamicCubemapsAmount > 0.0f) { - float3 dcColor = EnvReflectionsTexture.SampleLevel(LinearSampler, -rayDir, 0); - color = lerp(color, dcColor, SharedData::iblSettings.DynamicCubemapsAmount); - } -#endif + // Sample cubemap with optimized direction +#if defined(DYNAMIC_CUBEMAPS) + const float a = saturate(SharedData::iblSettings.DynamicCubemapsAmount); + float3 color; + if (a <= 0.0f) { + color = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; + } else if (a >= 1.0f) { + color = EnvReflectionsTexture.SampleLevel(LinearSampler, -rayDir, 0); + } else { + const float3 base = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; + const float3 dyn = EnvReflectionsTexture.SampleLevel(LinearSampler, -rayDir, 0); + color = lerp(base, dyn, a); + } +#else + float3 color = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; +#endifAlso confirm EnvReflectionsTexture is sampled in linear space (SRV created with a linear format, not sRGB), since gamma overrides were removed. See verification script below in IBL.cpp comment.
src/Features/IBL.cpp (3)
15-16: Serialization updated — migrate legacy SampleUnderHorizonFromDynCube to preserve user intentOld configs with SampleUnderHorizonFromDynCube=true will silently lose effect (defaults to 0.0). Perform a one-time migration during LoadSettings.
Add after deserialization (outside changed lines; shown for clarity):
void IBL::LoadSettings(json& o_json) { settings = o_json; // One-time migration from legacy boolean to new slider if (o_json.contains("SampleUnderHorizonFromDynCube")) { const bool legacy = o_json.value("SampleUnderHorizonFromDynCube", false); if (legacy && settings.DynamicCubemapsAmount == 0.0f) { settings.DynamicCubemapsAmount = 1.0f; } } // Defensive clamp for out-of-range configs settings.DynamicCubemapsAmount = std::clamp(settings.DynamicCubemapsAmount, 0.0f, 1.0f); }
26-26: UI: add defensive clamp after SliderFloat to keep value in-rangeImGui will clamp while interacting, but loading from JSON might bypass it. A second clamp keeps the value sane before pushing to GPU.
Apply right after the SliderFloat call:
- ImGui::SliderFloat("Dynamic Cubemaps Amount", &settings.DynamicCubemapsAmount, 0.0f, 1.0f, "%.2f"); + ImGui::SliderFloat("Dynamic Cubemaps Amount", &settings.DynamicCubemapsAmount, 0.0f, 1.0f, "%.2f"); + settings.DynamicCubemapsAmount = std::clamp(settings.DynamicCubemapsAmount, 0.0f, 1.0f);
29-31: Tooltip copy: clarify the caveatSmall copy tweak to be more specific about the edge case.
- "Samples from dynamic cubemaps.\n" - "Requires the Dynamic Cubemaps feature to be enabled.\n" - "May cause dynamic cubemaps sampling accumulation issues under certain conditions."); + "Samples ambient lighting from dynamic cubemaps.\n" + "Requires the Dynamic Cubemaps feature to be enabled.\n" + "Note: In rapidly changing scenes, accumulated sampling may produce minor instability.");
📜 Review details
Configuration used: Path: .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 (6)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl(1 hunks)features/IBL/Shaders/IBL/IBL.hlsli(1 hunks)package/Shaders/AmbientCompositeCS.hlsl(0 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/IBL.cpp(2 hunks)src/Features/IBL.h(1 hunks)
💤 Files with no reviewable changes (1)
- package/Shaders/AmbientCompositeCS.hlsl
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/IBL/Shaders/IBL/DiffuseIBLCS.hlslsrc/Features/IBL.hpackage/Shaders/Common/SharedData.hlslifeatures/IBL/Shaders/IBL/IBL.hlslisrc/Features/IBL.cpp
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/IBL/Shaders/IBL/DiffuseIBLCS.hlslfeatures/IBL/Shaders/IBL/IBL.hlsli
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/IBL.hsrc/Features/IBL.cpp
🧠 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-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
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.
📚 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/DiffuseIBLCS.hlslfeatures/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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Features/IBL.h (1)
49-51: DynamicCubemapsAmount added — confirm CPU↔GPU ABI and keep it future-proofLooks good. Please verify the struct packs exactly like the HLSL IBLSettings (two 16-byte rows). Add compile-time guards to catch drift.
You can add the following to enforce layout at build time:
#include <cstddef> static_assert(sizeof(IBL::Settings) == 32, "IBL::Settings must be 32 bytes (2 constant-buffer rows)."); static_assert(offsetof(IBL::Settings, DynamicCubemapsAmount) == 28, "Unexpected offset for DynamicCubemapsAmount.");features/IBL/Shaders/IBL/IBL.hlsli (1)
32-33: Switch to RGBToLuminance — confirm inputs are linear; guard against tiny luminanceChange aligns with the intent to preserve fog luminance. Ensure fogColor and iblColor are linear-space values at this point (they appear to be).
Optionally harden scaling to avoid blow-ups on near-zero iblLuminance:
// Replace the division line (outside the changed lines; shown for context) const float scale = fogLuminance / max(iblLuminance, 1e-5);package/Shaders/Common/SharedData.hlsli (1)
186-188: IBLSettings layout change — keep host mirror in lockstepThe new fields keep two 16-byte rows, which is ideal. Please ensure the C++ mirror (IBL::Settings) matches exactly and remain cautious about future insertions changing packing.
Consider pinning the ABI in a comment next to both structs:
- Row 0: uint, uint, float, float
- Row 1: float, float, float, float (DynamicCubemapsAmount, pad)
src/Features/IBL.cpp (1)
70-76: Action Required: Verify null-safety of Dynamic Cubemaps SRV bindingsPlease manually confirm that
envTextureandenvReflectionsTextureare only dereferenced when the feature is loaded and the textures are allocated.• Code location:
src/Features/IBL.cpp(around lines 70–76)
– The host currently does:
cpp const auto& envTexture = dynamicCubemaps.envTexture; const auto& envReflectionsTexture = dynamicCubemaps.envReflectionsTexture; std::array<ID3D11ShaderResourceView*, 3> srvs = { reflections.SRV, envTexture->srv.get(), envReflectionsTexture->srv.get() };
– IfdynamicCubemaps.loaded == falseor either texture isnullptr, these calls will null-dereference.• Verification steps:
- Ensure
globals::features::dynamicCubemaps.loadedis checked before any.srv.get()call.- Confirm that both
dynamicCubemaps.envTextureandenvReflectionsTexturedefault tonullptrwhen uninitialized.- Review the SRV creation paths to guarantee correct format (linear vs. sRGB) even when textures are absent.
• Recommended defensive pattern:
ID3D11ShaderResourceView* envSrv = nullptr; ID3D11ShaderResourceView* envRefSrv = nullptr; if (globals::features::dynamicCubemaps.loaded) { if (dynamicCubemaps.envTexture) envSrv = dynamicCubemaps.envTexture->srv.get(); if (dynamicCubemaps.envReflectionsTexture) envRefSrv = dynamicCubemaps.envReflectionsTexture->srv.get(); } std::array<ID3D11ShaderResourceView*, 3> srvs = { reflections.SRV, envSrv, envRefSrv };Note: Automated searches could not locate
IBL.cppin the sandbox—please verify the exact file path and ensure guard conditions are in place.
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl (1)
44-55: Hint the compiler to preserve branching (avoid accidental flattening).Because dcAmount is uniform across the group, branching is cheap and desired here to prevent sampling both cubemaps. Consider adding [branch] to discourage flattening on some driver/compiler combos.
- if (dcAmount <= 0.001f) { + [branch] if (dcAmount <= 0.001f) { color = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; - } else if (dcAmount >= 0.999f) { + } else [branch] if (dcAmount >= 0.999f) { color = EnvReflectionsTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; } else { const float3 base = ReflectionTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; const float3 dynamicCubemap = EnvReflectionsTexture.SampleLevel(LinearSampler, -rayDir, 0).xyz; color = lerp(base, dynamicCubemap, dcAmount); }
📜 Review details
Configuration used: Path: .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 (1)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl(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:
features/IBL/Shaders/IBL/DiffuseIBLCS.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/IBL/Shaders/IBL/DiffuseIBLCS.hlsl
🧠 Learnings (2)
📓 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.
📚 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/DiffuseIBLCS.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 (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl (3)
44-55: Dynamic cubemap blending logic is solid and avoids redundant sampling at the endpoints.Coherent, uniform branch on a cbuffer value; thresholds prevent double sampling; lerp path is clear. Nice use of saturate.
56-56: Fallback path remains correct without DYNAMIC_CUBEMAPS.Static-only sampling is preserved and consistent with prior behavior.
44-55: Ensure correct linear-space sampling for both cube mapsIt looks like in
DiffuseIBLCS.hlsl(lines 46–52) bothReflectionTextureandEnvReflectionsTextureare sampled directly—with noColor::GammaToLinear—whereas elsewhere in the codebase (e.g.Water.hlslandDeferredCompositeCS.hlsl) the environment reflections are explicitly decoded from sRGB (GammaToLinear) before use. Since the hardware will only auto-decode SRGB formats if the SRV is created with an_SRGBDXGI format (or texture is authored linear):
- You’ll need to confirm in the engine’s texture creation/bind code that both cube maps are:
- Created or cast as an
_SRGBformat (e.g.DXGI_FORMAT_R8G8B8A8_UNORM_SRGB), or- Written into linear-space targets (so no decode is needed).
If they aren’t, add an explicit
Color::GammaToLinear(...)around theSampleLevelcalls for whichever texture is actually in sRGB space to prevent lighting errors.• Features/IBL/Shaders/IBL/DiffuseIBLCS.hlsl:
– Lines 46–47, 49–52: sampling without gamma decode.
• Compare to package/Shaders/Water.hlsl (772–774) and DeferredCompositeCS.hlsl (140–144) which applyColor::GammaToLinear.
|
@coderabbitai is there a better conventional commit title < 50 chars? |
|
Tip For best results, initiate chat on the files or code changes. @alandtse Here are some better conventional commit title options under 50 characters:
The current title "refactor(ibl): sample from envreflections" (40 chars) is already under 50 characters, but these alternatives better capture the key change: replacing the boolean I'd recommend option 1 or 4 as they clearly describe the core architectural change from boolean gating to parametric blending. 🧠 Learnings used |
|
@jiayev do any of the suggestions for title seem better to you? I'm ready to squash this. |
jiayev
changed the title
retitled to refactor(ibl): smooth blend dynamic cubemaps. |
2 participants
This pull request refactors the handling of dynamic cubemaps in the image-based lighting (IBL) system. It removes the old experimental "SampleUnderHorizonFromDynCube" flag and introduces a new, more flexible
DynamicCubemapsAmountparameter. This allows dynamic cubemap blending to be smoothly controlled via a slider, improving usability and code clarity. Associated shader logic and UI elements have been updated to reflect this change, and some minor code cleanups were performed.Dynamic Cubemap Feature Refactor:
SampleUnderHorizonFromDynCubeflag from IBL settings and replaced it with a newDynamicCubemapsAmountfloat parameter for more granular blending control (IBL::Settings,SharedData::IBLSettings,IBL.h,SharedData.hlsli) [1] [2] [3].IBL::DrawSettings()to use a slider forDynamicCubemapsAmountinstead of a checkbox, with revised tooltip text for clarity (IBL.cpp).Shader Logic Updates:
DynamicCubemapsAmountvalue, removing the previous horizon-based sampling logic (DiffuseIBLCS.hlsl).Code Cleanup and Consistency:
AmbientCompositeCS.hlsl).IBL.hlsli).Summary by CodeRabbit
New Features
Changes