feat: linear lighting#1359
Conversation
This reverts commit 4d520e5.
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: 1
♻️ Duplicate comments (5)
features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl (1)
66-67: Clamp before gamma conversion to avoid undefined behavior.The HORIZONTAL path (line 56) correctly uses
max(0, color)before writing, but the VERTICAL path appliesIrradianceToGammawithout clamping. Gamma encoding on negative values is undefined.🔎 Proposed fix
- color.rgb = Color::IrradianceToGamma(color.rgb); - SSSRW[DTid.xy] = float4(color.rgb, 1.0); + color.rgb = Color::IrradianceToGamma(max(0, color.rgb)); + SSSRW[DTid.xy] = float4(color.rgb, 1.0);package/Shaders/Lighting.hlsl (2)
1868-1879: Hair tint/vertex color: prefer scalar weight instead ofColorToLinearon scalarBoth hair tint paths use
Color::ColorToLinear(input.Color.y)(a float3) as the lerp weight:// FACEGEN hair tint (CS_HAIR) hairTint = lerp(1, Color::Diffuse(TintColor.xyz), Color::ColorToLinear(input.Color.y)); // General HAIR vertexColor float3 vertexColor = lerp(1, Color::ColorToLinear(TintColor.xyz), Color::ColorToLinear(input.Color.y));HLSL will promote
1tofloat3(1,1,1)and treat the weight as a float3, but semantically this is a scalar mask. It’s clearer (and avoids any per‑channel surprises) to compute a scalar weight explicitly:float weight = SharedData::linearLightingSettings.enableLinearLighting ? pow(abs(input.Color.y), SharedData::linearLightingSettings.colorGamma) : input.Color.y; hairTint = lerp(1.0.xxx, Color::Diffuse(TintColor.xyz), weight); vertexColor = lerp(1.0.xxx, Color::ColorToLinear(TintColor.xyz), weight);This keeps behavior explicit and mirrors how other LL helpers treat scalar inputs.
Also applies to: 1882-1890, 2817-2818
2386-2388: Still vulnerable to INF·0 NaNs whendirLightMulthits zeroHere you compute:
float llDirLightMult = SharedData::linearLightingSettings.enableLinearLighting && !SharedData::linearLightingSettings.isDirLightLinear && (inWorld || inReflection) && !SharedData::InInterior ? SharedData::linearLightingSettings.dirLightMult : 1.0f; float3 dirLightColor = Color::DirectionalLight(DirLightColor.xyz / max(llDirLightMult, 1e-5), SharedData::linearLightingSettings.isDirLightLinear) * llDirLightMult;If
dirLightMultbecomes 0, this path does:
- divide by
1e-5→ very large colorColor::Lightthen appliespow(large, lightGamma)- result can overflow to
INF- multiplied by
llDirLightMult == 0→INF * 0→ NaNThe shader-side
maxfixes divide‑by‑zero but not this overflow/NaN case. ClampingdirLightMultonce inLinearLighting::Prepass()(as suggested in that file) is the safest fix; once clamped, this pattern becomes stable everywhere that usesllDirLightMult.src/Features/LinearLighting.cpp (1)
96-108: ClampdirLightMultinPrepass()to avoid NaNs downstream
dirLightMultis taken directly from ImageSpaceManager and can be 0 (e.g., exotic weather/sun mods). Shaders then compute:
llDirLightMult = ... ? dirLightMult : 1.0f;- divide colors by
max(llDirLightMult, 1e-5)- run them through
powinColor::Light- multiply back by
llDirLightMultIf
dirLightMult == 0, this becomespow(huge, gamma) * 0, which can still produce NaNs.Clamp once here to keep shader math safe:
dirLightMult = !globals::game::isVR ? imageSpaceManager->GetRuntimeData().data.baseData.hdr.sunlightScale : imageSpaceManager->GetVRRuntimeData().data.baseData.hdr.sunlightScale; + if (dirLightMult < 1e-5f) { + dirLightMult = 1e-5f; + }This also simplifies shader‑side epsilon handling.
package/Shaders/Common/Color.hlsli (1)
118-127: Bug:GammaToLinearLuminancePreservingcalls.xon a scalarHere:
float originalLuminance = max(RGBToLuminance(color), 1e-5); float3 linearColorRaw = GammaToLinear(color / originalLuminance); float scale = GammaToLinear(originalLuminance).x; return linearColorRaw * scale;
GammaToLinear(float)returns a scalar, so.xis invalid HLSL and will either fail compilation or rely on undefined behavior.Use the scalar overload directly:
- float scale = GammaToLinear(originalLuminance).x; + float scale = GammaToLinear(originalLuminance);This matches the intent (convert luminance once, then scale the normalized color).
🧹 Nitpick comments (8)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (1)
111-113: Minor optimization opportunity: avoid round-trip conversion.Converting
directionalAmbientColorSpecularto linear, multiplying byskylightingSpecular, then converting back to gamma is functionally correct but involves redundant conversions. Consider keeping in linear space through subsequent operations if possible.package/Shaders/DeferredCompositeCS.hlsl (1)
236-242: Consistent with DynamicCubemaps pattern, but consider optimization.The round-trip conversion (linear → multiply → gamma) at lines 236-238 mirrors the pattern in DynamicCubemaps.hlsli. While functionally correct, this creates extra ALU overhead.
package/Shaders/Effect.hlsl (1)
635-637: Consider refactoring repeated point light color computation.The pattern
Color::PointLight(PLightColorX.xxx).x * lightFadeMul * Color::EffectLightingMult()is repeated for R, G, B channels. Could extract to a helper or compute once.🔎 Suggested refactor
- color.x += dot(Color::PointLight(PLightColorR.xxx).x * lightFadeMul * Color::EffectLightingMult(), 1.0.xxxx); - color.y += dot(Color::PointLight(PLightColorG.xxx).x * lightFadeMul * Color::EffectLightingMult(), 1.0.xxxx); - color.z += dot(Color::PointLight(PLightColorB.xxx).x * lightFadeMul * Color::EffectLightingMult(), 1.0.xxxx); + float effectLightMult = Color::EffectLightingMult(); + color.x += dot(Color::PointLight(PLightColorR.xxx).x * lightFadeMul * effectLightMult, 1.0.xxxx); + color.y += dot(Color::PointLight(PLightColorG.xxx).x * lightFadeMul * effectLightMult, 1.0.xxxx); + color.z += dot(Color::PointLight(PLightColorB.xxx).x * lightFadeMul * effectLightMult, 1.0.xxxx);src/Features/InverseSquareLighting.cpp (1)
70-71: Remove commented-out code or add explanatory comment.The commented lines
// light.color *= intensity;and// light.color *= runtimeData->fade;appear to be intentionally disabled as part of the linear lighting refactor (separating color from intensity via the newfadefield). Either remove them entirely or add a comment explaining why they're preserved.🔎 Suggested cleanup
light.color /= std::max(0.001f, std::max(light.color.x, std::max(light.color.y, light.color.z))); - // light.color *= intensity; light.fade = intensity; } else { light.radius = runtimeData->radius; light.invRadius = 1.f / light.radius; - // light.color *= runtimeData->fade; light.fade = runtimeData->fade; }Also applies to: 75-76
src/Features/LightLimitFix.cpp (2)
238-242: Remove commented-out code.The commented line
// light.color *= runtimeData.fade;is obsolete and should be removed now that fade is handled via the separatelight.fadefield.🔎 Proposed cleanup
} else { light.radius = runtimeData.radius.x; - // light.color *= runtimeData.fade; light.fade = runtimeData.fade; }
420-424: Remove commented-out code.The commented line
// light.color *= runtimeData.fade;is obsolete and should be removed for consistency with the new fade handling approach.🔎 Proposed cleanup
} else { light.radius = runtimeData.radius.x; - // light.color *= runtimeData.fade; light.fade = runtimeData.fade; }package/Shaders/RunGrass.hlsl (1)
651-715: Use LightLimitFixLinearflag for clustered grass lights as wellIn the GRASS_LIGHTING + LIGHT_LIMIT_FIX path, clustered point lights currently use:
float3 lightColor = Color::PointLight(light.color.xyz) * intensityMultiplier * light.fade;The non‑grass path already respects
LightLimitFix::LightFlags::Linearand passesisPointLightLinearintoColor::PointLight. For consistency and to avoid double gamma on linear lights, mirror that here:- float3 lightColor = Color::PointLight(light.color.xyz) * intensityMultiplier * light.fade; + const bool isPointLightLinear = (light.lightFlags & LightLimitFix::LightFlags::Linear) != 0; + float3 lightColor = Color::PointLight(light.color.xyz, isPointLightLinear) * intensityMultiplier * light.fade;src/Features/LinearLighting.h (1)
3-26: PR metadata: consider tightening the conventional title and linking issuesThe current PR title
feat: linear lighting (#1359)is close to conventional commits; you might make it more specific and ensure it links the tracking issue:
- Suggested title:
feat(lighting): add linear lighting pipeline- In the PR description/body, add e.g.:
Implements #1359(for the feature)- or
Related to #1359if this is foundational work.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslifeatures/Hair Specular/Shaders/Hair/Hair.hlslifeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslifeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Skylighting/Shaders/Skylighting/Skylighting.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslpackage/Shaders/Common/Color.hlslipackage/Shaders/Common/LightingEval.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/Effect.hlslpackage/Shaders/ISCompositeLensFlareVolumetricLighting.hlslpackage/Shaders/ISHDR.hlslpackage/Shaders/ISSAOComposite.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslpackage/Shaders/Water.hlslsrc/Feature.cppsrc/Features/InverseSquareLighting.cppsrc/Features/InverseSquareLighting/Common.hsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.cppsrc/Features/LinearLighting.h
🚧 Files skipped from review as they are similar to previous changes (5)
- package/Shaders/ISSAOComposite.hlsl
- features/Hair Specular/Shaders/Hair/Hair.hlsli
- package/Shaders/ISCompositeLensFlareVolumetricLighting.hlsl
- src/Feature.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/InverseSquareLighting/Common.hfeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Water.hlslpackage/Shaders/Common/SharedData.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlslisrc/Features/InverseSquareLighting.cpppackage/Shaders/ISHDR.hlslpackage/Shaders/Effect.hlslpackage/Shaders/Common/LightingEval.hlslipackage/Shaders/DistantTree.hlslpackage/Shaders/RunGrass.hlslsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.cppfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslpackage/Shaders/Lighting.hlslfeatures/Skylighting/Shaders/Skylighting/Skylighting.hlslipackage/Shaders/DeferredCompositeCS.hlslsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/LinearLighting.hfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/Common/Color.hlsli
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/InverseSquareLighting/Common.hsrc/Features/InverseSquareLighting.cppsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.cppsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/LinearLighting.h
**/*
⚙️ 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:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/InverseSquareLighting/Common.hfeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Water.hlslpackage/Shaders/Common/SharedData.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlslisrc/Features/InverseSquareLighting.cpppackage/Shaders/ISHDR.hlslpackage/Shaders/Effect.hlslpackage/Shaders/Common/LightingEval.hlslipackage/Shaders/DistantTree.hlslpackage/Shaders/RunGrass.hlslsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.cppfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslpackage/Shaders/Lighting.hlslfeatures/Skylighting/Shaders/Skylighting/Skylighting.hlslipackage/Shaders/DeferredCompositeCS.hlslsrc/Features/InverseSquareLighting/LightEditor.cppsrc/Features/LinearLighting.hfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/Common/Color.hlsli
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/Light Limit Fix/Shaders/LightLimitFix/Common.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Skylighting/Shaders/Skylighting/Skylighting.hlslifeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
🧠 Learnings (13)
📓 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-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.
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders 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.
Applied to files:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders 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.
Applied to files:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Applied to files:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/Common/Color.hlsli
📚 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:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/Water.hlslpackage/Shaders/Common/SharedData.hlslifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlslisrc/Features/InverseSquareLighting.cpppackage/Shaders/ISHDR.hlslpackage/Shaders/Effect.hlslpackage/Shaders/Common/LightingEval.hlslipackage/Shaders/DistantTree.hlslpackage/Shaders/RunGrass.hlslsrc/Features/LightLimitFix.cppsrc/Features/LinearLighting.cppfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslpackage/Shaders/Lighting.hlslfeatures/Skylighting/Shaders/Skylighting/Skylighting.hlslipackage/Shaders/DeferredCompositeCS.hlslsrc/Features/LinearLighting.hfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlslipackage/Shaders/ISHDR.hlslpackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Water.hlslpackage/Shaders/ISHDR.hlslpackage/Shaders/Effect.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/RunGrass.hlslfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/DeferredCompositeCS.hlslfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
package/Shaders/Water.hlslpackage/Shaders/ISHDR.hlslpackage/Shaders/DistantTree.hlslpackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 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.
Applied to files:
package/Shaders/Water.hlslpackage/Shaders/DeferredCompositeCS.hlslfeatures/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli
📚 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:
package/Shaders/Common/SharedData.hlslipackage/Shaders/ISHDR.hlslpackage/Shaders/Effect.hlslpackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Common/Color.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/Features/LinearLighting.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
package/Shaders/Common/Color.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: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (35)
features/Light Limit Fix/Shaders/LightLimitFix/Common.hlsli (1)
19-19: LGTM!The new
Linearflag at bit 11 is correctly placed and consistent with the C++ enum definition inLightLimitFix.h. The bit position avoids conflicts with existing flags.features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (1)
52-52: LGTM!The migration from
GammaToLinear/LinearToGammatoIrradianceToLinear/IrradianceToGammais consistent with the PR's linear lighting approach. The MultiBounceAO computation correctly operates in linear space.features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (2)
41-43: LGTM!The addition of
Color::Ambientwrapper for directional ambient computation aligns with the linear lighting architecture.
62-63: LGTM!The pattern of converting
iblColorto gamma space before computing luminance is consistent across all IBL branches. This ensures luminance is computed in display-referenced space for proper weighting withdirectionalAmbientColorSpecular.Also applies to: 88-89, 126-127
package/Shaders/DeferredCompositeCS.hlsl (3)
112-112: LGTM!Initial diffuse color conversion to linear space sets up correct lighting computations throughout the shader.
140-154: LGTM!The SSGI path correctly:
- Converts diffuse to linear for AO application
- Applies MultiBounceAO in linear space
- Converts back to gamma for intermediate storage
- Re-converts for SSGI indirect lighting addition
The workflow maintains color accuracy through the multi-pass operations.
283-283: LGTM!Final gamma conversion at output is correct for display-ready color.
package/Shaders/Effect.hlsl (4)
550-554: LGTM!The directional light handling correctly:
- Retrieves the appropriate multiplier based on linear lighting state
- Applies
Color::DirectionalLightwith theisDirLightLinearflag- Re-applies the multiplier for proper intensity scaling
578-580: LGTM!Skylighting paths correctly convert ambient color to linear before applying skylighting diffuse, then convert back to gamma. This ensures proper blending in linear space.
Also applies to: 624-626
732-733: LGTM!Correctly reads the
Linearflag fromlight.lightFlagsand passes it toColor::PointLightfor proper color space handling of per-light linear state.
911-913: Verify the gamma correction condition logic.The condition
!(InWorld) && enableLinearLightingapplies gamma correction only for non-world effects when linear lighting is enabled. Ensure this matches the intended behavior—world effects would remain in linear space and need conversion elsewhere.src/Features/LightLimitFix.h (2)
41-41: LGTM!The
Linearflag at bit 11 is correctly mirrored from the HLSL definition, maintaining consistency between C++ and shader code.
52-53: LGTM!The
fadefield with default1.0fis correctly positioned aftercolorfor proper 16-byte alignment. This matches the HLSLLightstruct layout inCommon.hlsli.src/Features/InverseSquareLighting.cpp (1)
40-41: LGTM!Correctly propagates the
Linearflag from the TES light extension flags to the runtime data, mirroring theInverseSquareflag pattern above.src/Features/InverseSquareLighting/Common.h (1)
8-9: LGTM!The new
kLinearflag addition is correctly positioned at bit 15 and integrates cleanly with the existing light flags system for per-light linear lighting control.src/Features/InverseSquareLighting/LightEditor.cpp (2)
84-84: LGTM!The Linear Light checkbox follows the established pattern for flag controls in the light editor UI.
303-306: LGTM!The Linear flag synchronization correctly mirrors the existing InverseSquare flag handling pattern, ensuring consistent runtime state management.
features/Subsurface Scattering/Shaders/SubsurfaceScattering/Burley.hlsli (1)
62-62: LGTM!The migration from generic
GammaToLinear/LinearToGammato semanticIrradianceToLinear/IrradianceToGammacorrectly reflects that subsurface scattering operates on diffuse irradiance values. The conversions are applied consistently at initialization, sampling, and output stages.Also applies to: 117-117, 135-135
package/Shaders/Common/LightingEval.hlsli (2)
110-122: LGTM!The addition of
VanillaDiffuseMult()andVanillaSpecularMult()provides consistent per-category lighting intensity control across all vanilla lighting paths (base diffuse, soft lighting, rim lighting, back lighting, and specular).
194-194: LGTM!Replacing
Math::PIwithColor::PBRLightingCompensationprovides more accurate compensation for the GGX specular model when applied to wetness specular on traditional (non-PBR) materials.src/Features/LightLimitFix.cpp (2)
3-3: LGTM!Adding the LinearLighting.h include enables integration with the linear lighting feature for fade handling.
449-449: LGTM!The refactored attenuation check correctly computes intensity as the sum of color components multiplied by the separate fade factor, providing cleaner separation between color and intensity modulation.
package/Shaders/DistantTree.hlsl (4)
217-217: LGTM!Wrapping the base color with
Color::Diffuseensures consistent color space handling for diffuse albedo in the linear lighting pipeline.
237-238: LGTM!The directional light color calculation correctly:
- Guards against division by zero with
max(llDirLightMult, 1e-5)(addressing the previous review concern)- Uses
Color::DirectionalLightfor semantic color space conversion- Applies
VanillaDiffuseMult()for intensity control- Consistently implements the pattern in both DEFERRED and non-DEFERRED paths
Also applies to: 272-273
244-244: LGTM!Wrapping the directional ambient term with
Color::Ambientprovides consistent ambient color space handling across both DEFERRED and non-DEFERRED rendering paths.Also applies to: 279-279
254-254: LGTM!Replacing
Color::LinearToGammawithColor::IrradianceToGammacorrectly reflects that IBL output represents diffuse irradiance. The semantic conversion is consistently applied in both DEFERRED and non-DEFERRED paths.Also applies to: 289-289
package/Shaders/Common/SharedData.hlsli (1)
201-235: Verify C++ struct layout matches HLSL for proper constant buffer alignment.The
LinearLightingSettingsstruct contains 34 scalar fields (uint and float). Ensure the C++ counterpart has matching field order and types, with explicit padding as needed to maintain HLSL's 16-byte alignment requirements for constant buffers. Correct use ofuintinstead ofboolfor boolean flags (per shader compatibility requirements).package/Shaders/ISHDR.hlsl (1)
139-142: Remove the conditionalTrueLinearToGammacall beforeToSRGBColor.The code applies
Color::TrueLinearToGamma(which converts to gamma space) followed byFrameBuffer::ToSRGBColor(which expects linear input). When bothenableLinearLightingandenableGammaCorrectionare true, this creates a double gamma conversion. TheToSRGBColorfunction appliespow(abs(linearColor), FrameParams.x)whereFrameParams.xis the inverse gamma; applying this to already gamma-corrected data produces incorrect results.The linear pipeline should be: linear → tonemapping →
ToSRGBColor(which handles the final encoding). The conditional gamma correction step is redundant here.⛔ Skipped due to 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: CR Repo: doodlum/skyrim-community-shaders PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-17T18:37:35.839Z Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)package/Shaders/Water.hlsl (2)
858-869: IBL/SSR reflection conversions look consistent with the new irradiance pipelineSampling both env and SSR into linear via
Color::IrradianceToLinearand converting back withColor::IrradianceToGammakeeps reflection blending in linear space and matches the rest of the LL changes. No issues here.Also applies to: 918-919
1009-1028: Water/fog color now correctly routed through LL-aware wrappersUsing
Color::Waterfor shallow/deep water andColor::Fog/Color::FogAlphafor all fog/pre‑fog blends keeps these paths LL-aware and matches how other passes are handled. Implementation looks sound.Also applies to: 1200-1257
src/Features/LinearLighting.cpp (1)
182-199: Per-geometry constant buffer hookup matches shader cbuffer usageBinding
PerGeometryCBat PS slot 8 lines up withLLPerGeometry : register(b8)inColor.hlsli, and only happens when linear lighting is enabled. The RTTI-based check keeps this restricted toBSLightingShaderpasses. Looks correct.package/Shaders/RunGrass.hlsl (1)
499-579: Grass lighting: linear-space pipeline and buffers look coherentAlbedo (
Color::Diffuse), grass diffuse/specular multipliers, directional ambient viaColor::Ambient, IBL viaIrradianceToGamma, and writing specular asColor::IrradianceToLinearbeforepsout.Specularall line up with the new LL/Irradiance conventions. This should keep grass in sync with the rest of the deferred pipeline.Also applies to: 610-646, 722-793
package/Shaders/Lighting.hlsl (1)
2747-2783: Ambient/IBL/fog and final gamma encode are wired correctly for LL
- Ambient now goes through
Color::Ambientand, when enabled, diffuse IBL contributes viaImageBasedLightingthenColor::IrradianceToGamma, so irradiance stays linear until the last step.- Non‑world / non‑reflection passes re‑encode
psout.Diffuse.xyzwithColor::TrueLinearToGamma, while world passes stay in linear and rely on post‑tonemapping gamma.
These choices look consistent with the LL design.Also applies to: 3005-3015, 3197-3199
src/Features/LinearLighting.h (1)
27-99: Per-frame settings layout looks good; just double‑check C++ ↔ HLSL packingThe
Settings/PerFrameDatastructs mirror each other and useuintfor flags withalignas(16), which is what HLSL constant buffers expect in this codebase. As long as the HLSLlinearLightingSettingsstruct inSharedData.hlsliuses the exact same field order and types, the GPU side should read the right values.Please re-verify that the C++
LinearLighting::PerFrameDatafield order and sizes exactly match the HLSLlinearLightingSettingsdefinition (including any padding), since any drift there will be very hard to diagnose later.package/Shaders/Common/Color.hlsli (1)
129-138: LL wrappers centralize color-space and multiplier handling cleanlyThe new helpers (
GammaToLinearLuminancePreserving*,LLGammaToLinear/LLLinearToGamma,Light/DirectionalLight/PointLight,EmitColor,Glowmap,Ambient/Fog/FogAlpha,Effect*,Sky/Water/VolumetricLighting,RadianceToLinear,IrradianceToLinear/Gamma, and the various*Mult()accessors) give a single, LL-aware place to manage gamma, PBR compensation, and feature multipliers. Aside from the scalar.xbug already noted, the pow/abs patterns and ENABLE_LL guards look consistent and should make future tuning much easier.Also applies to: 140-178, 186-311
| void LinearLighting::DrawSettings() | ||
| { | ||
| ImGui::Checkbox("Enable Linear Lighting", (bool*)&settings.enableLinearLighting); | ||
| ImGui::Checkbox("Enable Gamma Correction", (bool*)&settings.enableGammaCorrection); |
There was a problem hiding this comment.
Avoid casting uint flags to bool* in ImGui checkboxes
settings.enableLinearLighting / enableGammaCorrection are uint, but they’re passed to ImGui as bool* via a C‑style cast. That’s undefined behaviour and only touches one byte of a 4‑byte value.
Prefer an explicit temporary:
bool enabled = settings.enableLinearLighting != 0;
if (ImGui::Checkbox("Enable Linear Lighting", &enabled)) {
settings.enableLinearLighting = enabled ? 1u : 0u;
}Same pattern for enableGammaCorrection.
🤖 Prompt for AI Agents
In src/Features/LinearLighting.cpp around lines 36-39, the code casts uint flags
to bool* when calling ImGui::Checkbox which is undefined behavior; instead
create a local bool initialized from the uint (e.g.
settings.enableLinearLighting != 0), pass the address of that bool to
ImGui::Checkbox, and if Checkbox returns true update
settings.enableLinearLighting to 1u or 0u accordingly; apply the same pattern
for settings.enableGammaCorrection.
| return ENABLE_LL ? pow(abs(color), SharedData::linearLightingSettings.colorGamma) : color; | ||
| } | ||
|
|
||
| float3 RadianceToLinear(float3 color) |
There was a problem hiding this comment.
consider adding doxygen comments for these similar functions (RadianceToLinear, IrradianceToLinear, etc) to help with future devs choosing what to use.
There was a problem hiding this comment.
also maybe consider organising by LL / legacy functions or something.
| float projectedEffectMult; | ||
| float deferredEffectMult; | ||
| float otherEffectMult; | ||
| }; |
davo0411
moved this from Priority Issues
to In review
in Community Shaders Development Tracker
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package/Shaders/Lighting.hlsl (2)
1868-1878: Consider refactoring facegen functions to accept linear input.The round-trip conversions (
LinearToGamma→GetFacegenBaseColor→GammaToLinear) are correct but inefficient. Consider updatingGetFacegenBaseColorandGetFacegenRGBTintBaseColorto work directly in linear space to avoid these conversions.♻️ Performance optimization
The current approach converts linear → gamma → linear on every pixel. If the facegen functions were updated to operate in linear space, these conversions could be eliminated, improving performance.
2386-2387: Add clarifying comment for directional light multiplier pattern.The divide-then-multiply pattern for
llDirLightMultis mathematically correct but non-obvious. Adding a brief comment explaining why the light is divided before conversion and multiplied after would improve maintainability.Example:
// Apply multiplier after conversion to preserve relative intensities during gamma correction float llDirLightMult = SharedData::linearLightingSettings.enableLinearLighting && ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package/Shaders/Lighting.hlslpackage/Shaders/Particle.hlslsrc/Features/InverseSquareLighting.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- package/Shaders/Particle.hlsl
- src/Features/InverseSquareLighting.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
package/Shaders/Lighting.hlsl
**/*
⚙️ 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:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Lighting.hlsl
🧠 Learnings (6)
📓 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-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: 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.
📚 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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
package/Shaders/Lighting.hlsl
📚 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:
package/Shaders/Lighting.hlsl
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Applied to files:
package/Shaders/Lighting.hlsl
⏰ 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 (10)
package/Shaders/Lighting.hlsl (10)
812-812: LGTM: LOD color conversion is consistent.Both LODOBJECTS and non-LOBOBJECTS branches correctly apply
Color::Diffuseto LOD color in linear space.Also applies to: 820-820
959-959: LGTM: Reflection context flag properly initialized.The
inReflectionflag is correctly derived from permutation descriptors and follows the same pattern asinWorld, enabling proper lighting behavior differentiation in reflection passes.
1923-1923: LGTM: LOD land color correctly converted to linear space.The texture sample is properly converted to linear via
Color::ColorToLinearand the vanilla diffuse multiplier is applied in linear space.
2012-2012: LGTM: Projected UV color conversions are correct.All projected UV color paths properly convert texture samples and parameters to linear space before use:
- Line 2012: Diffuse sample conversion with parameter multiply
- Line 2015: Environment data conversion
- Line 2034: Parameter conversion
Also applies to: 2015-2015, 2034-2034
2098-2102: LGTM: F0 computation correctly handles linear lighting.The conditional properly accounts for baseColor already being in linear space when linear lighting is enabled, avoiding double conversion. The metallic lerp follows standard PBR metallic workflow.
2529-2529: LGTM: Point light handling differs appropriately between LLF and non-LLF.The Light Limit Fix path correctly uses per-light Linear flags and fade, while the non-LLF path uses simpler uniform gamma assumptions. This is an intentional feature distinction.
Also applies to: 2614-2615
2716-2716: LGTM: Emissive and glowmap colors correctly converted.Both
Color::EmitColorandColor::Glowmapproperly convert to linear space, ensuring correct multiplication on line 2725.Also applies to: 2724-2724
2747-2747: LGTM: Directional ambient correctly converted to linear.
Color::Ambientproperly converts the directional ambient term, with appropriate negative value clamping.
2781-2781: Verify IBL irradiance-to-gamma conversion rationale.The IBL color is converted from irradiance to gamma space before being added to
directionalAmbientColor, which is in linear space (fromColor::Ambient). This seems inconsistent unless there's a specific reason for the color space mismatch. The subtract-then-re-add pattern at lines 2992-3002 suggests special handling for skylighting, but the rationale for gamma conversion here should be verified.Based on learnings, IBL irradiance handling should follow consistent linear-space patterns.
2932-2932: LGTM: Final color space conversions correctly structured.The conversion chain properly handles the transition from accumulated lighting to final output:
- Line 2932: Specular irradiance → linear for accumulation
- Line 2940: Environment contribution with diffuse conversion
- Line 3006: Combines diffuse irradiance with linear specular, converts to gamma irradiance
- Lines 3007, 3014: Fog color and alpha handling
- Lines 3197-3199: Non-world path gamma conversion for UI consistency
Also applies to: 2940-2940, 3006-3007, 3014-3014, 3197-3199
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
package/Shaders/Common/Color.hlsli (3)
118-127: Remove redundant.xswizzle on scalar.Line 125 applies
.xto the result ofGammaToLinear(originalLuminance), which already returns afloat. While valid HLSL, the swizzle is unnecessary and can be removed for clarity.♻️ Simplify scalar assignment
- float scale = GammaToLinear(originalLuminance).x; + float scale = GammaToLinear(originalLuminance);
263-286: Document the semantic behavior of Irradiance/Radiance conversion functions.The
IrradianceToLinear,IrradianceToGamma, andRadianceToLinearfunctions have conditional logic where they pass through unchanged whenENABLE_LLis true and perform conversions when false. This implies that when linear lighting is enabled, values are assumed to already be in the target space. The naming can be misleading to shader authors who expect the function to always perform the named conversion. Consider adding inline comments explaining this semantic, or renaming to clarify the conditional behavior (e.g.,IrradianceEnsureLinear).
109-311: Consider caching converted colors in calling shaders.Many conversion functions apply
pow()operations, which are moderately expensive. If calling shaders use the same color value through multiple conversions or in multiple operations, consider caching the converted result in a local variable to avoid redundantpow()calls.Example pattern for shader authors:
// Instead of: float3 finalColor = Color::Effect(baseColor) * Color::EffectMult(baseColor); // Cache the conversion: float3 effectColor = Color::Effect(baseColor); float3 finalColor = effectColor * Color::EffectMult(baseColor);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Common/Color.hlsli
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
package/Shaders/Common/Color.hlsli
**/*
⚙️ 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:Otherwise, use your standard review approach focusing on code quality.
Files:
package/Shaders/Common/Color.hlsli
🧠 Learnings (10)
📓 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-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: 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.
📚 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:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.
Applied to files:
package/Shaders/Common/Color.hlsli
📚 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:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-07-05T05:20:45.823Z
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.
Applied to files:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders 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.
Applied to files:
package/Shaders/Common/Color.hlsli
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders 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.
Applied to files:
package/Shaders/Common/Color.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). (4)
- GitHub Check: Run Shader Unit Tests
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (2)
package/Shaders/Common/Color.hlsli (2)
7-7: Verify that runtime ENABLE_LL branching is intentional.
ENABLE_LLevaluates to a constant buffer value rather than a compile-time constant, which means all conditional branches using it are evaluated at runtime. While this enables runtime toggling of linear lighting, it prevents the compiler from eliminating dead code paths for each mode. Confirm this design choice aligns with the performance requirements and the need for runtime switching.
10-14: Verify register b8 is protected by guard conditions.The concern about register b8 conflicts is valid:
RunGrass.hlslincludesColor.hlsliand both defineregister(b8). Confirm that the#ifdefguards aroundLLPerGeometry(such asPSHADERandLIGHTING) prevent this cbuffer from being defined when compilingRunGrass.hlsl, or reassign the register to avoid collision withRunGrass.hlsl'scb8buffer.
|
I am not against how this done, but may create issues with upcoming ENB PP. Will merge for now. |
* build: bump eastl to 3.27.01 (community-shaders#1673) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(grass collision): ignore hkpListShape (community-shaders#1661) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(upscaling): update SDKs (community-shaders#1684) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(terrain shadows): modernize (community-shaders#1678) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: add user wiki link to readme (community-shaders#1689) * feat(LLF): add debug overlay in llf visualiser (community-shaders#1666) * feat: linear lighting (community-shaders#1359) Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: weather and imagespace editor (community-shaders#1630) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(ui): load previously selected theme on startup (community-shaders#1664) --------- Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jiayev <l936249247@hotmail.com> Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
4 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.