fix(linear lighting): fix horizon seam#1718
Conversation
Pushes the clear render (horizon seam) through linear conversion. Added slider for clear render with default set at 1.62
📝 WalkthroughWalkthroughAdds a configurable clear-render gamma and applies gamma-correction to the renderer clear color during prepass/reflections when linear lighting is enabled; exposes the setting in UI and caches converted values to avoid repeated conversions. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Globals.cpp`:
- Around line 267-278: The thunk function may dereference a null
pRenderTargetView and crash; add a null check at the start of thunk (before
calling pRenderTargetView->GetResource) to return early if pRenderTargetView is
nullptr. Locate the static void thunk(ID3D11DeviceContext* This,
ID3D11RenderTargetView* pRenderTargetView, const FLOAT ColorRGBA[4]) and ensure
it guards pRenderTargetView (and optionally rtvResource usage) so GetResource is
only called when pRenderTargetView is valid.
🧹 Nitpick comments (1)
src/Features/LinearLighting.cpp (1)
52-52: Consider adding a tooltip to explain this setting.The "Clear Render Gamma" label may not be immediately clear to users. A tooltip explaining that this affects the background color visible at worldspace edges and in water reflections would improve discoverability.
💡 Optional: Add tooltip
ImGui::SliderFloat("Clear Render Gamma", &settings.clearRenderGamma, 0.1f, 3.0f, "%.2f"); + if (ImGui::IsItemHovered()) + ImGui::SetTooltip("Gamma correction for the clear color visible at worldspace edges and in water reflections.");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Features/LinearLighting.cppsrc/Features/LinearLighting.hsrc/Globals.cpp
🧰 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/LinearLighting.hsrc/Features/LinearLighting.cppsrc/Globals.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/LinearLighting.hsrc/Features/LinearLighting.cppsrc/Globals.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:
- "Fixes
#123" or "Closes#123" for bug fixes- "Implements
#123" or "Addresses#123" for features- "Related to
#123" for partial implementationsOtherwise, use your standard review approach focusing on code quality.
Files:
src/Features/LinearLighting.hsrc/Features/LinearLighting.cppsrc/Globals.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 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
Repo: doodlum/skyrim-community-shaders PR: 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: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 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:
src/Features/LinearLighting.hsrc/Features/LinearLighting.cppsrc/Globals.cpp
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders 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.
Applied to files:
src/Features/LinearLighting.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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Globals.cpp (2)
280-310: LGTM!The resource comparison logic correctly identifies render targets by their underlying texture rather than RTV pointer. The gamma-to-linear conversion using
std::powis appropriate, and the COM resource reference is properly released. Good defensive coding with the null check onrtvResourcebeforeRelease().
317-322: Hook registration is correct. The vtable indices 14 (Map), 15 (Unmap), and 50 (ClearRenderTargetView) are accurate per the D3D11 interface specification. The hooks are properly structured using REL::Relocation for runtime compatibility and installed alongside existing D3D device hooks.src/Features/LinearLighting.h (1)
31-31: LGTM!The new
clearRenderGammasetting is appropriately placed in the Settings struct. The default value of 1.62f (as mentioned in the PR description) differs from other gamma values (1.8f) which appears intentional for the horizon seam correction. Correctly excluded fromPerFrameDatasince it's only used CPU-side in theClearRenderTargetViewhook.src/Features/LinearLighting.cpp (1)
7-7: LGTM!The
clearRenderGammafield is correctly added to the JSON serialization macro, enabling proper save/load of the setting.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…Dlizzio/skyrim-community-shaders into fix(LL)-clear-render-conversion
|
✅ A pre-release build is available for this PR: |
|
Clear render uses fog far color from weather but needs to be converted differently from fog. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/LinearLighting.cpp`:
- Around line 108-137: The current skip logic in
LinearLighting::ConvertClearColor only compares rendererData.clearColor to
lastConvertedClearColor so changes to settings.clearRenderGamma are ignored; fix
by caching the original (pre-conversion) clear color and the lastAppliedGamma,
detect when either the renderer's original clear color or
settings.clearRenderGamma has changed, and then recompute: if originalClearColor
is uninitialized capture the current rendererData.clearColor as
originalClearColor, if settings.clearRenderGamma != lastAppliedGamma or
rendererData.clearColor differs from lastConvertedClearColor restore from
originalClearColor then apply std::pow(originalClearColor[i],
settings.clearRenderGamma) into rendererData.clearColor[i], and update
lastConvertedClearColor and lastAppliedGamma accordingly; reference
LinearLighting::ConvertClearColor, settings.clearRenderGamma,
rendererData.clearColor, lastConvertedClearColor and introduce
originalClearColor and lastAppliedGamma fields.
|
This is nonsense. Not going to review PRs that are just using AI slop. |
Pushes the clear render (horizon seam) through linear conversion. Added slider for clear render with default set at 1.62.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.