refactor(water-effects): move to core by davo0411 · Pull Request #1652 · community-shaders/skyrim-community-shaders

davo0411 · 2025-12-21T22:08:50Z

Summary by CodeRabbit

New Features
- Streamlined the Water Effects description shown in the UI: concise summary of parallax, flow map parallax, caustics and lighting tweaks.
- Water Effects is now designated as a core feature (affects feature classification in the UI).
Bug Fixes
- Structural change to the Water Effects implementation; no public API changes.

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

coderabbitai · 2025-12-21T22:08:58Z

📝 Walkthrough

Walkthrough

Added virtual bool IsCore() const override to WaterEffects and shortened GetFeatureSummary() text in the header. The implementation file gained an extra closing brace at EOF, introducing a stray block end and likely causing a compile/parse error. No public API signatures were changed.

Changes

Cohort / File(s)	Summary
WaterEffects header `src/Features/WaterEffects.h`	Added `virtual bool IsCore() const override { return true; }`; condensed `GetFeatureSummary()` description; minor formatting tweak.
WaterEffects implementation `src/Features/WaterEffects.cpp`	Stray extra closing brace added at EOF (duplicate end-block), likely syntactically invalid; file ends without newline.

Sequence Diagram(s)

(omitted — changes do not introduce a new multi-component control flow)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

doodlum

Poem

🐰 I nibble code by lantern light,
I tuck in a method snug and tight.
A stray brace jumped out to play—
Patch the fence and hop away! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(water-effects): move to core' directly and clearly describes the main change in the PR—moving water effects to core by adding the IsCore() method that returns true.

✨ Finishing touches

📝 Generate docstrings

📜 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 be4dfe9 and d954816.

📒 Files selected for processing (2)

src/Features/WaterEffects.cpp
src/Features/WaterEffects.h

🚧 Files skipped from review as they are similar to previous changes (1)

src/Features/WaterEffects.h

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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:

src/Features/WaterEffects.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

src/Features/WaterEffects.cpp

**/*

⚙️ CodeRabbit configuration file

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

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

Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:

"Fixes feat: increased portal strict light limit #123" or "Closes feat: increased portal strict light limit #123" for bug fixes

"Implements feat: increased portal strict light limit #123" or "Addresses feat: increased portal strict light limit #123" for features

"Related to feat: increased portal strict light limit #123" for partial implementations

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

Files:

src/Features/WaterEffects.cpp

🧠 Learnings (5)

📓 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: 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: 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: 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.

📚 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:

src/Features/WaterEffects.cpp

📚 Learning: 2025-06-24T07:17:36.604Z

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.

Applied to files:

src/Features/WaterEffects.cpp

📚 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:

src/Features/WaterEffects.cpp

📚 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:

src/Features/WaterEffects.cpp

⏰ 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). (1)

GitHub Check: Build plugin and addons

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 · 2025-12-21T22:10:18Z

Using provided base ref: bb5615e
Using base ref: bb5615e
Base commit date: 2026-01-04T00:44:58Z (Sunday, January 04, 2026 12:44 AM)
No actionable suggestions for changed features.

coderabbitai

Actionable comments posted: 1

📜 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 a359166 and 5ad35fd.

📒 Files selected for processing (2)

src/Features/WaterEffects.cpp (1 hunks)
src/Features/WaterEffects.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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:

src/Features/WaterEffects.cpp
src/Features/WaterEffects.h

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

src/Features/WaterEffects.cpp
src/Features/WaterEffects.h

**/*

⚙️ CodeRabbit configuration file

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

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

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:

src/Features/WaterEffects.cpp
src/Features/WaterEffects.h

🧠 Learnings (2)

📓 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: 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: 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: 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-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/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()

Applied to files:

src/Features/WaterEffects.h

⏰ 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). (1)

GitHub Check: Build plugin and addons

🔇 Additional comments (3)

src/Features/WaterEffects.cpp (1)

24-28: Informational-only DrawSettings implementation looks good.

The DrawSettings() implementation correctly displays helpful information about parallax alpha channel requirements. While it doesn't provide interactive settings controls, this is acceptable if the feature currently has no user-configurable options.

src/Features/WaterEffects.h (2)

18-28: Feature summary description is clear and accurate.

The updated summary text provides a concise overview of the water effects capabilities, listing specific features like parallax, flow map parallax, caustics, and lighting tweaks.

30-30: Core feature designation implemented correctly.

The IsCore() override returning true properly marks this as a core feature, aligning with the PR objective to move WaterEffects to core status.

coderabbitai

Actionable comments posted: 1

📜 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 5ad35fd and de68b40.

📒 Files selected for processing (2)

src/Features/WaterEffects.cpp
src/Features/WaterEffects.h

🚧 Files skipped from review as they are similar to previous changes (1)

src/Features/WaterEffects.h

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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:

src/Features/WaterEffects.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

src/Features/WaterEffects.cpp

**/*

⚙️ CodeRabbit configuration file

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

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

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:

src/Features/WaterEffects.cpp

🧠 Learnings (1)

📓 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: 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: 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: 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.

⏰ 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). (1)

GitHub Check: Build plugin and addons

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.

doodlum

Failed checks

github-actions · 2026-01-04T05:28:27Z

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

fixes

5ad35fd

coderabbitai Bot reviewed Dec 21, 2025

View reviewed changes

Comment thread src/Features/WaterEffects.h Outdated

davo0411 changed the title ~~refactor(water-effects): move to core, description changes~~ refactor(water-effects): move to core Dec 30, 2025

remove broken description

de68b40

coderabbitai Bot reviewed Dec 30, 2025

View reviewed changes

Comment thread src/Features/WaterEffects.cpp

davo0411 and others added 2 commits December 31, 2025 18:22

Update src/Features/WaterEffects.cpp

b6e3364

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

style: 🎨 apply pre-commit.ci formatting

be4dfe9

Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.

doodlum requested changes Jan 1, 2026

View reviewed changes

Merge branch 'doodlum:dev' into water-effects-semantics

7e07e13

davo0411 requested a review from doodlum January 4, 2026 04:33

fix: file corruption

d954816

davo0411 closed this Jan 6, 2026

davo0411 deleted the water-effects-semantics branch January 26, 2026 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(water-effects): move to core#1652

refactor(water-effects): move to core#1652
davo0411 wants to merge 6 commits into
community-shaders:devfrom
davo0411:water-effects-semantics

davo0411 commented Dec 21, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 21, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Dec 21, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

doodlum left a comment

Uh oh!

github-actions Bot commented Jan 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

davo0411 commented Dec 21, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions Bot commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

doodlum left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davo0411 commented Dec 21, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 21, 2025 •

edited

Loading

github-actions Bot commented Dec 21, 2025 •

edited

Loading