Skip to content

refactor(weather-editor): remove duplicate code#2154

Merged
doodlum merged 2 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:fix/WE-precipitation-texture-dedup
Apr 20, 2026
Merged

refactor(weather-editor): remove duplicate code#2154
doodlum merged 2 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:fix/WE-precipitation-texture-dedup

Conversation

@SkrubbySkrubInAShrub
Copy link
Copy Markdown
Collaborator

@SkrubbySkrubInAShrub SkrubbySkrubInAShrub commented Apr 19, 2026

After revisiting my recent PR I noticed that I had a duplicated HasDdsExtension() call.
This PR cleans up this code so that there is only 1 filesystem read instead of 2.

Summary by CodeRabbit

  • Refactor
    • Improved particle texture input flow in the editor: validation is more reliable, unnecessary disk checks during rendering are reduced, and texture changes are only applied once the file is confirmed present.
    • Clarified error display: fewer false "file not found" warnings and more consistent ".dds" validation messaging.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cf72fd7f-ea16-4fee-99e7-33ed1d5ba4f7

📥 Commits

Reviewing files that changed from the base of the PR and between 04b6aef and d553deb.

📒 Files selected for processing (1)
  • src/WeatherEditor/Weather/PrecipitationWidget.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/WeatherEditor/Weather/PrecipitationWidget.cpp

📝 Walkthrough

Walkthrough

Refactored the ImGui particle texture input flow in PrecipitationWidget: compute a single inputChanged flag, refresh cached buffer/existence when the text differs, and only assign settings.particleTexture when input changed and the prior existence check succeeded; removed redundant existence checks during error display.

Changes

Cohort / File(s) Summary
Particle Texture Input Validation
src/WeatherEditor/Weather/PrecipitationWidget.cpp
Refactored ImGui InputText handling to capture inputChanged once, updated logic to refresh lastCheckedBuffer/lastCheckedExists when buffer differs, gated settings.particleTexture assignment to inputChanged && lastCheckedExists, and simplified error-message conditions by removing redundant existence re-checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • davo0411

Poem

🐰 I hopped the fields of buffer text and checks,

One flag now guides the path, no nested wrecks.
Cache refreshed, the file's fate gently known,
Settings wait till proof and change are shown.
A tiny thump — the widget rules are sewn.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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(weather-editor): remove duplicate code' accurately summarizes the main objective of removing duplicate filesystem read calls in the precipitation texture widget logic.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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.

🧹 Nitpick comments (1)
src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)

79-95: Nit: redundant block scope.

The anonymous { ... } block around lines 79–95 previously isolated locals that are no longer declared inside it (buf is the only remaining local and doesn't need its own scope). Optional cleanup:

Proposed cleanup
 				const bool inputChanged = ImGui::InputText("Particle Texture", textureBuffer, sizeof(textureBuffer));
-				{
-					std::string_view buf(textureBuffer);
-					if (buf != lastCheckedBuffer) {
-						lastCheckedBuffer = std::string(buf);
-						lastCheckedExists = WeatherUtils::TexturePath::ExistsOnDisk(buf);
-					}
-					if (inputChanged && lastCheckedExists) {
-						settings.particleTexture = lastCheckedBuffer;
-						changed = true;
-					}
-					if (settings.particleTexture != buf && !buf.empty()) {
-						if (!WeatherUtils::TexturePath::HasDdsExtension(buf))
-							ImGui::TextColored(globals::menu->GetTheme().StatusPalette.Error, "Path must end with '.dds'");
-						else if (!lastCheckedExists)
-							ImGui::TextColored(globals::menu->GetTheme().StatusPalette.Error, "Texture file not found under Data/textures/.");
-					}
-				}
+				std::string_view buf(textureBuffer);
+				if (buf != lastCheckedBuffer) {
+					lastCheckedBuffer = std::string(buf);
+					lastCheckedExists = WeatherUtils::TexturePath::ExistsOnDisk(buf);
+				}
+				if (inputChanged && lastCheckedExists) {
+					settings.particleTexture = lastCheckedBuffer;
+					changed = true;
+				}
+				if (settings.particleTexture != buf && !buf.empty()) {
+					if (!WeatherUtils::TexturePath::HasDdsExtension(buf))
+						ImGui::TextColored(globals::menu->GetTheme().StatusPalette.Error, "Path must end with '.dds'");
+					else if (!lastCheckedExists)
+						ImGui::TextColored(globals::menu->GetTheme().StatusPalette.Error, "Texture file not found under Data/textures/.");
+				}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 79 - 95,
Remove the redundant anonymous scope surrounding the texture validation code:
unnest the block so the logic using std::string_view buf, lastCheckedBuffer,
lastCheckedExists, WeatherUtils::TexturePath::ExistsOnDisk,
WeatherUtils::TexturePath::HasDdsExtension and settings.particleTexture runs in
the current function scope without an extra `{ ... }`; preserve all existing
checks and assignments (updating lastCheckedBuffer/lastCheckedExists, setting
settings.particleTexture when inputChanged, and showing ImGui error texts) and
only eliminate the unnecessary braces and any extra indentation to keep behavior
identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 79-95: Remove the redundant anonymous scope surrounding the
texture validation code: unnest the block so the logic using std::string_view
buf, lastCheckedBuffer, lastCheckedExists,
WeatherUtils::TexturePath::ExistsOnDisk,
WeatherUtils::TexturePath::HasDdsExtension and settings.particleTexture runs in
the current function scope without an extra `{ ... }`; preserve all existing
checks and assignments (updating lastCheckedBuffer/lastCheckedExists, setting
settings.particleTexture when inputChanged, and showing ImGui error texts) and
only eliminate the unnecessary braces and any extra indentation to keep behavior
identical.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f1c5d241-d865-4a2b-ad22-4014f4c99913

📥 Commits

Reviewing files that changed from the base of the PR and between 31a7b55 and 04b6aef.

📒 Files selected for processing (1)
  • src/WeatherEditor/Weather/PrecipitationWidget.cpp

@github-actions
Copy link
Copy Markdown

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

@doodlum doodlum merged commit 310f577 into community-shaders:dev Apr 20, 2026
17 checks passed
InTheBottle added a commit to InTheBottle/skyrim-community-shaders that referenced this pull request Apr 21, 2026
Reverts upstream PRs community-shaders#2151, community-shaders#2152, community-shaders#2154 which removed the inline ImageSpaceSettings struct from WeatherWidget and replaced HDR/bloom/cinematic/tint/DoF inline storage with TESImageSpace form pickers.

These changes broke existing per-weather edits saved in user JSON files (old 'imageSpaces' array key is silently ignored by the new loader, and SetWeatherValues unconditionally overwrites weather->imageSpaces[] with possibly-null form pointers from missing keys).

Restoring the old behavior on the fork pending a forward-compatible migration.
ParticleTroned pushed a commit to ParticleTroned/skyrim-community-shaders that referenced this pull request May 2, 2026
Co-authored-by: SkrubbySkrubInAShrub <skrubbyskrubinashrub@gmail.com>
(cherry picked from commit 310f577)
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