Skip to content

feat: lighting for rain and snow#1577

Merged
davo0411 merged 9 commits into
devfrom
particle-lighting
Jan 6, 2026
Merged

feat: lighting for rain and snow#1577
davo0411 merged 9 commits into
devfrom
particle-lighting

Conversation

@doodlum
Copy link
Copy Markdown
Collaborator

@doodlum doodlum commented Oct 20, 2025

image image

Summary by CodeRabbit

  • New Features

    • Richer particle lighting with per-fragment light accumulation for more realistic visuals.
    • Screen-clustered light sampling for more accurate local illumination.
    • Optional inverse-square attenuation for improved light falloff.
  • Bug Fixes / Improvements

    • Better integration of directional and ambient lighting with shadow-aware contributions.
    • Improved handling across environment/rain render paths and a lighting limit fix for consistent results.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 20, 2025

📝 Walkthrough

Walkthrough

Vertex shader now outputs view-space position (ViewPositionVS). Pixel shader includes SharedData, reads directional and ambient lighting, converts view-space to world-space, and conditionally samples clustered lights (LIGHT_LIMIT_FIX) using ISL or quadratic attenuation to accumulate final particle color.

Changes

Cohort / File(s) Summary
Particle shader
package/Shaders/Particle.hlsl
Added float3 ViewPositionVS : TEXCOORD2 to VS_OUTPUT. VS writes view-space position across ENVCUBE/RAIN branches. PS now includes SharedData.hlsli, reads directional & ambient lighting, converts ViewPositionVS->worldPos via FrameBuffer, replaces ColorScale logic with per-light accumulation, and conditionally includes/uses LightLimitFix and InverseSquareLighting (skips ignored/shadow lights, applies ISL or quadratic falloff, accumulates contributions, multiplies by baseColor).

Sequence Diagram

sequenceDiagram
    participant VS as Vertex Shader
    participant PS as Pixel Shader
    participant SD as SharedData
    participant Cluster as Light Cluster
    participant FB as FrameBuffer

    VS->>PS: emit VS_OUTPUT (includes ViewPositionVS)
    PS->>PS: sample baseColor
    PS->>SD: read directional & ambient
    PS->>PS: propertyColor = directional + ambient
    PS->>FB: convert ViewPositionVS -> worldPos

    alt LIGHT_LIMIT_FIX defined
        PS->>Cluster: fetch cluster by screen UV
        Cluster-->>PS: visible lights list
        loop per light
            alt light ignored or shadow-caster
                PS-->>PS: skip light
            else
                PS->>PS: compute lightDir & distance
                alt ISL defined
                    PS-->>PS: compute inverse-square attenuation
                else
                    PS-->>PS: compute quadratic/radius falloff
                end
                PS->>PS: accumulate contribution into propertyColor (×0.5)
            end
        end
    end

    PS->>PS: finalColor = propertyColor * baseColor
    PS-->>Framebuffer: output final fragment color
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • jiayev

Poem

🐇 I carried view-pos in a vertex glove,
I chased bright clusters that the pixels love,
Ambient hum and directional cheer,
I nudge each light so particles appear,
A hopping rabbit shaders the grove.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: lighting for rain and snow' directly describes the main feature added—lighting functionality for particle effects (rain and snow)—which aligns with the core changes in Particle.hlsl that implement a lighting-accumulation approach for these particle types.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 20, 2025

Using provided base ref: a28a512
Using base ref: a28a512
Base commit date: 2026-01-05T16:43:17Z (Monday, January 05, 2026 04:43 PM)
No actionable suggestions for changed features.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 79aebb6 and 311fa6e.

📒 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 uses ViewPosition, not ViewPositionVS.

The current code at line 296–297 is functionally correct—both FrameBuffer::ViewToWorld() and FrameBuffer::ViewToUV() correctly expect view-space input. However, the proposed diff to rename input.ViewPosition to input.ViewPositionVS will not compile without also updating the input struct definition at line 44, which still declares float3 ViewPosition : POSITION0;.

Either:

  1. The input struct renaming was already applied elsewhere in this PR (requires verification), or
  2. The struct definition change must be included: change line 44 from float3 ViewPosition : POSITION0; to float3 ViewPositionVS : POSITION0;

Additionally, update all three usages:

  • Line 296: input.ViewPosition.xyzinput.ViewPositionVS
  • Line 302: FrameBuffer::ViewToUV(viewPosition, ...)FrameBuffer::ViewToUV(viewPositionVS, ...)
  • Line 305: viewPosition.zviewPositionVS.z

Comment thread package/Shaders/Particle.hlsl Outdated
Comment thread package/Shaders/Particle.hlsl Outdated
Comment thread package/Shaders/Particle.hlsl Outdated
doodlum and others added 2 commits October 22, 2025 22:14
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
+#endif

Also 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

📥 Commits

Reviewing files that changed from the base of the PR and between 311fa6e and 5a2f44e.

📒 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.

Comment thread package/Shaders/Particle.hlsl Outdated
Comment thread package/Shaders/Particle.hlsl
@davo0411
Copy link
Copy Markdown
Collaborator

needs minor rewrite to fix reliance on lightLimitFixSettings that no longer exist. also other shader failures

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Particle.hlsl (1)

88-169: Critical: ViewPositionVS assigned with clip-space instead of view-space, and missing in non-ENVCUBE path.

Two critical issues remain unresolved from previous reviews:

  1. Line 116: finalViewPosition is computed using WorldViewProj (clip-space), but the pixel shader expects view-space coordinates for lighting calculations. You must use WorldView matrix instead.

  2. Non-ENVCUBE path (lines 127-198): ViewPositionVS is never assigned, leaving it uninitialized. The pixel shader will read garbage data.

🔎 Proposed fix

For the ENVCUBE path, compute view-space positions separately:

 	float4 viewPosition = mul(WorldViewProj[eyeIndex], msPosition);
 #		if defined(RAIN)
+	float3 viewVS = mul(WorldView[eyeIndex], msPosition).xyz;
 	float4 adjustedMsPosition = msPosition - float4(Velocity.xyz, 0);
+	float3 adjustedViewVS = mul(WorldView[eyeIndex], adjustedMsPosition).xyz;
 	float positionBlendParam = 0.5 * (1 + input.TexCoord1.y);
 	float4 adjustedViewPosition = mul(WorldViewProj[eyeIndex], adjustedMsPosition);
 	float4 finalViewPosition = lerp(adjustedViewPosition, viewPosition, positionBlendParam);
+	float3 finalViewPositionVS = lerp(adjustedViewVS, viewVS, positionBlendParam);
 #		else
 	float4 finalViewPosition = viewPosition;
+	float3 finalViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
 #		endif
 	vsout.Position.xy = positionOffset + finalViewPosition.xy;
 	vsout.Position.zw = finalViewPosition.zw;
 
-	vsout.ViewPositionVS = finalViewPosition.xyz;
+	vsout.ViewPositionVS = finalViewPositionVS;

For the non-ENVCUBE path, add the assignment before line 198:

 	vsout.Color.w = fVars3.w * color.w;
 	vsout.Color.xyz = color.xyz;
+	
+	// Compute view-space position for lighting
+	vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
 #	endif
♻️ Duplicate comments (2)
package/Shaders/Particle.hlsl (2)

322-323: Critical: Division by zero when light.radius == 0.

If light.radius is zero, line 322 divides by zero, producing INF/NaN that will corrupt the lighting calculation and potentially cause rendering artifacts.

🔎 Proposed fix
-				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;

292-292: Fix implicit float4→float3 truncation in ambient calculation.

The mul() returns float4, but ambientColor is float3. This causes implicit truncation and the max(0, ...) operates on mismatched types.

🔎 Proposed fix
-	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);
🧹 Nitpick comments (1)
package/Shaders/Particle.hlsl (1)

