From 8803139deff91775cf4a483c08a07fed48bb92e6 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Mon, 25 May 2026 02:21:12 -0700 Subject: [PATCH 1/5] refactor(ui): adopt RestartGatedAnnotate across Upscaling + VL Apply the RestartGatedAnnotate helper from PR #40 to the existing restart-gated controls that were missing standardized tooltips. Two classes of change: 1. Surface a restart-required hover tooltip where there was none. Previously these controls had no IsItemHovered branch at all, so users got no indication that toggling required a restart until they saved and observed no change in-game: - Upscaling.cpp: frameGenerationMode, frameGenerationForceEnable, streamlineLogLevel (Combo). Adopts the suffix-only or suffix-plus-body overload of RestartGatedAnnotate. - VolumetricLighting.cpp: ExteriorEnabled, InteriorEnabled. The VR-only conditional gating is preserved (non-VR resizes resources live on toggle; only VR pre-allocates at boot). 2. Normalize the inline "Restart required to enable/disable" wording into the standard suffix: - Upscaling.cpp: enableDLSSperf (DLSSperf VR control). The HoverTooltipWrapper body no longer carries the restart claim mid- paragraph; the helper appends the canonical "Requires a game restart to change." line after a Spacing separator. The dlssAvailable-gated banner is preserved -- DLSSperf can't take effect outside DLSS, so a pending banner there would mislead. Sites intentionally left as-is: - DynamicCubemaps.cpp SSR (no restart claim today, isVR-gated diff only -- not actually restart-required in non-VR). - VRStereoOptimizations.cpp stereoMode (uses Util::AddTooltip which is a different tooltip API; no restart claim in body). - RenderDoc.cpp enableCapture (debug-only feature, no tooltip at call site). - ShadowCasterManager / LightLimitFix (handled in a separate adoption on shadow-limit-fix). Depends on PR #40 for the RestartGatedAnnotate helper definition. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Features/Upscaling.cpp | 26 +++++++++++++++++--------- src/Features/VolumetricLighting.cpp | 11 +++++++++-- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/Features/Upscaling.cpp b/src/Features/Upscaling.cpp index 660bd9da35..2e025824a4 100644 --- a/src/Features/Upscaling.cpp +++ b/src/Features/Upscaling.cpp @@ -329,6 +329,10 @@ void Upscaling::DrawSettings() ImGui::Checkbox("Render engine at upscaled resolution", &settings.renderAtUpscaleRes); if (!methodSupportsPerf) ImGui::EndDisabled(); + // Hover tooltip always renders (so users learn what the option does + // even when greyed out). The pending-restart banner only fires when + // DLSS is the active upscaler -- the feature can't take effect + // otherwise, so a "pending restart" hint there would mislead. if (auto _tt = Util::HoverTooltipWrapper()) { ImGui::Text( "When enabled, the engine pipeline allocates render targets at the upscaled-render\n" @@ -336,9 +340,8 @@ void Upscaling::DrawSettings() "its output to a private DisplayRes texture. Substantial VRAM and bandwidth savings,\n" "especially at high HMD resolutions.\n" "\n" - "Requires DLSS or FSR. Restart required to enable/disable. Method and Upscale\n" - "Preset changes also require a restart while this is active; sharpness / model preset\n" - "/ Reflex remain live."); + "Requires DLSS or FSR. Method and Upscale Preset changes also require a restart\n" + "while this is active; sharpness / model preset / Reflex remain live."); } if (!methodSupportsPerf && settings.renderAtUpscaleRes) Util::Text::Disabled("Render-at-upscaled-resolution requires DLSS or FSR — switch upscaler Method to activate."); @@ -372,7 +375,10 @@ void Upscaling::DrawSettings() bool fgEnabled = settings.frameGenerationMode != 0; if (ImGui::Checkbox("Frame Generation", &fgEnabled)) settings.frameGenerationMode = fgEnabled ? 1 : 0; - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::frameGenerationMode); + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::frameGenerationMode, + "Interpolate real frames with generated ones for a smoother experience. Uses AMD FSR Frame\n" + "Generation. Requires a D3D11-to-D3D12 proxy swapchain which can introduce compatibility\n" + "issues; in particular, frame generation works only in windowed mode."); if (!frameGenerationDx12PathActive) ImGui::BeginDisabled(); @@ -388,7 +394,10 @@ void Upscaling::DrawSettings() bool fgForce = settings.frameGenerationForceEnable != 0; if (ImGui::Checkbox("Force Enable Frame Generation", &fgForce)) settings.frameGenerationForceEnable = fgForce ? 1 : 0; - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::frameGenerationForceEnable); + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::frameGenerationForceEnable, + "Bypass the high-refresh-rate monitor check so Frame Generation can run on lower-Hz\n" + "displays. Useful for laptops and older monitors at the cost of less headroom for the\n" + "generated frames."); ImGui::Checkbox("Frame Generation in Menus", &settings.frameGenerationAllowInMenus); if (auto _tt = Util::HoverTooltipWrapper()) { @@ -503,10 +512,9 @@ void Upscaling::DrawSettings() if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) { settings.streamlineLogLevel = static_cast(logLevelIdx); } - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::streamlineLogLevel); - if (auto _tt = Util::HoverTooltipWrapper()) { - ImGui::Text("Streamline logging controls the verbosity of NVIDIA Streamline backend logs. Useful for debugging issues with DLSS/DLSS-G."); - } + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::streamlineLogLevel, + "Verbosity of the NVIDIA Streamline backend logs. Useful for debugging issues with DLSS / " + "DLSS-G."); // VR Debug visualization -- per-eye buffers and native inputs if (globals::game::isVR) { diff --git a/src/Features/VolumetricLighting.cpp b/src/Features/VolumetricLighting.cpp index c7f9739194..b6aecff051 100644 --- a/src/Features/VolumetricLighting.cpp +++ b/src/Features/VolumetricLighting.cpp @@ -22,10 +22,15 @@ NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT( void VolumetricLighting::DrawSettings() { + // VR pre-allocates the volumetric lighting render targets once at boot, so + // toggling at runtime won't size them correctly -- the restart-gating + // applies only in VR. The non-VR path resizes resources live on toggle. if (ImGui::Checkbox("Enable Volumetric Lighting in Exteriors", &settings.ExteriorEnabled)) SetupVL(); if (globals::game::isVR) - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::ExteriorEnabled); + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::ExteriorEnabled, + "Volumetric god-rays / fog scattering in exterior cells. In VR the render targets are\n" + "sized at boot, so toggling this requires a restart to take effect."); if (settings.ExteriorEnabled) DrawVolumetricLightingSettings(settings.ExteriorQuality, settings.ExteriorCustomSize, false, !inInterior); @@ -33,7 +38,9 @@ void VolumetricLighting::DrawSettings() if (ImGui::Checkbox("Enable Volumetric Lighting in Interiors", &settings.InteriorEnabled)) SetupVL(); if (globals::game::isVR) - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::InteriorEnabled); + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::InteriorEnabled, + "Volumetric god-rays / fog scattering in interior cells. In VR the render targets are\n" + "sized at boot, so toggling this requires a restart to take effect."); if (settings.InteriorEnabled) DrawVolumetricLightingSettings(settings.InteriorQuality, settings.InteriorCustomSize, true, inInterior); From 6a2472ff6f21a1b538bf72305a98e5ebcaac8aa9 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Mon, 25 May 2026 10:50:06 -0700 Subject: [PATCH 2/5] refactor(ui): address PR review feedback Address 4 Copilot review comments on PR #41: 1. VolumetricLighting.cpp: drop "In VR the render targets are sized at boot, so toggling this requires a restart to take effect" from both Exterior and Interior tooltip bodies. RestartGatedAnnotate already appends the standard "Requires a game restart to change." suffix, so repeating the restart claim in the body produces a redundant tooltip. Tooltip bodies now state only WHAT the option does; WHEN it takes effect is conveyed once via the standard suffix. 2. Util::UI::RestartGatedAnnotate: relax the callable overload's constraint from `std::invocable` to `std::invocable`. The body is invoked as an lvalue inside the function (`body()`), so constraining on the rvalue form could reject valid lvalue-only or move-only callables. The new constraint matches actual usage. 3. Util::Settings::BootSnapshot::HasPendingChange: document the contract that the registered field type T must be POD-shaped (no padding inside sizeof(T)). The per-field memcmp would otherwise see false-positive diffs from uninitialised padding bytes. In practice all registered fields across the codebase are bool/uint32_t/float/enum -- none with meaningful padding -- and the trivially-copyable Settings path uses memcpy that preserves padding verbatim, so the issue is theoretical. Comment makes the contract explicit for future contributors. Comments (3) and (4) target code that was squash-merged into dev via PR #40, but the substantive fixes are small and atomic enough to bundle into this PR rather than spawning a separate cleanup PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Features/VolumetricLighting.cpp | 9 +++++---- src/Utils/BootSnapshot.h | 14 ++++++++++++++ src/Utils/UI.h | 7 ++++++- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/Features/VolumetricLighting.cpp b/src/Features/VolumetricLighting.cpp index b6aecff051..39beae2229 100644 --- a/src/Features/VolumetricLighting.cpp +++ b/src/Features/VolumetricLighting.cpp @@ -25,12 +25,14 @@ void VolumetricLighting::DrawSettings() // VR pre-allocates the volumetric lighting render targets once at boot, so // toggling at runtime won't size them correctly -- the restart-gating // applies only in VR. The non-VR path resizes resources live on toggle. + // RestartGatedAnnotate appends the standard "Requires a game restart to + // change." suffix, so the tooltip bodies stay focused on what the option + // does -- they don't repeat the restart claim. if (ImGui::Checkbox("Enable Volumetric Lighting in Exteriors", &settings.ExteriorEnabled)) SetupVL(); if (globals::game::isVR) Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::ExteriorEnabled, - "Volumetric god-rays / fog scattering in exterior cells. In VR the render targets are\n" - "sized at boot, so toggling this requires a restart to take effect."); + "Volumetric god-rays / fog scattering in exterior cells."); if (settings.ExteriorEnabled) DrawVolumetricLightingSettings(settings.ExteriorQuality, settings.ExteriorCustomSize, false, !inInterior); @@ -39,8 +41,7 @@ void VolumetricLighting::DrawSettings() SetupVL(); if (globals::game::isVR) Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::InteriorEnabled, - "Volumetric god-rays / fog scattering in interior cells. In VR the render targets are\n" - "sized at boot, so toggling this requires a restart to take effect."); + "Volumetric god-rays / fog scattering in interior cells."); if (settings.InteriorEnabled) DrawVolumetricLightingSettings(settings.InteriorQuality, settings.InteriorCustomSize, true, inInterior); diff --git a/src/Utils/BootSnapshot.h b/src/Utils/BootSnapshot.h index 79a3bed2e8..a5224aeeb8 100644 --- a/src/Utils/BootSnapshot.h +++ b/src/Utils/BootSnapshot.h @@ -98,6 +98,20 @@ namespace Util::Settings return *reinterpret_cast(reinterpret_cast(&bootCopy_) + offset); } + // Contract: `T` (the type of the registered restart field, NOT + // `SettingsT`) is expected to be POD-shaped -- bool, integral, float, + // scoped enum, or a struct whose object representation has no + // uninitialised padding. The per-field memcmp compares + // `sizeof(T)` raw bytes; a `T` with padding inside its layout could + // see false-positive diffs if the boot copy and live copy have + // different padding bytes (the standard doesn't require copy + // assignment to copy padding). In practice the registered fields + // across the codebase are bool/uint32_t/float/enum -- none of which + // have meaningful padding -- and the trivially-copyable Settings + // path in Latch uses memcpy that DOES preserve padding bytes + // verbatim, sidestepping the issue entirely. If a future field is a + // padded struct, document it here or constrain via + // `std::has_unique_object_representations_v`. template bool HasPendingChange(const SettingsT& live, T SettingsT::* member) const noexcept { diff --git a/src/Utils/UI.h b/src/Utils/UI.h index 77289ada38..a59605200f 100644 --- a/src/Utils/UI.h +++ b/src/Utils/UI.h @@ -1070,7 +1070,12 @@ namespace Util } template - requires std::invocable + // body is invoked as an lvalue (`body()` below). Constrain on + // `Body&` so a non-const-callable, move-only, or otherwise + // lvalue-only invocable is not falsely rejected -- the prior + // `std::invocable` form tested invocation on a forwarded-as + // expression that may decay to rvalue and reject lvalue-only types. + requires std::invocable inline void RestartGatedAnnotate(const Util::Settings::BootSnapshot& snapshot, const SettingsT& live, T SettingsT::* field, From 546dd5eaf65d0281cf30c4452d8e12d58a735cd7 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Tue, 26 May 2026 20:35:44 -0700 Subject: [PATCH 3/5] fix(ui,bootsnapshot): restore restart wording + bounds-check runtime field PR #41 review pass: - Upscaling.cpp Render-engine-at-upscaled-resolution tooltip lost the "toggling this requires a restart" sentence when the conflict with dev was resolved. The DrawSettingDiff banner only surfaces AFTER a toggle, so a user reading the tooltip before changing the setting would miss the restart requirement. Re-add the sentence in the body so the requirement is visible up-front. (cid 3308256039) - BootSnapshot::HasPendingChange(const SettingsT&, const RestartFieldInfo&) is the runtime-descriptor overload -- field.offset/size come from the RestartTable that, while currently authored statically, is read as generic runtime data. Add overflow-safe bounds checks (size == 0, offset > sizeof(SettingsT), size > sizeof(SettingsT) - offset) before memcmp so a malformed descriptor degrades to "no pending change" instead of an OOB read. Subtraction form avoids size_t wrap when offset+size would overflow. (cid 3308256259) Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Features/Upscaling.cpp | 5 +++-- src/Features/VolumetricLighting.cpp | 8 ++------ src/Utils/BootSnapshot.h | 32 ++++++++++++++++------------- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/Features/Upscaling.cpp b/src/Features/Upscaling.cpp index 2e025824a4..ca13345d69 100644 --- a/src/Features/Upscaling.cpp +++ b/src/Features/Upscaling.cpp @@ -340,8 +340,9 @@ void Upscaling::DrawSettings() "its output to a private DisplayRes texture. Substantial VRAM and bandwidth savings,\n" "especially at high HMD resolutions.\n" "\n" - "Requires DLSS or FSR. Method and Upscale Preset changes also require a restart\n" - "while this is active; sharpness / model preset / Reflex remain live."); + "Requires DLSS or FSR. Toggling this option requires a game restart to take effect.\n" + "While active, Method and Upscale Preset changes also require a restart;\n" + "sharpness / model preset / Reflex remain live."); } if (!methodSupportsPerf && settings.renderAtUpscaleRes) Util::Text::Disabled("Render-at-upscaled-resolution requires DLSS or FSR — switch upscaler Method to activate."); diff --git a/src/Features/VolumetricLighting.cpp b/src/Features/VolumetricLighting.cpp index 39beae2229..b9dc846205 100644 --- a/src/Features/VolumetricLighting.cpp +++ b/src/Features/VolumetricLighting.cpp @@ -22,12 +22,8 @@ NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT( void VolumetricLighting::DrawSettings() { - // VR pre-allocates the volumetric lighting render targets once at boot, so - // toggling at runtime won't size them correctly -- the restart-gating - // applies only in VR. The non-VR path resizes resources live on toggle. - // RestartGatedAnnotate appends the standard "Requires a game restart to - // change." suffix, so the tooltip bodies stay focused on what the option - // does -- they don't repeat the restart claim. + // VR pre-allocates VL render targets at boot, so a runtime toggle can't + // resize them -- gate only in VR. Non-VR resizes live. if (ImGui::Checkbox("Enable Volumetric Lighting in Exteriors", &settings.ExteriorEnabled)) SetupVL(); if (globals::game::isVR) diff --git a/src/Utils/BootSnapshot.h b/src/Utils/BootSnapshot.h index a5224aeeb8..743a771e70 100644 --- a/src/Utils/BootSnapshot.h +++ b/src/Utils/BootSnapshot.h @@ -98,23 +98,17 @@ namespace Util::Settings return *reinterpret_cast(reinterpret_cast(&bootCopy_) + offset); } - // Contract: `T` (the type of the registered restart field, NOT - // `SettingsT`) is expected to be POD-shaped -- bool, integral, float, - // scoped enum, or a struct whose object representation has no - // uninitialised padding. The per-field memcmp compares - // `sizeof(T)` raw bytes; a `T` with padding inside its layout could - // see false-positive diffs if the boot copy and live copy have - // different padding bytes (the standard doesn't require copy - // assignment to copy padding). In practice the registered fields - // across the codebase are bool/uint32_t/float/enum -- none of which - // have meaningful padding -- and the trivially-copyable Settings - // path in Latch uses memcpy that DOES preserve padding bytes - // verbatim, sidestepping the issue entirely. If a future field is a - // padded struct, document it here or constrain via - // `std::has_unique_object_representations_v`. + // T's bytes are memcmp'd, so a padded struct could false-positive on + // compare. All registered restart fields are bool/uint/float/enum (no + // padding); constrain with has_unique_object_representations_v if that changes. template bool HasPendingChange(const SettingsT& live, T SettingsT::* member) const noexcept { + // SettingsT may hold std::string, but a registered restart field must be + // trivially copyable -- memcmp on a std::string compares control blocks, not text. + static_assert(std::is_trivially_copyable_v, + "BootSnapshot::HasPendingChange requires a trivially-copyable field type " + "(memcmp on non-trivial types is not a meaningful equality check)."); if (!latched_) { return false; } @@ -129,6 +123,16 @@ namespace Util::Settings if (!latched_ || !field.jsonKey) { return false; } + // Defensive bounds check on the runtime-supplied descriptor: a + // malformed RestartFieldInfo (size zero, offset past the end, or + // offset+size extending beyond sizeof(SettingsT)) would otherwise + // drive memcmp out of bounds. Subtract instead of add to avoid + // overflow when offset+size would wrap size_t. + if (field.size == 0 || + field.offset > sizeof(SettingsT) || + field.size > sizeof(SettingsT) - field.offset) { + return false; + } return std::memcmp(reinterpret_cast(&bootCopy_) + field.offset, reinterpret_cast(&live) + field.offset, field.size) != 0; From 586c1e87b2bd2ba37403b69c0faeb24fa6aa31c1 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Thu, 28 May 2026 22:19:25 -0700 Subject: [PATCH 4/5] fix(upscaling): clamp streamlineLogLevel before combo index use Copilot review (cid 3321974081): settings.streamlineLogLevel is JSON-persisted, so a stale config or hand-edit could land an out-of-range value (or one that overflows the int cast). Without a clamp ImGui::Combo would index logLevels[] out of bounds. Apply std::clamp to [0, ARRAYSIZE-1] before passing to Combo. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Features/Upscaling.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Features/Upscaling.cpp b/src/Features/Upscaling.cpp index ca13345d69..393a4d019f 100644 --- a/src/Features/Upscaling.cpp +++ b/src/Features/Upscaling.cpp @@ -509,7 +509,11 @@ void Upscaling::DrawSettings() if (ImGui::TreeNodeEx("Backend Diagnostics")) { // Streamline log level selection const char* logLevels[] = { "Off", "Default", "Verbose" }; - int logLevelIdx = static_cast(settings.streamlineLogLevel); + // Clamp before use: streamlineLogLevel is JSON-persisted and could be out + // of range (or a value that overflows the int cast) on a stale or + // hand-edited config; an unclamped value would index logLevels OOB. + int logLevelIdx = std::clamp(static_cast(settings.streamlineLogLevel), + 0, IM_ARRAYSIZE(logLevels) - 1); if (ImGui::Combo("Streamline Logging", &logLevelIdx, logLevels, IM_ARRAYSIZE(logLevels))) { settings.streamlineLogLevel = static_cast(logLevelIdx); } From aa9b880029d02485a96666dbc2a36cc7e6e06230 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Fri, 29 May 2026 22:48:30 -0700 Subject: [PATCH 5/5] refactor(ui): adopt RestartGatedAnnotate for RenderDoc capture enableCapture is restart-gated and already on the BootSnapshot infra, but its hover tooltip never mentioned the restart requirement and the DrawSettingDiff banner was drawn between the checkbox and the tooltip -- so a pending banner stole the hover from the checkbox. Route it through the shared helper: tooltip-first (attaches to the checkbox) with the standard restart suffix, banner after. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Features/RenderDoc.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Features/RenderDoc.cpp b/src/Features/RenderDoc.cpp index 9baa3c1568..d02dfb21ae 100644 --- a/src/Features/RenderDoc.cpp +++ b/src/Features/RenderDoc.cpp @@ -147,12 +147,14 @@ void RenderDoc::DrawSettings() globals::state->frameAnnotations = globals::state->useFrameAnnotations; } } - Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::enableCapture); - - if (auto _tt = Util::HoverTooltipWrapper()) { - ImGui::Text("Enable RenderDoc frame capture for providing debug captures to the Open Shaders team (or upstream Community Shaders for upstream-relevant issues)."); - ImGui::Text("Enabling capture will force-enable frame annotations for easier debugging and will restore the previous setting when disabled."); - } + // enableCapture is restart-gated (renderdoc.dll only injects at boot). The + // helper renders the tooltip first so it attaches to the checkbox, then the + // pending banner below it -- the previous ordering drew the banner between + // the checkbox and the tooltip, so a pending banner would steal the hover. + Util::UI::RestartGatedAnnotate(bootSnapshot, settings, &Settings::enableCapture, [] { + ImGui::TextUnformatted("Enable RenderDoc frame capture for providing debug captures to the Open Shaders team (or upstream Community Shaders for upstream-relevant issues)."); + ImGui::TextUnformatted("Enabling capture will force-enable frame annotations for easier debugging and will restore the previous setting when disabled."); + }); // The rest of the UI renders only when capture is active bool renderDocCaptureEnabled = settings.enableCapture;