build: bump deps#2018
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImGui/third-party bumps and API migrations: PushFont signature updates, ImGui flag/name changes, FullPalette reorder + sanitization, thread-pool template/usage migration to detached tasks, shader compilation scheduling change, vcpkg/CMake/CI pinning and related UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ShaderCache
participant CompilationPool as "BS::thread_pool<>\n(compilationPool)"
participant Worker as "Detached worker"
Caller->>ShaderCache: construct / enable watcher
ShaderCache->>CompilationPool: detach_task(lambda token -> ManageCompilationSet(token))
CompilationPool->>Worker: start ManageCompilationSet(token)
Worker->>ShaderCache: ManageCompilationSet schedules per-item detach_task(ProcessCompilationSet, token)
Worker->>CompilationPool: detach_task(ProcessCompilationSet, token)
CompilationPool->>Worker: run detached ProcessCompilationSet(task, token)
Worker->>ShaderCache: perform compilation work (checks stop token)
Note over ShaderCache,CompilationPool: shutdown uses compilationPool.wait_for(timeout) before exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.2.510)CMakePresets.json2026-03-27 00:12:22,226 [MainThread ] [ERROR] Template file not found: CMakePresets.json ... [truncated 2592 characters] ... NotFoundError: [Errno 2] No such file or directory: 'CMakePresets.json' 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. Comment |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/ShaderCache.cpp (2)
2337-2350:⚠️ Potential issue | 🔴 CriticalCapture the watcher instance at submission time.
This lambda dereferences the mutable
listenermember when the worker eventually starts. IfStopFileWatcher()runs while the pool is busy, it nulls that member first and this task crashes onlistener->processQueue().🐛 Minimal fix
- compilationPool.detach_task([this] { listener->processQueue(); }); + auto* queueListener = listener; + compilationPool.detach_task([queueListener] { queueListener->processQueue(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ShaderCache.cpp` around lines 2337 - 2350, The task lambda submitted via compilationPool.detach_task captures the mutable member listener by reference which may be nulled by StopFileWatcher() before the task runs; fix it by capturing the current listener instance into a local variable at submission time (e.g., copy the pointer/shared_ptr into a local named like localListener) and have the lambda call localListener->processQueue() (optionally check localListener for null before invoking) so the worker uses a stable reference even if the member is later cleared.
1927-1934:⚠️ Potential issue | 🔴 CriticalStop the detached loops before teardown.
The destructor waits on
compilationPool.wait_for(std::chrono::milliseconds(1000))without first requesting cooperative shutdown from the two long-lived detached loops:ManageCompilationSet()loops on!stoken.stop_requested()andprocessQueue()loops oncache->UseFileWatcher(). Neither condition is triggered before the wait, so this timeout path is reached during normal shutdown. The fallback toTerminateThread()is unsafe even if it worked correctly—but it won't:managementThreadstores the pseudo-handle fromGetCurrentThread(), which only works in that thread's context, not when called from the destructor.Additionally,
StopFileWatcher()nullifieslistenerwithout clearinguseFileWatcher, creating a race whereprocessQueue()may dereference a dangling pointer.Call
ssource.request_stop()andSetFileWatcher(false)beforecompilationPool.wait_for()to let the loops exit cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ShaderCache.cpp` around lines 1927 - 1934, Before waiting on compilationPool.wait_for(...) request cooperative shutdown: call ssource.request_stop() and SetFileWatcher(false) so the ManageCompilationSet() loop (which checks !stoken.stop_requested()) and processQueue() (which checks cache->UseFileWatcher()) can exit; also update StopFileWatcher() to clear useFileWatcher (SetFileWatcher(false)) before nulling listener to avoid a race where processQueue() dereferences a dangling listener, and remove/replace the TerminateThread/GetCurrentThread pseudo-handle logic around managementThread with a proper thread shutdown/join pattern (store a real thread handle/USE std::thread join or equivalent) rather than calling TerminateThread.src/Menu/ThemeManager.cpp (1)
99-106:⚠️ Potential issue | 🟠 MajorPersist the computed font scale when rebuilding the style.
Line 464 sets
FontScaleMaininReloadFont(), butSetupImGuiStyle()rebuilds the entireImGuiStyleby assigningstyleCopy(created from the base theme) on line 124, which overwrites the computed scale. This happens both at initialization and every frame inOverlayRenderer::Render(). The fix is to store the computed value before the full style assignment:Set FontScaleMain in SetupImGuiStyle
const float scaleFactor = fontScale * exp2(globalScale); styleCopy.ScaleAllSizes(scaleFactor); +styleCopy.FontScaleMain = exp2(globalScale); styleCopy.MouseCursorScale = 1.f; style = styleCopy;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Menu/ThemeManager.cpp` around lines 99 - 106, SetupImGuiStyle currently overwrites the computed font scale when it assigns styleCopy; compute and persist the font scale inside SetupImGuiStyle (using the same logic as in ReloadFont: inspect ImGui::GetIO().FontDefault, kBaselineFontSize and globalScale), apply the scale via styleCopy.ScaleAllSizes(scaleFactor) and then store that computed value into the persistent FontScaleMain (or the same member used by ReloadFont) before assigning styleCopy to ImGui::GetStyle() so the rebuilt style keeps the computed font scale.vcpkg.json (1)
21-23:⚠️ Potential issue | 🔴 CriticalThe vcpkg manifest cannot resolve at baseline
dddca6fa…—all four pinned dependency versions are missing.Verification found that none of these versions exist in the vcpkg registry at the specified baseline commit:
bshoshany-thread-pool 5.1.0detours 4.0.1(port-version 8)imgui 1.92.6directx-headers 1.614.1Update the
builtin-baselineto a commit where these versions are available, or adjust the version pins to match what exists at the current baseline. The manifest will fail to resolve during builds otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vcpkg.json` around lines 21 - 23, The vcpkg manifest's pinned versions cannot be resolved at the current builtin-baseline; update the manifest so the versions exist at the baseline or change the baseline to one that contains them. Either set "builtin-baseline" to a commit SHA that includes bshoshany-thread-pool 5.1.0, detours 4.0.1 (port-version 8), imgui 1.92.6, and directx-headers 1.614.1, or change the package version entries for "bshoshany-thread-pool", "detours", "imgui", and "directx-headers" to versions that are present at the current baseline so the manifest can resolve during builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Menu.cpp`:
- Around line 186-195: SanitizeFullPaletteJson currently returns early for
non-array "FullPalette" values, leaving malformed entries to later
deserialization in Menu::Load/Menu::LoadTheme; change the logic so that any
"FullPalette" that is not a proper array of size ImGuiCol_COUNT is erased: if
themeJson.contains("FullPalette") then check if it's not an array OR its size
differs from static_cast<size_t>(ImGuiCol_COUNT) and in either case call
themeJson.erase("FullPalette"); otherwise leave a valid array untouched.
---
Outside diff comments:
In `@src/Menu/ThemeManager.cpp`:
- Around line 99-106: SetupImGuiStyle currently overwrites the computed font
scale when it assigns styleCopy; compute and persist the font scale inside
SetupImGuiStyle (using the same logic as in ReloadFont: inspect
ImGui::GetIO().FontDefault, kBaselineFontSize and globalScale), apply the scale
via styleCopy.ScaleAllSizes(scaleFactor) and then store that computed value into
the persistent FontScaleMain (or the same member used by ReloadFont) before
assigning styleCopy to ImGui::GetStyle() so the rebuilt style keeps the computed
font scale.
In `@src/ShaderCache.cpp`:
- Around line 2337-2350: The task lambda submitted via
compilationPool.detach_task captures the mutable member listener by reference
which may be nulled by StopFileWatcher() before the task runs; fix it by
capturing the current listener instance into a local variable at submission time
(e.g., copy the pointer/shared_ptr into a local named like localListener) and
have the lambda call localListener->processQueue() (optionally check
localListener for null before invoking) so the worker uses a stable reference
even if the member is later cleared.
- Around line 1927-1934: Before waiting on compilationPool.wait_for(...) request
cooperative shutdown: call ssource.request_stop() and SetFileWatcher(false) so
the ManageCompilationSet() loop (which checks !stoken.stop_requested()) and
processQueue() (which checks cache->UseFileWatcher()) can exit; also update
StopFileWatcher() to clear useFileWatcher (SetFileWatcher(false)) before nulling
listener to avoid a race where processQueue() dereferences a dangling listener,
and remove/replace the TerminateThread/GetCurrentThread pseudo-handle logic
around managementThread with a proper thread shutdown/join pattern (store a real
thread handle/USE std::thread join or equivalent) rather than calling
TerminateThread.
In `@vcpkg.json`:
- Around line 21-23: The vcpkg manifest's pinned versions cannot be resolved at
the current builtin-baseline; update the manifest so the versions exist at the
baseline or change the baseline to one that contains them. Either set
"builtin-baseline" to a commit SHA that includes bshoshany-thread-pool 5.1.0,
detours 4.0.1 (port-version 8), imgui 1.92.6, and directx-headers 1.614.1, or
change the package version entries for "bshoshany-thread-pool", "detours",
"imgui", and "directx-headers" to versions that are present at the current
baseline so the manifest can resolve during builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f45150c-6b78-462c-b5f2-2967ac838ac0
📒 Files selected for processing (17)
src/Features/UnifiedWater/WaterCache.cppsrc/Features/UnifiedWater/WaterCache.hsrc/Features/VR/SettingsUI.cppsrc/Menu.cppsrc/Menu.hsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/Fonts.cppsrc/Menu/HomePageRenderer.cppsrc/Menu/MenuHeaderRenderer.cppsrc/Menu/SettingsTabRenderer.cppsrc/Menu/ThemeManager.cppsrc/ShaderCache.cppsrc/ShaderCache.hsrc/Utils/UI.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cppvcpkg.json
|
Ok the build failures are due to an issue with the actions in our repo's security settings. Since PRs can't run their own build changes, the fixes in this PR won't change the CI results. I'll likely merge this in and monitor for any additional build failures. |
vcpkg commit a44707152fab changed X_VCPKG_RECURSIVE_DATA from
single-line to multi-line JSON format:
X_VCPKG_RECURSIVE_DATA={
"vcpkg-root-env": "D:\\a\\..."
}
lukka/run-cmake@v10 parses vcpkg's env output line-by-line, so it only
captures { — which vcpkg then rejects as malformed JSON when cmake runs.
lukka/run-cmake@5d55ea7 — the commit
from today (March 26, 2026) that contains "consume lib v4.1.11", which
is the final fix for issue community-shaders#155
|
Upstream run-cmake had some fixes. This may be resolved. |
|
✅ A pre-release build is available for this PR: |
(cherry picked from commit 6e897fd)
3 participants
Summary by CodeRabbit
Chores
Bug Fixes / Improvements