249-249: Consider removing or documenting the unused ColorScale member.

The ColorScale cbuffer member is defined but never used in the new lighting path. If this is intentional deprecation, consider either removing it in a follow-up or adding a comment explaining why it's preserved for compatibility.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a2f44e and 2c7386f.

📒 Files selected for processing (1)
  • package/Shaders/Particle.hlsl (5 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 (7)
📓 Common learnings
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.
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: 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.
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.
📚 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/Particle.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/Particle.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:

  • package/Shaders/Particle.hlsl
📚 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/Particle.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/Particle.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/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: 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 (3)
package/Shaders/Particle.hlsl (3)

3-3: LGTM!

The SharedData.hlsli include and ViewPositionVS field with TEXCOORD2 semantic are correctly implemented for passing view-space position to the pixel shader.

Also applies to: 36-36


220-227: LGTM!

The conditional includes for lighting systems are properly guarded and follow the codebase patterns.


333-333: LGTM!

The final color multiplication correctly uses matching RGB components and produces proper lighting output.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
package/Shaders/Particle.hlsl (2)

104-116: Critical: ViewPositionVS receives clip-space instead of view-space.

Line 116 assigns finalViewPosition.xyz which comes from WorldViewProj multiplication (lines 104-112), producing clip-space coordinates. However, the pixel shader at line 298 passes this to FrameBuffer::ViewToWorld() expecting view-space input. This causes incorrect world-position reconstruction and breaks all lighting calculations.

🔎 Proposed fix: compute view-space position using WorldView
 	float4 viewPosition = mul(WorldViewProj[eyeIndex], msPosition);
 #		if defined(RAIN)
 	float4 adjustedMsPosition = msPosition - float4(Velocity.xyz, 0);
 	float positionBlendParam = 0.5 * (1 + input.TexCoord1.y);
 	float4 adjustedViewPosition = mul(WorldViewProj[eyeIndex], adjustedMsPosition);
 	float4 finalViewPosition = lerp(adjustedViewPosition, viewPosition, positionBlendParam);
 #		else
 	float4 finalViewPosition = viewPosition;
 #		endif
 	vsout.Position.xy = positionOffset + finalViewPosition.xy;
 	vsout.Position.zw = finalViewPosition.zw;
 
-	vsout.ViewPositionVS = finalViewPosition.xyz;
+	// Compute view-space position for lighting
+#		if defined(RAIN)
+	float3 viewVS = mul(WorldView[eyeIndex], msPosition).xyz;
+	float3 adjustedViewVS = mul(WorldView[eyeIndex], adjustedMsPosition).xyz;
+	vsout.ViewPositionVS = lerp(adjustedViewVS, viewVS, positionBlendParam);
+#		else
+	vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
+#		endif

126-198: Critical: ViewPositionVS never assigned in non-ENVCUBE path.

The vsout.ViewPositionVS field is never initialized in the non-ENVCUBE vertex shader path (lines 126-198). The pixel shader unconditionally reads input.ViewPositionVS at line 297, which will contain undefined data for non-ENVCUBE particles, leading to incorrect lighting calculations or undefined behavior.

🔎 Proposed fix: add ViewPositionVS assignment

Add this assignment after line 168 in the non-ENVCUBE path:

 	float4 viewPosition = mul(WorldViewProj[eyeIndex], msPosition);
 	vsout.Position.xy = positionOffset * ScaleAdjust + viewPosition.xy;
 	vsout.Position.zw = viewPosition.zw;
+
+	// Compute view-space position for lighting
+	vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz;
 
 	float4 color1, color2;
♻️ Duplicate comments (1)
package/Shaders/Particle.hlsl (1)

322-323: Critical: Division by zero when light.radius == 0.

Line 322 computes intensityFactor = saturate(lightDist / light.radius) without guarding against light.radius == 0, which can produce INF/NaN and corrupt the lighting result.

🔎 Proposed fix: guard the denominator
-				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;

Note: This issue was previously flagged in past review comments and remains unaddressed.

🧹 Nitpick comments (1)
package/Shaders/Particle.hlsl (1)

289-296: Consider explicit float3 initialization for clarity.

Line 289 initializes propertyColor with scalar 0.0, relying on implicit conversion when accumulating float3 values. Line 292 uses scalar 0 in max() with a float3 result. While both work, explicit float3(0, 0, 0) or 0.0.xxx would improve readability.

🔎 Optional: make types explicit
-	float3 propertyColor = 0.0;
+	float3 propertyColor = 0.0.xxx;
 
 	float3 dirLightColor = SharedData::DirLightColor.xyz * 0.5;
-	float3 ambientColor = max(0, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1)).xyz);
+	float3 ambientColor = max(0.0.xxx, mul(SharedData::DirectionalAmbient, float4(0, 0, 1, 1)).xyz);
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c7386f and fcf8b19.

