feat: hdr processing improvements#1311
Conversation
WalkthroughThis change replaces the LUT binding logic and associated hooks in the deferred rendering pipeline with a new HDR adaptation texture mechanism. It introduces double-buffered adaptation textures, new binding methods for HDR image space shaders, and updates the tone mapping shader to remove hue-preserving logic and apply a new chroma blending and vibrance boost. Hooks related to LUT binding are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant State
participant Deferred
participant Shader
State->>Deferred: Draw()
Deferred->>Deferred: HDRShaderHacks()
alt Shader is Adaptation Shader
Deferred->>Deferred: BindAdaptationShader()
Deferred->>Shader: Bind adaptation SRV/RTV
else Shader is HDR Shader
Deferred->>Deferred: BindHDRShader()
Deferred->>Shader: Bind adaptation SRV, LUT SRV
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–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
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
✅ A pre-release build is available for this PR: |
![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 (1)
src/Deferred.cpp (1)
769-799: Implementation looks correct, but consider the architectural approach.The shader name-based dispatch logic is sound and uses efficient data structures (
ankerl::unordered_dense::map). However, the method name "HDRShaderHacks" suggests this is a temporary workaround rather than a proper architectural solution.Consider renaming to something like
HandleHDRImageSpaceShaders()orProcessHDRShaderBinding()to better reflect its purpose as part of the HDR processing pipeline rather than a "hack."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/ISHDR.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (1)
src/Deferred.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
src/Deferred.cpp (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: 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 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: 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#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). (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 (3)
src/Deferred.cpp (3)
111-124: LGTM! Standard HDR adaptation texture setup.The implementation correctly creates double-buffered 2x2 adaptation textures with appropriate format (
DXGI_FORMAT_R16G16B16A16_FLOAT) for HDR luminance adaptation. The small texture size and ping-pong buffer approach is standard for HDR adaptation algorithms.
801-810: Correct ping-pong buffer implementation for HDR adaptation.The double-buffering logic properly alternates between reading from the previous frame's adaptation texture and writing to the current frame's texture. Using
frameCount % 2for synchronization and the!frameSwappattern for write targeting is standard and correct.
812-822: Correct HDR shader binding with proper sequencing.The method correctly binds the most recently updated adaptation texture (
!frameSwap) to slot 2, ensuring HDR shaders receive current adaptation data. The optional LUT texture binding with null check is also properly implemented.
2 participants
Fixed vanilla adaptation, replaced with one which actually works. Courtesy of kingeric under MIT. This means no more reports of adaptation going whack, no reports of black screens.
More traditional tonemapping for filmic tonemap, with a slight saturation boost to compensate.
Summary by CodeRabbit
New Features
Improvements
Removals