fix: fix invalid vanilla adaptation texture#1314
Conversation
WalkthroughThis change introduces HDR adaptation texture support and dynamic shader binding for HDR image space shaders in the deferred rendering pipeline. It adds new methods for adaptation shader management, replaces the LUT binding mechanism, updates class members, and removes related hooks. HDR shader hacks are now invoked during the draw phase. Changes
Sequence Diagram(s)sequenceDiagram
participant State as State::Draw
participant Deferred as Deferred
participant Shader as ImageSpaceShader
State->>Deferred: HDRShaderHacks()
alt Shader is image space shader
Deferred->>Shader: Check shader name
alt Name matches adaptation shader
Deferred->>Deferred: BindAdaptationShader()
else Name matches HDR shader
Deferred->>Deferred: BindHDRShader()
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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
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
🔭 Outside diff range comments (1)
src/Deferred.cpp (1)
812-822: Add null checks for adaptationTextures and confirm LUT slot safetyTo prevent potential null dereferences in
BindHDRShader(), guard against missing adaptation textures before indexing. A repository-wide search shows slot 100 is only used here, so the LUT binding is safe.
- File:
src/Deferred.cpp, lines 812–822void Deferred::BindHDRShader() { uint frameSwap = (globals::state->frameCount % 2); + + // Guard against missing adaptation textures + if (!adaptationTextures[0] || !adaptationTextures[1]) + return; auto srv = adaptationTextures[!frameSwap]->srv.get(); globals::d3d::context->PSSetShaderResources(2, 1, &srv); auto view = lutTexture.get(); if (view) globals::d3d::context->PSSetShaderResources(100, 1, &view); }
🧹 Nitpick comments (1)
src/Deferred.h (1)
82-82: Consider documenting the ping-pong buffer usage.The two-element array suggests a ping-pong buffer pattern for temporal adaptation. Consider adding a comment explaining this design choice to help future maintainers understand why two textures are needed.
+ // Ping-pong textures for HDR light adaptation across frames Texture2D* adaptationTextures[2];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Deferred.cpp(2 hunks)src/Deferred.h(2 hunks)src/State.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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#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-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
src/State.cpp (2)
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
src/Deferred.cpp (5)
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: alandtse
PR: #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.
Learnt from: alandtse
PR: #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 size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: #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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
src/Deferred.h (6)
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: alandtse
PR: #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 size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: #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.
Learnt from: alandtse
PR: #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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
⏰ 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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/State.cpp (1)
42-42: HDRShaderHacks edge-case handling verified
All requested checks are already in place and safe:
globals::state->currentShaderis guarded inHDRShaderHacks().adaptationTextures[0/1]are allocated unconditionally in the Deferred initialization (Deferred.cpp 115–125), soBindAdaptationShaderandBindHDRShadercan safely dereference them.- Frame-swap index uses
frameCount % 2, guaranteeing a 0 or 1 index even on overflow.BindHDRShaderadditionally checkslutTexture.get()before binding.No further error handling is needed. Approved.
src/Deferred.h (1)
38-42: LGTM! Well-structured API for HDR adaptation handling.The new methods provide a clean interface for managing HDR adaptation textures. The separation of concerns between detection (
HDRShaderHacks), adaptation binding (BindAdaptationShader), and HDR binding (BindHDRShader) is good design.src/Deferred.cpp (2)
111-124: Adaptation texture setup looks correct.The 2x2 R16G16B16A16_FLOAT format is appropriate for HDR adaptation textures. The creation of both SRV and RTV for each texture properly supports the ping-pong pattern.
769-799: Add null check and optimize static map usage.The current implementation correctly identifies HDR-related shaders, but has two issues:
- While
currentShaderis checked for null, the code should return early if it's null- The static map should be initialized once
void Deferred::HDRShaderHacks() { - if (globals::state->currentShader && globals::state->currentShader->shaderType.get() == RE::BSShader::Type::ImageSpace) { + if (!globals::state->currentShader) + return; + + if (globals::state->currentShader->shaderType.get() == RE::BSShader::Type::ImageSpace) { const auto& isShader = static_cast<const RE::BSImagespaceShader&>(*globals::state->currentShader); enum class ShaderAction { Adaptation, HDR }; - static const ankerl::unordered_dense::map<std::string_view, ShaderAction> shaderMap{ + static const ankerl::unordered_dense::map<std::string_view, ShaderAction> shaderMap = { { "BSImagespaceShaderHDRDownSample4LightAdapt", ShaderAction::Adaptation }, { "BSImagespaceShaderHDRDownSample16LightAdapt", ShaderAction::Adaptation }, { "BSImagespaceShaderHDRTonemapBlendCinematic", ShaderAction::HDR }, { "BSImagespaceShaderHDRTonemapBlendCinematicFade", ShaderAction::HDR } };⛔ Skipped due to learnings
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#0 File: :0-0 Timestamp: 2025-07-01T18:01:07.079Z Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.Learnt from: jiayev PR: doodlum/skyrim-community-shaders#0 File: :0-0 Timestamp: 2025-07-18T15:21:03.641Z Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.Learnt from: davo0411 PR: doodlum/skyrim-community-shaders#1107 File: src/Menu.cpp:310-315 Timestamp: 2025-05-31T02:51:24.195Z Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.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.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 size, alignment, and cross-platform compatibility when passing data to shader constant buffers.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.Learnt from: davo0411 PR: doodlum/skyrim-community-shaders#1070 File: src/State.cpp:79-83 Timestamp: 2025-05-30T11:44:15.542Z Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
|
✅ A pre-release build is available for this PR: |
2 participants
Fixes the vanilla adaptation texture randomly not being set, by just replacing it.
This could cause black screens because trying to access the texture would have random results.
Summary by CodeRabbit
New Features
Refactor