feat(ibl): use static ibl textures for outworld objects#1434
Conversation
WalkthroughAdds a static IBL pathway: new static cube textures, a GetStaticDiffuseIBL API, shader conditionals to use static diffuse IBL for out-of-world/reflection cases, early-return logic in dynamic cubemap shaders, a new UseStaticIBL setting with UI, resource loading/binding for static IBL textures, and a SharedData IBLSettings layout update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PS as Pixel Shader (Lighting.hlsl)
participant SD as SharedData::IBLSettings
participant IBL as IBL.hlsli
participant TexD as StaticDiffuseIBLTexture
participant Dyn as Dynamic IBL/Dyn Cubemap
PS->>SD: Read EnableDiffuseIBL, UseStaticIBL, flags
alt Diffuse IBL enabled AND UseStaticIBL AND not InWorld/InReflection
PS->>IBL: GetStaticDiffuseIBL(worldNormal, SampColorSampler)
IBL->>TexD: Sample N.xzy
TexD-->>IBL: radiance/PI
IBL-->>PS: diffuse color
note right of PS: directionalAmbientColor set (static)
else
PS->>Dyn: Existing dynamic IBL path
Dyn-->>PS: diffuse contribution
end
sequenceDiagram
autonumber
participant DC as DynamicCubemaps.hlsli
participant SD as SharedData::IBLSettings
participant IBL as IBL.hlsli
participant TexD as StaticDiffuseIBLTexture
DC->>SD: Check EnableDiffuseIBL & UseStaticIBL
alt IBL+LIGHTING and not InWorld/InReflection
DC->>IBL: GetStaticDiffuseIBL(R, sampler)
IBL->>TexD: Sample R.xzy
TexD-->>IBL: radiance/PI
IBL-->>DC: color
DC-->>DC: Early return (finalIrradiance / specular term path)
else
DC-->>DC: Continue existing dynamic cubemap logic
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/Features/IBL.h (1)
53-55: Unify resource ownership for IBL textures.Currently diffuseIBLTexture is a raw pointer while the new static textures use eastl::unique_ptr. Prefer a consistent ownership model to avoid lifetime surprises.
- Option A (preferred): make diffuseIBLTexture a unique_ptr too.
- Option B: keep raw pointers but document ownership and ensure a single owner handles release.
Nit: unique_ptrs default-initialize to nullptr; the explicit “= nullptr” is redundant.
Apply this minimal style tweak:
- eastl::unique_ptr<Texture2D> staticDiffuseIBLTexture = nullptr; - eastl::unique_ptr<Texture2D> staticSpecularIBLTexture = nullptr; + eastl::unique_ptr<Texture2D> staticDiffuseIBLTexture; + eastl::unique_ptr<Texture2D> staticSpecularIBLTexture;Outside this hunk, consider (if feasible):
// Replace Texture2D* diffuseIBLTexture = nullptr; // With eastl::unique_ptr<Texture2D> diffuseIBLTexture;features/IBL/Shaders/IBL/IBL.hlsli (1)
32-37: Static diffuse IBL color space and parity with dynamic path.
- Color space: other env textures are sampled with Color::GammaToLinear(...). If the SRV for StaticDiffuseIBLTexture is not created with an sRGB format, we should convert here for correctness.
- Parity: returning irradiance/PI is fine. The IBLSaturation and DiffuseIBLScale are applied at call site (Lighting.hlsl), which is good for consistency.
If SRV isn’t sRGB, consider:
- return StaticDiffuseIBLTexture.SampleLevel(samp, N.xzy, 0).xyz / Math::PI; + return Color::GammaToLinear(StaticDiffuseIBLTexture.SampleLevel(samp, N.xzy, 0).xyz) / Math::PI;features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (2)
40-48: Early static-specular path: color space and gating.
- Color space: Env textures use Color::GammaToLinear(...) but StaticSpecularIBLTexture does not. If the SRV isn’t sRGB, we’ll under-light. Align sampling:
- float3 specularIrradiance = ImageBasedLighting::StaticSpecularIBLTexture.SampleLevel(SampColorSampler, R.xzy, level).xyz; + float3 specularIrradiance = Color::GammaToLinear(ImageBasedLighting::StaticSpecularIBLTexture.SampleLevel(SampColorSampler, R.xzy, level).xyz);
- Gating: Using EnableDiffuseIBL to gate a specular fallback is surprising. Consider basing this solely on UseStaticIBL (or a separate specular flag) so disabling diffuse IBL doesn’t inadvertently disable static specular.
107-116: Mirror the same fixes in GetDynamicCubemap().Apply the same color-space conversion and gating rationale as above for consistency:
- float3 specularIrradiance = ImageBasedLighting::StaticSpecularIBLTexture.SampleLevel(SampColorSampler, R.xzy, level).xyz; + float3 specularIrradiance = Color::GammaToLinear(ImageBasedLighting::StaticSpecularIBLTexture.SampleLevel(SampColorSampler, R.xzy, level).xyz);Also confirm this header is only included from contexts that define SampColorSampler; otherwise consider using LinearSampler or threading a sampler parameter.
src/Features/IBL.cpp (4)
164-184: Add nullptr check after texture creation.The code correctly handles exceptions but doesn't check if
pResourceis nullptr after successful creation. While unlikely with proper error handling, a defensive check would be prudent.ID3D11Resource* pResource = nullptr; try { DX::ThrowIfFailed(CreateTexture(device, image.GetImages(), image.GetImageCount(), image.GetMetadata(), &pResource)); } catch (const DX::com_exception& e) { logger::error("{}", e.what()); return; } + +if (!pResource) { + logger::error("Failed to create diffuse IBL texture resource"); + return; +}
212-220: Add nullptr check for consistency.Similar to the diffuse texture loading, add a defensive nullptr check after resource creation.
ID3D11Resource* pResource = nullptr; try { DX::ThrowIfFailed(CreateTexture(device, image.GetImages(), image.GetImageCount(), image.GetMetadata(), &pResource)); } catch (const DX::com_exception& e) { logger::error("{}", e.what()); return; } + +if (!pResource) { + logger::error("Failed to create specular IBL texture resource"); + return; +}
168-169: Consider making texture paths configurable.The hardcoded paths
"Data\\Shaders\\IBL\\DiffuseIBL.dds"and"Data\\Shaders\\IBL\\SpecIBL.dds"could be made configurable or at least defined as constants for better maintainability.Add path constants at the top of the file:
namespace { constexpr const char* DIFFUSE_IBL_PATH = "Data\\Shaders\\IBL\\DiffuseIBL.dds"; constexpr const char* SPECULAR_IBL_PATH = "Data\\Shaders\\IBL\\SpecIBL.dds"; }Then use them in the loading code:
-std::filesystem::path path = "Data\\Shaders\\IBL\\DiffuseIBL.dds"; +std::filesystem::path path = DIFFUSE_IBL_PATH;-std::filesystem::path path = "Data\\Shaders\\IBL\\SpecIBL.dds"; +std::filesystem::path path = SPECULAR_IBL_PATH;Also applies to: 204-205
164-234: Consider refactoring duplicated texture loading logic.The loading code for diffuse and specular IBL textures is nearly identical. Consider extracting a helper function to reduce duplication and improve maintainability.
eastl::unique_ptr<Texture2D> LoadStaticIBLTexture(const std::filesystem::path& path, uint32_t mipLevels, const char* textureName) { auto device = globals::d3d::device; logger::debug("Loading static {} IBL texture...", textureName); DirectX::ScratchImage image; try { DX::ThrowIfFailed(LoadFromDDSFile(path.c_str(), DirectX::DDS_FLAGS_NONE, nullptr, image)); } catch (const DX::com_exception& e) { logger::error("Failed to load {}: {}", textureName, e.what()); return nullptr; } ID3D11Resource* pResource = nullptr; try { DX::ThrowIfFailed(CreateTexture(device, image.GetImages(), image.GetImageCount(), image.GetMetadata(), &pResource)); } catch (const DX::com_exception& e) { logger::error("Failed to create {} texture: {}", textureName, e.what()); return nullptr; } if (!pResource) { logger::error("Failed to create {} texture resource", textureName); return nullptr; } auto texture = eastl::make_unique<Texture2D>(reinterpret_cast<ID3D11Texture2D*>(pResource)); texture->desc.MiscFlags |= D3D11_RESOURCE_MISC_TEXTURECUBE; D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc = { .Format = texture->desc.Format, .ViewDimension = D3D11_SRV_DIMENSION_TEXTURECUBE, .TextureCube = { .MostDetailedMip = 0, .MipLevels = mipLevels } }; texture->CreateSRV(srvDesc); return texture; }Then replace the loading blocks with:
staticDiffuseIBLTexture = LoadStaticIBLTexture("Data\\Shaders\\IBL\\DiffuseIBL.dds", 1, "Diffuse"); staticSpecularIBLTexture = LoadStaticIBLTexture("Data\\Shaders\\IBL\\SpecIBL.dds", 8, "Specular");
📜 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 ignored due to path filters (2)
features/IBL/Shaders/IBL/DiffuseIBL.ddsis excluded by!**/*.ddsfeatures/IBL/Shaders/IBL/SpecIBL.ddsis excluded by!**/*.dds
📒 Files selected for processing (6)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli(3 hunks)features/IBL/Shaders/IBL/IBL.hlsli(4 hunks)package/Shaders/Common/SharedData.hlsli(1 hunks)package/Shaders/Lighting.hlsl(1 hunks)src/Features/IBL.cpp(4 hunks)src/Features/IBL.h(1 hunks)
🧰 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:
src/Features/IBL.hfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslifeatures/IBL/Shaders/IBL/IBL.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/Features/IBL.cpp
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
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.hlslifeatures/IBL/Shaders/IBL/IBL.hlsli
🧠 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.
📚 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.hlslifeatures/IBL/Shaders/IBL/IBL.hlslipackage/Shaders/Lighting.hlsl
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
package/Shaders/Common/SharedData.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 (7)
package/Shaders/Common/SharedData.hlsli (1)
178-188: UseStaticIBL added as uint; layout stays 32 bytes — good. Also ensure cache invalidation.
- The field type matches our HLSL/C++ interop convention for flags (uint), and the struct still packs cleanly: [3 uints + 1 float] = 16 bytes, followed by 4 floats = 16 bytes (total 32). No padding needed.
- Action: because SharedData::IBLSettings layout changed (pad removed), please ensure shader caches are invalidated and all permutations are rebuilt to avoid stale bytecode mismatches.
- Action: confirm the CPU-side struct (src/Features/IBL.h::Settings) mirrors this exact order and size across serialization and upload paths.
src/Features/IBL.h (1)
41-51: New flag default: double-check serialization/back-compat and user-visible default.
- Defaulting UseStaticIBL to 1 will change visuals for out-of-world objects on first run for existing users. Confirm the NLOHMANN serialization macro includes this field with a sane default when absent in older configs, and that UI text/tooltips reflect the scope (out-of-world, non-reflection).
- Also verify that failure to load the static DDS assets gracefully forces the flag off (or falls back) to avoid black ambient/specular if SRVs end up null.
features/IBL/Shaders/IBL/IBL.hlsli (2)
1-3: Include guard added — good.Prevents multiple inclusion when IBL.hlsli is pulled in from multiple shader modules (e.g., Lighting and DynamicCubemaps).
18-20: Please confirm no register conflicts for t77/t78 across all pixel-shader permutations
- A full-text scan for
register(t77)andregister(t78)only finds the two lines infeatures/IBL/Shaders/IBL/IBL.hlsli(our new StaticDiffuseIBLTexture/StaticSpecularIBLTexture bindings).- No occurrences of a direct
PSSetShaderResources(76, 3, …)call were detected; however, the engine may bind resources via wrapper APIs or descriptor tables instead of that exact function.- Because absence of matches isn’t definitive, please double-check any engine code paths (e.g.,
commandList->SetShaderResources, descriptor-table setups, or other PS bindings) to ensure no other feature ever targets slots t77 or t78 alongside IBL.features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (1)
7-10: Including IBL.hlsli behind an IBL define is fine.IBL.hlsli now has guards; no circular include issue. Keeps symbol visibility scoped to IBL permutations.
src/Features/IBL.cpp (2)
8-9: Verify presence and usage of DDSTextureLoader in your IBL implementationI was unable to locate
src/Features/IBL.cppin the repository or find any usages of DDSTextureLoader APIs. Please manually verify:
- That the IBL implementation exists at the expected path (e.g.
src/Features/IBL.cppor a similarly named file).- That there are no remaining calls to
CreateDDSTextureFrom…orCreateWICTextureFrom…in that file.- That you only rely on DirectXTex routines (e.g.
LoadFromDDSFile,CreateTexture) before removing#include <DDSTextureLoader.h>.If the file path or name differs, adjust your search accordingly. Once confirmed, you can safely remove the redundant DDS loader header.
186-198: Unable to locateTexture2Ddefinition—please verify descriptor ownership post-constructionI wasn’t able to find the
Texture2Dclass or its constructor in the repo (no matching headers or source files), so I can’t confirm whether its internal descriptor (desc) is safely copy-on-construct or if mutating it later may desynchronize the actual D3D11 resource. Without that certainty, this remains a potential ownership/consistency risk.• Please locate and review the
Texture2D(ID3D11Texture2D*)constructor to see howdescis initialized or cached.
• Ensure that modifyingdesc.MiscFlagsafter construction accurately reflects the real resource’s flags and doesn’t conflict with any internal invariants.
|
✅ A pre-release build is available for this PR: |
2 participants
This pull request adds support for static image-based lighting (IBL) textures for out-of-world objects, such as inventory items, and refactors the relevant shader and engine code to enable this feature. The main changes include new shader resource bindings, logic to select static IBL textures, and updates to configuration and resource management.
Static IBL Feature Implementation:
StaticDiffuseIBLTexture,StaticSpecularIBLTexture) toImageBasedLightingand bound them as shader resources. [1] [2] [3]DynamicCubemaps.hlsli,Lighting.hlsl) to use static IBL textures for objects not in the world or reflection contexts, controlled by the newUseStaticIBLsetting. [1] [2] [3]Configuration and UI:
UseStaticIBLsetting to the IBL configuration, exposed it in the UI with a tooltip, and updated serialization. [1] [2] [3]Resource Management:
DiffuseIBL.dds,SpecIBL.dds), including error handling and creation of cube map shader resource views.Shader Inclusion and Guarding:
IBL.hlsliand ensured it is included where needed, such as inDynamicCubemaps.hlsli. [1] [2]These changes collectively enable improved lighting for out-of-world objects and allow for flexible configuration and resource management of static IBL textures.
Summary by CodeRabbit
New Features
Improvements