refactor: break apart menu#1330
Conversation
WalkthroughThis change modularizes the menu and overlay UI rendering by refactoring monolithic logic into dedicated renderer classes. It moves all key and input translation utilities to a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant FeatureListRenderer
participant AdvancedSettingsRenderer
participant DisplaySettingsRenderer
participant MenuHeaderRenderer
participant OverlayRenderer
participant ThemeManager
participant UtilInput
User->>Menu: Open menu/settings/overlay
Menu->>MenuHeaderRenderer: RenderHeader(...)
Menu->>FeatureListRenderer: RenderFeatureList(...)
FeatureListRenderer->>Menu: Callbacks for settings/feature UIs
Menu->>AdvancedSettingsRenderer: RenderAdvancedSettings(...)
Menu->>DisplaySettingsRenderer: RenderDisplaySettings(...)
Menu->>OverlayRenderer: RenderOverlay(...)
OverlayRenderer->>ThemeManager: SetupImGuiStyle(...)
OverlayRenderer->>UtilInput: Key translation/string conversion
Menu->>UtilInput: Key translation/string conversion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~70 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. ✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
src/Menu/ThemeManager.cpp (1)
19-24: Verify exp2 usage and consider caching.The
exp2(themeSettings.GlobalScale)calculation is performed multiple times (lines 20, 139). Consider caching this value for better performance, especially if this function is called frequently during rendering.void ThemeManager::SetupImGuiStyle(const Menu& menu) { auto& style = ImGui::GetStyle(); auto& colors = style.Colors; auto& themeSettings = menu.GetTheme(); + float globalScaleFactor = exp2(themeSettings.GlobalScale); // rescale here auto styleCopy = themeSettings.Style; - styleCopy.ScaleAllSizes(exp2(themeSettings.GlobalScale)); + styleCopy.ScaleAllSizes(globalScaleFactor);src/Menu/MenuHeaderRenderer.h (1)
15-15: Consider const-correctness for Menu reference.The
Menu::UIIcons& uiIconsparameter suggests the icons might be modified, but the method nameRenderHeaderimplies read-only operations. Consider making the Menu reference const if possible.- static void RenderHeader(bool isDocked, bool showLogo, bool canShowIcons, float uiScale, const Menu::UIIcons& uiIcons); + static void RenderHeader(bool isDocked, bool showLogo, bool canShowIcons, float uiScale, const Menu::UIIcons& uiIcons, const Menu& menu);Or if the icons are truly read-only:
- static void RenderHeader(bool isDocked, bool showLogo, bool canShowIcons, float uiScale, const Menu::UIIcons& uiIcons); + static void RenderHeader(bool isDocked, bool showLogo, bool canShowIcons, float uiScale, const Menu& menu);src/Utils/UI.cpp (3)
928-928: Verify IM_VK_KEYPAD_ENTER definition placement.The macro
IM_VK_KEYPAD_ENTERis defined within the Input namespace. Consider moving it outside the namespace or using a more conventional naming pattern to avoid potential naming conflicts.+#define IM_VK_KEYPAD_ENTER (VK_RETURN + 256) + namespace Input { -#define IM_VK_KEYPAD_ENTER (VK_RETURN + 256)
930-1144: Large switch statement may impact performance.The
VirtualKeyToImGuiKeyfunction contains a very large switch statement with 100+ cases. While functionally correct, this could impact performance if called frequently. Consider using a lookup table for better performance.ImGuiKey VirtualKeyToImGuiKey(WPARAM vkKey) { + // Static lookup table for better performance + static const std::unordered_map<WPARAM, ImGuiKey> keyMap = { + {VK_TAB, ImGuiKey_Tab}, + {VK_LEFT, ImGuiKey_LeftArrow}, + // ... continue with all mappings + }; + + auto it = keyMap.find(vkKey); + return (it != keyMap.end()) ? it->second : ImGuiKey_None; - switch (vkKey) { - case VK_TAB: - return ImGuiKey_Tab; - // ... all cases - default: - return ImGuiKey_None; - }; }
121-126: Missing error handling for CreateTexture2D.While the code checks the HRESULT from
CreateTexture2D, it doesn't handle specific error cases that might require different responses (e.g., out of memory vs. invalid parameters).HRESULT hr = device->CreateTexture2D(&desc, &subResource, &pTexture); if (FAILED(hr) || !pTexture) { - logger::warn("LoadTextureFromFile: Failed to create D3D11 texture, HRESULT: 0x{:08X}", static_cast<uint32_t>(hr)); + logger::warn("LoadTextureFromFile: Failed to create D3D11 texture for {}, HRESULT: 0x{:08X}", filename, static_cast<uint32_t>(hr)); + if (hr == E_OUTOFMEMORY) { + logger::error("LoadTextureFromFile: Out of GPU memory while loading texture"); + } stbi_image_free(image_data); return false; }src/Utils/UI.h (1)
457-457: Consider more specific return type documentation.The documentation mentions "ImGuiKey_None if unmapped" but could be more specific about which keys are guaranteed to be supported vs. which might return ImGuiKey_None.
* @return Corresponding ImGuiKey value, or ImGuiKey_None if unmapped + * + * @note All standard keyboard keys (A-Z, 0-9, F1-F12) are guaranteed to be mapped. + * Some OEM-specific or rare keys may return ImGuiKey_None.src/Menu/OverlayRenderer.h (1)
38-44: Well-designed callback interface.The use of
std::functioncallbacks provides good flexibility while maintaining separation of concerns. The parameter list is comprehensive but could benefit from grouping related parameters.Consider grouping related parameters into a struct for better maintainability:
+struct OverlayRenderParams { + const std::function<void()>& processInputEventQueue; + const std::function<void()>& drawSettings; + const std::function<const char*(uint32_t)>& keyIdToString; + float cachedFontSize; + float currentFontSize; +}; + static void RenderOverlay( Menu& menu, - const std::function<void()>& processInputEventQueue, - const std::function<void()>& drawSettings, - const std::function<const char*(uint32_t)>& keyIdToString, - float cachedFontSize, - float currentFontSize); + const OverlayRenderParams& params);src/Menu/OverlayRenderer.cpp (1)
86-95: Remove unused variableThe
oldStylevariable is saved but never used.void OverlayRenderer::InitializeImGuiFrame(Menu& menu) { // Start the Dear ImGui frame ImGui_ImplDX11_NewFrame(); ImGui_ImplWin32_NewFrame(); ImGui::NewFrame(); - ImGuiStyle oldStyle = ImGui::GetStyle(); ThemeManager::SetupImGuiStyle(menu); }src/Menu/MenuHeaderRenderer.cpp (1)
189-202: Consider capturing by reference in the lambda.The lambda captures
shaderCacheby value, which creates an unnecessary copy of the pointer. Since the lambda is executed synchronously, capturing by reference would be more efficient.- [shaderCache]() { + [&shaderCache]() { shaderCache->Clear(); if (shaderCache->IsDiskCache()) { shaderCache->DeleteDiskCache(); } } });src/Menu/SettingsTabRenderer.cpp (2)
65-130: Consider extracting keybinding UI logic to reduce duplication.The keybinding UI code follows the same pattern for each key. Consider extracting a helper function.
Add a helper function:
static void RenderKeybindingSetting( const char* label, uint32_t currentKey, bool& isSettingKey, const char* changeButtonId, const std::function<const char*(uint32_t)>& keyIdToString, const ImVec4& hotkeyColor) { if (isSettingKey) { ImGui::Text("Press any key to set as %s...", label); } else { ImGui::AlignTextToFramePadding(); ImGui::Text("%s:", label); ImGui::SameLine(); ImGui::AlignTextToFramePadding(); ImGui::TextColored(hotkeyColor, "%s", keyIdToString(currentKey)); ImGui::AlignTextToFramePadding(); ImGui::SameLine(); if (ImGui::Button(changeButtonId)) { isSettingKey = true; } } }Then use it:
- // Toggle Key - if (state.settingToggleKey) { - ImGui::Text("Press any key to set as toggle key..."); - } else { - ImGui::AlignTextToFramePadding(); - ImGui::Text("Toggle Key:"); - ImGui::SameLine(); - ImGui::AlignTextToFramePadding(); - ImGui::TextColored(themeSettings.StatusPalette.CurrentHotkey, "%s", keyIdToString(settings.ToggleKey)); - - ImGui::AlignTextToFramePadding(); - ImGui::SameLine(); - if (ImGui::Button("Change##toggle")) { - state.settingToggleKey = true; - } - } + RenderKeybindingSetting( + "Toggle Key", + settings.ToggleKey, + state.settingToggleKey, + "Change##toggle", + keyIdToString, + themeSettings.StatusPalette.CurrentHotkey);
178-183: Add comment explaining the exponential scaling.The use of
exp2for global scale might not be immediately obvious to future maintainers.if (ImGui::SliderFloat("Global Scale", &themeSettings.GlobalScale, -1.f, 1.f, "%.2f")) { + // Convert linear slider value to exponential scale for better UX + // -1 = 0.5x, 0 = 1x, 1 = 2x float trueScale = exp2(themeSettings.GlobalScale);src/Menu/FeatureListRenderer.cpp (2)
92-92: Consider making category order configurable.The category order is hardcoded. Consider moving this to a configuration or constant.
+ static const std::vector<std::string> DEFAULT_CATEGORY_ORDER = { + "Debug", "Characters", "Grass", "Lighting", "Materials", + "Sky", "Landscape & Textures", "Water", "Other" + }; + // Define category order - std::vector<std::string> categoryOrder = { "Debug", "Characters", "Grass", "Lighting", "Materials", "Sky", "Landscape & Textures", "Water", "Other" }; + std::vector<std::string> categoryOrder = DEFAULT_CATEGORY_ORDER;
386-389: Document the rationale for the epsilon value.The epsilon value of 0.1f appears arbitrary. Consider documenting why this specific value was chosen.
- const float epsilon = 0.1f; // Simple check to ensure we don't trigger on minor cursor movements / weird imgui math + // Epsilon value chosen to ignore floating-point precision errors and minor + // cursor adjustments from ImGui's layout calculations while still detecting + // actual UI element rendering + const float epsilon = 0.1f;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/VR.cpp(1 hunks)src/Features/WeatherPicker.cpp(2 hunks)src/Menu.cpp(10 hunks)src/Menu.h(4 hunks)src/Menu/AdvancedSettingsRenderer.cpp(1 hunks)src/Menu/AdvancedSettingsRenderer.h(1 hunks)src/Menu/DisplaySettingsRenderer.cpp(1 hunks)src/Menu/DisplaySettingsRenderer.h(1 hunks)src/Menu/FeatureListRenderer.cpp(1 hunks)src/Menu/FeatureListRenderer.h(1 hunks)src/Menu/MenuHeaderRenderer.cpp(1 hunks)src/Menu/MenuHeaderRenderer.h(1 hunks)src/Menu/OverlayRenderer.cpp(1 hunks)src/Menu/OverlayRenderer.h(1 hunks)src/Menu/SettingsTabRenderer.cpp(1 hunks)src/Menu/SettingsTabRenderer.h(1 hunks)src/Menu/ThemeManager.cpp(1 hunks)src/Menu/ThemeManager.h(1 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/Features/WeatherPicker.cpp (2)
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: #1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
src/Menu/MenuHeaderRenderer.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.
src/Menu/FeatureListRenderer.cpp (1)
Learnt from: alandtse
PR: #1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
🔇 Additional comments (31)
src/Features/PerformanceOverlay.cpp (1)
161-162: Namespace migration looks correctThe call has been updated to the new
Util::Input::KeyIdToStringhelper, matching the input-utility refactor. Signature and lifetime assumptions stay compatible with the surroundingImGui::TextColoredcall. No further action required.src/Features/WeatherPicker.cpp (2)
5-5: Required header addedImporting
"Utils/UI.h"is necessary for the newUtil::Inputnamespace and does not disturb existing include order or create dependency cycles.
50-51: Call site updated toUtil::Input
Menu::KeyIdToStringcorrectly replaced byUtil::Input::KeyIdToString; the returned C-string integrates with the existingImGui::TextColoredformatting. Looks good.src/Features/VR.cpp (1)
1728-1729: LGTM! Clean refactoring to new input utility namespace.The migration from
menu->VirtualKeyToImGuiKeytoUtil::Input::VirtualKeyToImGuiKeycorrectly implements the modularization effort described in the PR. The functionality remains identical while improving code organization by centralizing input handling utilities.src/Menu/AdvancedSettingsRenderer.h (1)
1-19: Excellent modular design for advanced settings rendering.This header exemplifies clean separation of concerns with:
- Minimal dependencies via forward declarations
- Static methods indicating utility/renderer pattern
- Flexible callback-based public interface
- Logically organized private rendering methods
The design aligns perfectly with the menu modularization objectives and follows modern C++ best practices.
src/Menu/ThemeManager.h (1)
1-20: Well-designed theme management utility with sensible defaults.The
ThemeManagerclass demonstrates excellent modularization with:
- Clear separation between style setup and font management
- Well-chosen font configuration constants (size range 8-32, 3x1 oversampling)
- Proper use of
constexprfor compile-time constants- Immutable access patterns with
constreferencesThe nested
Constantsstruct provides a clean way to centralize font rendering parameters.src/Menu/DisplaySettingsRenderer.h (1)
7-32: Exemplary class design with outstanding documentation.This renderer class demonstrates best practices:
- Comprehensive documentation explaining extraction rationale and design decisions
- Well-architected callback interface maintaining loose coupling with Menu system
- Clear parameter naming (
upscalerLoaded,isFeatureDisabled,drawUpscalingSettings)- Logical public/private separation
The callback-based design allows the renderer to remain focused on presentation while delegating state management to the caller. Excellent work on the modularization effort.
src/Menu/SettingsTabRenderer.h (1)
8-35: Clean modular design with excellent organization.The
SettingsTabRendererclass showcases solid architectural decisions:
SettingsStatestruct elegantly groups related boolean settings using references for direct modification- Logical method organization by functional areas (shaders, keybindings, interface)
- Granular sub-tab methods promote maintainability (UI options, sizes, colors)
- Consistent callback pattern for
keyIdToStringmaintains flexibilityThe design aligns perfectly with the other renderer classes in this modularization effort.
src/Menu/ThemeManager.cpp (3)
15-16: Good separation of concerns with theme access.The code properly accesses theme settings through the Menu interface, maintaining good encapsulation while allowing the ThemeManager to be stateless.
137-137: Verify DirectX device state managementI couldn’t locate
src/Menu/ThemeManager.cppin the workspace to confirm whether the call below (around line 137) is properly guarded:ImGui_ImplDX11_InvalidateDeviceObjects();Please manually verify that:
ImGui_ImplDX11_InvalidateDeviceObjects()is only invoked after the DirectX/ImGui back-end has been initialized (e.g. viaImGui_ImplDX11_InitorImGui_ImplDX11_CreateDeviceObjects).- Calls are protected against uninitialized or already-destroyed device objects (for example, with a boolean “initialized” flag).
- Shutdown or re-initialization paths cannot trigger
InvalidateDeviceObjects()when the device isn’t valid.
108-109: Verify C++20 support and palette bounds manuallyI wasn’t able to locate
ThemeManager.cppin the current branch, so please double-check the following:
- Confirm your build is compiled with C++20 (or later) so that
std::spanis available.- Ensure
themeSettings.FullPalette.size()exactly matches the size of thecolorsbuffer (typicallyImGuiCol_COUNT) to avoid out-of-bounds writes.- If C++20 isn’t guaranteed or sizes could diverge, consider using an explicit size check or
std::copy_nwith the lesser of the two counts.src/Menu/MenuHeaderRenderer.h (2)
8-13: Well-designed ActionIcon structure.The ActionIcon struct provides a clean interface combining texture, tooltip, and callback functionality. The use of std::function for callbacks provides good flexibility.
17-22: Good separation of rendering concerns.The private methods show good decomposition of the header rendering process, separating icon building, rendering logic for different states, and watermark handling.
src/Utils/UI.cpp (3)
4-8: Good DirectInput version management.The conditional definition of
DIRECTINPUT_VERSIONensures compatibility while allowing overrides. This is a good defensive programming practice.
1208-1233: Static array provides good performance but verify bounds.The static
keyboard_keys_internationalarray provides good performance for key name lookups. The bounds check at line 1210-1211 properly prevents buffer overruns.
169-191: Good defensive programming in texture cleanup.The code properly releases existing textures before loading new ones, preventing memory leaks. The array-based approach for cleanup is efficient and maintainable.
src/Utils/UI.h (2)
7-7: Good addition of Windows header dependency.Adding
#include <windows.h>properly provides the WPARAM and VK_* constants needed by the Input namespace functions.
418-517: Excellent documentation for Input namespace.The documentation is comprehensive and includes:
- Clear purpose descriptions
- Parameter documentation with types and ranges
- Return value explanations
- Usage examples
- Implementation notes about fallback behavior
This level of documentation significantly improves maintainability.
src/Menu/OverlayRenderer.h (2)
7-19: Excellent architectural documentation.The class documentation clearly explains the extraction rationale, callback-based architecture, and design decisions. This level of documentation helps maintainers understand the architectural choices.
47-55: Good decomposition of overlay rendering responsibilities.The private methods show excellent separation of concerns, breaking down overlay rendering into logical, testable components. Each method has a clear, single responsibility.
src/Menu/DisplaySettingsRenderer.cpp (2)
8-25: LGTM!The display settings rendering logic is well-structured with proper separation of concerns through callbacks.
27-55: LGTM!The feature section rendering properly handles both enabled and disabled states with appropriate visual feedback and tooltips.
src/Menu.h (2)
18-19: Good practice: Deleted copy operationsExplicitly deleting the copy constructor and assignment operator prevents accidental copying of the singleton.
287-289: LGTM!Explicit typing of the container members improves code clarity.
src/Menu/AdvancedSettingsRenderer.cpp (2)
18-32: LGTM!Clean orchestration of advanced settings sections with proper use of callbacks.
158-201: LGTM!Proper use of ImGui table API with correct handling of developer mode restrictions.
src/Menu.cpp (2)
221-301: Excellent refactoring!The extraction of rendering logic to specialized components greatly improves code organization and maintainability. The use of lambda callbacks maintains proper encapsulation.
493-548: LGTM!Proper migration to the centralized Util::Input utilities while maintaining the original logic.
src/Menu/OverlayRenderer.cpp (1)
18-55: LGTM!Well-orchestrated overlay rendering with proper separation of concerns and logical flow.
src/Menu/SettingsTabRenderer.cpp (1)
11-54: LGTM!The general settings and shaders tab implementation is clean and well-structured with appropriate tooltips.
src/Menu/FeatureListRenderer.h (1)
1-79: LGTM!Well-structured header with good use of modern C++ patterns (variants and visitors) for polymorphic menu handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Menu/FeatureListRenderer.cpp (1)
345-364: Excellent refactoring of the feature rendering logic.The previously large 200+ line method has been successfully refactored into focused helper methods (
RenderFeatureSettingsTab,RenderFeatureAboutTab,RenderFeatureActionButtons), greatly improving maintainability and readability.
🧹 Nitpick comments (2)
src/Utils/UI.cpp (1)
928-928: Document the magic number used for keypad enter distinction.The value 256 is used to distinguish the keypad enter key from the regular enter key. Consider adding a comment explaining this convention or defining it as a named constant.
+// Add 256 to VK_RETURN to create a distinct identifier for keypad enter +// This follows ImGui's convention for distinguishing keypad keys #define IM_VK_KEYPAD_ENTER (VK_RETURN + 256)src/Menu.h (1)
45-52: Consider using friend class or accessor methods instead of public members.While the comment explains these are made public for
InputEventHandleraccess, exposing internal state as public members breaks encapsulation. Consider:
- Making
InputEventHandlera friend class- Providing getter/setter methods
- Using a dedicated input configuration struct
Example using friend class:
-public: - // Input handling flags (made public for InputEventHandler access) +private: + friend class InputEventHandler; + // Input handling flags bool settingToggleKey = false; bool settingSkipCompilationKey = false; bool settingsEffectsToggle = false; bool settingOverlayToggleKey = false; uint32_t priorShaderKey = VK_PRIOR; uint32_t nextShaderKey = VK_NEXT;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/VR.cpp(1 hunks)src/Features/WeatherPicker.cpp(2 hunks)src/Menu.cpp(9 hunks)src/Menu.h(4 hunks)src/Menu/AdvancedSettingsRenderer.cpp(1 hunks)src/Menu/AdvancedSettingsRenderer.h(1 hunks)src/Menu/DisplaySettingsRenderer.cpp(1 hunks)src/Menu/DisplaySettingsRenderer.h(1 hunks)src/Menu/FeatureListRenderer.cpp(1 hunks)src/Menu/FeatureListRenderer.h(1 hunks)src/Menu/MenuHeaderRenderer.cpp(1 hunks)src/Menu/MenuHeaderRenderer.h(1 hunks)src/Menu/OverlayRenderer.cpp(1 hunks)src/Menu/OverlayRenderer.h(1 hunks)src/Menu/SettingsTabRenderer.cpp(1 hunks)src/Menu/SettingsTabRenderer.h(1 hunks)src/Menu/ThemeManager.cpp(1 hunks)src/Menu/ThemeManager.h(1 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/Features/WeatherPicker.cpp
- src/Menu/AdvancedSettingsRenderer.h
- src/Features/VR.cpp
- src/Features/PerformanceOverlay.cpp
- src/Menu/DisplaySettingsRenderer.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Menu/DisplaySettingsRenderer.cpp
- src/Menu/OverlayRenderer.h
- src/Menu/ThemeManager.cpp
- src/Menu/MenuHeaderRenderer.h
- src/Menu/ThemeManager.h
- src/Utils/UI.h
- src/Menu/SettingsTabRenderer.h
- src/Menu/SettingsTabRenderer.cpp
🧰 Additional context used
🧠 Learnings (3)
src/Menu/AdvancedSettingsRenderer.cpp (1)
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.
src/Menu/MenuHeaderRenderer.cpp (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-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.
src/Menu/FeatureListRenderer.cpp (1)
Learnt from: alandtse
PR: #1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
⏰ 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 (12)
src/Utils/UI.cpp (1)
926-1234: Well-structured centralization of input utilities.The extraction of keyboard input handling into the
Util::Inputnamespace improves code organization and reusability. The comprehensive key mapping functions and clear separation of concerns make this a good architectural improvement.src/Menu.h (2)
18-19: Good practice: Explicitly deleted copy operations for singleton.Explicitly deleting the copy constructor and assignment operator prevents accidental copying of the singleton instance and provides clear compile-time errors if attempted.
288-289: Good: Explicit type declarations improve clarity.Explicitly declaring the types of
_keyEventQueueand_inputEventMuteximproves code readability and prevents potential issues with auto type deduction.src/Menu/AdvancedSettingsRenderer.cpp (1)
18-32: Well-structured modular rendering implementation.The separation of advanced settings into distinct rendering methods (
RenderAdvancedSection,RenderShaderReplacementSection,RenderDeveloperSection) with callbacks for external components creates a clean, maintainable architecture.src/Menu/OverlayRenderer.cpp (1)
18-55: Excellent modular overlay rendering design.The
RenderOverlaymethod effectively coordinates multiple rendering aspects (VR setup, input processing, font management, shader compilation status, feature overlays, A/B testing) while maintaining clean separation through helper methods. The conditional rendering logic optimizes performance by skipping unnecessary work.src/Menu.cpp (3)
221-233: Excellent documentation additions improve code maintainability.The comprehensive documentation blocks added to each method clearly explain their purpose, responsibilities, and architectural role. This significantly improves code readability and maintainability for future developers.
Also applies to: 303-309, 326-332, 394-400, 418-427, 439-451, 624-630, 645-653
273-273: Clean delegation to specialized renderer components.The refactoring successfully extracts rendering logic to
MenuHeaderRendererandFeatureListRendererwhile maintaining clean interfaces through parameter passing and lambda callbacks. This improves separation of concerns and testability.Also applies to: 284-292
311-324: Excellent architectural refactoring with clean separation of concerns.The systematic extraction of rendering logic to dedicated renderer classes (
SettingsTabRenderer,AdvancedSettingsRenderer,DisplaySettingsRenderer,OverlayRenderer) while maintaining the Menu class as a coordinator is a significant architectural improvement. The consistent use of lambdas and callbacks preserves encapsulation while enabling modular design.Also applies to: 335-339, 403-407, 430-437
src/Menu/FeatureListRenderer.h (1)
1-86: Well-structured header with good design patterns.The header demonstrates excellent software design:
- Clean separation of concerns with visitor pattern for polymorphic rendering
- Appropriate use of
std::variantfor type-safe menu item handling- Good encapsulation with private helper methods
- Forward declaration of
Featurereduces compilation dependenciessrc/Menu/MenuHeaderRenderer.cpp (3)
35-37: Good use of extracted constants from ThemeManager.The implementation properly uses constants from
ThemeManager::Constantsinstead of magic numbers, which improves maintainability.
167-207: Well-structured icon building logic.The method properly validates texture availability and creates appropriate action callbacks. Good separation of concerns.
225-227: Excellent use of ThemeManager constants.The implementation properly uses named constants for icon sizing and spacing, addressing previous feedback about magic numbers.
|
✅ A pre-release build is available for this PR: |
|
Duplicate constants in |
|
Thanks. And I assume these issues (aside from constants) aren't in the pre refactor version? |
The active tab color works in the previous PR, I compiled the previous PR and checked it. Also increasing the font size to 108 worked there |
4 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
These changes deliver a more intuitive, customizable, and responsive user interface experience.