refactor: unify restart-required infrastructure#39
Conversation
alandtse
left a comment
There was a problem hiding this comment.
@codex Thanks for the infrastructure work — BootSnapshot + RestartFieldInfo + MCP introspection + Upscaling canary look solid and CodeQL is green. A few items from the issue are still outstanding before this can come out of WIP. Please address:
1. PR title — conventional commit format
[WIP] Add unified restart-required settings infrastructure doesn't parse as a conventional commit. The repo squash-merges and semantic-release reads the PR title, so the title becomes the commit message. Please retitle to match the issue:
feat(settings): unified restart-required infrastructure + MCP introspection
(Drop the [WIP] prefix when ready to merge.)
2. Missed refactor sites from the issue's enumerated list
The issue's site list was authored against a branch that had DlssEnhancer/Bridge — those don't exist on dev, so ignore those entries. But these are still present on dev and untouched:
Features/DynamicCubemaps— explicitly enumerated in the issue.src/Features/DynamicCubemaps.cpp:33and:183,src/Features/DynamicCubemaps.h:122still use the ad-hocenabledAtBootpattern. Please convert to akRestartFieldstable +BootSnapshot<Settings>(or document why this site stays manual — VR-only gating may complicate it).- Static "Requires restart" hints not converted to pending-diff banners at
src/Features/Upscaling.cppBackend Diagnostics (streamlineLogLevel, the issue's line-508 site) and the Method combo's restart tooltip (issue's line 309). Add these fields tokRestartFieldsor document why they stay static. - Raw
TextColored(StatusPalette.RestartNeeded, ...)callsites the issue flagged for migration toUtil::Text::RestartNeededregardless:src/Features/RenderDoc.cpp:158,src/Features/VRStereoOptimizations.cpp:264, plussrc/Features/WeatherEditor.cpp:606andsrc/WeatherEditor/EditorWindow.cpp:362(same pattern, not in the issue list). - The remaining sites (
UnifiedWater.cpp,VR/SettingsUI.cpp,Menu/FeatureListRenderer.cpp) were flagged in the issue as "may stay static." Defensible to defer, but please call them out as deferred follow-ups in the PR body so future readers know they were considered.
3. Latent sibling sites with the same pattern
These weren't in the issue but match the enabledAtBoot shape and are worth converting (or at least acknowledging) in the same PR:
src/Features/VolumetricLighting.h:25src/Features/Upscaling/Streamline.h:31
4. Ordering invariant in kRestartFields is silently load-bearing
src/Features/Upscaling.h:130-137 — GetRestartRequiredFields() returns a prefix kRestartFields.data() ... +baseCount+extra. The first 3 entries must be the always-restart fields; the last 2 must be the DLSSperf-conditional ones. Reordering the array silently breaks MCP's list_restart_required and the pending-diff banners. Please either:
- split into two tables (
kAlwaysRestartFields+kDlssPerfRestartFields) and concat the spans, or - filter by predicate at runtime, or
- at minimum add a
// ORDER MATTERS:comment pinning the layout.
5. PR body scope note
list_pending_restart walks every loaded feature, but since only Upscaling overrides GetRestartRequiredFields() so far, MCP only returns Upscaling fields. Please add a line to the PR body noting that DynamicCubemaps / VolumetricLighting / Streamline conversions are follow-ups (or land them here per item 2/3), so users don't expect global coverage from day one.
Once these are addressed and the title is fixed, this should be ready for review. Ping me when you've pushed updates.
Addressed the outstanding refactor sites + ordering concerns in commit
PR title: I can’t retitle the PR from this environment; please update it to |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified, reusable “restart-required settings” infrastructure (metadata table + boot snapshot diffing) and wires it into UI helpers and MCP/RemoteControl introspection, then migrates several features (notably Upscaling + VR-only toggles) onto the new mechanism.
Changes:
- Add
Util::Settings::RestartFieldInfo/RestartTableandUtil::Settings::BootSnapshotto latch “boot” settings and diff against current selections. - Add UI helpers for consistent “Pending restart” banners and update multiple UIs to use
Util::Text::RestartNeeded. - Extend RemoteControl MCP “feature” tool with
list_restart_requiredandlist_pending_restart, and add unit tests for BootSnapshot.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpp/test_bootsnapshot.cpp | Adds Catch2 unit tests covering unlatch/latch behavior, diffs, and field lookup for BootSnapshot. |
| tests/cpp/CMakeLists.txt | Adds the new BootSnapshot test to the cpp_tests target. |
| src/WeatherEditor/EditorWindow.cpp | Formatting cleanup and switches an “Active:” label to Util::Text::RestartNeeded. |
| src/Utils/UI.h | Adds restart-required UI helpers (DrawSettingDiff, DrawPendingBanners) and includes BootSnapshot. |
| src/Utils/RestartSettings.h | Introduces restart-field metadata (RestartFieldInfo, RestartTable) + helper macro and lookup. |
| src/Utils/BootSnapshot.h | Adds boot-latching + memcmp-based diffing + member metadata lookup for restart-gated settings. |
| src/Hooks.cpp | Latches Upscaling boot snapshot early during render-target creation initialization. |
| src/Features/WeatherEditor.cpp | Replaces custom colored text with Util::Text::RestartNeeded for wind relation display. |
| src/Features/VRStereoOptimizations.cpp | Replaces manual RestartNeeded coloring with Util::Text::RestartNeeded. |
| src/Features/VolumetricLighting.h | Adds restart-field table + boot snapshot + Feature introspection overrides (VR-only gating). |
| src/Features/VolumetricLighting.cpp | Draws per-setting restart diffs in VR and latches snapshot in PostPostLoad. |
| src/Features/Upscaling/Streamline.h | Removes an unused boot-latch flag from Streamline state. |
| src/Features/Upscaling/Streamline.cpp | Switches DLSS qualityMode boot selection to use Upscaling’s bootSnapshot. |
| src/Features/Upscaling/DLSSperf.h | Removes DLSSperf’s bespoke boot snapshot storage/accessors. |
| src/Features/Upscaling/DLSSperf.cpp | Ensures Upscaling boot snapshot is latched robustly even if call order changes. |
| src/Features/Upscaling.h | Adds restart-field table + boot snapshot + Feature introspection overrides for Upscaling settings. |
| src/Features/Upscaling.cpp | Replaces bespoke restart-required UI logic with BootSnapshot/UI helpers; adds qualityMode range clamp on load. |
| src/Features/RenderDoc.cpp | Uses Util::Text::RestartNeeded for the “requires restart” capture hint. |
| src/Features/RemoteControl.cpp | Adds restart-required/pending-restart MCP actions and byte/hex reporting. |
| src/Features/DynamicCubemaps.h | Adds restart-field table + boot snapshot + Feature introspection overrides (VR-only gating). |
| src/Features/DynamicCubemaps.cpp | Uses boot snapshot diffing for VR SSR “pending restart” messaging; latches in PostPostLoad. |
| src/Feature.h | Adds virtual introspection surface for restart-required fields + boot/live blob accessors. |
| features/Weather Editor/Shaders/Features/WeatherEditor.ini | Bumps WeatherEditor feature version. |
| features/Volumetric Lighting/Shaders/Features/VolumetricLighting.ini | Bumps VolumetricLighting feature version. |
| features/Dynamic Cubemaps/Shaders/Features/DynamicCubemaps.ini | Bumps DynamicCubemaps feature version. |
| .claude/CLAUDE.md | Documents the new restart-gated settings mechanism and points to Upscaling as a canary. |
Comments suppressed due to low confidence (2)
src/Features/Upscaling.cpp:306
- DLSS Model Preset is shown with DrawSettingDiff + a tooltip saying it "requires a restart", but the backend applies presetDLSS at runtime in Streamline::SetDLSSOptions (called every frame). This banner/text will incorrectly tell users (and MCP) that the change won't apply until restart. Update the UI copy and/or remove the restart-diff rendering for this field unless you intentionally make the preset boot-latched.
const char* presets[] = { "Default", "Preset J", "Preset K", "Preset L", "Preset M" };
ImGui::Combo("DLSS Model Preset", (int*)&settings.presetDLSS, presets, 5);
Util::UI::DrawSettingDiff(bootSnapshot, settings, &Settings::presetDLSS);
if (auto _tt = Util::HoverTooltipWrapper()) {
ImGui::Text("Choose which DLSS AI model preset to use.");
ImGui::Text("Each model offers different visual quality, performance, and motion stability.");
ImGui::Text("Set to 'Default' for automatic selection based on your Upscale Preset and hardware.");
ImGui::Text("Changing this setting requires a restart to take effect.");
}
src/Features/RemoteControl.cpp:587
- The MCP tool schema describes shortName as "Required for all actions except 'list'", but this PR makes it optional for list_restart_required and list_pending_restart. Update the parameter description to match the actual behavior so clients don't treat shortName as mandatory for these actions.
.with_string_param("action",
"One of: 'list', 'list_restart_required', "
"'list_pending_restart', 'get', 'set', 'reset', 'toggle'.")
.with_string_param("shortName",
"Required for all actions except 'list'. From the "
"list response.",
/*required=*/false)
|
✅ A pre-release build is available for this PR: |
…tion Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com>
Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com>
Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com>
The three-table split (kBootSnapshotFields + kAlwaysRestartFields + kDlssPerfRestartFields) violated the issue's "single point of truth per field" goal and reintroduced the v2-deferred conditional-discovery mechanism. The per-widget banner gating in DrawSettings already conditions on dlssPerf.IsHookActive() at the call site, so the discovery table doesn't need its own gating — MCP can report all restart-gated fields and clients can check feature state. Drops the mutable runtime concat buffer along with it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No behavior change. Pre-commit hook surfaced legacy formatting drift when touching this file in the next commit; landing the format pass separately so the logic diff stays minimal. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The earlier TextColored→Util::Text::RestartNeeded conversion left WeatherEditor::DisplayWindInfo with an unused `theme` local (treated as error under /W4) and removed EditorWindow::ShowObjectsWindow's `theme` declaration while leaving a later `theme.Palette.Text` reference dangling. Drop the unused WeatherEditor local; restore the EditorWindow local so the surviving caller compiles. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Feature Version Audit flagged these as needing bumps because the restart-required infrastructure work touched their feature classes. - Dynamic Cubemaps: 2-3-1 → 2-4-0 (BootSnapshot integration) - Volumetric Lighting: 1-1-0 → 1-2-0 (BootSnapshot integration) - Weather Editor: 2-0-1 → 2-1-0 (RestartNeeded helper migration) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Latch via memcpy so padding bytes copy verbatim. Assignment copies the object representation for trivially-copyable types per the standard, but memcpy removes any compiler latitude and matches the field-slice memcmp comparisons in HasPendingChange. - Test name typo: "starts unlatching" -> "starts unlatched". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move loose enableRenderDocCapture/captureFrameCount fields into a
proper Settings struct so the boot-snapshot pattern can hook in.
Replace the static "Requires restart to enable..." line with the
DrawSettingDiff banner. The disable path keeps its performance-impact
warning since that's load-bearing context the banner doesn't carry.
JSON keys are unchanged ("Enable RenderDoc Capture", "Capture Frame
Count"), so existing user configs round-trip cleanly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
stereoMode is restart-gated (stencil/CS resources are only set up during VR's SetupResources hook). Latch from VR::PostPostLoad and replace the static "Restart is required..." hint with DrawSettingDiff so the user sees the active vs selected values. VRStereoOptimizations isn't itself a Feature subclass (it's a child of the VR feature), so MCP discovery via GetRestartRequiredFields doesn't pick this up. UI behavior still works through the bootSnapshot helper; a follow-up could expose VR's restart-gated sub-fields via the VR Feature's discovery override. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ant hints - presetDLSS is runtime-effective: Streamline::SetDLSSOptions reads settings.presetDLSS per-frame and applies it via slDLSSSetOptions. Treating it as restart-gated was misleading the UI and MCP discovery. (Catch from Copilot review.) - Remove two static "Changing this setting requires a restart" lines next to the DLSS Model Preset and Streamline Logging combos; their pending-diff DrawSettingDiff banners (where applicable) cover the same signal without the duplicate hint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This PR is now a refactor with no behavior change for end users — settings shapes are unchanged, JSON round-trips unchanged. Bumping feature versions implies the per-feature settings schema changed, which it didn't. Revert the bumps back to their dev-branch values. - Dynamic Cubemaps: 2-4-0 -> 2-3-1 - Volumetric Lighting: 1-2-0 -> 1-1-0 - Weather Editor: 2-1-0 -> 2-0-1 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Catch2 doesn't link the std::string_view StringMaker by default, so `REQUIRE(std::string_view(...) == "literal")` fails to link with LNK2001. Cast to std::string instead. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
913ec49 to
2ae2836
Compare
alandtse
left a comment
There was a problem hiding this comment.
See comments. The REL::Module::IsVR should be checked throughout.
Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com>
Follow-up in dd972af: standardized VR-only restart gating to use PR title: I still can’t retitle from this environment; please rename to |
Per the minimal-but-semantically-rich design philosophy
(EdenLabs/agentic-renderdoc), drop the two new `list_restart_required`
and `list_pending_restart` actions and fold their information into
the existing `list` response.
Each feature entry now carries an optional `restartFields` array of
`{ key, label, pending }` — `pending=true` means the user has staged
a change that won't apply until next launch. One tool call answers
"what features exist", "which fields are restart-gated", and "what's
currently pending"; clients filter as needed.
Drops the BytesToHex helper that the old list_pending_restart used to
emit raw boot/live bytes. Clients that need typed values for diffing
can use `get` to read live settings; the staged value is right there.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When a loaded feature has a restart-gated setting whose live value differs from the boot-latched value, paint its name in the feature list with the same StatusPalette.RestartNeeded green that already signals "INI exists but feature not loaded — toggled off at boot." Same color, same meaning from the user's POV: "this feature has unmade changes that take effect on restart." Adds `Feature::HasAnyPendingRestart()` for the per-frame check — short-circuits to false for features with no restart-gated fields, so the per-frame cost is one virtual call + an empty-span check for the vast majority of features. Features with restart-gated fields walk their table doing field-slice memcmp, same data path the MCP `list` response uses for its per-field `pending` flag. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brings in: - 045e72e refactor: unify restart-required infrastructure (#39) - 5c00a38 build: drop /XO from robocopy auto-deploy (#37) The /XO drop is identical to our local cc65edc -- merge auto-resolved cleanly. The restart-required infrastructure (BootSnapshot, RestartSettings) will be adopted into ShadowCasterManager in a follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.