refactor(ui): improve menu layout and organization#1162
Conversation
## Walkthrough
The changes refactor the feature configuration UI in the `Menu::DrawSettings()` function by replacing a two-column layout with a tabbed interface. The new design introduces "Settings" and "About" tabs, reorganizes management controls, displays feature status and information more clearly, and removes inline tooltips from the feature list.
## Changes
| File(s) | Change Summary |
|----------------------------------|------------------------------------------------------------------------------------------------------------------------------------------|
| src/Menu.cpp | Refactored feature configuration UI: replaced two-column layout with tabbed interface, reorganized controls, updated tooltip and error display logic, and removed inline tooltips from feature list. No changes to public API. |
| .github/workflows/build.yaml | Updated Windows runner to `windows-2022`, improved MSVC version detection and caching, added drive letter mismatch cleanup in build workflow. |
| package/Shaders/Common/BRDF.hlsli| Added comprehensive BRDF functions namespace for physically based rendering models and approximations. |
| package/Shaders/Common/PBR.hlsli | Refactored PBR shader code to use external BRDF functions from `BRDF.hlsli` instead of local implementations. |
| src/Features/LightLimitFix.cpp | Changed `roomNodes.empty()` call to `roomNodes.clear()` in light update to properly clear container. |
| src/Features/TerrainHelper.cpp | Added `enabled` flag to `TerrainHelper`; methods now early-exit if flag is false, disabling terrain helper logic when mod not loaded. |
| src/Features/TerrainHelper.h | Added `bool enabled` member variable to `TerrainHelper` struct, initialized to false. |
| src/XSEPlugin.cpp | Moved shader cache validation and file watcher start to after features' `PostPostLoad()` calls. |
## Possibly related PRs
- doodlum/skyrim-community-shaders#1175: Removes debug logging in `Menu::DrawSettings()`, related to UI icon availability debug log removal in `Menu.cpp`.
- doodlum/skyrim-community-shaders#1177: Modifies `LightLimitFix::UpdateLights()` by clearing `roomNodes`, directly related to the same container handling change.
- doodlum/skyrim-community-shaders#1171: Moves shader cache validation and file watcher start after features' `PostPostLoad()`, matching the same control flow adjustment in `src/XSEPlugin.cpp`.
## Poem
> In the menu’s gentle warren, tabs now hop in line,
> "Settings" and "About"—two carrots to define.
> No more tooltips hiding, just clarity in view,
> With colors and sections, each feature feels new.
> So let’s nibble through options, in this fresh UI field,
> Where every bunny’s settings are smartly revealed!
> 🐇✨✨ Finishing Touches
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
|
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: 0
🧹 Nitpick comments (1)
src/Menu.cpp (1)
672-679: Consider extracting feature status determination logic.The logic for determining feature state (disabled, loaded, failed) and corresponding colors is duplicated between the Settings and About tabs. Consider extracting this into a helper method to improve maintainability and reduce code duplication.
Example refactor:
struct FeatureDisplayInfo { ImVec4 textColor; const char* statusText; bool isDisabled; bool isLoaded; bool hasFailedMessage; }; FeatureDisplayInfo GetFeatureDisplayInfo(Feature* feat) { // Extract common logic here // Return structured info for consistent display }Also applies to: 756-770
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Menu.cpp (2)
666-805: Excellent UI refactoring with improved organization.The tabbed interface successfully separates feature management controls from informational content, creating a much more organized and user-friendly experience. The implementation properly handles different feature states and uses ImGui correctly with appropriate Begin/End pairing.
726-735: Verify that feature mod links are safe to execute.The
ShellExecuteAcall opens URLs fromfeat->GetFeatureModLink()without validation. While this appears to be controlled by feature authors, ensure that these URLs come from trusted sources or implement URL validation to prevent potential security issues.#!/bin/bash # Description: Search for GetFeatureModLink implementations to verify URL validation # Expected: Find implementations that show how mod links are set and validated # Search for GetFeatureModLink method implementations ast-grep --pattern $'$_GetFeatureModLink($_) { $$$ }' # Search for where mod links are set rg -A 5 -B 5 "FeatureModLink|ModLink" --type cpp
|
✅ A pre-release build is available for this PR: |
|
Screenshot preview? |
|
Ok build failure isn't caused by this but by something in the action. There's a prior ci run without a failure. |
* ci: unpin ilammy/msvc-dev-cmd * ci: unpin remaining
* ci: swap to microsoft/setup-msbuild * ci: add cl debug * ci: skip error from cl.exe check * ci: invalidate cache when drive changes
community-shaders#1176) * fix(terrain helper): Remove potential nullptr lookup that is not needed * fix(terrain helper): Terrain helper will disable itself if its plugin is not found * fix(terrain helper): included missing log message for terrain helper missing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package/Shaders/Common/BRDF.hlsli (1)
34-47: Fix indentation inconsistency.The file mixes tabs and spaces for indentation. Some functions like
Diffuse_OrenNayar,Diffuse_Gotanda,Diffuse_Chan, and various others use tabs while the rest use spaces. Please standardize on one approach for consistency.Also applies to: 50-62, 65-80, 84-196
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build.yaml(8 hunks)package/Shaders/Common/BRDF.hlsli(1 hunks)package/Shaders/Common/PBR.hlsli(10 hunks)src/Features/LightLimitFix.cpp(1 hunks)src/Features/TerrainHelper.cpp(2 hunks)src/Features/TerrainHelper.h(1 hunks)src/Menu.cpp(1 hunks)src/XSEPlugin.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Features/TerrainHelper.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Menu.cpp
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (15)
src/Features/TerrainHelper.cpp (2)
33-38: Well-implemented feature gating based on mod presence.Excellent defensive programming! The code properly detects whether TerrainHelper.esp is loaded by checking for the LandscapeDefault texture set and sets the enabled flag accordingly.
43-47: Good use of early returns for disabled feature.The early returns in both
TESObjectLAND_SetupMaterialandBSLightingShader_SetupMaterialprevent unnecessary processing when the feature is disabled. This is a clean implementation pattern.Also applies to: 155-159
src/XSEPlugin.cpp (1)
93-104: Improved lifecycle management with proper initialization order.Smart change! Moving shader cache validation after feature
PostPostLoad()calls ensures features can properly initialize or disable themselves before cache validation occurs. This aligns well with the TerrainHelper changes where features may disable themselves based on runtime conditions..github/workflows/build.yaml (3)
110-110: Good practice: Pin Windows runner version for consistency.Updating from
windows-latesttowindows-2022provides build consistency and prevents unexpected changes when GitHub updates the latest alias.Also applies to: 260-260, 431-431, 523-523
123-188: Robust MSVC version extraction with comprehensive fallbacks.The MSVC version extraction logic is well-implemented with multiple fallback patterns to handle different output formats. Using the compiler version in cache keys prevents cache conflicts between different MSVC versions.
Also applies to: 295-356
203-220: Smart cache invalidation for drive letter changes.Excellent addition! Detecting drive letter mismatches between the cached CMakeCache.txt and current working directory prevents build failures from stale cache paths. This is particularly useful in CI environments where drive letters may vary.
Also applies to: 373-391
package/Shaders/Common/BRDF.hlsli (1)
1-216: Excellent modular BRDF library with comprehensive implementations.This is a well-structured shader library that consolidates various BRDF models with proper documentation and academic references. The namespace encapsulation and consistent naming conventions make it easy to use and maintain.
package/Shaders/Common/PBR.hlsli (8)
4-4: LGTM: Clean modularization with centralized BRDF include.The addition of the BRDF module include is well-positioned and supports the comprehensive refactoring to centralize BRDF computations.
158-165: LGTM: Consistent BRDF function namespace usage in glint calculations.The refactoring properly replaces local BRDF implementations with centralized module calls. All function signatures appear consistent:
D_GGX,Vis_SmithJointApprox, andF_Schlickare correctly namespaced.
171-178: LGTM: Microfacet BRDF functions properly centralized.The standard microfacet BRDF calculation now uses the centralized implementations. The function signatures and usage patterns are consistent with the original implementations.
180-187: LGTM: Microflakes BRDF functions correctly updated.The Charlie distribution and Neubelt visibility functions are properly namespaced. This completes the BRDF centralization for the microflakes shading model.
245-245: LGTM: Hair shading BRDF functions consistently updated.All Fresnel calculations in the Marschner hair model now use the centralized
BRDF::F_Schlickimplementation. The.xaccessor pattern is maintained for scalar extraction.Also applies to: 252-252, 260-260
326-326: LGTM: Diffuse Lambert function calls properly centralized.All diffuse lighting calculations now use
BRDF::Diffuse_Lambert()from the centralized module. The usage is consistent across direct lighting, subsurface transmission, and coat diffuse calculations.Also applies to: 355-355, 380-380
337-337: LGTM: Environment BRDF approximation consistently updated.All
EnvBRDFApproxLazarovcalls now use the centralized implementation. The function is used consistently for specular BRDF calculations in both direct and indirect lighting paths.Also applies to: 435-435, 444-444, 487-487
448-448: LGTM: Coat Fresnel calculation properly centralized.The two-layer coat Fresnel calculation now uses the centralized
BRDF::F_Schlickimplementation, maintaining consistency with other Fresnel calculations throughout the shader.
| // Process point lights | ||
|
|
||
| roomNodes.empty(); | ||
| roomNodes.clear(); |
There was a problem hiding this comment.
Critical bug fix: Correctly clear roomNodes container.
Good catch! The original code roomNodes.empty(); only checks if the container is empty without actually clearing it. This would cause stale room node data to accumulate across frames, potentially leading to incorrect lighting calculations.
🤖 Prompt for AI Agents
In src/Features/LightLimitFix.cpp at line 755, replace the incorrect call to
roomNodes.empty() with roomNodes.clear() to properly clear the container. This
change ensures that stale room node data does not accumulate across frames,
preventing incorrect lighting calculations.
6 participants
Nice new feature page layout.
Summary by CodeRabbit