refactor(ui): adopt RestartGatedAnnotate helper#41
Merged
Conversation
📝 WalkthroughWalkthroughMigrates several UI controls from DrawSettingDiff to RestartGatedAnnotate, tightens the RestartGatedAnnotate template constraint to accept lvalue-invocable bodies, and documents plus bounds-checks BootSnapshot's memcmp-based pending-change logic. ChangesRestartGatedAnnotate UI migrations and infrastructure updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
There was a problem hiding this comment.
Pull request overview
This PR standardizes restart-gated UI annotations by introducing (and then adopting) Util::UI::RestartGatedAnnotate for previously un-tooltipped restart-required controls in Upscaling and VolumetricLighting, and extends Util::Settings::BootSnapshot to support non-trivially-copyable Settings (with a new regression test).
Changes:
- Add
Util::UI::RestartGatedAnnotatehelper to unify restart-required tooltip suffix + pending-restart banner behavior. - Refactor restart-gated settings UI in
UpscalingandVolumetricLightingto use the helper (preserving existing VR/non-VR gating behavior). - Relax
BootSnapshotto accept copy-assignable settings (not just trivially-copyable) and add a regression test coveringstd::stringdeep-copy behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpp/test_bootsnapshot.cpp | Adds regression coverage for BootSnapshot with non-trivial members (std::string). |
| src/Utils/UI.h | Introduces RestartGatedAnnotate helper and related includes. |
| src/Utils/BootSnapshot.h | Relaxes BootSnapshot constraints and updates Latch implementation accordingly. |
| src/Features/VolumetricLighting.cpp | Adopts RestartGatedAnnotate for VR-only restart-gated toggles. |
| src/Features/Upscaling.cpp | Adopts RestartGatedAnnotate for additional restart-gated controls and normalizes tooltip suffix wording. |
|
✅ A pre-release build is available for this PR: |
alandtse
added a commit
that referenced
this pull request
May 25, 2026
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<Body>` to `std::invocable<Body&>`. 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) <noreply@anthropic.com>
alandtse
force-pushed
the
refactor/restart-tooltip-coverage
branch
from
819b1d8 to
f18fd81
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/Upscaling.cpp (1)
325-329: ⚡ Quick winShorten the PR title to match the commit-title length rule.
Suggested title:
refactor(ui): gate restart hints in upscaling/vl
If this maps to a tracked task, addImplements #<issue>(orAddresses #<issue>) in the PR description.As per coding guidelines "
**/*: ... Format: type(scope): description ... Length: 50 characters limit ... Issue References ...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/Upscaling.cpp` around lines 325 - 329, Update the PR title to follow the commit-title length rule by shortening it to the suggested format (for example use "refactor(ui): gate restart hints in upscaling/vl") and, if this change maps to a tracked task, add "Implements #<issue>" or "Addresses #<issue>" to the PR description; this keeps the PR aligned with the repository naming convention while still referencing the relevant change around upscaling restart hints (see context around Util::HoverTooltipWrapper and the upscaler restart banner logic in Upscaling.cpp).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Features/Upscaling.cpp`:
- Around line 325-329: Update the PR title to follow the commit-title length
rule by shortening it to the suggested format (for example use "refactor(ui):
gate restart hints in upscaling/vl") and, if this change maps to a tracked task,
add "Implements #<issue>" or "Addresses #<issue>" to the PR description; this
keeps the PR aligned with the repository naming convention while still
referencing the relevant change around upscaling restart hints (see context
around Util::HoverTooltipWrapper and the upscaler restart banner logic in
Upscaling.cpp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 971a208e-fcf9-41eb-a9d0-f17f94cb210b
📒 Files selected for processing (4)
src/Features/Upscaling.cppsrc/Features/VolumetricLighting.cppsrc/Utils/BootSnapshot.hsrc/Utils/UI.h
alandtse
added a commit
that referenced
this pull request
May 27, 2026
Conflict in src/Features/Upscaling.cpp (VR Render-at-upscaled-resolution
tooltip): HEAD carried a separate trailing `TextUnformatted("Requires
a game restart to change.")` line that duplicated what DrawSettingDiff
already surfaces via its banner. Took the dev-side single-block phrasing
that folds the restart caveat into the body once and drops the trailing
duplicate line.
Also tightens BootSnapshot::HasPendingChange per Copilot review #41 (cid
3297249367): now `static_assert(std::is_trivially_copyable_v<T>)` on the
registered field type. Without this guard a future caller could register
a std::string field, and the per-field memcmp would compare control
blocks instead of the underlying text -- a silent correctness bug. The
outer SettingsT may still carry non-trivial members (e.g. formula
strings); only registered restart fields are constrained.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Utils/BootSnapshot.h`:
- Around line 115-122: The HasPendingChange(const SettingsT&, const
RestartFieldInfo&) path currently performs a memcmp using
RestartFieldInfo::offset and ::size without validating they lie inside the
SettingsT object; add bounds checks before the memcmp to ensure 0 <= offset,
size > 0, and offset + size <= sizeof(SettingsT) (use size_t and check for
overflow when adding), and if any check fails avoid the memcmp (return false or
treat as no change) and optionally record/log the malformed descriptor; update
only the HasPendingChange overload and any helper that reads the field,
referencing RestartFieldInfo, field.offset, field.size, and memcmp in your fix.
🪄 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: 83a9a388-8674-48b7-bc4d-b0ef388693dc
📒 Files selected for processing (3)
src/Features/Upscaling.cppsrc/Utils/BootSnapshot.hsrc/Utils/UI.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Utils/UI.h
- src/Features/Upscaling.cpp
alandtse
added a commit
that referenced
this pull request
May 27, 2026
…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) <noreply@anthropic.com>
alandtse
changed the title
alandtse
added a commit
that referenced
this pull request
May 29, 2026
…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) <noreply@anthropic.com>
alandtse
force-pushed
the
refactor/restart-tooltip-coverage
branch
from
198926e to
ce371c5
Compare
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) <noreply@anthropic.com>
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<Body>` to `std::invocable<Body&>`. 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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
alandtse
force-pushed
the
refactor/restart-tooltip-coverage
branch
from
359a709 to
aa9b880
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Apply the
Util::UI::RestartGatedAnnotatehelper from #40 to the restart-gated controls inUpscaling,VolumetricLighting, andRenderDocthat were missing standardized tooltips. Two classes of change:1. Surface a restart-required hover tooltip where there was none
Previously these controls had no
IsItemHoveredbranch 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).VolumetricLighting.cpp:ExteriorEnabled,InteriorEnabled. The VR-only conditional gating is preserved (non-VR resizes resources live on toggle; only VR pre-allocates at boot).RenderDoc.cpp:enableCapture. Tooltip body is rendered first via the lambda overload so the hover stays attached to the checkbox; the pending-restart banner now lands below the tooltip instead of between checkbox and tooltip (the prior ordering caused the banner to steal the hover when a change was pending).2. Normalize inline restart wording into the standard suffix
Upscaling.cppenableDLSSperf(DLSSperf VR control): theHoverTooltipWrapperbody no longer carries the restart claim mid-paragraph; the helper appends the canonical "Requires a game restart to change." line after aSpacingseparator. ThedlssAvailable-gated banner is preserved — DLSSperf can't take effect outside DLSS, so a pending banner there would mislead.Sites intentionally left as-is
DynamicCubemaps.cppSSR (no restart claim today, isVR-gated diff only — not actually restart-required in non-VR).VRStereoOptimizations.cppstereoMode (usesUtil::AddTooltipwhich is a different tooltip API; no restart claim in body).ShadowCasterManager/LightLimitFix(handled in a separate adoption onshadow-limit-fix).Dependency
Builds on #40 for the
RestartGatedAnnotatehelper definition. PR #40 merge first; this PR's diff stays clean (just the consumer changes). If reviewed before #40 lands, the inline helper definition shows up as extra context.Test plan
🤖 Generated with Claude Code