feat: diffuse ibl#1054
Conversation
|
What's with the new submodules? Make sure the PR is clean. |
…ut in GetDiffuseIBL
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
All feature loading must be at defined entry points. The feature should also respect the disable at boot feature ideally with 0 impact if disabled at boot.
|
sure, i'll fix it later |
sure, I added some info in the PR. |
This reverts commit fa69c11. modified: features/IBL/Shaders/IBL/IBL.hlsli modified: package/Shaders/AmbientCompositeCS.hlsl
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
Looks good, please just update this PR to resolve conflicts, and fix distant trees not compiling. Also maybe we should bump the default dalc mix a bit higher, I don't know.
|
@coderabbitai please review this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2778 🏁 Script executed: Length of output: 4180 🏁 Script executed: Length of output: 22730 🏁 Script executed: Length of output: 4419 🏁 Script executed: Length of output: 68160 @alandtse I'll review this PR thoroughly. 🏁 Script executed: Length of output: 630 @alandtse I've completed a thorough review of PR #1054 which implements Image-Based Lighting (IBL). Submodule CheckI've verified there are no unwanted submodule changes in this PR:
IBL Implementation ReviewThis feature enhances Skyrim's lighting system by:
The implementation includes user-configurable settings:
Code QualityThe code is well-structured and follows project patterns:
The PR author has addressed all requested changes:
ConclusionThis PR looks ready to merge. It adds a valuable lighting enhancement to the shader system while maintaining code quality and addressing all review feedback. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis update introduces a new Image-Based Lighting (IBL) feature into the rendering pipeline. It adds shader code, configuration files, resource management, settings serialization, ImGui-based UI, and integrates the IBL feature into the deferred rendering process. The implementation handles compute shader dispatch, resource binding, and exposes new parameters for user control. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImGui
participant IBL
participant Renderer
participant Shader
participant Texture
User->>ImGui: Adjust IBL settings
ImGui->>IBL: Update Settings struct
Renderer->>IBL: EarlyPrepass()
IBL->>Texture: Bind DiffuseIBLTexture SRV to pixel shader
Renderer->>IBL: Prepass()
IBL->>Texture: Bind SRVs/UAVs for compute shader
IBL->>Shader: Dispatch DiffuseIBL compute shader
Shader->>Texture: Update DiffuseIBLTexture
IBL->>Texture: Rebind SRV for rendering
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
features/IBL/Shaders/IBL/IBL.hlsli (1)
15-24: Efficient spherical harmonics-based diffuse lighting.The
GetDiffuseIBLfunction implements an efficient approach to image-based lighting by:
- Loading spherical harmonics coefficients for each color channel
- Computing irradiance for each channel using the
SHHallucinateZH3Irradiancefunction- Returning the combined RGB result
This is more performant than traditional cubemap sampling for diffuse lighting.
Consider adding a brief comment explaining the spherical harmonics approach for maintainability:
float3 GetDiffuseIBL(float3 rayDir) { + // Sample spherical harmonics coefficients for each color channel + // and evaluate diffuse irradiance using 2nd order SH approximation sh2 shR = DiffuseIBLTexture.Load(int3(0, 0, 0)); sh2 shG = DiffuseIBLTexture.Load(int3(1, 0, 0)); sh2 shB = DiffuseIBLTexture.Load(int3(2, 0, 0)); float colorR = SphericalHarmonics::SHHallucinateZH3Irradiance(shR, rayDir); float colorG = SphericalHarmonics::SHHallucinateZH3Irradiance(shG, rayDir); float colorB = SphericalHarmonics::SHHallucinateZH3Irradiance(shB, rayDir); return float3(colorR, colorG, colorB); }src/Features/IBL.cpp (1)
83-91: Dispatch performed even when Diffuse IBL is disabled
settings.EnableDiffuseIBLis never consulted inPrepass(). Skipping the CS dispatch when the feature is disabled saves GPU cycles and avoids unnecessary state churn.-// IBL -{ +// IBL +if (settings.EnableDiffuseIBL) { … -} +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
features/IBL/Shaders/IBL/DiffuseIBLCS.hlslis excluded by!**/*.hlslpackage/Shaders/AmbientCompositeCS.hlslis excluded by!**/*.hlslpackage/Shaders/DistantTree.hlslis excluded by!**/*.hlslpackage/Shaders/Effect.hlslis excluded by!**/*.hlslpackage/Shaders/Lighting.hlslis excluded by!**/*.hlslpackage/Shaders/RunGrass.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (11)
features/IBL/Shaders/Features/IBL.ini(1 hunks)features/IBL/Shaders/IBL/IBL.hlsli(1 hunks)package/Shaders/Common/Color.hlsli(1 hunks)package/Shaders/Common/SharedData.hlsli(2 hunks)src/Deferred.cpp(9 hunks)src/Feature.cpp(2 hunks)src/FeatureBuffer.cpp(2 hunks)src/Features/IBL.cpp(1 hunks)src/Features/IBL.h(1 hunks)src/Globals.cpp(3 hunks)src/Globals.h(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Features/IBL.cpp (2)
src/Features/IBL.h (2)
o_json(25-25)o_json(26-26)src/Deferred.cpp (4)
SetupResources(56-179)SetupResources(56-56)ClearShaderCache(621-639)ClearShaderCache(621-621)
🔇 Additional comments (24)
features/IBL/Shaders/Features/IBL.ini (1)
1-2: Properly structured version identifierThe version identifier follows the project's version convention and correctly identifies this as version 1-0-0 of the IBL feature.
src/Globals.h (2)
9-9: Forward declaration for IBL struct looks goodThe forward declaration follows the project's pattern and is placed correctly alphabetically among other feature declarations.
59-59: Properly added global IBL pointerThe external IBL pointer is correctly added to the features namespace, maintaining alphabetical ordering among other feature pointers.
src/Feature.cpp (2)
10-10: IBL header include added appropriatelyThe IBL header is included in the proper alphabetical order with other feature headers.
151-152: Feature list registration looks goodThe IBL feature is correctly registered in the global features list, following the established pattern.
src/Globals.cpp (3)
20-20: IBL header inclusion is appropriateThe IBL header is correctly included alongside other feature headers in alphabetical order.
57-57: Properly initialized IBL pointer to nullptrThe global IBL pointer is properly declared and initialized to nullptr, following the pattern used for other features.
139-139: Singleton initialization follows established patternThe IBL feature is correctly initialized using the singleton pattern, consistent with how other features are initialized in the OnInit function.
src/FeatureBuffer.cpp (2)
8-8: Header inclusion in correct locationThe IBL header is appropriately included with other feature headers.
45-46: Properly added IBL settings to feature buffer dataThe IBL settings are correctly included in the feature buffer data, allowing the settings to be accessed in shaders.
package/Shaders/Common/Color.hlsli (1)
44-51: Well-implemented saturation adjustment function.The new
Saturationfunction provides a clean implementation for adjusting color saturation by linearly interpolating between grayscale and original color values. This supports the IBL feature's saturation control functionality.package/Shaders/Common/SharedData.hlsli (2)
164-172: Well-structured IBL settings with appropriate alignment.The
IBLSettingsstructure follows the project's established pattern for feature settings and includes all the required parameters for diffuse IBL:
- Enable/disable flag for diffuse IBL
- Scale factor to control intensity
- DALC (Directional Ambient Light Contribution) mixing amount
- Saturation control
- Option to sample under-horizon areas from dynamic cubemaps
- Proper padding for memory alignment
186-186: Correctly integrated into FeatureData buffer.The IBL settings are properly added to the FeatureData constant buffer, making them accessible to shaders.
src/Deferred.cpp (8)
8-8: Proper header inclusion.The IBL feature header is correctly included alongside other feature headers.
404-405: Proper singleton instance reference.The code correctly obtains a reference to the global IBL feature instance.
413-423: IBL texture properly integrated into ambient composite pass.The diffuse IBL texture is correctly added as a conditional shader resource view (SRV) in the ambient composite pass. The SRV array size is properly expanded from 8 to 9 elements.
438-439: Properly updated null SRV array for clearing.The array of null SRVs used for clearing is correctly expanded to match the new size of 9 elements.
462-478: IBL texture properly integrated into deferred composite pass.The diffuse IBL texture is correctly added as a conditional shader resource view (SRV) in the deferred composite pass. The SRV array size is properly expanded from 14 to 15 elements.
496-497: Properly updated null SRV array for clearing.The array of null SRVs used for clearing is correctly expanded to match the new size of 15 elements.
657-659: Proper shader compilation configuration.The code correctly adds the "IBL" define to the ambient composite shader compilation if the IBL feature is loaded, enabling the appropriate shader code paths.
700-702: Proper shader compilation configuration.The code correctly adds the "IBL" define to the main composite shader compilation if the IBL feature is loaded, enabling the appropriate shader code paths.
features/IBL/Shaders/IBL/IBL.hlsli (2)
1-5: Appropriate includes for required functionality.The shader includes all necessary dependencies:
- Math utilities for vector operations
- Random functions (though not used in this file)
- SharedData for access to the IBL settings
- SphericalHarmonics for efficient diffuse lighting calculations
6-14: Flexible texture register binding based on context.The conditional register binding for
DiffuseIBLTextureis well-implemented, handling different usage contexts:
- t8 for ambient composite pass
- t14 for deferred composite pass
- t76 as a fallback for other contexts
This approach ensures the texture is correctly bound across different shader stages.
src/Features/IBL.cpp (1)
155-163: Mismatch between shader compile-time define and runtime SRV set
DYNAMIC_CUBEMAPSis only defined at compile time if the feature is loaded whenGetDiffuseIBLCS()first runs. If Dynamic Cubemaps is loaded later (e.g., feature toggled on in-game), the compute shader will lack the define but the dispatch still passes cubemap SRVs, causing undefined shader behaviour.Compile the shader unconditionally with both code paths or detect feature state every frame:
bool dynLoaded = globals::features::dynamicCubemaps->loaded; defines.push_back({ "DYNAMIC_CUBEMAPS", dynLoaded ? "1" : "0" });and recompile when the flag changes (or just compile two variants and pick at runtime).
| Texture2D* diffuseIBLTexture = nullptr; | ||
| ID3D11ComputeShader* diffuseIBLCS = nullptr; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential GPU resource leak – diffuseIBLTexture never released
diffuseIBLTexture is created with new in SetupResources() but is not deleted anywhere (ClearShaderCache() only releases the CS). If the feature is toggled or the renderer is restarted, you will leak a GPU texture and its SRV/UAV.
Diff illustrating a minimal fix inside ClearShaderCache() (full RAII via std::unique_ptr<Texture2D> would be even cleaner):
void IBL::ClearShaderCache()
{
if (diffuseIBLCS)
diffuseIBLCS->Release();
diffuseIBLCS = nullptr;
+
+ delete diffuseIBLTexture;
+ diffuseIBLTexture = nullptr;
}🤖 Prompt for AI Agents
In src/Features/IBL.h around lines 19 to 21, the diffuseIBLTexture pointer is
allocated but never released, causing a GPU resource leak. To fix this, modify
the ClearShaderCache() method to properly release and delete diffuseIBLTexture,
ensuring the texture and its associated resources are freed when the feature is
toggled or the renderer restarts. Consider adding a safe release and delete call
for diffuseIBLTexture similar to how diffuseIBLCS is handled, or refactor to use
std::unique_ptr<Texture2D> for automatic resource management.
There was a problem hiding this comment.
No description provided.
There was a problem hiding this comment.
A summary of the edits CodeRabbit can apply:
Convert diffuseIBLTexture to std::unique_ptr in src/Features/IBL.h for automatic memory management.
Update src/Features/IBL.cpp to allocate diffuseIBLTexture using std::make_unique.
Will add the header include to src/Features/IBL.h for std::unique_ptr usage
Will update diffuseIBLTexture member to use std::unique_ptr in src/Features/IBL.h
Will replace raw allocation with std::make_unique in src/Features/IBL.cpp
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):
✏️ src/Features/IBL.h
After #pragma once on line 1, insert an include for the memory header so the top of the file reads:
#pragma once
#include <memory>
On line 19, replace the raw pointer declaration
Texture2D* diffuseIBLTexture = nullptr;
with a unique_ptr declaration:
std::unique_ptr<Texture2D> diffuseIBLTexture = nullptr;
✏️ src/Features/IBL.cpp
Locate the line:
diffuseIBLTexture = new Texture2D(texDesc);
and replace it with:
diffuseIBLTexture = std::make_unique<Texture2D>(texDesc);
Confirm that <memory> is available. Since IBL.cpp includes IBL.h (which now has <memory>), no additional include is required. If compilation errors occur, add:
#include <memory>
near the top of IBL.cpp alongside other headers.
Replace the allocation of diffuseIBLTexture with a std::make_unique call on line 142:
```
diffuseIBLTexture = std::make_unique(texDesc);
```
A summary of the context that CodeRabbit is considering across the codebase:
Locate the declaration of diffuseIBLTexture in IBL.h to determine where to apply the unique_ptr conversion.
Locate the allocation of diffuseIBLTexture in IBL.cpp to update it to use std::make_unique.
Find the declaration of diffuseIBLTexture in IBL.h to update its type to std::unique_ptr
Locate where to insert the memory header in IBL.h
Identify the line in IBL.cpp where diffuseIBLTexture is allocated with new to replace it with make_unique
- ✅ Create PR with these edits
- 📌 Create commit in current branch
- 📋 Get copyable edits
| struct alignas(16) Settings | ||
| { | ||
| uint EnableDiffuseIBL = 1; | ||
| float DiffuseIBLScale = 1.0f; | ||
| float DALCAmount = 0.3f; | ||
| float IBLSaturation = 0.75f; | ||
| uint SampleUnderHorizonFromDynCube = 0; | ||
| uint pad[3]; | ||
| } settings; |
There was a problem hiding this comment.
Use bool rather than uint for boolean flags to avoid UB with ImGui casts
EnableDiffuseIBL and SampleUnderHorizonFromDynCube are declared as uint but are manipulated through ImGui::Checkbox via a bool* cast in the CPP. This relies on the first byte of the uint matching bool layout – undefined behaviour on strict‐aliasing ABIs and a maintenance land-mine.
Recommended diff for the settings struct (padding can be adjusted afterward):
- uint EnableDiffuseIBL = 1;
+ bool EnableDiffuseIBL = true;
float DiffuseIBLScale = 1.0f;
float DALCAmount = 0.3f;
float IBLSaturation = 0.75f;
- uint SampleUnderHorizonFromDynCube = 0;
- uint pad[3];
+ bool SampleUnderHorizonFromDynCube = false;
+ uint32_t pad[2] = {};Then drop the (bool*) casts in DrawSettings() and pass the bool members directly.
This removes UB and makes the intent explicit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct alignas(16) Settings | |
| { | |
| uint EnableDiffuseIBL = 1; | |
| float DiffuseIBLScale = 1.0f; | |
| float DALCAmount = 0.3f; | |
| float IBLSaturation = 0.75f; | |
| uint SampleUnderHorizonFromDynCube = 0; | |
| uint pad[3]; | |
| } settings; | |
| struct alignas(16) Settings | |
| { | |
| bool EnableDiffuseIBL = true; | |
| float DiffuseIBLScale = 1.0f; | |
| float DALCAmount = 0.3f; | |
| float IBLSaturation = 0.75f; | |
| bool SampleUnderHorizonFromDynCube = false; | |
| uint32_t pad[2] = {}; | |
| } settings; |
🤖 Prompt for AI Agents
In src/Features/IBL.h around lines 33 to 41, change the types of
EnableDiffuseIBL and SampleUnderHorizonFromDynCube from uint to bool to
correctly represent boolean flags and avoid undefined behavior caused by casting
uint pointers to bool pointers in ImGui::Checkbox calls. After updating the
struct, remove the (bool*) casts in DrawSettings() and pass the bool members
directly to ImGui::Checkbox. Adjust padding if necessary to maintain alignment.
| void IBL::Prepass() | ||
| { | ||
| auto context = globals::d3d::context; | ||
|
|
||
| auto renderer = globals::game::renderer; | ||
| auto& reflections = renderer->GetRendererData().cubemapRenderTargets[RE::RENDER_TARGET_CUBEMAP::kREFLECTIONS]; | ||
|
|
||
| auto dynamicCubemaps = globals::features::dynamicCubemaps; | ||
|
|
||
| const auto& envTexture = dynamicCubemaps->envTexture; | ||
| const auto& envReflectionsTexture = dynamicCubemaps->envReflectionsTexture; | ||
|
|
||
| std::array<ID3D11ShaderResourceView*, 3> srvs = { reflections.SRV, envTexture->srv.get(), envReflectionsTexture->srv.get() }; | ||
| std::array<ID3D11UnorderedAccessView*, 1> uavs = { diffuseIBLTexture->uav.get() }; | ||
| std::array<ID3D11SamplerState*, 1> samplers = { Deferred::GetSingleton()->linearSampler }; |
There was a problem hiding this comment.
Null-dereference risk when Dynamic Cubemaps feature is disabled
dynamicCubemaps may not be loaded, yet envTexture and envReflectionsTexture are always accessed and pushed into srvs. If the feature is disabled these pointers will be nullptr, causing invalid SRVs and potential device removal.
Guard the entire IBL dispatch with both feature-enable and texture availability checks:
if (settings.EnableDiffuseIBL && globals::features::dynamicCubemaps->loaded) {
// gather SRVs, bind, dispatch …
}and early-out otherwise.
🤖 Prompt for AI Agents
In src/Features/IBL.cpp around lines 60 to 74, the code accesses envTexture and
envReflectionsTexture from dynamicCubemaps without checking if the
dynamicCubemaps feature is enabled or loaded, risking null dereference. To fix
this, add a guard that checks if settings.EnableDiffuseIBL is true and
globals::features::dynamicCubemaps->loaded is true before accessing these
textures and proceeding with the IBL dispatch. If the check fails, early return
from the function to avoid invalid SRVs and potential device removal.
| ImGui::Checkbox("Enable Diffuse IBL", (bool*)&settings.EnableDiffuseIBL); | ||
| 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::Checkbox("[EXP] Sample Under Horizon From Dynamic Cubemaps", (bool*)&settings.SampleUnderHorizonFromDynCube); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Undefined behaviour: casting uint* to bool* for ImGui checkboxes
Because EnableDiffuseIBL / SampleUnderHorizonFromDynCube are uint, the cast below violates strict aliasing and may write only one byte:
ImGui::Checkbox("Enable Diffuse IBL", (bool*)&settings.EnableDiffuseIBL);Switch the members to bool (see header comment) and pass them directly:
-ImGui::Checkbox("Enable Diffuse IBL", (bool*)&settings.EnableDiffuseIBL);
+ImGui::Checkbox("Enable Diffuse IBL", &settings.EnableDiffuseIBL);🤖 Prompt for AI Agents
In src/Features/IBL.cpp around lines 18 to 23, the code casts uint pointers to
bool pointers for ImGui::Checkbox, causing undefined behavior due to strict
aliasing violations. To fix this, change the types of EnableDiffuseIBL and
SampleUnderHorizonFromDynCube in the settings struct from uint to bool, then
pass their addresses directly to ImGui::Checkbox without casting.
| void IBL::SetupResources() | ||
| { | ||
| GetDiffuseIBLCS(); | ||
|
|
||
| { | ||
| D3D11_TEXTURE2D_DESC texDesc{ | ||
| .Width = 3, | ||
| .Height = 1, | ||
| .MipLevels = 1, | ||
| .ArraySize = 1, | ||
| .Format = DXGI_FORMAT_R16G16B16A16_FLOAT, | ||
| .SampleDesc = { 1, 0 }, | ||
| .Usage = D3D11_USAGE_DEFAULT, | ||
| .BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS, | ||
| .CPUAccessFlags = 0, | ||
| .MiscFlags = 0 | ||
| }; | ||
| D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc = { | ||
| .Format = texDesc.Format, | ||
| .ViewDimension = D3D11_SRV_DIMENSION_TEXTURE2D, | ||
| .Texture2D = { | ||
| .MostDetailedMip = 0, | ||
| .MipLevels = texDesc.MipLevels } | ||
| }; | ||
| D3D11_UNORDERED_ACCESS_VIEW_DESC uavDesc = { | ||
| .Format = texDesc.Format, | ||
| .ViewDimension = D3D11_UAV_DIMENSION_TEXTURE2D, | ||
| .Texture2D = { .MipSlice = 0 } | ||
| }; | ||
|
|
||
| diffuseIBLTexture = new Texture2D(texDesc); | ||
| diffuseIBLTexture->CreateSRV(srvDesc); | ||
| diffuseIBLTexture->CreateUAV(uavDesc); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Texture recreated without freeing old instance → leak & dangling SRV
If the renderer re-initialises resources (window resize, device reset) SetupResources() may be called again, allocating a new Texture2D without deleting the previous one. Pair this with the deletion highlighted in the header comment or convert to a smart pointer:
-if (diffuseIBLTexture)
- delete diffuseIBLTexture;
-diffuseIBLTexture = new Texture2D(texDesc);
+delete diffuseIBLTexture; // safe when nullptr
+diffuseIBLTexture = new Texture2D(texDesc);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Features/IBL.cpp around lines 112 to 145, the SetupResources() function
creates a new Texture2D instance for diffuseIBLTexture without releasing the
previous one, causing a memory leak and dangling shader resource views. To fix
this, before assigning a new Texture2D to diffuseIBLTexture, explicitly delete
or release the existing instance if it exists, or better, convert
diffuseIBLTexture to a smart pointer (e.g., std::unique_ptr) to manage its
lifetime automatically and prevent leaks.
|
Maybe you shouldn't use acronyms (IBL) as feature names to be more in line with other features |
|
✅ A pre-release build is available for this PR: |
1 similar comment
|
✅ A pre-release build is available for this PR: |
* migrated from https://github.com/doodlum/skyrim-community-shaders/tree/weather * fix: compile errors * fix: missing stuff * work (not really) * ibl using dynamic cubemap * fix: update IBL condition to exclude interiors * feat: add SampleUnderHorizonFromDynCube setting for dynamic cubemaps * style: 🎨 apply clang-format changes * feat: enhance dynamic cubemaps settings with tooltip and improved checkbox label * style: 🎨 apply clang-format changes * feat: update DALCAmount default value to 0.1 and normalize color output in GetDiffuseIBL * fix: refactor IBL methods and remove unused subprojects * refactor: remove unused Bind * add ignored 1/PI to ambient * using gamma for ibl color * change srv number to 76 to avoid potential conflict * Revert "using gamma for ibl color" This reverts commit fa69c11. modified: features/IBL/Shaders/IBL/IBL.hlsli modified: package/Shaders/AmbientCompositeCS.hlsl * feat: add ibl saturation, add saturation tool func, add ibl for effect * fix compile error * style: 🎨 apply clang-format changes * merge dev * fix hlsl compile error * set default dalc to 0.3 * remove PI, small restructure * change default --------- Co-authored-by: jiayev <jiayev@users.noreply.github.com>
4 participants
feature:
using ibl as/mixing with dalc
option to sample dynamic cubemaps for under horizon
This feature is basically sampling from ReflectionTexture to provide a global ambient diffuse light. The core implementation was mostly written by doodlum in the weather branch, I asked him and he said it's better to make it a seperate feature.
What I added is to optionally sample from dynamic cubemaps's env texture, because ReflectionTexture has only the sky and a black ground.
Summary by CodeRabbit