fix(VL): potential negative vl values#1423
Conversation
WalkthroughClamps volumetric lighting accumulation to non-negative values in ISApplyVolumetricLighting.hlsl by computing adjustedVl = max(0, noiseGrad + vl - 0.0078125). This affects both non-temporal and temporal VL assignment paths; overall control flow and interfaces remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/ISApplyVolumetricLighting.hlsl (1)
67-67: Clamp temporal blend weight to [0,1] and harden VL ≥ 0 invariantPlease apply the following refactors in package/Shaders/ISApplyVolumetricLighting.hlsl to avoid extrapolation or negative lighting values:
• At line 84 (compute blend weight):
- float temporalContribution = g_IntensityX_TemporalY.y - * (1 - smoothstep(0, 1, min(1, 100 * abs(depth - previousDepth)))); + // Prevent overshoot when temporal weight >1 + float temporalContribution = saturate( + g_IntensityX_TemporalY.y + * (1 - smoothstep(0, 1, min(1, 100 * abs(depth - previousDepth)))) + );• At line 85 (final VL write):
- psout.VL = lerp(adjustedVl, previousVl, temporalContribution * isValid); + // Enforce non-negative volumetric lighting + psout.VL = max( + 0.0, + lerp(adjustedVl, previousVl, temporalContribution * isValid) + );(Optional) If you foresee cases where
previousVlmay come from an untrusted source, you can also clamp it immediately after sampling:previousVl = max(0.0, previousVl);Please confirm whether the rendering invariant “VL is always ≥ 0 across frames” is intended; if so, the above ensures that negative history or over-driven temporal weights cannot introduce invalid values.
🧹 Nitpick comments (2)
package/Shaders/ISApplyVolumetricLighting.hlsl (2)
52-52: Use a float literal and express the bias as a fraction for clarity.Avoid implicit int→float promotions in HLSL and make the bias self-documenting.
- float adjustedVl = max(0, noiseGrad + vl - 0.0078125); + float adjustedVl = max(0.0, noiseGrad + vl - (1.0/128.0));
17-17: Minor naming cleanup: drop the duplicated “Sampler” suffix.Purely cosmetic, but improves readability and consistency with other samplers.
-SamplerState NoiseGradSamplerSampler : register(s4); +SamplerState NoiseGradSampler : register(s4); @@ - float noiseGrad = 0.03125 * NoiseGradSamplerTex.Sample(NoiseGradSamplerSampler, 0.125 * input.Position.xy).x; + float noiseGrad = 0.03125 * NoiseGradSamplerTex.Sample(NoiseGradSampler, 0.125 * input.Position.xy).x;Also applies to: 50-50
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
package/Shaders/ISApplyVolumetricLighting.hlsl(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/ISApplyVolumetricLighting.hlsl
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#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/ISApplyVolumetricLighting.hlsl
🔇 Additional comments (1)
package/Shaders/ISApplyVolumetricLighting.hlsl (1)
52-52: Clamp-to-zero is correct and keeps VL physically plausible.Clamping the accumulation to non-negative at the source prevents underflow-induced negative VL. This is a safe, targeted fix with minimal side effects.
2 participants
Summary by CodeRabbit