feat(ui): subheading icons, fixed icon scaling issues, feature subheading changes#1298
Conversation
|
""" WalkthroughThis update standardizes and reorganizes feature category assignments by modifying the return values of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant UIUtils
participant Features
User->>Menu: Open settings menu
Menu->>UIUtils: DrawSettings()
loop For each category in new order
UIUtils->>Features: GetCategory() for each feature
Features-->>UIUtils: Return category string
UIUtils->>Menu: Select category icon based on category
UIUtils->>Menu: DrawCategoryHeader(categoryName, isExpanded, count)
Menu-->>User: Rendered header with icon and text
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Poem
""" Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
davo0411
changed the title
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
package/Interface/CommunityShaders/Icons/Categories/characters.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/debug.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/grass.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/landscape.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/lighting.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/sky.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/water.pngis excluded by!**/*.png
📒 Files selected for processing (9)
package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt(1 hunks)src/Features/DynamicCubemaps.h(1 hunks)src/Features/ExtendedMaterials.h(1 hunks)src/Features/ExtendedTranslucency.h(1 hunks)src/Features/Skylighting.h(1 hunks)src/Features/VR.h(1 hunks)src/Menu.cpp(2 hunks)src/Menu.h(1 hunks)src/Utils/UI.cpp(4 hunks)
🧠 Learnings (3)
src/Features/ExtendedTranslucency.h (1)
Learnt from: alandtse
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.
src/Features/DynamicCubemaps.h (1)
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
src/Utils/UI.cpp (1)
Learnt from: davo0411
PR: #1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
🧰 Additional context used
🧠 Learnings (3)
src/Features/ExtendedTranslucency.h (1)
Learnt from: alandtse
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.
src/Features/DynamicCubemaps.h (1)
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
src/Utils/UI.cpp (1)
Learnt from: davo0411
PR: #1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
⏰ 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 (11)
src/Features/ExtendedTranslucency.h (1)
13-13: LGTM: Clean category reassignmentThe category change from "Lighting" to "Materials" aligns with the PR's UI reorganization objectives and maintains consistent method signature.
package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt (1)
1-21: LGTM: Standard MIT license for UI iconsThe MIT license is properly formatted and appropriately placed for the new category icons. The copyright attribution and terms are clear and standard.
src/Features/VR.h (1)
49-49: LGTM: Consistent GetCategory() implementationThe new GetCategory() method follows the established pattern with proper const qualifier and string_view return type. The "Debug" category assignment is appropriate for the VR feature.
src/Features/ExtendedMaterials.h (1)
14-14: LGTM: Appropriate category reassignmentThe change from "Landscape & Textures" to "Materials" better reflects the feature's focus on material effects and aligns with the broader UI reorganization effort.
src/Features/DynamicCubemaps.h (1)
105-105: LGTM: Logical category updateThe reassignment from "Lighting" to "Materials" makes sense since dynamic cubemaps primarily serve material reflection purposes. This change maintains consistency with other features in the Materials category.
src/Features/Skylighting.h (1)
21-21: Category update looks good.The change from "Sky" to "Lighting" is a logical categorization for the Skylighting feature, as it primarily deals with lighting calculations rather than sky rendering.
src/Menu.h (1)
78-89: UIIcon additions are well-structured.The new category icon members follow the existing pattern and are clearly named. Good documentation with the placeholder comment for materials and the generic purpose of the "other" icon.
src/Menu.cpp (2)
216-224: Proper cleanup of new icon resources.Good addition of release calls for all new category icons in the destructor, preventing resource leaks.
1049-1049: Logical category ordering.The updated category order with "Debug" first and "Materials" positioned between "Lighting" and "Sky" provides a sensible grouping of related features.
src/Utils/UI.cpp (2)
458-521: Well-implemented icon rendering with dynamic scaling.The icon rendering implementation properly scales with font size and applies consistent color tinting. The layout calculations correctly account for the icon dimensions.
166-305: Verify complete integration of the "other" category icon.The "other" icon appears to be only partially implemented. It's declared in Menu.h but missing from:
- The texture pointer array for cleanup (line 166)
- The loading logic (lines 239-304)
- The destructor in Menu.cpp
Please ensure the "other" icon is fully integrated across all relevant files.
| if (strcmp(categoryName, "Characters") == 0) { | ||
| categoryIcon = menu.characters.texture; | ||
| } else if (strcmp(categoryName, "Grass") == 0) { | ||
| categoryIcon = menu.grass.texture; | ||
| } else if (strcmp(categoryName, "Lighting") == 0) { | ||
| categoryIcon = menu.lighting.texture; | ||
| } else if (strcmp(categoryName, "Sky") == 0) { | ||
| categoryIcon = menu.sky.texture; | ||
| } else if (strcmp(categoryName, "Landscape & Textures") == 0) { | ||
| categoryIcon = menu.landscape.texture; | ||
| } else if (strcmp(categoryName, "Water") == 0) { | ||
| categoryIcon = menu.water.texture; | ||
| } else if (strcmp(categoryName, "Debug") == 0) { | ||
| categoryIcon = menu.debug.texture; | ||
| } else if (strcmp(categoryName, "Materials") == 0) { | ||
| categoryIcon = menu.materials.texture; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add icon mapping for "Other" category.
The icon selection logic should include handling for the "Other" category to ensure all categories have proper icon display.
} else if (strcmp(categoryName, "Materials") == 0) {
categoryIcon = menu.materials.texture;
+} else if (strcmp(categoryName, "Other") == 0) {
+ categoryIcon = menu.other.texture;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (strcmp(categoryName, "Characters") == 0) { | |
| categoryIcon = menu.characters.texture; | |
| } else if (strcmp(categoryName, "Grass") == 0) { | |
| categoryIcon = menu.grass.texture; | |
| } else if (strcmp(categoryName, "Lighting") == 0) { | |
| categoryIcon = menu.lighting.texture; | |
| } else if (strcmp(categoryName, "Sky") == 0) { | |
| categoryIcon = menu.sky.texture; | |
| } else if (strcmp(categoryName, "Landscape & Textures") == 0) { | |
| categoryIcon = menu.landscape.texture; | |
| } else if (strcmp(categoryName, "Water") == 0) { | |
| categoryIcon = menu.water.texture; | |
| } else if (strcmp(categoryName, "Debug") == 0) { | |
| categoryIcon = menu.debug.texture; | |
| } else if (strcmp(categoryName, "Materials") == 0) { | |
| categoryIcon = menu.materials.texture; | |
| } | |
| if (strcmp(categoryName, "Characters") == 0) { | |
| categoryIcon = menu.characters.texture; | |
| } else if (strcmp(categoryName, "Grass") == 0) { | |
| categoryIcon = menu.grass.texture; | |
| } else if (strcmp(categoryName, "Lighting") == 0) { | |
| categoryIcon = menu.lighting.texture; | |
| } else if (strcmp(categoryName, "Sky") == 0) { | |
| categoryIcon = menu.sky.texture; | |
| } else if (strcmp(categoryName, "Landscape & Textures") == 0) { | |
| categoryIcon = menu.landscape.texture; | |
| } else if (strcmp(categoryName, "Water") == 0) { | |
| categoryIcon = menu.water.texture; | |
| } else if (strcmp(categoryName, "Debug") == 0) { | |
| categoryIcon = menu.debug.texture; | |
| } else if (strcmp(categoryName, "Materials") == 0) { | |
| categoryIcon = menu.materials.texture; | |
| } else if (strcmp(categoryName, "Other") == 0) { | |
| categoryIcon = menu.other.texture; | |
| } |
🤖 Prompt for AI Agents
In src/Utils/UI.cpp around lines 431 to 447, the icon selection logic lacks a
case for the "Other" category, causing it to miss an appropriate icon. Add an
else-if condition to check if categoryName equals "Other" and assign the
corresponding icon from menu.other.texture to categoryIcon to ensure all
categories have proper icon display.
There was a problem hiding this comment.
🧩 Analysis chain
Let me investigate whether the "Other" category is actually used in the codebase and if the missing mapping would cause issues.
🏁 Script executed:
# Search for "Other" category usage in the codebase
rg '"Other"' -A 2 -B 2
# Look for category definitions and usage patterns
rg 'GetCategory.*return' -A 1 -B 1
# Check what categories are defined in Menu.cpp predefined order
rg 'predefinedOrder.*=|predefinedOrder' -A 10 -B 2
Length of output: 9565
@doodlum You're right to question this! After investigating the codebase, this is actually a problem. Here's what I found:
The search results show that:
-
"Other" is a real category - it's included in the predefined category order in Menu.cpp:
"Debug", "Characters", "Grass", "Lighting", "Materials", "Sky", "Landscape & Textures", "Water", "Other" -
"Other" is the default fallback - in Feature.h, the base
GetCategory()method returns"Other"by default for any features that don't override it -
The UI will display "Other" category headers - since it's in the category order, but without the icon mapping, these headers won't show an icon while all other categories will
This creates inconsistent UI behavior where some category headers have icons and others don't, even though the menu.other.texture icon exists (as mentioned in the AI summary).
The missing mapping means any features using the default "Other" category will have headers without icons, which breaks the visual consistency of the interface. My original suggestion to add the "Other" category mapping is indeed necessary for proper UI functionality.
🏁 Script executed:
# Check if "other" icon is loaded in InitializeMenuIcons
rg "other\." src/Utils/UI.cpp -A 2 -B 2
# Verify if there's an "other" icon file that should be loaded
fd "other.png" --type f
Length of output: 65
Let me check what's actually defined in the Menu.h structure and what category icon files exist:
🏁 Script executed:
# Check the UIIcons struct definition in Menu.h
rg "struct UIIcons|UIIcon.*other|UIIcon.*materials" src/Menu.h -A 10 -B 2
# List all icon files in the Categories directory
fd . "package/Interface/CommunityShaders/Icons/Categories" --type f
# Check if there are any icon files with "other" in the name
fd "other" --type f
Length of output: 1507
Perfect! Now I can see the full scope of the issue. You're absolutely right to be concerned - this is a problem, and it's actually bigger than just the missing mapping.
Here's what I found:
-
The
othericon IS defined in Menu.h:UIIcon other; // Generic icon for other/misc categories -
But the
other.pngfile doesn't exist - the Categories directory has icons for all other categories (characters, debug, grass, landscape, lighting, materials, sky, water) but noother.png -
The loading code is also missing -
InitializeMenuIconsdoesn't attempt to load theothericon -
And the mapping is missing - which is what my original suggestion addressed
So there are actually three missing pieces:
- The
other.pngicon file itself - Loading the icon in
InitializeMenuIcons - Mapping the icon in
DrawCategoryHeader
This means any features using the default "Other" category (which is the fallback for features that don't specify a category) will have inconsistent header display compared to other categories.
My original suggestion to add the mapping is still valid and necessary - even if the icon file doesn't exist yet, the mapping should be there so when the icon is eventually added, it will work properly. The code gracefully handles missing textures (nullptr), so adding the mapping won't break anything.
The issue affects UI consistency since "Other" is a real category that appears in the interface but won't display an icon like all the others do.
🧬 Code Graph Analysis Results
Relevant snippets from src/Utils/UI.h (lines 53-350 approx)
This header declares the classes and functions implemented in src/Utils/UI.cpp. It includes:
HoverTooltipWrapperclass: RAII wrapper for ImGui tooltips on hovered items.DisableGuardclass: RAII wrapper for ImGui disabled state.StyledButtonWrapperclass: RAII wrapper for pushing/popping button style colors.SectionWrapperclass: RAII wrapper for drawing section headers with optional description and color.- Functions for UI elements:
PercentageSlider: slider for float values shown as percentages.GetNativeViewportSizeScaled: returns scaled viewport size.LoadTextureFromFile: loads a texture from file into a D3D11 shader resource view.InitializeMenuIcons: loads multiple icons for the menu.DrawSharpText: draws text aligned to pixel grid with optional scaling.DrawAlignedTextWithLogo: draws text with a logo icon aligned vertically.DrawCategoryHeader: draws a collapsible category header with icon and lines.DrawSectionHeader: draws a styled section header, optionally collapsible.ColorCodedValueConfigstruct and related functions for color-coded value display.DrawColorCodedValue: draws a label and a color-coded value with optional tooltip.DrawMultiLineTooltipandDrawColoredMultiLineTooltip: draw multi-line tooltips with optional colors.- Table sorting and rendering helpers:
ShowSortedStringTableStrings: renders a sortable ImGui table for string data.ShowSortedStringTableCustom: templated function to render sortable tables for custom row types.
These declarations provide context for the implementations in UI.cpp.
Relevant snippets from src/Menu.h (lines 16-20)
static Menu* GetSingleton()
{
static Menu menu;
return &menu;
}This singleton accessor is used in UI.cpp to get the global menu instance for accessing UI icons and theme colors.
Relevant snippets from src/Menu.cpp (lines 721-732)
void operator()(const CategoryHeader& header)
{
// Get expansion state from static map
bool isExpanded = categoryExpansionStates[header.name];
// Draw category header with custom styling using util:UI function
int count = categoryCounts[std::string(header.name)];
Util::DrawCategoryHeader(header.name.c_str(), isExpanded, count);
// Update expansion state
categoryExpansionStates[header.name] = isExpanded;
}This snippet shows usage of Util::DrawCategoryHeader from UI.cpp to render category headers with expansion state.
Relevant snippets from src/Menu.cpp (lines 733-779)
void operator()(Feature* feat)
{
const auto featureName = feat->GetShortName();
bool isDisabled = globals::state->IsFeatureDisabled(featureName);
bool isLoaded = feat->loaded;
bool hasFailedMessage = !feat->failedLoadedMessage.empty();
auto& themeSettings = globals::menu->settings.Theme;
ImVec4 textColor;
// Determine the text color based on the state
if (isDisabled) {
textColor = themeSettings.StatusPalette.Disable;
} else if (isLoaded) {
textColor = ImGui::GetStyleColorVec4(ImGuiCol_Text);
} else if (hasFailedMessage) {
textColor = feat->version.empty() ? themeSettings.StatusPalette.Disable : themeSettings.StatusPalette.Error;
} else {
// No failed message but not loaded - check if INI file exists
if (!std::filesystem::exists(Util::PathHelpers::GetFeatureIniPath(feat->GetShortName()))) {
// INI file missing - treat as missing feature (grey)
textColor = themeSettings.StatusPalette.Disable;
} else {
// INI file exists but feature not loaded - truly pending restart (green)
textColor = themeSettings.StatusPalette.RestartNeeded;
}
}
// Set text color
ImGui::PushStyleColor(ImGuiCol_Text, textColor);
// Create selectable item
if (ImGui::Selectable(fmt::format(" {} ", feat->GetName()).c_str(), selectedMenuRef == listId, ImGuiSelectableFlags_SpanAllColumns)) {
selectedMenuRef = listId;
}
// Restore original text color
ImGui::PopStyleColor();
// Display version if loaded
if (isLoaded) {
ImGui::SameLine();
std::string formattedVersion = feat->version;
std::replace(formattedVersion.begin(), formattedVersion.end(), '-', '.');
ImGui::TextDisabled(fmt::format("({})", formattedVersion).c_str());
}
}This snippet shows feature list rendering using ImGui, with color coding and selectable items. It uses Util::PathHelpers::GetFeatureIniPath and theme colors from the menu singleton.
Relevant snippets from src/Menu.cpp (lines 800-1007)
This large snippet shows detailed UI rendering for a feature's settings and about tabs, including:
- Handling disabled, loaded, failed, and unloaded states.
- Drawing feature settings or unloaded UI.
- Showing error messages.
- About tab with status and description.
- Buttons for enabling/disabling at boot and restoring defaults.
- Tooltips using
Util::HoverTooltipWrapper. - Uses
Util::PathHelpers::GetFeatureIniPathand theme colors.
Summary of relevance
- The
UI.cppfile implements many UI helper functions and wrappers used throughout the UI code. - The
InitializeMenuIconsfunction loads icons used in category headers and elsewhere. - The
DrawCategoryHeaderandDrawSectionHeaderfunctions provide styled headers with icons and lines. - The
HoverTooltipWrapperis used for tooltips in multiple places. - The
Menusingleton is accessed for icons and theme colors. - The
Menu.cppsnippets show usage of these UI helpers for rendering feature lists and category headers. - The
PathHelpers::GetFeatureIniPathfunction is referenced for checking feature INI files, relevant to feature UI states.
No other snippets from the provided list appear directly relevant to understanding or addressing the code in src/Utils/UI.cpp.
🧠 Learnings used
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt (2)
4-7: Pronoun/agreement nit – “it” → “them”Plural subject “images” should take “them/they” to avoid legal ambiguity.
-These images are free software: you can redistribute it and/or modify +These images are free software: you can redistribute them and/or modify
14-15: Terminate sentence with a period for legal completenessThe final sentence is missing a terminating period which is present in the canonical GPL header.
-along with this program. If not, see <http://www.gnu.org/licenses/> +along with this program. If not, see <http://www.gnu.org/licenses/>.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Interface/CommunityShaders/Icons/Categories/materials.pngis excluded by!**/*.png
📒 Files selected for processing (2)
README.md(1 hunks)package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt(1 hunks)
🧠 Learnings (2)
📓 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.
package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt (2)
Learnt from: davo0411
PR: #1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (2)
📓 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.
package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt (2)
Learnt from: davo0411
PR: #1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
| Community Shaders Category Icons | ||
| Copyright (C) 2025 dem6nd | ||
|
|
||
| These images are free software: you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License as published by | ||
| the Free Software Foundation, either version 3 of the License, or | ||
| (at your option) any later version. | ||
|
|
||
| These images are distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License for more details. | ||
|
|
||
| You should have received a copy of the GNU General Public License | ||
| along with this program. If not, see <http://www.gnu.org/licenses/> No newline at end of file |
There was a problem hiding this comment.
Missing the full GPL-3.0 text – risk of non-compliance
GPL requires that the “complete corresponding license text” accompany the work.
This stub only contains the boiler-plate notice; the ~5-page legal text is absent. Ship the full COPYING (or LICENSE) content or reference an existing root-level GPL file to stay compliant.
🤖 Prompt for AI Agents
In package/Interface/CommunityShaders/Icons/Categories/LICENCE.txt at lines 1 to
15, the file currently contains only a brief GPL notice but lacks the full
GPL-3.0 license text required for compliance. To fix this, replace the current
content with the complete GPL-3.0 license text or add a clear reference to a
root-level LICENSE or COPYING file that contains the full license text, ensuring
the full corresponding license is included with the work.
There was a problem hiding this comment.
This should be dealt with properly. Honestly just have the author make PR to your repo.
There was a problem hiding this comment.
No description provided.
|
✅ A pre-release build is available for this PR: |
| Community Shaders Category Icons | ||
| Copyright (C) 2025 dem6nd | ||
|
|
||
| These images are free software: you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License as published by | ||
| the Free Software Foundation, either version 3 of the License, or | ||
| (at your option) any later version. | ||
|
|
||
| These images are distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License for more details. | ||
|
|
||
| You should have received a copy of the GNU General Public License | ||
| along with this program. If not, see <http://www.gnu.org/licenses/> No newline at end of file |
There was a problem hiding this comment.
This should be dealt with properly. Honestly just have the author make PR to your repo.
| if (strcmp(categoryName, "Characters") == 0) { | ||
| categoryIcon = menu.characters.texture; | ||
| } else if (strcmp(categoryName, "Grass") == 0) { | ||
| categoryIcon = menu.grass.texture; | ||
| } else if (strcmp(categoryName, "Lighting") == 0) { | ||
| categoryIcon = menu.lighting.texture; | ||
| } else if (strcmp(categoryName, "Sky") == 0) { | ||
| categoryIcon = menu.sky.texture; | ||
| } else if (strcmp(categoryName, "Landscape & Textures") == 0) { | ||
| categoryIcon = menu.landscape.texture; | ||
| } else if (strcmp(categoryName, "Water") == 0) { | ||
| categoryIcon = menu.water.texture; | ||
| } else if (strcmp(categoryName, "Debug") == 0) { | ||
| categoryIcon = menu.debug.texture; | ||
| } else if (strcmp(categoryName, "Materials") == 0) { | ||
| categoryIcon = menu.materials.texture; | ||
| } |
|
all fixes addressed, removed the line of other; code I missed, the "other" category should not really appear and does not need an icon. Licence updated to dem6nd 's original they sent. I thought GPL3.0 would be easier but this matches other feature licences. |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
Error: D:\a\skyrim-community-shaders\skyrim-community-shaders\src\Menu.cpp(224,10): error C2039: 'other': is not a member of 'Menu::UIIcons' [D:\a\skyrim-community-shaders\skyrim-community-shaders\build\ALL\CommunityShaders.vcxproj]
(compiling source file '../../src/Menu.cpp')
D:\a\skyrim-community-shaders\skyrim-community-shaders\src\Menu.h(72,9):
see declaration of 'Menu::UIIcons'
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Utils/UI.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
src/Utils/UI.cpp (5)
Learnt from: davo0411
PR: #1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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
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 size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
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.
🧬 Code Graph Analysis (1)
src/Utils/UI.cpp (1)
src/Menu.h (1)
GetSingleton(16-20)
⏰ 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 (4)
src/Utils/UI.cpp (4)
166-179: LGTM - Icon array properly expanded for new category icons.The texturePointers array has been correctly expanded from 4 to 12 elements to include the new category icons (characters, grass, lighting, sky, landscape, water, debug, materials). This aligns with the PR objective of adding category icons to subheadings.
229-294: Comprehensive category icon loading implementation looks good.The individual icon loading blocks follow a consistent pattern with proper error handling and logging. Each category icon (characters, grass, lighting, sky, landscape, water, debug, materials) is loaded independently, allowing the system to gracefully handle missing icons while still loading available ones.
418-438: Category icon mapping implementation is well-structured.The string-based category mapping logic covers all the major categories mentioned in the PR objectives. The approach using strcmp is appropriate for this use case and provides clear, readable category-to-icon mappings.
Note: Based on past review comments, there was discussion about an "other" category icon, but per the PR objectives, this was intentionally removed as the "other" category should not appear.
448-512: Excellent icon scaling implementation that addresses the PR objectives.The icon rendering logic successfully addresses the main PR objective of "synchronizing icon scaling with text scaling." Key improvements:
- Icons scale proportionally with font size (
iconSize = currentFontSize * 1.2f)- Consistent spacing relative to text (
iconSpacing = currentFontSize * 0.3f)- Proper content width calculation that includes icon dimensions
- Vertical alignment centers icons with text
- Color tinting matches the header text color
This ensures icons and text scale together perfectly, resolving the scaling issues mentioned in the PR description.
2 participants
Summary by CodeRabbit
New Features
Improvements