📒 Files selected for processing (1)
  • package/Shaders/Particle.hlsl
🧰 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 (7)
📓 Common learnings
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.
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: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
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.
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.
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)
📚 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/Particle.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/Particle.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:

  • package/Shaders/Particle.hlsl
📚 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/Particle.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/Particle.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/Particle.hlsl
🔇 Additional comments (4)
package/Shaders/Particle.hlsl (4)

3-3: LGTM!

Unconditional inclusion of SharedData.hlsli is appropriate since the pixel shader now requires directional and ambient lighting data in all paths.


36-36: LGTM!

The TEXCOORD2 semantic is correct for passing view-space position from vertex to pixel shader.


220-227: LGTM!

Conditional inclusion of LightLimitFix and InverseSquareLighting headers matches their usage patterns in the pixel shader. The ISL inclusion correctly depends on LIGHT_LIMIT_FIX being defined.


333-333: LGTM!

The final color computation correctly multiplies propertyColor (float3) by baseColor.xyz (float3), avoiding the type mismatch that was present in earlier versions.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Fix all issues with AI Agents 🤖
In @package/Shaders/Particle.hlsl:
- Line 182: vsout.ViewPositionVS is a float3 but mul(WorldView[eyeIndex],
msPosition) returns a float4; update the assignment in the vertex shader to
extract the float3 by appending .xyz to the multiplication result (i.e., set
vsout.ViewPositionVS = mul(WorldView[eyeIndex], msPosition).xyz) so the types
match and compilation succeeds.
♻️ Duplicate comments (1)
package/Shaders/Particle.hlsl (1)

336-337: Guard against division by zero when light.radius is zero.

Past review identified this critical issue. If light.radius equals zero, the division produces INF/NaN, corrupting the lighting calculation.

🔎 Proposed fix
-				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;
🧹 Nitpick comments (2)
package/Shaders/Particle.hlsl (2)

303-303: Use explicit vector initialization for clarity.

While scalar 0.0 will broadcast to float3, using 0.0.xxx or float3(0, 0, 0) makes the type explicit and improves readability.

🔎 Proposed refactor
-	float3 propertyColor = 0.0;
+	float3 propertyColor = 0.0.xxx;

263-263: Consider removing unused ColorScale in a follow-up.

The new lighting system no longer uses ColorScale from the constant buffer. If this parameter is deprecated, remove it to clean up the shader interface.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb3501 and 5c66163.

📒 Files selected for processing (1)
  • package/Shaders/Particle.hlsl
🧰 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/Particle.hlsl
**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

  1. 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
  2. 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 implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

  • package/Shaders/Particle.hlsl
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/Particle.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/Particle.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:

  • package/Shaders/Particle.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/Particle.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/Particle.hlsl
📚 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/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). (4)
  • GitHub Check: Run Shader Unit Tests
  • 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)

305-305: Verify the 0.5 multiplier on directional light is intentional.

The directional light is reduced to 50% intensity. Ensure this matches the desired artistic/technical requirements for particle lighting, as a similar multiplier appears on line 340 for clustered lights.


