feat(snow-cover): add snow cover feature#2047
Closed
InTheBottle wants to merge 169 commits into
Closed
Conversation
# Conflicts: # package/Shaders/Common/Color.hlsli # package/Shaders/DistantTree.hlsl # src/FeatureBuffer.cpp
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new snow cover feature is introduced with regional shader configurations, HLSL shader logic for snow/ice material blending and foliage tinting, C++ feature implementation with seasonal dynamics and configuration management, and integration with existing rendering pipelines through permutation flags and buffer updates. Changes
Sequence DiagramsequenceDiagram
participant CPU as SnowCover Feature
participant Cfg as Config System
participant Tex as Texture Manager
participant RenderPass as Render Pass (Lighting/Grass/Tree)
participant Shader as GPU Shader (SnowCover.hlsli)
participant Output as Frame Buffer
CPU->>Cfg: LoadSettings() / LoadWorldConfig()
Cfg-->>CPU: Parse JSON + Whitelist/Blacklist
CPU->>Tex: CreateTextures(DDS paths)
Tex-->>CPU: Bind SRVs (t38–t44)
CPU->>CPU: ComputeSeasonalAltitude()
CPU->>CPU: UpdateTimeSnowing(weather, speed)
CPU->>RenderPass: Prepass() binds SRVs + sets NoSnow permutation
RenderPass->>RenderPass: Permute shader based on NoSnow flag
RenderPass->>Shader: Execute (SnowCover::ApplySnow[*] functions)
Shader->>Shader: ComputeSnowHeight(SnowMap + seasonal offset)
Shader->>Shader: DeriveEnvironmentalMultiplier(height, peak angles)
Shader->>Shader: ApplySnowMaterial(main/alt textures, normals, PBR params)
Shader->>Shader: ApplySnowFoliage(foliage tinting, hue shift)
Shader-->>Output: Blended snow/ice base color + normals
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Actionable Suggestions
|
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
package/Shaders/Common/Color.hlsli (1)
88-88: Optional PR metadata polishConsider updating the PR title to a scoped conventional form, e.g.
feat(snow-cover): add snow cover feature, and add an issue keyword likeImplements #<id>if this tracks an issue.As per coding guidelines: "provide suggestions for Conventional Commit Titles ... and Issue References."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/Color.hlsli` at line 88, Update the PR metadata: change the PR title to a scoped conventional commit style (e.g., "feat(snow-cover): add snow cover feature") and add an issue reference like "Implements #<id>" in the PR description; you can locate this change while reviewing the diff touching package/Shaders/Common/Color.hlsli (the comment line with the RGB/HSV HLSL link) to ensure the PR title and description reflect the code intention and tracking number.src/Features/SnowCover.h (1)
41-41: NarrowHasShaderDefine()to the shader families that actually use snow.Returning
truefor everyRE::BSShader::TypewidensSNOW_COVERpermutations even for shader families that immediately ignore the define. Restricting this to the lighting/tree/grass path will keep shader-cache churn down.As per coding guidelines, "Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/SnowCover.h` at line 41, The HasShaderDefine(RE::BSShader::Type) implementation currently returns true for every shader type which forces SNOW_COVER into all shader permutations; change HasShaderDefine to only return true for the shader families that actually use snow (e.g., the lighting, tree and grass shader types) by checking the incoming RE::BSShader::Type against those specific enum values and returning true only for them, otherwise return false; update the HasShaderDefine method (and any associated comments) to enumerate the allowed types so shader-cache churn is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Snow` Cover/Shaders/SnowCover/SnowCover.hlsli:
- Around line 27-29: GetEnvironmentalMultiplier currently divides by
SharedData::snowCoverSettings.BlendSmoothness which can be zero; clamp the
denominator to a small epsilon (e.g.
max(SharedData::snowCoverSettings.BlendSmoothness, EPSILON)) before dividing or
validate/replace BlendSmoothness on settings load; update the
GetEnvironmentalMultiplier function to use the clamped value so division cannot
produce inf/NaN and downstream snow blends remain stable.
In `@package/Shaders/AmbientCompositeCS.hlsl`:
- Around line 74-80: linAmbient is converted to linear (via
Color::GammaToLinear) and attenuated but later the shader adds
directionalAmbientColor * albedo in gamma space, causing incorrect lighting; fix
by keeping ambient compositing entirely in linear space: compute linAmbient =
Color::GammaToLinear(albedo * directionalAmbientColor) (or multiply linAlbedo *
linDirectionalAmbientColor), accumulate the ambient term into the linear
lighting accumulator (e.g., add to linDiffuseColor or a dedicated linear ambient
accumulator) and only gamma-encode once when writing out the final color (use
Color::LinearToGamma at the output), updating usages around linAmbient and the
final write where directionalAmbientColor * albedo is added so no ambient term
is mixed in gamma space.
In `@package/Shaders/Lighting.hlsl`:
- Around line 31-33: The SNOW_COVER macro is being undefined whenever
EXTENDED_MATERIALS is not defined, which prevents non-EMAT snow fallbacks from
running; update the preprocessor condition so SNOW_COVER is only undefined for
incompatible geometry (SKINNED, SKIN, EYE, HAIR) and not tied to
EXTENDED_MATERIALS—i.e., remove the "!defined(EXTENDED_MATERIALS)" clause around
the `#undef` SNOW_COVER (leave checks for SKINNED/SKIN/EYE/HAIR intact) and verify
the existing non-EMAT fallback code paths (e.g., disp = 0) still execute when
EXTENDED_MATERIALS is not defined.
In `@package/Shaders/RunGrass.hlsl`:
- Around line 558-592: The foliage snow path in the SNOW_COVER block runs
unconditionally and needs to be skipped when the world is interior; wrap the
snow occlusion / SnowCover::ApplySnowFoliage logic with a check against
SharedData::InInterior (e.g. if (!SharedData::InInterior) { ... } ), or
early-return/skip that entire SNOW_COVER section when SharedData::InInterior is
true. Update the block that computes snowOcclusion and calls
SnowCover::ApplySnowFoliage (inside RunGrass.hlsl’s SNOW_COVER handling) so it
only executes when SharedData::InInterior is false, keeping existing use of
FrameBuffer::CameraPosAdjust, eyeIndex, and other symbols intact.
In `@src/Features/SnowCover.cpp`:
- Around line 404-419: SnowCover::SaveConfig currently saves to disk then calls
Reload(), but Reload() exits early because last_worldspace still equals
GetWorldspace(); to force the post-save reload, reset last_worldspace (e.g.,
clear or set to an empty value) before calling Reload() so Reload() will treat
the worldspace as changed and reapply textures/derived maps; ensure this reset
happens in both normal and exception paths so Reload() always runs.
- Around line 607-623: The blacklist check is performed too late so a
blacklisted movable with AffectHavok enabled can have NoSnow cleared by the
velocity branch; reorder the logic so that after the whitelist check
(whitelist.contains(FormIdParser::fnv_hash(name))) you immediately test
blacklist.contains(FormIdParser::fnv_hash(name)) and set
state->permutationData.ExtraShaderDescriptor |=
(uint)State::ExtraShaderDescriptors::NoSnow if true, otherwise proceed into the
existing animation/movable/Havok branch (settings.AffectHavok,
userData->CanBeMoved(), userData->GetLinearVelocity(...)) to decide whether to
set or clear NoSnow; keep all referenced symbols (settings.AffectHavok,
userData, whitelist.contains, blacklist.contains, FormIdParser::fnv_hash(name),
state->permutationData.ExtraShaderDescriptor,
State::ExtraShaderDescriptors::NoSnow) unchanged.
- Around line 148-150: The "Defaults" button currently only assigns wsettings =
WorldSettings(), but other persisted fields used by ToWorldConfig() (seasonal
months/heights, map bounds, texture paths, snow/melt speeds, etc.) remain stale;
update the ImGui::Button("Defaults") handler to also reset all individual
members that ToWorldConfig() serializes (e.g., seasonalMonths, seasonalHeights,
mapBounds, texturePaths, snowSpeed, meltSpeed or any similarly named member
variables) back to their default values (or copy them from the new
WorldSettings() instance) so clicking Defaults truly resets every persisted
field before Save.
---
Nitpick comments:
In `@package/Shaders/Common/Color.hlsli`:
- Line 88: Update the PR metadata: change the PR title to a scoped conventional
commit style (e.g., "feat(snow-cover): add snow cover feature") and add an issue
reference like "Implements #<id>" in the PR description; you can locate this
change while reviewing the diff touching package/Shaders/Common/Color.hlsli (the
comment line with the RGB/HSV HLSL link) to ensure the PR title and description
reflect the code intention and tracking number.
In `@src/Features/SnowCover.h`:
- Line 41: The HasShaderDefine(RE::BSShader::Type) implementation currently
returns true for every shader type which forces SNOW_COVER into all shader
permutations; change HasShaderDefine to only return true for the shader families
that actually use snow (e.g., the lighting, tree and grass shader types) by
checking the incoming RE::BSShader::Type against those specific enum values and
returning true only for them, otherwise return false; update the HasShaderDefine
method (and any associated comments) to enumerate the allowed types so
shader-cache churn is avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a71e8f8-6405-4390-9d95-517c80d7839c
⛔ Files ignored due to path filters (4)
features/Snow Cover/Shaders/SnowCover/Skyrim.ddsis excluded by!**/*.ddsfeatures/Snow Cover/Shaders/SnowCover/default/main.ddsis excluded by!**/*.ddsfeatures/Snow Cover/Shaders/SnowCover/default/main_n.ddsis excluded by!**/*.ddsfeatures/Snow Cover/Shaders/SnowCover/default/main_rmaos.ddsis excluded by!**/*.dds
📒 Files selected for processing (26)
features/Snow Cover/Shaders/Features/SnowCover.inifeatures/Snow Cover/Shaders/SnowCover/MarkarthWorld.jsonfeatures/Snow Cover/Shaders/SnowCover/RiftenWorld.jsonfeatures/Snow Cover/Shaders/SnowCover/SnowCover.hlslifeatures/Snow Cover/Shaders/SnowCover/SolitudeWorld.jsonfeatures/Snow Cover/Shaders/SnowCover/Tamriel.jsonfeatures/Snow Cover/Shaders/SnowCover/WhiterunWorld.jsonfeatures/Snow Cover/Shaders/SnowCover/WindhelmWorld.jsonfeatures/Snow Cover/Shaders/SnowCover/blacklist.txtfeatures/Snow Cover/Shaders/SnowCover/whitelist.txtpackage/Shaders/AmbientCompositeCS.hlslpackage/Shaders/Common/Color.hlslipackage/Shaders/Common/Permutation.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/DistantTree.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/SnowCover.cppsrc/Features/SnowCover.hsrc/Globals.cppsrc/Globals.hsrc/State.hsrc/Utils/FormIdParser.cppsrc/Utils/FormIdParser.h
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores