feat: lighting for rain and snow #1577
Conversation
WalkthroughAdded per-fragment lighting to the particle shader: vertex shader now outputs view-space position, and pixel shader accumulates directional, ambient, and clustered light contributions with conditional attenuation and compile-time includes. Changes
Sequence DiagramsequenceDiagram
participant VS as Vertex Shader
participant PS as Pixel Shader
participant Cluster as Light Cluster (LightLimitFix)
participant Math as Lighting Math
VS->>VS: compute finalViewPosition
VS->>VS: vsout.ViewPosition = finalViewPosition.xyz
VS-->>PS: emit VS_OUTPUT (with ViewPositionVS)
PS->>PS: sample baseColor
PS->>PS: read directional & ambient (SharedData)
PS->>PS: propertyColor = dir+ambient
PS->>PS: worldPos = ViewToWorld * ViewPositionVS
rect rgb(235,245,255)
Note over PS,Cluster: if LIGHT_LIMIT_FIX defined
PS->>Cluster: fetch lights for fragment/cluster
Cluster-->>PS: list of lights
end
loop for each light
PS->>Math: compute lightDir, distance
alt ISL enabled
Math-->>PS: inverse-square attenuation
else
Math-->>PS: quadratic falloff attenuation
end
PS->>PS: accumulate propertyColor += lightContribution
end
PS->>PS: finalColor = propertyColor * baseColor
PS-->>PS: output final fragment color
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
package/Shaders/Particle.hlsl (1)
299-331: Clustered light loop is OK; confirm shadow flag semantics and add safety for zero radius.
- Skipping lights with LightFlags::Shadow may drop all shadow‑casting lights. Confirm intent.
- Consider guarding against zero/denormal radius to avoid NaNs.
Apply:
- float intensityFactor = saturate(lightDist / light.radius); + float safeRadius = max(light.radius, 1e-3); + float intensityFactor = saturate(lightDist / safeRadius); float intensityMultiplier = 1 - intensityFactor * intensityFactor;If shadow flag means “shadow‑maps only,” keep skip; otherwise remove the check and rely on IsLightIgnored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Particle.hlsl(4 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/Particle.hlsl
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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)
package/Shaders/Particle.hlsl (3)
3-11: Includes look fine; verify build flags across all permutations.Conditional includes are correct. Please confirm the shader compiles in these permutations: plain, LIGHT_LIMIT_FIX only, ISL+LIGHT_LIMIT_FIX, and VR+ENVCUBE combos.
Would you run your shader CI/build for all macro sets used by the project and share failures, if any?
288-295: Minor: tighten ambient computation types and clamping.Ensure consistent vector dimensions and clamp to [0,1] to avoid negative ambient.
[ suggest_recommended_refactor ]
Apply:- float3 propertyColor = 0.0; - float3 dirLightColor = SharedData::DirLightColor.xyz * 0.5; - float3 ambientColor = max(0, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1))); + float3 propertyColor = 0.0.xxx; + float3 dirLightColor = SharedData::DirLightColor.xyz * 0.5; + float3 ambientColor = saturate(mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1)).xyz);Optional: consider exposing the 0.5 scalars as tunables.
296-299: Incomplete renaming: Input struct definition still usesViewPosition, notViewPositionVS.The current code at line 296–297 is functionally correct—both
FrameBuffer::ViewToWorld()andFrameBuffer::ViewToUV()correctly expect view-space input. However, the proposed diff to renameinput.ViewPositiontoinput.ViewPositionVSwill not compile without also updating the input struct definition at line 44, which still declaresfloat3 ViewPosition : POSITION0;.Either:
- The input struct renaming was already applied elsewhere in this PR (requires verification), or
- The struct definition change must be included: change line 44 from
float3 ViewPosition : POSITION0;tofloat3 ViewPositionVS : POSITION0;Additionally, update all three usages:
- Line 296:
input.ViewPosition.xyz→input.ViewPositionVS- Line 302:
FrameBuffer::ViewToUV(viewPosition, ...)→FrameBuffer::ViewToUV(viewPositionVS, ...)- Line 305:
viewPosition.z→viewPositionVS.z
| vsout.ViewPosition = finalViewPosition.xyz; | ||
|
|
There was a problem hiding this comment.
Wrong space passed to PS; provide view‑space, not clip‑space. Also not set in all paths.
You assign clip‑space (post‑projection) to ViewPosition, then PS treats it as view‑space. Provide view‑space and write it in both ENVCUBE and non‑ENVCUBE paths.
Apply (paired with the TEXCOORD rename):
- vsout.ViewPosition = finalViewPosition.xyz;
+ // Compute view-space for precipitation motion blend
+#if defined(ENVCUBE)
+# if defined(RAIN)
+ float3 viewVS = mul(WorldView[eyeIndex], msPosition).xyz;
+ float3 adjustedViewVS = mul(WorldView[eyeIndex], adjustedMsPosition).xyz;
+ float3 finalViewPositionVS = lerp(adjustedViewVS, viewVS, positionBlendParam);
+ vsout.ViewPositionVS = finalViewPositionVS;
+# else
+ vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
+# endif
+#else
+ vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
+#endifNote: This also fixes the missing assignment when ENVCUBE is not defined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vsout.ViewPosition = finalViewPosition.xyz; | |
| // Compute view-space for precipitation motion blend | |
| #if defined(ENVCUBE) | |
| # if defined(RAIN) | |
| float3 viewVS = mul(WorldView[eyeIndex], msPosition).xyz; | |
| float3 adjustedViewVS = mul(WorldView[eyeIndex], adjustedMsPosition).xyz; | |
| float3 finalViewPositionVS = lerp(adjustedViewVS, viewVS, positionBlendParam); | |
| vsout.ViewPositionVS = finalViewPositionVS; | |
| # else | |
| vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz; | |
| # endif | |
| #else | |
| vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz; | |
| #endif |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
package/Shaders/Particle.hlsl (2)
124-125: Wrong space + wrong field name; also not written in non-ENVCUBE.You’re assigning clip-space xyz to a field named ViewPosition (which doesn’t exist anymore) and never writing the view-space varying in the non-ENVCUBE path. Provide view-space and write ViewPositionVS in all VS paths.
Apply in ENVCUBE block:
- vsout.ViewPosition = finalViewPosition.xyz; + // Provide view-space position for PS lighting +#if defined(RAIN) + float3 viewVS = mul(WorldView[eyeIndex], msPosition).xyz; + float3 adjustedViewVS = mul(WorldView[eyeIndex], adjustedMsPosition).xyz; + float3 finalViewVS = lerp(adjustedViewVS, viewVS, positionBlendParam); + vsout.ViewPositionVS = finalViewVS; +#else + vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz; +#endifAlso add in the non-ENVCUBE path near where you set viewPosition/Position (after Line 176):
vsout.Position.zw = viewPosition.zw; + // Provide view-space position for PS lighting + vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
296-303: PS reads the wrong field and wrong space. Use ViewPositionVS consistently.Consume the TEXCOORD varying you added and keep naming consistent across all uses.
- float3 viewPosition = input.ViewPosition.xyz; - float3 worldPosition = FrameBuffer::ViewToWorld(viewPosition); + float3 viewPositionVS = input.ViewPositionVS; + float3 worldPosition = FrameBuffer::ViewToWorld(viewPositionVS); @@ - float2 screenUV = FrameBuffer::ViewToUV(viewPosition, true, eyeIndex); + float2 screenUV = FrameBuffer::ViewToUV(viewPositionVS, true, eyeIndex);Run to confirm no stale uses remain:
#!/bin/bash rg -nP -C2 '(\bvsout|input)\.ViewPosition\b' package/Shaders/Particle.hlsl
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/Shaders/Particle.hlsl(4 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/Particle.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 (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (2)
package/Shaders/Particle.hlsl (2)
44-45: Good: TEXCOORD varying for PS.Using TEXCOORD2 for view-space transfer is correct and PS-friendly.
332-334: Final color combine looks correct.RGB multiply and alpha passthrough align with prior feedback.
| float3 ambientColor = max(0, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1))); | ||
|
|
There was a problem hiding this comment.
Avoid implicit float4→float3 truncation in ambient.
Make the type explicit and clamp only the RGB you use.
- float3 ambientColor = max(0, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1)));
+ float3 ambientColor = max(0.0.xxx, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1)).xyz);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package/Shaders/Particle.hlsl around lines 291-292, the current expression
uses an implicit float4→float3 truncation when computing ambientColor; replace
that implicit truncation by extracting the .xyz from the mul result and perform
the clamp/max on a float3 explicitly (e.g. use mul(...).xyz and clamp or max
with a float3(minR,minG,minB) so only RGB are clamped and types match).
| float intensityFactor = saturate(lightDist / light.radius); | ||
| float intensityMultiplier = 1 - intensityFactor * intensityFactor; | ||
| # endif | ||
|
|
||
| float3 lightColor = light.color.xyz * intensityMultiplier * 0.5; | ||
| propertyColor += lightColor; |
There was a problem hiding this comment.
Division by zero when light.radius == 0.
Guard the denominator to avoid INF/NaN in clustered lights.
- float intensityFactor = saturate(lightDist / light.radius);
- float intensityMultiplier = 1 - intensityFactor * intensityFactor;
+ float radiusSafe = max(light.radius, 1e-3);
+ float intensityFactor = saturate(lightDist / radiusSafe);
+ float intensityMultiplier = 1.0 - intensityFactor * intensityFactor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float intensityFactor = saturate(lightDist / light.radius); | |
| float intensityMultiplier = 1 - intensityFactor * intensityFactor; | |
| # endif | |
| float3 lightColor = light.color.xyz * intensityMultiplier * 0.5; | |
| propertyColor += lightColor; | |
| float radiusSafe = max(light.radius, 1e-3); | |
| float intensityFactor = saturate(lightDist / radiusSafe); | |
| float intensityMultiplier = 1.0 - intensityFactor * intensityFactor; | |
| # endif | |
| float3 lightColor = light.color.xyz * intensityMultiplier * 0.5; | |
| propertyColor += lightColor; |
🤖 Prompt for AI Agents
In package/Shaders/Particle.hlsl around lines 321 to 326, the computation float
intensityFactor = saturate(lightDist / light.radius) can divide by zero when
light.radius == 0; change the division to use a guarded denominator (e.g. use
max(light.radius, a small epsilon like 1e-6) or branch on radius > 0) so the
divisor is never zero, then compute intensityMultiplier and subsequent color
using that safe intensityFactor.
Summary by CodeRabbit