327-329: Verify that skipping shadow-casting lights is the intended behavior.

Lights with the Shadow flag are excluded from particle lighting. Confirm this aligns with the design goal—particles may benefit from shadow-casting lights in some scenarios.


235-237: Verify LightLimitFix integration after settings removal.

Per PR feedback, lightLimitFixSettings no longer exists and the integration requires updates. Confirm that the LightLimitFix.hlsli include and usage (lines 315-344) are compatible with the current LightLimitFix implementation.

Comment thread package/Shaders/Particle.hlsl Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Fix all issues with AI Agents 🤖
In @package/Shaders/Particle.hlsl:
- Around line 336-337: Prevent divide-by-zero in the intensity calculation by
guarding the division: replace the direct division used to compute
intensityFactor with a conditional that uses saturate(lightDist / light.radius)
only when light.radius > 0, and otherwise sets intensityFactor to 1 (so
intensityMultiplier becomes 0 for zero-radius lights); update the expressions
around intensityFactor, intensityMultiplier, light.radius and lightDist
accordingly.
🧹 Nitpick comments (2)
package/Shaders/Particle.hlsl (2)

305-305: Consider extracting hardcoded intensity factors.

The hardcoded 0.5 multipliers for directional and clustered lights could be extracted as named constants for better maintainability and clarity about the lighting model.

Example approach

At the top of the pixel shader section, define:

static const float PARTICLE_DIRECTIONAL_INTENSITY = 0.5;
static const float PARTICLE_CLUSTERED_LIGHT_INTENSITY = 0.5;

Then use these constants in the lighting calculations.

Also applies to: 340-340


263-263: Consider removing unused ColorScale member.

The ColorScale constant buffer member appears unused after transitioning to the lighting-based approach. If it's deprecated by design, consider removing it to clean up the shader interface.

If ColorScale is intentionally preserved for backward compatibility or future use, you can safely ignore this suggestion.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c66163 and c6f43d2.

📒 Files selected for processing (1)
  • package/Shaders/Particle.hlsl
🧰 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/Particle.hlsl
**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

  1. 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
  2. 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 implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

  • package/Shaders/Particle.hlsl
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/Particle.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/Particle.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:

  • package/Shaders/Particle.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/Particle.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/Particle.hlsl
📚 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/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). (4)
  • 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)
  • GitHub Check: Run Shader Unit Tests
🔇 Additional comments (6)
package/Shaders/Particle.hlsl (6)

3-3: LGTM: SharedData include necessary for lighting.

The unconditional include of SharedData.hlsli provides access to directional and ambient lighting data used in the pixel shader.


36-36: LGTM: ViewPositionVS varying properly declared.

The view-space position is correctly declared as a TEXCOORD2 varying, enabling lighting calculations in the pixel shader.


116-128: LGTM: View-space position correctly computed for ENVCUBE path.

The view-space position calculation properly handles both RAIN (with motion blend) and non-RAIN scenarios, with correct .xyz extraction from the matrix multiplication result.


182-182: LGTM: View-space position correctly computed for non-ENVCUBE path.

The view-space position is properly computed with correct .xyz extraction.


347-347: LGTM: Final color computation correctly typed.

The multiplication properly uses .xyz to maintain type consistency between propertyColor and baseColor.


235-241: No action required — the code is compatible with the current LightLimitFix implementation.

The lightLimitFixSettings struct still exists in SharedData.hlsli and is actively used by LightLimitFix.hlsli. Both included files (LightLimitFix/LightLimitFix.hlsli and InverseSquareLighting/InverseSquareLighting.hlsli) are present and properly export the required types and functions that Particle.hlsl uses at lines 320–327.

Likely an incorrect or invalid review comment.

Comment thread package/Shaders/Particle.hlsl
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 6, 2026

✅ A pre-release build is available for this PR:
Download

@davo0411 davo0411 merged commit ba6e43e into dev Jan 6, 2026
18 checks passed
@davo0411 davo0411 deleted the particle-lighting branch January 13, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants