feat(ui): add Icon Support#1107
Conversation
Function for crispier large text like titles. Helps subtly but does make a difference, especially when high-res icons are visible near text.
|
""" WalkthroughThis update introduces configurable icon-based action buttons in the menu header, with a fallback to text buttons. It adds new structures and utilities for icon texture management, improves font rendering for sharper text, and incorporates a logo in the header. Additional UI options, icon licensing information, and dependencies for image loading are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant Util
User->>Menu: Open Menu
Menu->>Util: InitializeMenuIcons()
Util-->>Menu: Load icon textures (logo, save, load, clear, disk)
Menu->>Menu: Init font config for sharp text
Menu->>Menu: DrawSettings()
alt ShowActionIcons enabled and icons loaded
Menu->>Menu: Render header with logo and icon buttons
else
Menu->>Menu: Render header with text buttons
end
User->>Menu: Interact with action buttons
Menu-->>User: Perform Save/Load/Clear actions
Suggested reviewers
Poem
""" 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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: 5
🧹 Nitpick comments (4)
src/UITextHelper.h (1)
38-70: Good implementation of logo-text alignment with proper resource management.The function correctly handles complex vertical alignment calculations and properly manages ImGui style state with push/pop operations. The font scaling is appropriately restored.
Consider adding a comment explaining the vertical offset calculation for future maintainability:
// Calculate vertical offset to center align logo with text +// When text is scaled larger than logo, offset logo down by half the height difference float verticalOffset = (fontHeight - logoHeight) * 0.5f;src/UIIconLoader.cpp (1)
63-63: Consider making the icon path configurable.The hardcoded path
"Data\\Interface\\CommunityShaders\\Icons\\"might cause issues if the installation directory structure changes or on different platforms.Consider making the path configurable through a parameter or configuration system:
- std::string basePath = "Data\\Interface\\CommunityShaders\\Icons\\"; + // TODO: Make icon path configurable or use relative path resolution + std::string basePath = "Data\\Interface\\CommunityShaders\\Icons\\";src/Menu.h (1)
47-57: Consider adding bounds validation for icon resources.The
UIIconstruct correctly implements resource cleanup, but lacks validation for texture initialization and size constraints.struct UIIcon { ID3D11ShaderResourceView* texture = nullptr; ImVec2 size = ImVec2(32.0f, 32.0f); + bool IsValid() const { + return texture != nullptr && size.x > 0.0f && size.y > 0.0f; + } + void Release() { if (texture) { texture->Release(); texture = nullptr; } } };src/Menu.cpp (1)
253-256: Consider handling partial icon loading failures.While the code logs a warning if icon loading fails, it might be beneficial to track which specific icons failed to load for better debugging.
Consider enhancing the error handling to identify which specific icons failed to load:
-if (!UIIconLoader::InitializeMenuIcons(this)) { - logger::warn("Failed to load UI icons. Will fallback to text buttons"); -} +auto loadResult = UIIconLoader::InitializeMenuIcons(this); +if (!loadResult.success) { + logger::warn("Failed to load UI icons: {}. Will fallback to text buttons", loadResult.errorDetails); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
package/Interface/CommunityShaders/Higher Res Icons (256x)/clear-cache.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Higher Res Icons (256x)/clear-disk.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Higher Res Icons (256x)/cs-logo.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Higher Res Icons (256x)/load-settings.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Higher Res Icons (256x)/save-settings.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/clear-cache.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/clear-disk.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/cs-logo(white).pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/cs-logo.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/load-settings.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/save-settings.pngis excluded by!**/*.png
📒 Files selected for processing (6)
src/Menu.cpp(7 hunks)src/Menu.h(1 hunks)src/UIIconLoader.cpp(1 hunks)src/UIIconLoader.h(1 hunks)src/UITextHelper.h(1 hunks)vcpkg.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/UITextHelper.h (1)
src/Menu.cpp (1)
logoSize(287-287)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (12)
vcpkg.json (1)
26-26: LGTM! Dependency addition supports the new icon loading feature.The addition of the "stb" package is necessary for the icon loading functionality implemented in
UIIconLoader.cpp, which usesstb_image.hfor PNG loading.src/UITextHelper.h (1)
9-35: Well-implemented text sharpening utility.The function correctly implements pixel-perfect text alignment and font scaling with proper state management. Using
std::roundfor pixel alignment and the save/restore pattern for font scaling follows best practices.src/UIIconLoader.h (1)
1-16: Clean and well-structured interface design.The header provides a clear interface for icon loading functionality with proper forward declarations and separation of concerns. The function signatures are descriptive and follow good naming conventions.
src/UIIconLoader.cpp (1)
61-72: Good use of accumulated success pattern.The function correctly uses bitwise AND to accumulate success status from multiple icon loads, ensuring that all icons must load successfully for the function to return true. The null pointer check is also appropriate.
src/Menu.h (1)
58-64: Good icon organization!The
UIIconsstruct provides a clean encapsulation of all menu icon resources, making it easy to manage and extend.src/Menu.cpp (7)
187-193: Excellent resource management!The destructor properly releases all icon textures before shutting down ImGui, preventing resource leaks.
221-232: Good font rendering improvements!The enhanced font configuration with oversampling and pixel snapping will result in sharper text rendering. The settings are appropriate for UI text clarity.
275-306: Excellent adaptive UI implementation!The logo rendering logic gracefully handles both cases (with and without logo) and uses helper functions for aligned text rendering. The scaling calculations are appropriate for maintaining visual consistency.
324-369: Well-implemented icon button styling!The icon buttons have appropriate styling with transparent backgrounds and subtle hover effects. The tooltip implementation provides good user feedback.
382-446: Good fallback implementation for text buttons!The code properly handles the case when icons are disabled, maintaining full functionality with text-based buttons. The layout using tables ensures consistent spacing.
447-468: Clean conditional separator logic!The separator placement logic correctly handles all cases: when icons are disabled, when there are failed tasks, and the default case.
809-826: Well-structured UI options tab!The new tab provides clear user control over the icon display preference with an informative tooltip explaining the behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
✅ A pre-release build is available for this PR: |
1 similar comment
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Without diving in to the specific lines or actually running it yet:
- Where'd the images come from and what's their license?
- Build failure for unreachable code. Please review and resolve.
- Do the icons make sense immediately? Do they need a hover tip for people to know what they are?
I will refresh the icons, some good feedback in the discord channel. The icons are from Microsoft, open licence, it’s linked in the UIIconloader.cpp code at the top. I’ll have to investigate the build fail tomorrow, but i didn’t see any issues, could you point me where to look please? |
Build failures are in the CI and failures should be listed in the UI. |
|
✅ A pre-release build is available for this PR: |
|
Build issues fixed |
There was a problem hiding this comment.
- Changes needed for the images:
A. CS logos need to be in their own directory. The README.md needs to be modified to say that the CS Logos (and link to the directory), is not subject to the GPL-3.0 license and may only be used in unmodified form for local use. No trademark license is granted for the logos use. Just start the edit, I'll finish it. I'm debating whether we even post the logo publicly. People can grab it.
B. All Microsoft logos need to be in their on directory and include the MIT license copied from the Microsoft repo in that directory. In the README, mention that the icons in the directory (link) are subject to the MIT license, link to it. - For UX changes, I don't think they should be toggleable. It adds too much maintenance burden for no real benefit. Make sure the hover tip identifies what they are. We either move to icons, or not.
| ImFontConfig font_config; | ||
| font_config.GlyphExtraSpacing.x = -0.5; | ||
|
|
||
| font_config.GlyphExtraSpacing.x = 0.0f; // Neutral spacing for cleaner look |
There was a problem hiding this comment.
I don't believe we hard coded any of the UI settings. Confirm.
There was a problem hiding this comment.
This just threw off some alignment, can revert but I don't super understand the reference to hard coding, some alignment/text size etc sorta ones are hard coded, less frequent but does occur here and there
There was a problem hiding this comment.
Don't we allow configuration of imgui settings? If yes for this type of setting, then this should be added to that configuration, if no, then fine to hard code.
There was a problem hiding this comment.
Ah okay i understand.
It was hard coded before anyway, it was something i changed alongside the other sharper text rendering tweaks for the title that are in this pr, you can see that comparison image above.
I don't think any user-configurability is really needed for this, it exists to make the title nice and crisp.
FlayaN
left a comment
FlayaN
left a comment
There was a problem hiding this comment.
Intentional to add the Unused folder with duplicated images? Maybe upload somewhere else if maybe needed in the future
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Utils/UI.h (2)
50-53: MakeLoadTextureFromFiledevice-agnostic and clarify ownership
The function implicitly relies on a globally available D3D11 device/context (seen in the implementation elsewhere). That hides a hard dependency and makes unit-testing impossible. Pass the device pointer explicitly and document who owns / releases the returned SRV:-bool LoadTextureFromFile(const char* filename, ID3D11ShaderResourceView** out_srv, ImVec2& out_size); +// `device` must remain alive for the SRV lifetime. Caller owns *out_srv and must `Release()` it. +bool LoadTextureFromFile(ID3D11Device* device, + const char* filename, + ID3D11ShaderResourceView** out_srv, + ImVec2& out_size);This also removes the hidden global and invites reuse in other render back-ends.
55-56: Minor API polish for the text helpers
RenderSharpTextandRenderAlignedTextWithLogomutate global ImGui state but do not indicate that in their name/signature. Consider adding a prefix (Draw*,ImDraw*) or returning the rendered size so callers can lay out subsequent widgets more predictably.Nit: default
textScale = 1.5fis a magic number—expose a named constant or document why 1.5.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
package/Interface/CommunityShaders/Icons/Community Shaders Logo/cs-logo.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Microsoft Icons/clear-cache.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Microsoft Icons/clear-disk.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Microsoft Icons/load-settings.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Microsoft Icons/save-settings.pngis excluded by!**/*.png
📒 Files selected for processing (6)
README.md(1 hunks)package/Interface/CommunityShaders/Icons/LICENSE.txt(1 hunks)package/Interface/CommunityShaders/Icons/Microsoft Icons/LICENSE(1 hunks)src/Menu.cpp(7 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- package/Interface/CommunityShaders/Icons/LICENSE.txt
- package/Interface/CommunityShaders/Icons/Microsoft Icons/LICENSE
🧰 Additional context used
🧠 Learnings (1)
src/Menu.cpp (1)
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.
🧬 Code Graph Analysis (1)
src/Menu.cpp (1)
src/Utils/UI.cpp (8)
InitializeMenuIcons(101-119)InitializeMenuIcons(101-101)RenderAlignedTextWithLogo(148-180)RenderAlignedTextWithLogo(148-148)RenderSharpText(122-146)RenderSharpText(122-122)HoverTooltipWrapper(11-18)HoverTooltipWrapper(20-26)
🪛 markdownlint-cli2 (0.17.2)
README.md
142-142: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🔇 Additional comments (1)
src/Utils/UI.cpp (1)
101-118:Details
❌ Incorrect review comment
Partial icon load leaves UI without any buttons
InitializeMenuIconsreturnsfalseif any icon fails.
Later,DrawSettingschecks every texture pointer, but the text-button fallback is only triggered whenShowActionIcons == false, not when textures are incomplete. Users end up with no action buttons.Recommend returning
voidand letting callers decide, or (simpler) compute:bool showIcons = settings.Theme.ShowActionIcons && uiIcons.saveSettings.texture && uiIcons.loadSettings.texture && uiIcons.clearCache.texture && uiIcons.clearDiskCache.texture; if (!showIcons) { /* text buttons */ }and delete the current
if (!settings.Theme.ShowActionIcons)block.
I’ll inspect how
InitializeMenuIcons,DrawSettings, and theShowActionIconsflag interact to confirm whether partial icon loads can leave the UI without buttons.
🏁 Script executed:
#!/bin/bash set -e # Locate InitializeMenuIcons and its callers rg --context 3 "InitializeMenuIcons" || true # Dump DrawSettings body to see fallback logic rg --context 10 "DrawSettings" || true # Find definition and use of ShowActionIcons rg --context 3 "ShowActionIcons" || trueLength of output: 66190
Ignore outdated ShowActionIcons fallback suggestion
The codebase does not define or use asettings.Theme.ShowActionIconsflag or any block keyed off it.InitializeMenuIconsonly logs a warning on failure and does not disable icon drawing elsewhere, so the proposed refactor targeting that flag isn’t applicable.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Utils/UI.cpp (1)
86-118: Use sRGB format & optional mips for correct colour and samplingThe raw PNGs are authored in sRGB space, yet the texture is created with
DXGI_FORMAT_R8G8B8A8_UNORM. Rendering in a linear shader pipeline will wash out colours.
Switching toDXGI_FORMAT_R8G8B8A8_UNORM_SRGB(and addingMiscFlags |= D3D11_RESOURCE_MISC_GENERATE_MIPS+ a subsequentGenerateMips) gives accurate colour and smooth minification for high-DPI windows.- desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; + // Preserve icon colour fidelity + desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM_SRGB; + desc.MiscFlags = D3D11_RESOURCE_MISC_GENERATE_MIPS; ... - hr = globals::d3d::device->CreateShaderResourceView(pTexture, &srvDesc, out_srv); + hr = globals::d3d::device->CreateShaderResourceView(pTexture, &srvDesc, out_srv); + if (SUCCEEDED(hr) && *out_srv) + globals::d3d::context->GenerateMips(*out_srv);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)src/Menu.cpp(7 hunks)src/Menu.h(1 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- src/Utils/UI.h
- src/Menu.h
- src/Menu.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Utils/UI.cpp (2)
src/Menu.h (1)
menu(30-34)src/Utils/UI.h (9)
LoadTextureFromFile(117-117)StyledButtonWrapper(58-58)StyledButtonWrapper(63-63)StyledButtonWrapper(66-66)StyledButtonWrapper(68-68)SectionWrapper(89-90)SectionWrapper(95-95)SectionWrapper(103-103)SectionWrapper(105-105)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Utils/UI.cpp (4)
10-13: Include-once requirement forSTB_IMAGE_IMPLEMENTATIONDefining
STB_IMAGE_IMPLEMENTATIONinside a widely-used translation unit risks ODR violations if any other TU also defines it (easy to happen in large repos or via 3rd-party code).
Isolate the implementation in a dedicatedstb_image.cpp(or guard the define via a CMake / pre-build flag) to avoid duplicate symbol errors.
195-205: Null-check logo SRV before rendering
ImGui::ImagedereferenceslogoTexture; passingnullptr(e.g., when icon loading fails) triggers an access violation.
Guard the call and fall back to text-only rendering.if (!logoTexture) { RenderSharpText(text, /*alignToPixelGrid=*/true, textScale); return; }
216-232: RAII colour scope looks goodThe
StyledButtonWrappercleanly pushes/pops three colour styles and is exception-safe. 👍
254-261: SectionWrapper destructor tidies layout properlySpacing/separator restoration is handled; no issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (5)
include/ffx_api_types.h (1)
23-26:⚠️ Potential issueMissing
<stdbool.h>– breaks plain-C builds
boolis used later (line 239) but<stdbool.h>is no longer included.
In a C (not C++) translation unit this will raise “unknown type name ‘bool’”.#include <stdint.h> +#ifndef __cplusplus +# include <stdbool.h> +#endifsrc/Feature.cpp (1)
46-55:⚠️ Potential issueWide-character path fed to
CSimpleIniA‑ will not compile on MSVC / may mis-decode on Win32
CSimpleIniAexpectsconst char*, yetini_pathis astd::wstring. Passing awchar_t*to a narrow API is UB and on MSVC is a hard error unless an implicit cast sneaks in through a non-standard overload.Two robust fixes:
-CSimpleIniA ini; -ini.SetUnicode(); -ini.LoadFile(ini_path.c_str()); +// Use the wide-char version throughout +CSimpleIniW ini; +ini.SetUnicode(); +ini.LoadFile(ini_path.c_str());or convert the path to UTF-8 before the call.
Failing to address this will break non-ASCII install paths and, depending on compiler flags, may fail to build.
src/State.cpp (1)
660-667:⚠️ Potential issuePotential null-pointer dereference when directional light is absent
dirLightcan benullptrif the cast fails (e.g. rare weather transitions).
Dereferencing it without a guard will crash.-auto dirLight = skyrim_cast<RE::NiDirectionalLight*>(shadowSceneNode->GetRuntimeData().sunLight->light.get()); -data.DirLightColor = { dirLight->GetLightRuntimeData().diffuse.red, ... }; +auto dirLight = shadowSceneNode && shadowSceneNode->GetRuntimeData().sunLight + ? skyrim_cast<RE::NiDirectionalLight*>(shadowSceneNode->GetRuntimeData().sunLight->light.get()) + : nullptr; +if (dirLight) { + data.DirLightColor = { + dirLight->GetLightRuntimeData().diffuse.red, + dirLight->GetLightRuntimeData().diffuse.green, + dirLight->GetLightRuntimeData().diffuse.blue, + 1.0f }; + ... +} else { + data.DirLightColor = { 1.0f, 1.0f, 1.0f, 1.0f }; +}src/Hooks.cpp (1)
448-453: 🛠️ Refactor suggestionEnsure D3D11 Map/Unmap detours are active on all code paths
After the non-proxy
D3D11CreateDeviceAndSwapChaincall you runPostDevice()
but you do not callstreamline->InstallHooks, leaving the Map/Unmap
detours absent. Frame-buffer caching therefore never happens outside the proxy
case.+ if (streamline->initialized) + { + streamline->slSetD3DDevice(*ppDevice); + streamline->PostDevice(); + streamline->InstallHooks(*ppImmediateContext); // <-- add + }Without this change subsequent uses of
frameBufferCachedare unsafe.src/Upscaling.cpp (1)
276-296:⚠️ Potential issueRelease happens too early – use-after-release risk
inputTextureSRVandoutputTextureRTVareRelease()d immediately after acquisition, yet they are still dereferenced (GetResource, CS bindings) several lines later.
Releasing before the final use leaves you with dangling pointers and will trigger ref-count underflow / access violations on some drivers.Suggested quick fix:
- inputTextureSRV->Release(); ... - outputTextureRTV->Release(); + // defer Release until after the last usePlace the
Release(); inputTextureSRV = nullptr;and same foroutputTextureRTVafter line 373, once all uses are done.
Repeat the same pattern for the depth-stencil view.
♻️ Duplicate comments (1)
src/Upscaling.cpp (1)
385-405:⚠️ Potential issueDuplicate early-release bug in
SharpenTAAThe same premature
Release()pattern appears here. Defer releasinginputTextureSRVandoutputTextureRTVuntil after their final usage (post-copy at line 437). This avoids undefined behaviour identical to the issue outlined above.
🧹 Nitpick comments (12)
include/ffx_api_types.h (1)
82-82: Typo-compatibility alias should be marked deprecatedRetaining
FfxApiResorceUsagefor compatibility is fine, but without a deprecation marker new code may keep using the misspelt name.
Consider annotating it, e.g.:#if defined(__cplusplus) && __cplusplus >= 201402L using FfxApiResorceUsage [[deprecated("Use FfxApiResourceUsage")]] = FfxApiResourceUsage; #else typedef FfxApiResourceUsage FfxApiResorceUsage /* deprecated – use FfxApiResourceUsage */; #endifREADME.md (1)
137-138: Link to the community logo license file and hyperlink GPL-3.0
For consistency with the Microsoft Icons entry, hyperlink the "GPL-3.0" term to the main COPYING file and add a direct link to the Community Shaders Logo’s LICENSE file.- [Community Shaders Logo](package/Interface/CommunityShaders/Icons/Community%20Shaders%20Logo/) is not subject to the GPL-3.0 license and may only be used in unmodified form for local use. No trademark license is granted for the logo's use. + [Community Shaders Logo](package/Interface/CommunityShaders/Icons/Community%20Shaders%20Logo/) is not subject to the [GPL-3.0](COPYING) license and may only be used in unmodified form for local use. No trademark license is granted for the logo’s use. + See the [logo license file](package/Interface/CommunityShaders/Icons/Community%20Shaders%20Logo/LICENSE.txt) for details.features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
69-71: Switching toSampleLevelmay re-introduce aliasing and disables anisotropic filtering
SampleLevelignores the hardware derivatives, so up-close surfaces will always fetch the explicitmipLevel, potentially causing shimmering or blurred results whenenableTilingFixis disabled.
PreviouslySampleGradpreserved derivatives and let the sampler choose the correct mip.Consider reverting (or gate the choice on a QUALITY flag), e.g.:
return tex.SampleGrad(samp, uv, dx, dy); // higher quality fallbackIf the intent is performance, please document the trade-off and verify visual regressions.
src/Features/TerrainHelper.cpp (1)
8-8: Nit: keep wording consistent across featuresMost features now start with “This feature is not installed!”.
Consider mirroring that phrasing for consistency:-ImGui::Text("Terrain Helper is only required if a terrain mod you are using requires it, otherwise it does nothing."); +ImGui::Text("This feature is not installed! Terrain Helper is only required if a terrain mod you are using requires it.");src/Features/TerrainVariation.h (1)
13-13: Hard-coding Nexus URL: minor maintainability caveatHard-coding the full URL is fine, but you lose the compile-time benefit of a central
MOD_ID.
If multiple places need updates when the ID changes, consider a smallconstexpr uint32_t ModId = 148123;and build the link once.src/Features/TerrainHelper.h (1)
30-30: Hard-coded URL ‑ considerconstexprorstaticstorage
GetFeatureModLink()now allocates a freshstd::stringon every call. If this method is hit frequently (e.g. every UI frame) the tiny allocation can still show up in a profiler. Returning astatic constexpr std::string_view(or even aconstexpr char[]) would avoid the extra heap churn.-virtual std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/143149"; }; +static constexpr std::string_view kModURL = + "https://www.nexusmods.com/skyrimspecialedition/mods/143149"; +virtual std::string GetFeatureModLink() override { return std::string(kModURL); };src/Feature.cpp (2)
69-72: Trimming the last two characters of the version string is fragile
minimalVersionString.substr(0, size() - 2)silently assumes the version text always ends with “.0”. If a hot-fix tag becomes “.10” you’ll chop the wrong digit.-auto minimalVersionString = minimalFeatureVersion.string(); -minimalVersionString = minimalVersionString.substr(0, minimalVersionString.size() - 2); +auto minimalVersionString = minimalFeatureVersion.string(); +if (auto pos = minimalVersionString.find_last_of('.'); pos != std::string::npos) + minimalVersionString.erase(pos);This keeps the intent (hide patch component) but is resilient to future version formats.
80-81: Storedversionkeeps the raw “x-y-z” form
You normalisevalue(replace-with.) for comparison but then save the original literal intoversion, leading to"1-2-3"vs"1.2.3"discrepancies when the value is later logged or written to cache.Recommend storing the sanitised string:
-REL::Version featureVersion(std::regex_replace(value, std::regex("-"), ".")); -... -version = value; +std::string cleaned = std::regex_replace(value, std::regex("-"), "."); +REL::Version featureVersion(cleaned); +... +version = cleaned;Also applies to: 55-57
src/Feature.h (1)
44-46: EmptyDrawUnloadedUI()– remove or document
With the body now empty every override in derived classes must implement the UI, otherwise the call chain does nothing. Either declare the function pure virtual if it’s mandatory, or add a doc-comment clarifying that a no-op is intentional so static polymorphism remains safe.src/Menu.h (1)
46-68: Avoid SRV leaks – callRelease()for every icon inMenu::~Menu()
UIIconowns a COM pointer butMenu’s destructor is empty in this header.
UnlessMenu.cppexplicitly callsuiIcons.saveSettings.Release()… etc.,
each SRV will leak on shutdown.Add:
Menu::~Menu() { uiIcons.saveSettings.Release(); uiIcons.loadSettings.Release(); uiIcons.clearCache.Release(); uiIcons.clearDiskCache.Release(); uiIcons.logo.Release(); }(or use
winrt::com_ptr/wil::com_ptrto get RAII).src/Utils/UI.cpp (1)
126-128: Prefer the passed context instead of a global
GenerateMipsis executed throughglobals::d3d::context, yet the function already receives a validID3D11Device*.
For testability and to avoid tight coupling usedevice->GetImmediateContext(&ctx)(or accept anID3D11DeviceContext*parameter) rather than reaching into globals.src/Menu.cpp (1)
186-192: Destructor could just call a loopReleasing five textures one-by-one is repetitive and easy to forget when new icons are added.
Store them in an array/vector and iterate, or add aReleaseAll()helper inUIIcons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
features/Frame Generation/SKSE/Plugins/FidelityFX/amd_fidelityfx_dx12.dllis excluded by!**/*.dllpackage/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (61)
.coderabbit.yaml(0 hunks).github/workflows/build.yaml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(0 hunks)BuildRelease.bat(1 hunks)README.md(1 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(1 hunks)include/ffx_api.h(0 hunks)include/ffx_api_types.h(1 hunks)include/ffx_framegeneration.h(5 hunks)include/ffx_framegeneration.hpp(1 hunks)src/DX12SwapChain.cpp(0 hunks)src/DX12SwapChain.h(0 hunks)src/Deferred.cpp(0 hunks)src/Deferred.h(0 hunks)src/Feature.cpp(1 hunks)src/Feature.h(1 hunks)src/FeatureIssues.cpp(0 hunks)src/FeatureIssues.h(0 hunks)src/Features/CloudShadows.h(0 hunks)src/Features/DynamicCubemaps.h(0 hunks)src/Features/ExtendedMaterials.h(0 hunks)src/Features/GrassCollision.h(0 hunks)src/Features/GrassLighting.h(0 hunks)src/Features/HairSpecular.h(1 hunks)src/Features/IBL.h(0 hunks)src/Features/InteriorSunShadows.h(0 hunks)src/Features/InverseSquareLighting.h(0 hunks)src/Features/LODBlending.h(0 hunks)src/Features/LightLimitFix.h(0 hunks)src/Features/ScreenSpaceGI.h(0 hunks)src/Features/ScreenSpaceShadows.h(0 hunks)src/Features/SkySync.h(0 hunks)src/Features/Skylighting.h(0 hunks)src/Features/SubsurfaceScattering.h(0 hunks)src/Features/TerrainBlending.h(0 hunks)src/Features/TerrainHelper.cpp(1 hunks)src/Features/TerrainHelper.h(1 hunks)src/Features/TerrainShadows.h(0 hunks)src/Features/TerrainVariation.cpp(1 hunks)src/Features/TerrainVariation.h(2 hunks)src/Features/VR.h(0 hunks)src/Features/VolumetricLighting.h(0 hunks)src/Features/WaterEffects.h(0 hunks)src/Features/WetnessEffects.h(0 hunks)src/FidelityFX.cpp(0 hunks)src/Hooks.cpp(3 hunks)src/Menu.cpp(16 hunks)src/Menu.h(1 hunks)src/ShaderCache.cpp(0 hunks)src/State.cpp(2 hunks)src/State.h(0 hunks)src/Streamline.cpp(4 hunks)src/Streamline.h(1 hunks)src/Upscaling.cpp(1 hunks)src/Upscaling.h(0 hunks)src/Util.h(0 hunks)src/Utils/FileSystem.cpp(0 hunks)src/Utils/FileSystem.h(0 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
💤 Files with no reviewable changes (37)
- src/Util.h
- src/Features/TerrainBlending.h
- src/Features/IBL.h
- include/ffx_api.h
- src/FidelityFX.cpp
- src/Features/InverseSquareLighting.h
- src/State.h
- src/Features/ScreenSpaceShadows.h
- src/DX12SwapChain.h
- src/Features/LODBlending.h
- src/Features/VolumetricLighting.h
- src/Features/InteriorSunShadows.h
- src/Features/GrassCollision.h
- src/ShaderCache.cpp
- src/Features/SkySync.h
- src/Deferred.h
- src/Features/WaterEffects.h
- .coderabbit.yaml
- src/Features/ExtendedMaterials.h
- src/Features/ScreenSpaceGI.h
- src/Features/DynamicCubemaps.h
- src/Features/Skylighting.h
- src/Deferred.cpp
- src/Features/LightLimitFix.h
- .pre-commit-config.yaml
- src/Features/GrassLighting.h
- src/Features/SubsurfaceScattering.h
- src/Features/VR.h
- src/Upscaling.h
- src/DX12SwapChain.cpp
- src/Utils/FileSystem.h
- src/Features/TerrainShadows.h
- src/Features/CloudShadows.h
- src/Utils/FileSystem.cpp
- src/Features/WetnessEffects.h
- src/FeatureIssues.h
- src/FeatureIssues.cpp
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- BuildRelease.bat
- src/Features/HairSpecular.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/UI.h
🧰 Additional context used
🧠 Learnings (1)
src/Menu.cpp (1)
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.
🧬 Code Graph Analysis (3)
src/Features/TerrainVariation.cpp (1)
src/Features/TerrainHelper.cpp (2)
DrawUnloadedUI(6-9)DrawUnloadedUI(6-6)
src/Utils/UI.cpp (2)
src/Menu.h (1)
menu(28-32)src/Utils/UI.h (1)
LoadTextureFromFile(53-56)
src/Streamline.h (1)
src/Streamline.cpp (4)
thunk(371-379)thunk(371-371)thunk(381-386)thunk(381-381)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
README.md (1)
135-136: Heading level is correct
The new### Iconssection is properly nested under## Licenseand follows the existing heading hierarchy..github/workflows/build.yaml (1)
31-34:Details
❌ Incorrect review comment
if:expression can access undefined objects for non-PR eventsFor
pushorworkflow_dispatcheventsgithub.event.pull_requestis null, yet the expression still dereferences nested fields (draft,labels).
GitHub Actions does not short-circuit||, so the entire expression is evaluated and the workflow will fail withThe template is invalid: .github/workflows/build.yaml (Line X, Col Y): Unexpected value ''Suggest guarding the dereference:
if: > github.event_name != 'pull_request_target' || (github.event.pull_request && github.event.pull_request.draft == false) || (github.event.pull_request && contains(github.event.pull_request.labels.*.name, 'force-build'))
To verify how GitHub Actions handles logical operators and null event payloads in
if:expressions, let’s check the official behavior:
🌐 Web query:
Does GitHub Actions `if:` expression short-circuit logical operators (|| and &&)? And how does it handle null or undefined `github.event.pull_request` when evaluating conditions on non-PR events?💡 Result:
GitHub Actions'
if:expressions do support short-circuit evaluation of logical operators||and&&. This means:
- For
&&(AND), if the left operand is false, the right operand is not evaluated.- For
||(OR), if the left operand is true, the right operand is not evaluated.So you can safely write expressions that rely on this behavior for conditional checks.
Regarding the handling of
nullorundefinedvalues likegithub.event.pull_requestin non-PR events, GitHub Actions does not throw an error when you try to access a property that doesn't exist; instead, it treats such references asnullorundefined. When you useif:expressions, conditions involving these null or undefined values evaluate gracefully without causing a workflow error.For example, this expression safely checks if the event is a pull request and then accesses
github.event.pull_request.merged:if: github.event.pull_request && github.event.pull_request.merged == true
- On a non-PR event,
github.event.pull_requestisnullorundefined.- The
&&operator short-circuits since the first operand is falsey, so it doesn't evaluate the second operand.- The condition evaluates to false without error.
In summary:
- Logical operators in
if:expressions short-circuit.- Accessing
nullorundefinedproperties in expressions won't cause errors; they evaluate to false or null-like values.- Using short-circuit logic is a good practice to safely check properties that may not exist on all event types.
Unnecessary guard in GitHub Actions
if:expressionGitHub Actions’ expression syntax already short-circuits
||/&&and treats undefined properties (likegithub.event.pull_requeston non-PR events) as null without throwing errors. Your original condition is safe and does not require additional null checks.Likely an incorrect or invalid review comment.
src/Features/TerrainVariation.cpp (2)
140-144:DrawFailLoadMessage()now always returnsfalse– confirm callersPrevious implementations might have relied on
trueto suppress default UI.
Ensure callers (if any) still display an error state when the feature fails to load; otherwise the message path becomes dead code.
145-159: Nice, concise unloaded-UI 👍The new
DrawUnloadedUI()provides immediate feedback without overwhelming the user.
No functional issues spotted.src/Features/TerrainVariation.h (1)
56-56: Header/CPP method signature alignment confirmed
DrawUnloadedUI()is declared here and defined in the CPP with identical signature – good.include/ffx_framegeneration.hpp (1)
38-42: Nice cleanup – duplicate specialisations removed
Removing the redundantHudlessspecialisations keeps the header succinct and avoids ODR hazards.src/Streamline.cpp (1)
207-216: ValidateframeBufferCachedbefore use
frameBufferCachedis populated via the Map/Unmap hooks.
If those hooks failed (e.g. not yet installed) this dereference reads
uninitialised memory and the matrices become garbage.Guard with a boolean flag or nullptr check before using the cached data and
log a warning when it hasn’t been captured yet.include/ffx_framegeneration.h (1)
34-42: No actionable issues noticed in the new enum definitions – looks fine.src/Utils/UI.cpp (1)
10-13: Ensure single definition of stb_image
#define STB_IMAGE_IMPLEMENTATIONinside this TU is correct if no other source file defines it.
Double-defining will break the build with multiple-definition errors. Please verify (or wrap in a dedicated cpp, e.g.stb_image.cpp).
|
I'm not sure what you mean by pinned,
|
I meant docked, drag the CS window to the top of the skyrim window for example. Technically it doesn't fail to load the icons then but doesn't render the top bar |
Ah i understand,
It looks a little funky, but not too bad. I'll brainstorm some ways to fix it and do a follow up later. Either spacing it out or pushing it into the top bar or something. Not something I had tested |
|
An easy fix for now might be to add |
Yeah good idea, I don't think it would cause an issue to hide them when its docked. I can't remember if the old buttons went away when docked but ill check that. |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
* Full Icon framework & support. * UI Text helper function for better looking large text Function for crispier large text like titles. Helps subtly but does make a difference, especially when high-res icons are visible near text. * Update Menu.cpp * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Icon folder changes, fix for compile issue. * Reorganise * Update README.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fixes * rabbit suggested fixes * fix * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * build fix * Fixes * Update Menu.cpp * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * Licence tweaks * Comment clean & Mipmap fixes * Texture memory leak cleanup * fixes * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Full Icon framework & support. * UI Text helper function for better looking large text Function for crispier large text like titles. Helps subtly but does make a difference, especially when high-res icons are visible near text. * Update Menu.cpp * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Icon folder changes, fix for compile issue. * Reorganise * Update README.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fixes * rabbit suggested fixes * fix * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * build fix * Fixes * Update Menu.cpp * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * Licence tweaks * Comment clean & Mipmap fixes * Texture memory leak cleanup * fixes * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Full Icon framework & support. * UI Text helper function for better looking large text Function for crispier large text like titles. Helps subtly but does make a difference, especially when high-res icons are visible near text. * Update Menu.cpp * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/UIIconLoader.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Icon folder changes, fix for compile issue. * Reorganise * Update README.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fixes * rabbit suggested fixes * fix * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * build fix * Fixes * Update Menu.cpp * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * Licence tweaks * Comment clean & Mipmap fixes * Texture memory leak cleanup * fixes * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
Part 1 of UI changes.
Added framework for icon support in ImGui. Added icons for Community Shaders Logo, and optional logos for Save/Load/Cache/Disk. When "Use action icons" is untoggled, the UI reverts to as it was with text-based buttons.
Added "UITextHelper.h" which contains some code to enforce better per-pixel text rendering, which was noticeably blurry for larger text fields like the Community Shaders title, especially when compared to the high-res icons.
Icons are 128x128 .png files with Lanczos 3 filtering.
(Second image is just to show the action icon button, ignore formatting issues, this is not present in the first image (final)).
(Top new, bottom old. More obvious in game)
Summary by CodeRabbit
Summary by CodeRabbit