feat(upscaling): improve ui and diagnostics#1229
Conversation
|
""" WalkthroughThis update introduces DLL version caching and diagnostics for both the FidelityFX and Streamline backends. It adds static members to store DLL version info, dynamically constructs plugin paths, and enhances the UI with backend diagnostics, including sortable tables of DLL versions and logging level controls. Frame generation status tracking is also improved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Upscaling
participant FidelityFX
participant Streamline
participant Util
User->>UI: Open Settings
UI->>Upscaling: DrawSettings()
Upscaling->>FidelityFX: Access dllVersions
Upscaling->>Streamline: Access dllVersions
Upscaling->>UI: Show DLL version tables
UI->>User: Display diagnostics, log level controls, and DLL versions
Possibly related PRs
Poem
✨ 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
|
![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 selected for processing (9)
src/FidelityFX.cpp(5 hunks)src/FidelityFX.h(2 hunks)src/Streamline.cpp(4 hunks)src/Streamline.h(3 hunks)src/Upscaling.cpp(3 hunks)src/Upscaling.h(1 hunks)src/Utils/FileSystem.h(2 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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/Upscaling.h (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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: doodlum/skyrim-community-shaders#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.
src/FidelityFX.h (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.034Z
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/Upscaling.cpp (1)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
🧬 Code Graph Analysis (2)
src/FidelityFX.cpp (2)
src/Utils/FileSystem.h (2)
EnumerateDllVersions(81-94)EnumerateDllVersions(81-81)src/FidelityFX.h (1)
a_useFrameGeneration(46-46)
src/Utils/UI.cpp (1)
src/Utils/UI.h (1)
VersionStringLess(265-265)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (21)
src/Upscaling.h (1)
44-44: LGTM: Clean addition of Streamline log level control.The
streamlineLogLevelmember provides the necessary control for Streamline SDK logging verbosity, aligning with the PR objectives. The use ofuintis consistent with the project's pattern for shader compatibility.src/Streamline.h (1)
95-96: LGTM: Well-structured additions for diagnostics and status tracking.The three new members effectively support the PR objectives:
PluginDirprovides consistent path managementisFrameGenActiveenables runtime frame generation status trackingdllVersionssupports the diagnostic UI functionalityThe implementation maintains consistency with the FidelityFX counterpart.
Also applies to: 113-114, 160-161
src/FidelityFX.cpp (4)
2-2: LGTM: Clean addition of filesystem utilities and static member definition.The inclusion of
Utils/FileSystem.hand static member definition properly support the DLL version enumeration feature.Also applies to: 12-13
35-40: LGTM: Excellent improvement to dynamic path construction and DLL enumeration.The changes improve maintainability by:
- Using the static
PluginDirconstant for consistent path management- Automatically enumerating all DLL versions in the plugin directory
- Caching version information for diagnostic purposes
This aligns perfectly with the PR objectives for enhanced diagnostics.
60-64: LGTM: Proper feature flag management based on context creation result.The
featureFSR3FGflag is correctly set based on the success/failure of frame generation context creation, providing accurate feature availability information.
200-201: LGTM: Accurate frame generation status tracking.The
isFrameGenActiveflag correctly reflects the runtime state of frame generation, providing valuable information for diagnostics and UI display.src/FidelityFX.h (1)
19-19: LGTM: Consistent and well-structured additions for enhanced diagnostics.The four new members provide excellent support for the PR objectives:
PluginDirenables consistent path managementfeatureFSR3FGtracks feature availability (configured at setup)isFrameGenActivetracks runtime status (updated per frame)dllVersionssupports diagnostic UI functionalityThe implementation maintains consistency with the Streamline backend, ensuring a unified approach to diagnostics across both upscaling technologies.
Also applies to: 34-37, 39-40
src/Utils/UI.cpp (4)
557-564: LGTM!The sorting implementation correctly handles out-of-bounds column indices by returning false, preventing crashes.
566-596: Well-implemented version string comparator!The function correctly handles version strings with different numbers of components and includes proper error handling for non-numeric parts.
598-600: LGTM!Clean lambda wrapper for version string comparison.
602-649: Comprehensive sortable table implementation!The function properly handles ImGui's table sorting API and correctly falls back to default string comparison when custom sorters aren't provided.
src/Streamline.cpp (5)
16-66: Excellent log message preprocessing!The implementation properly handles trailing newlines, bracketed metadata removal, and edge cases like empty or bracket-only messages.
69-69: LGTM!Proper static member initialization.
75-100: Well-implemented DLL version enumeration!The code properly constructs the interposer path dynamically and systematically enumerates all DLLs in the plugin directory with appropriate error handling.
113-124: LGTM!The log level mapping correctly handles all cases with a sensible default.
396-398: LGTM!Clear status tracking for frame generation.
src/Upscaling.cpp (4)
6-6: LGTM!Windows.h is required for ShellExecuteW used in the backend diagnostics UI.
19-19: LGTM!Correctly added streamlineLogLevel to the serialization macro.
93-159: Comprehensive frame generation UI implementation!The code properly checks for feature availability and provides clear user feedback. Good use of dynamic backend labels.
162-195: Excellent backend diagnostics implementation!The UI properly uses the new table sorting utilities with version comparators and provides convenient folder access via ShellExecuteW.
src/Utils/UI.h (1)
2-6: Well-structured UI utilities extension!The new table sorting utilities are properly declared with comprehensive documentation. The TableSortFunc type alias provides good flexibility for custom sorting implementations.
Also applies to: 221-270
| #include "Menu.h" | ||
| #include "UI.h" |
There was a problem hiding this comment.
Remove duplicate includes.
These headers are already included at the beginning of the file (lines 2 and 1).
Apply this diff to remove the duplicate includes:
-#include "Menu.h"
-#include "UI.h"📝 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.
| #include "Menu.h" | |
| #include "UI.h" |
🤖 Prompt for AI Agents
In src/Utils/UI.cpp at lines 12 to 13, remove the duplicate #include directives
for "Menu.h" and "UI.h" since they are already included at the beginning of the
file on lines 1 and 2. This will clean up the code and avoid redundant includes.
|
✅ A pre-release build is available for this PR: |
- Display DLL versions for Streamline and FidelityFX in diagnostics UI - Display active Frame Generation backend - Add Streamline log level selection to Backend Diagnostics menu - Pass cleaned up Streamline SDK log output - Only show frame generation menu if backend is truly available - Add feature flags for FSR3 Frame Generation availability
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Upscaling.cpp (1)
118-126: Warning messages have been correctly fixed.The warning messages that were previously swapped have been properly corrected:
streamlineMissingnow correctly shows "Warning: Streamline is not loaded"fidelityFXMissingnow correctly shows "Warning: amd_fidelityfx_dx12.dll is not loaded"This resolves the issue identified in the previous review.
🧹 Nitpick comments (2)
src/Upscaling.cpp (2)
93-160: Excellent conditional UI implementation with minor organization suggestion.The Frame Generation UI enhancements are well-implemented:
- Correctly checks backend availability before showing options
- Proper warning system for various configuration issues
- Dynamic backend labeling shows active Frame Generation technology
- Proper UI tree structure with matching TreePop() calls
Consider refactoring this large conditional block into separate helper methods for improved maintainability.
+ void DrawFrameGenerationAvailable(); + void DrawFrameGenerationUnavailable(); + if (frameGenAvailable) { - if (ImGui::TreeNodeEx("Frame Generation", ImGuiTreeNodeFlags_DefaultOpen)) { - // ... existing content ... - ImGui::TreePop(); - } + DrawFrameGenerationAvailable(); } else { - if (ImGui::TreeNodeEx("Frame Generation", ImGuiTreeNodeFlags_DefaultOpen)) { - ImGui::Text("Frame Generation is not available on your system.\nThis requires either NVIDIA DLSS-G or AMD FSR 3.1 Frame Generation support and D3D12 interop."); - ImGui::TreePop(); - } + DrawFrameGenerationUnavailable(); }
162-195: Well-implemented diagnostics section with minor enhancement suggestions.The Backend Diagnostics section provides valuable functionality:
- Streamline log level control with proper restart notification
- Convenient folder access via clickable entries
- Sortable DLL version tables with proper version comparison
Consider these minor enhancements:
Error handling for folder operations:
if (ImGui::Selectable("AMD FidelityFX DLLs (click to open folder)")) { - ShellExecuteW(nullptr, L"open", FidelityFX::PluginDir, nullptr, nullptr, SW_SHOWNORMAL); + if (ShellExecuteW(nullptr, L"open", FidelityFX::PluginDir, nullptr, nullptr, SW_SHOWNORMAL) <= (HINSTANCE)32) { + logger::warn("Failed to open FidelityFX plugin directory"); + } }Organization improvement:
Consider extracting this section into a separateDrawBackendDiagnostics()method for better code organization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/FidelityFX.cpp(5 hunks)src/FidelityFX.h(2 hunks)src/Streamline.cpp(4 hunks)src/Streamline.h(3 hunks)src/Upscaling.cpp(3 hunks)src/Upscaling.h(1 hunks)src/Utils/FileSystem.h(2 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Upscaling.h
- src/Streamline.h
- src/FidelityFX.cpp
- src/Utils/FileSystem.h
- src/Utils/UI.cpp
- src/FidelityFX.h
- src/Utils/UI.h
- src/Streamline.cpp
🧰 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/Upscaling.cpp (1)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
🧬 Code Graph Analysis (1)
src/Upscaling.cpp (2)
src/Utils/UI.h (3)
HoverTooltipWrapper(31-31)HoverTooltipWrapper(32-32)ShowSortedStringTable(250-256)src/Utils/UI.cpp (4)
HoverTooltipWrapper(23-30)HoverTooltipWrapper(32-38)ShowSortedStringTable(600-647)ShowSortedStringTable(600-606)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
src/Upscaling.cpp (3)
6-6: LGTM - Necessary include for folder opening functionality.The Windows.h include is needed to support the ShellExecuteW calls in the Backend Diagnostics section.
18-19: LGTM - Proper settings field addition.The new
streamlineLogLevelfield is correctly added to the JSON serialization macro, ensuring proper persistence of the setting.
21-195: Excellent implementation of PR objectives.This update successfully delivers the stated goals:
- ✅ Enhanced UI with backend diagnostics and DLL version visibility
- ✅ Conditional Frame Generation menu display based on actual availability
- ✅ Streamline log level control with proper user guidance
- ✅ Improved transparency into active backends and system status
The code is well-structured, handles edge cases appropriately, and provides a significantly improved user experience for upscaling diagnostics and configuration.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes