feat(menu): add simple mode quality preset system#2216
Closed
alandtse wants to merge 6 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Simple Mode UI (Off/On + quality-preset slider) and a generic preset abstraction with VR-aware dispatch; integrates quality-preset support into ScreenSpaceGI and WetnessEffects; captures boot-state for restart detection; wires a global Menu.SimpleMode setting and a single-page Simple Mode feature list renderer; bumps two feature INI versions. ChangesFeature core + presets + Simple Mode UI
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuUI as "Menu UI (Simple Mode)"
participant Feature
participant PresetSys as "Preset System"
participant Runtime as "Runtime Settings"
User->>MenuUI: toggle feature or move preset slider
MenuUI->>Feature: DrawSimpleSettings() processes input
Feature->>PresetSys: ApplyQualityPreset(level) or ApplyPreset(preset)
PresetSys->>Feature: invoke preset.apply / preset.vrApply
Feature->>Runtime: set enabled flag or ToggleAtBootSetting()
Feature->>Feature: update lastAppliedQualityIdx / simpleModeBootCaptured
Feature->>MenuUI: render tooltip / restart-required indicator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Feature.h (1)
52-65: Minor: state still mutates whenapply/vrApplyare both null.If a registered
QualityPresettier ever hasapply == nullptr(andvrApply == nullptror non-VR),ApplyPresetis a silent no-op but this method still sets*en = trueand updateslastAppliedQualityIdx, leaving the feature "enabled at tier X" without any tier values having been applied. For the current SSGI/WetnessEffects entries this can't happen (tiers always supplyapply), so it's purely a defensive concern for future presets — feel free to ignore if the convention is that quality tiers never use the Custom-sentinel pattern.🛡️ Optional defensive variant
bool ApplyQualityPreset(QualityLevel level) { auto presets = GetQualityPresets(); for (size_t i = 0; i < presets.size(); ++i) { if (presets[i].id == level) { + if (!presets[i].apply && !presets[i].vrApply) + return false; ApplyPreset(this, presets[i]); if (auto* en = GetEnabledFlag()) *en = true; lastAppliedQualityIdx = static_cast<int>(i); return true; } } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.h` around lines 52 - 65, ApplyQualityPreset currently marks the feature enabled and updates lastAppliedQualityIdx even when the matched QualityPreset has no apply/vrApply and ApplyPreset is a no-op; change ApplyQualityPreset to first verify that the preset actually performs work (e.g., check presets[i].apply != nullptr || presets[i].vrApply != nullptr) or make ApplyPreset return a bool indicating success and only then set *GetEnabledFlag() and lastAppliedQualityIdx. Update references to ApplyPreset/QualityPreset/apply/vrApply/GetEnabledFlag/lastAppliedQualityIdx accordingly so state is only mutated when a preset was actually applied.
🤖 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/Feature.cpp`:
- Around line 367-403: The slider logic currently conflates "first preset" and
"Custom" because lastAppliedQualityIdx alone is used to determine UI state; fix
by deriving the active preset from the feature state on render (compare current
runtime/boot settings to each presets entry and set lastAppliedQualityIdx to
matching index or -1 if none), and then map UI values explicitly as 0 = Off,
1..n = preset 0..n-1, and (n+1) = Custom when you need an explicit custom
position (or omit Custom position and show custom by lastAppliedQualityIdx == -1
and currentlyOn true); for n == 0 show a checkbox (use ImGui::Checkbox with
runtimeEnabled / ToggleAtBootSetting) instead of SliderInt; update SliderInt
bounds to 0..n (or 0..n+1 if you include a Custom slot), compute overlay from
the derived lastAppliedQualityIdx or "Off"/"Custom", and in the value-change
handling apply presets via ApplyPreset(this, presets[idx]) and set
lastAppliedQualityIdx accordingly, or call ToggleAtBootSetting()/assign
*runtimeEnabled when toggling; ensure you reference lastAppliedQualityIdx,
currentlyOn, presets, ApplyPreset, ToggleAtBootSetting, runtimeEnabled,
runtimeAvailable and the ImGui::SliderInt/ImGui::Checkbox code paths when making
these changes.
In `@src/Feature.h`:
- Around line 152-170: Feature.h uses std::span in the GetQualityPresets()
signature but does not include <span>, relying on transitive includes; make the
header self-contained by adding an explicit `#include` <span> at the top of
Feature.h so GetQualityPresets() (and any future consumers) do not depend on
indirect includes for std::span.
In `@src/Menu/FeatureListRenderer.cpp`:
- Around line 245-252: The loop currently adds every unloaded feature (if
feat->loaded is false) to issues, causing deleted/obsolete features to show up
in Simple Mode; update the branch so that before pushing feat into issues you
skip any feature that is considered obsolete by the feature API (e.g., call
feat->IsObsolete() or feat->isObsolete()) and only add it when it is not
obsolete — also respect the existing developer-mode behavior used by the
advanced menu (check the same developerMode/IsDeveloper() flag used elsewhere)
so obsolete features remain hidden in Simple Mode but can still surface when
developer mode is enabled.
---
Nitpick comments:
In `@src/Feature.h`:
- Around line 52-65: ApplyQualityPreset currently marks the feature enabled and
updates lastAppliedQualityIdx even when the matched QualityPreset has no
apply/vrApply and ApplyPreset is a no-op; change ApplyQualityPreset to first
verify that the preset actually performs work (e.g., check presets[i].apply !=
nullptr || presets[i].vrApply != nullptr) or make ApplyPreset return a bool
indicating success and only then set *GetEnabledFlag() and
lastAppliedQualityIdx. Update references to
ApplyPreset/QualityPreset/apply/vrApply/GetEnabledFlag/lastAppliedQualityIdx
accordingly so state is only mutated when a preset was actually applied.
🪄 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: 387e8008-bb05-4390-bfd7-6d6108bea431
📒 Files selected for processing (9)
src/Feature.cppsrc/Feature.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Features/WetnessEffects.cppsrc/Menu.cppsrc/Menu.hsrc/Menu/FeatureListRenderer.cppsrc/Menu/SettingsTabRenderer.cpp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Feature.cpp (1)
367-415:⚠️ Potential issue | 🟡 Minor
n == 0still renders a slider, not a checkbox.For features with no quality presets the control is a
SliderIntover0..1with"Off"/"On"overlays (lines 380, 387-389, 399). The PR description states these features should render as a checkbox, and a previous review raised the same concern; the Custom/preset conflation half of that comment has been resolved via the explicitn+1Custom slot, but the checkbox path is still missing. Either branch toImGui::Checkbox(label, &boolView)forn == 0or update the PR description to reflect the unified-slider design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.cpp` around lines 367 - 415, The control currently renders a SliderInt even when there are no presets (n == 0); change the UI to render an ImGui::Checkbox in that case. Replace the SliderInt path when n == 0 with ImGui::Checkbox(label.c_str(), &boolView) where boolView reflects currentlyOn and updates the setting the same way the SliderInt branch does: if runtimeAvailable write through *runtimeEnabled, otherwise call ToggleAtBootSetting(); ensure lastAppliedQualityIdx is set to -1 when turning off and left unchanged when toggling on (no presets to select). Keep SliderInt only for n > 0 and preserve the existing inCustom/sliderMax/currentValue logic for that path.
🧹 Nitpick comments (1)
src/Features/ScreenSpaceGI.cpp (1)
66-106: Single source of truth for tier values.The flat/VR
QualityTiervalues in thekQualityPresetslambdas (lines 70-77) are duplicated in thetiersarray used byDetectCurrentQuality(lines 91-95). A future tweak to one tier risks silently breaking detection (slider would render "Custom" for what is still a real preset). Consider lifting them into a single shared table.♻️ Suggested refactor
namespace { // Per-tier values applied by ApplyTier. struct QualityTier { uint numSlices; uint numSteps; int resolutionMode; bool enableGI; }; void ApplyTier(Feature* f, const QualityTier& q) { auto* ssgi = static_cast<ScreenSpaceGI*>(f); ssgi->settings.Enabled = true; ssgi->settings.NumSlices = q.numSlices; ssgi->settings.NumSteps = q.numSteps; ssgi->settings.ResolutionMode = q.resolutionMode; ssgi->settings.EnableBlur = true; ssgi->settings.EnableGI = q.enableGI; ssgi->recompileFlag = true; } + // Shared tier table — apply lambdas and DetectCurrentQuality both read from this. + // VR High intentionally mirrors flat (vrApply=nullptr -> ApplyPreset falls back). + static constexpr std::array<std::pair<QualityTier, QualityTier>, 3> kTierValues = { { + { { 10, 12, 2, true }, { 1, 6, 2, false } }, // Low + { { 4, 8, 1, true }, { 3, 8, 1, false } }, // Medium + { { 4, 8, 0, true }, { 4, 8, 0, true } }, // High (no VR variant) + } }; + - // VR variants force AO-only (EnableGI=false) for performance; vrApply is null where - // the SE/AE tuning is acceptable on VR and ApplyPreset falls back to apply. static constexpr std::array<Feature::QualityPreset, 3> kQualityPresets = { { { Feature::QualityLevel::Low, "Low", "Quarter resolution, blurred. AO + basic GI.", - [](Feature* f, Feature::QualityLevel) { ApplyTier(f, { 10, 12, 2, true }); }, - [](Feature* f, Feature::QualityLevel) { ApplyTier(f, { 1, 6, 2, false }); } }, + [](Feature* f, Feature::QualityLevel) { ApplyTier(f, kTierValues[0].first); }, + [](Feature* f, Feature::QualityLevel) { ApplyTier(f, kTierValues[0].second); } }, { Feature::QualityLevel::Medium, "Medium", "Half resolution, balanced quality and performance.", - [](Feature* f, Feature::QualityLevel) { ApplyTier(f, { 4, 8, 1, true }); }, - [](Feature* f, Feature::QualityLevel) { ApplyTier(f, { 3, 8, 1, false }); } }, + [](Feature* f, Feature::QualityLevel) { ApplyTier(f, kTierValues[1].first); }, + [](Feature* f, Feature::QualityLevel) { ApplyTier(f, kTierValues[1].second); } }, { Feature::QualityLevel::High, "High", "Full resolution, clean output with full GI.", - [](Feature* f, Feature::QualityLevel) { ApplyTier(f, { 4, 8, 0, true }); }, + [](Feature* f, Feature::QualityLevel) { ApplyTier(f, kTierValues[2].first); }, nullptr }, } }; } @@ int ScreenSpaceGI::DetectCurrentQuality() const { - // SSGI tiers vary by platform — VR uses AO-only variants. Each tier matches when - // its critical knobs (slices, steps, resolution mode, GI on/off) all match. const bool isVR = REL::Module::IsVR(); - static constexpr std::array<std::pair<QualityTier, QualityTier>, 3> tiers = { { - { { 10, 12, 2, true }, { 1, 6, 2, false } }, // Low (flat / VR) - { { 4, 8, 1, true }, { 3, 8, 1, false } }, // Medium - { { 4, 8, 0, true }, { 4, 8, 0, true } }, // High (no VR variant — uses flat) - } }; - for (size_t i = 0; i < tiers.size(); ++i) { - const auto& q = isVR ? tiers[i].second : tiers[i].first; + for (size_t i = 0; i < kTierValues.size(); ++i) { + const auto& q = isVR ? kTierValues[i].second : kTierValues[i].first; if (settings.NumSlices == q.numSlices && settings.NumSteps == q.numSteps && settings.ResolutionMode == q.resolutionMode && settings.EnableGI == q.enableGI && settings.EnableBlur) return static_cast<int>(i); } return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceGI.cpp` around lines 66 - 106, The preset QualityTier values are duplicated between kQualityPresets (the ApplyTier lambdas) and the local tiers array in DetectCurrentQuality; extract a single constexpr array (e.g., kQualityTiers) of QualityTier pairs (flat, VR) and have kQualityPresets' lambdas call ApplyTier using entries from that array (matching by index) and have DetectCurrentQuality reference the same kQualityTiers for matching; ensure ordering matches (Low, Medium, High) and that the isVR branch in DetectCurrentQuality uses the pair.second for VR and pair.first for flat so detection and application remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Feature.cpp`:
- Around line 367-415: The control currently renders a SliderInt even when there
are no presets (n == 0); change the UI to render an ImGui::Checkbox in that
case. Replace the SliderInt path when n == 0 with ImGui::Checkbox(label.c_str(),
&boolView) where boolView reflects currentlyOn and updates the setting the same
way the SliderInt branch does: if runtimeAvailable write through
*runtimeEnabled, otherwise call ToggleAtBootSetting(); ensure
lastAppliedQualityIdx is set to -1 when turning off and left unchanged when
toggling on (no presets to select). Keep SliderInt only for n > 0 and preserve
the existing inCustom/sliderMax/currentValue logic for that path.
---
Nitpick comments:
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 66-106: The preset QualityTier values are duplicated between
kQualityPresets (the ApplyTier lambdas) and the local tiers array in
DetectCurrentQuality; extract a single constexpr array (e.g., kQualityTiers) of
QualityTier pairs (flat, VR) and have kQualityPresets' lambdas call ApplyTier
using entries from that array (matching by index) and have DetectCurrentQuality
reference the same kQualityTiers for matching; ensure ordering matches (Low,
Medium, High) and that the isVR branch in DetectCurrentQuality uses the
pair.second for VR and pair.first for flat so detection and application remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccebd8e2-a13c-485f-9721-0e2ede7e445a
📒 Files selected for processing (5)
src/Feature.cppsrc/Feature.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Menu/FeatureListRenderer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Menu/FeatureListRenderer.cpp
|
✅ A pre-release build is available for this PR: |
Collaborator
Author
|
Taking to draft since this is for May. |
alandtse
added
deferred
alandtse
force-pushed
the
feat/simple-mode-quality-presets
branch
from
aae0131 to
83c0d5f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Feature.cpp (1)
387-399:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a checkbox when a feature has no quality presets.
Line 387 still routes the
n == 0case through a 0/1SliderInt. That keeps the old simple-mode mismatch alive; preset-less features are supposed to render as a checkbox.Proposed fix
- int v = currentValue; - if (ImGui::SliderInt(label.c_str(), &v, 0, sliderMax, overlay.c_str(), ImGuiSliderFlags_AlwaysClamp)) { + int v = currentValue; + if (n == 0) { + bool enabled = currentlyOn; + if (ImGui::Checkbox(label.c_str(), &enabled)) { + if (runtimeAvailable) + *runtimeEnabled = enabled; + else + ToggleAtBootSetting(); + } + } else if (ImGui::SliderInt(label.c_str(), &v, 0, sliderMax, overlay.c_str(), ImGuiSliderFlags_AlwaysClamp)) { const bool wantOn = v >= 1; if (currentlyOn != wantOn) { if (runtimeAvailable) *runtimeEnabled = wantOn; else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.cpp` around lines 387 - 399, The n == 0 branch is still driving a 0/1 ImGui::SliderInt instead of a checkbox for preset-less features; change the UI path so when n == 0 you render an ImGui::Checkbox rather than calling ImGui::SliderInt. Concretely, before the ImGui::SliderInt call detect the preset-less case (the same condition used earlier: n == 0), create a temporary bool (e.g., checked = currentValue == 1), call ImGui::Checkbox(label.c_str(), &checked), and if the checkbox state changed update currentValue = checked ? 1 : 0; otherwise keep the existing SliderInt usage for non-empty presets (the existing ImGui::SliderInt call with sliderMax, overlay, label, and currentValue mapping remains unchanged). Ensure you reference the same variables used now: n, currentValue, inCustom, lastAppliedQualityIdx, presets, overlay, label and sliderMax.
🤖 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/Feature.cpp`:
- Around line 428-434: The code reads presets[currentValue - 1] without ensuring
currentValue refers to a preset index (it can be n+1 for "Custom"); update the
guard to prevent out-of-bounds by either changing the condition to require
currentValue >= 1 && currentValue <= n or by explicitly checking (currentValue -
1) < presets.size() before accessing presets[currentValue - 1]. Apply this check
around the block that fetches desc (the usage of currentValue, n, presets and
desc) so the tooltip lookup is skipped for the Custom slot.
---
Duplicate comments:
In `@src/Feature.cpp`:
- Around line 387-399: The n == 0 branch is still driving a 0/1 ImGui::SliderInt
instead of a checkbox for preset-less features; change the UI path so when n ==
0 you render an ImGui::Checkbox rather than calling ImGui::SliderInt.
Concretely, before the ImGui::SliderInt call detect the preset-less case (the
same condition used earlier: n == 0), create a temporary bool (e.g., checked =
currentValue == 1), call ImGui::Checkbox(label.c_str(), &checked), and if the
checkbox state changed update currentValue = checked ? 1 : 0; otherwise keep the
existing SliderInt usage for non-empty presets (the existing ImGui::SliderInt
call with sliderMax, overlay, label, and currentValue mapping remains
unchanged). Ensure you reference the same variables used now: n, currentValue,
inCustom, lastAppliedQualityIdx, presets, overlay, label and sliderMax.
🪄 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 Plus
Run ID: 75a1100b-ea6e-4bf5-95d5-569557ddc785
📒 Files selected for processing (9)
src/Feature.cppsrc/Feature.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Features/WetnessEffects.cppsrc/Menu.cppsrc/Menu.hsrc/Menu/FeatureListRenderer.cppsrc/Menu/SettingsTabRenderer.cpp
✅ Files skipped from review due to trivial changes (3)
- src/Feature.h
- src/Features/ScreenSpaceGI.cpp
- src/Menu/SettingsTabRenderer.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Features/ScreenSpaceGI.h
- src/Menu/FeatureListRenderer.cpp
- src/Features/WetnessEffects.cpp
Collaborator
|
Isn't bottle working on something like this already? |
Collaborator
Author
|
The architecture is what's important here. His is all implementation. This has a simple example MVP and can be replaced by whatever he decides. |
Adds a shared Preset<E> template on Feature plus a global QualityLevel enum for the simple menu. SSGI declares Low/Medium/High tiers (with VR variants) via the new system. WetnessEffects refactored so its existing ClimatePreset combo routes through the same Feature::ApplyPreset dispatch via one shared thunk. Simple mode hides the feature list and renders a single scrollable page. Each loaded feature renders inline as a slider (Off / preset names / Custom) when it has quality presets, or as a checkbox otherwise. The on/off uses the runtime GetEnabledFlag() when overridden, otherwise falls back to ToggleAtBootSetting() — features on the boot path get their label tinted with StatusPalette.RestartNeeded and a "(restart required)" hint. Features that failed to load appear under "Installation Issues" colored with StatusPalette.Error and the failure message in the tooltip. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add #include <span> to Feature.h so the header is self-contained. - Add Feature::DetectCurrentQuality() virtual hook; DrawSimpleSettings calls it on first render so a feature whose JSON-loaded settings match a preset starts at that preset instead of "Custom". Override on ScreenSpaceGI by matching the (slices, steps, resolutionMode, enableGI) tuple against the SE/AE and VR tier values. - Make "Custom" a separate slider stop at n+1 (only present while actually custom). Without this, Custom and the first preset shared position 1 and clicking the first preset failed to fire SliderInt's change event. - Skip obsolete features whose INI was removed from the Installation Issues list, matching the advanced menu's filter; surfaced again in developer mode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Replace SliderInt with Checkbox for features with no quality presets (n == 0 path), matching the documented intent of the simple mode UI - Guard preset tooltip lookup against out-of-bounds access when slider is in Custom state (currentValue == n+1) - Bump ScreenSpaceGI to 4.2.0, WetnessEffects to 3.2.0 per bot audit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
alandtse
force-pushed
the
feat/simple-mode-quality-presets
branch
from
83c0d5f to
fcae96a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Features/ScreenSpaceGI.cpp (1)
68-78: ⚡ Quick winUse
kQualityPresetsas the single source of truth for shared tiers.These Low/Medium/High values now live here and in the advanced preset buttons below, so the simple and advanced menus can drift independently. Reusing
kQualityPresetsfor the shared buttons would keep both UIs aligned and avoid retuning the same numbers twice.🤖 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/ScreenSpaceGI.cpp` around lines 68 - 78, The Low/Medium/High tier data duplicated in the simple and advanced UI should be sourced from the existing kQualityPresets array so both menus stay in sync: update the simple shared-button creation and the advanced-preset button code to read the tier parameters (and labels/descriptions if used) from kQualityPresets[n] rather than hardcoding numbers; use the Feature::QualityPreset entries (accessing their Apply lambdas or stored tier data) and, for entries where the advanced lambda is nullptr (e.g., High), provide a safe fallback (call the default ApplyTier or reuse the simple lambda) so the UI wiring (button callbacks that previously called ApplyTier with literals) now delegates to kQualityPresets to remain the single source of truth.
🤖 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/Feature.cpp`:
- Around line 351-356: The restartPending calculation wrongly depends on the
first Simple Mode render captured in
simpleModeBootInitial/simpleModeBootCaptured; instead compute restartPending
from the actual live vs next-boot stored state. Remove the capture logic
(simpleModeBootInitial/simpleModeBootCaptured) and replace restartPending with a
comparison between the current runtime setting and the persisted next-boot
setting (use globals::state->IsFeatureDisabled(shortName) vs the state API that
returns the next-boot/boot-time value for shortName — or add such an accessor if
missing) so restartPending = !runtimeAvailable && (currentIsDisabled !=
nextBootIsDisabled).
In `@src/Menu/FeatureListRenderer.cpp`:
- Around line 247-253: The loop currently adds every unloaded feature
(feat->loaded == false) to issues, which incorrectly flags features the user
intentionally disabled at boot; update the else-if condition that pushes to
issues (the block using FeatureIssues::IsObsoleteFeature(feat->GetShortName()))
to first exclude boot-disabled features by checking the feature's boot-disabled
state (e.g., feat->IsBootDisabled() or feat->bootDisabled) and only push to
issues when the feature is neither loaded nor boot-disabled and not obsolete
(preserve the existing devMode behavior and DrawSimpleSettings/ImGui::PushID
usage).
---
Nitpick comments:
In `@src/Features/ScreenSpaceGI.cpp`:
- Around line 68-78: The Low/Medium/High tier data duplicated in the simple and
advanced UI should be sourced from the existing kQualityPresets array so both
menus stay in sync: update the simple shared-button creation and the
advanced-preset button code to read the tier parameters (and labels/descriptions
if used) from kQualityPresets[n] rather than hardcoding numbers; use the
Feature::QualityPreset entries (accessing their Apply lambdas or stored tier
data) and, for entries where the advanced lambda is nullptr (e.g., High),
provide a safe fallback (call the default ApplyTier or reuse the simple lambda)
so the UI wiring (button callbacks that previously called ApplyTier with
literals) now delegates to kQualityPresets to remain the single source of truth.
🪄 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 Plus
Run ID: d4850bd1-7d3a-413e-bf35-c7b6c8df80d3
📒 Files selected for processing (11)
features/Screen Space GI/Shaders/Features/ScreenSpaceGI.inifeatures/Wetness Effects/Shaders/Features/WetnessEffects.inisrc/Feature.cppsrc/Feature.hsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceGI.hsrc/Features/WetnessEffects.cppsrc/Menu.cppsrc/Menu.hsrc/Menu/FeatureListRenderer.cppsrc/Menu/SettingsTabRenderer.cpp
✅ Files skipped from review due to trivial changes (2)
- features/Wetness Effects/Shaders/Features/WetnessEffects.ini
- features/Screen Space GI/Shaders/Features/ScreenSpaceGI.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/ScreenSpaceGI.h
Reverts the checkbox change from the previous commit. The slider provides a uniform look across all features in simple mode regardless of preset count, which is preferable to mixing widget types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract a single constexpr kTierValues array shared by both kQualityPresets lambdas and DetectCurrentQuality, so a change to one tier cannot silently diverge and cause preset detection to return Custom for a setting that still matches a real preset. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove simpleModeBootCaptured/simpleModeBootInitial members; loaded features were enabled at boot by definition, so restartPending simplifies to !runtimeAvailable && IsFeatureDisabled(shortName) - Show boot-disabled features as an Off slider in simple mode instead of incorrectly routing them to the Installation Issues section - Advanced SSGI Low/Standard/Extreme preset buttons now delegate to ApplyTier(kTierValues[N]) instead of inlining the same literals Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Adds a shared Preset template on Feature plus a global QualityLevel enum for the simple menu. SSGI declares Low/Medium/High tiers (with VR variants) via the new system. WetnessEffects refactored so its existing ClimatePreset combo routes through the same Feature::ApplyPreset dispatch via one shared thunk.
Simple mode hides the feature list and renders a single scrollable page. Each loaded feature renders inline as a slider (Off / preset names / Custom) when it has quality presets, or as a checkbox otherwise. The on/off uses the runtime GetEnabledFlag() when overridden, otherwise falls back to ToggleAtBootSetting() — features on the boot path get their label tinted with StatusPalette.RestartNeeded and a "(restart required)" hint. Features that failed to load appear under "Installation Issues" colored with StatusPalette.Error and the failure message in the tooltip.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
Summary by CodeRabbit
New Features
Improvements