Skip to content

Conversation

@davo0411
Copy link
Collaborator

@davo0411 davo0411 commented Dec 19, 2025

removes dead snow define code from lighting.hlsl. no failed shaders, looks the same.

code was dead anyway, we explicitly set bImprovedSnow=0.

Summary by CodeRabbit

  • Bug Fixes

    • Removed snow-specific rendering paths and related outputs to simplify terrain visuals and prevent snow-related artifacts.
    • Adjusted sparkle handling so sparkle sampling and scaling now run whenever sparkle is enabled, independent of snow.
  • Chores

    • Cleaned up conditional rendering paths to streamline final color and specular processing.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

📝 Walkthrough

Walkthrough

Removed SNOW feature logic from Lighting.hlsl (functions, PS_OUTPUT field, and all SNOW-conditioned code paths). Also simplified SPARKLE conditional in LightingEval.hlsli by dropping the SNOW exclusion so SPARKLE compiles regardless of SNOW.

Changes

Cohort / File(s) Summary
SPARKLE conditional
package/Shaders/Common/LightingEval.hlsli
Changed #if defined(SPARKLE) && !defined(SNOW) to #if defined(SPARKLE) so sparkle UV scale/sampling code compiles regardless of SNOW.
SNOW feature removal
package/Shaders/Lighting.hlsl
Removed SNOW-specific PS_OUTPUT field (float4 Parameters : SV_Target7), deleted functions GetLandSnowMaskValue, GetLandNormal, GetSnowSpecularColor, and eliminated all SNOW-conditioned sampling, masks, normals, and branching across the shader.

Sequence Diagram(s)

(omitted — changes do not introduce a multi-component sequential flow that benefits from a diagram)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • jiayev

Poem

🐇✨ I nudged the snowflakes out of sight,
Sparkles now glow in freer light,
Lines pruned tidy, shaders bright,
Hopping on through code tonight —
A happy rabbit's debugging delight.

🚥 Pre-merge checks | ✅ 3
✅ 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 clearly and accurately describes the main change: removing dead/unused snow-related code from the shader system.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.



📜 Recent 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 331b492 and ec5f2b0.

📒 Files selected for processing (2)
  • package/Shaders/Common/LightingEval.hlsli
  • package/Shaders/Lighting.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/Common/LightingEval.hlsli
  • package/Shaders/Lighting.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/Common/LightingEval.hlsli
  • package/Shaders/Lighting.hlsl
🧠 Learnings (9)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp: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: 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: 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.
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)
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-05T18:22:40.578Z
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.

Applied to files:

  • package/Shaders/Common/LightingEval.hlsli
  • package/Shaders/Lighting.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/Common/LightingEval.hlsli
  • 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/Common/LightingEval.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:

  • package/Shaders/Common/LightingEval.hlsli
  • 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-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/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-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} : Place all feature shaders under features/YourFeature/Shaders/

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: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
  • GitHub Check: Build plugin and addons
🔇 Additional comments (7)
package/Shaders/Common/LightingEval.hlsli (1)

80-92: LGTM! SPARKLE conditional simplified correctly.

Removing the && !defined(SNOW) condition is appropriate since the SNOW define is being removed entirely from the codebase. The SPARKLE code path now compiles unconditionally when SPARKLE is defined, which correctly reflects the dead code removal.

package/Shaders/Lighting.hlsl (6)

336-345: LGTM! DEFERRED PS_OUTPUT struct correctly simplified.

The removal of the Parameters : SV_Target7 field aligns with the dead SNOW code removal. The struct now only contains the necessary render targets for the deferred rendering pipeline.


778-784: LGTM! Base game snow flag handling correctly preserved.

GetSnowParameterY handles the vanilla game's BaseObjectIsSnow permutation flag, which is distinct from the removed improved snow feature (where bImprovedSnow was explicitly set to 0). Correctly retained.


1917-1922: LGTM! PROJECTED_UV code path simplified correctly.

The code path now directly assigns ProjectedUVParams2.xyz to baseColor.xyz without the removed SNOW-specific branching logic. This is consistent with the dead code removal.


2792-2793: LGTM!

Standard specular color processing retained correctly after SNOW code removal.


718-728: LGTM! World map snow overlay correctly preserved.

The world map overlay snow functionality (sampling and blending for LODLANDNOISE/LODLANDSCAPE) is a separate visual feature from the removed improved snow system. Correctly retained along with the associated sampler declarations.


3052-3053: Overall: Clean dead code removal.

The PR correctly removes the dead improved snow code (bImprovedSnow = 0) while preserving:

  • Base game BaseObjectIsSnow flag handling
  • World map snow overlay functionality

The changes are complete and consistent across both shader files.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

github-actions bot commented Dec 19, 2025

Using provided base ref: 331b492
Using base ref: 331b492
Base commit date: 2026-01-15T18:32:24+10:00 (Thursday, January 15, 2026 06:32 PM)
No actionable suggestions for changed features.

@davo0411 davo0411 marked this pull request as ready for review January 15, 2026 09:01
@davo0411
Copy link
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

✅ Actions performed

Full review triggered.

@github-actions
Copy link

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

Copy link
Owner

@doodlum doodlum left a comment

Choose a reason for hiding this comment

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

we should leave mask code in if we want to support snow deformation in the future. So still making the mask, just not doing anything with it and not writing to anything.

@davo0411
Copy link
Collaborator Author

still

I forgor

I fix soon

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.

2 participants