fix: persist screen effects settings per game#1108
fix: persist screen effects settings per game#1108xXJSONDeruloXx wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughCentralized screen-effects configuration: added ScreenEffectsConfig with load/persist/apply functions; UI and runtime now read/write this config; PrefManager gained persisted fields for effect parameters and quick-menu tab state; QuickMenu and XServerScreen updated to use the new config flow. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI (Dialog/Panel)"
participant Pref as "PrefManager"
participant Config as "ScreenEffectsConfig"
participant Renderer as "GLRenderer"
participant Composer as "EffectComposer"
rect rgba(200,230,255,0.5)
UI->>Pref: loadScreenEffectsConfig()
Pref-->>UI: returns ScreenEffectsConfig
end
rect rgba(220,255,200,0.5)
UI->>Config: build config from controls
UI->>Pref: persistScreenEffectsConfig(config)
end
rect rgba(255,240,200,0.5)
UI->>Renderer: applyScreenEffectsConfig(renderer, config)
Renderer->>Composer: get/set effects (construct effects based on config)
Composer-->>Renderer: effects applied
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
xXJSONDeruloXx
changed the title
phobos665
left a comment
phobos665
left a comment
There was a problem hiding this comment.
Just FYI if this gets merged, we'll also need to update the "Vivid Effect" PR to also do this behaviour for persistence.
|
@phobos665 yeah I'm prepared to do the conflict resolution, but wanted to separate concerns |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/ui/util/ScreenEffectsConfig.kt (1)
49-56: ReuseColorEffectinstead of recreating it.The live-preview path hits this block frequently, but unlike the other effects it always allocates a fresh
ColorEffectand letssetEffects()tear down the previous one. Reusingcomposer.getEffect(ColorEffect::class.java)here will avoid extra allocation and GL cleanup churn.♻️ Proposed change
- val colorEffect = ColorEffect().apply { + val colorEffect = (composer.getEffect(ColorEffect::class.java) ?: ColorEffect()).apply { brightness = config.brightness / 100f contrast = config.contrast / 100f gamma = config.gamma }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/util/ScreenEffectsConfig.kt` around lines 49 - 56, The ColorEffect is being recreated on each preview update; change the block to reuse the existing instance via composer.getEffect(ColorEffect::class.java) (or create only if null), then set its properties (brightness = config.brightness / 100f, contrast = config.contrast / 100f, gamma = config.gamma) and add that instance to effects instead of instantiating a new ColorEffect(); ensure you still only add it when the abs checks on config.brightness/contrast/gamma indicate a change so behavior with setEffects() remains the same.app/src/main/java/app/gamenative/ui/component/ScreenEffectsPanel.kt (1)
86-132: Extract the shared screen-effects state wiring.These two blocks mirror each other almost field-for-field: load, seven
remembers, config reconstruction, andresetEffects(). The next effect/default change now has to stay in sync in both places, which makes the tab and panel UIs easy to drift apart. A small shared state/helper aroundScreenEffectsConfigwould remove that risk.Also applies to: 222-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/component/ScreenEffectsPanel.kt` around lines 86 - 132, Extract the duplicated state wiring for ScreenEffectsConfig into a single reusable state holder (e.g., a data class or stateful helper like ScreenEffectsState with fields brightness, contrast, gamma, enableToon, enableFXAA, enableCRT, enableNTSC plus methods toConfig(), reset(), and updateFromConfig()). Replace the repeated remember(...) blocks (initialConfig, brightness, contrast, gamma, enableToon, enableFXAA, enableCRT, enableNTSC) with a single remember/rememberSaveable call that returns ScreenEffectsState, load initial values via loadScreenEffectsConfig() into that state, and then use state.toConfig() in LaunchedEffect to call persistScreenEffectsConfig(config) and applyScreenEffectsConfig(renderer, config). Update both locations (the panel and the tab) to use the same helper so resetEffects() becomes state.reset() and the LaunchedEffect observes state fields through state.toConfig() or state snapshot.app/src/main/java/app/gamenative/ui/component/dialog/ScreenEffectDialog.kt (1)
80-91: Don't persist every intermediate slider value.This effect runs on every
brightness/contrast/gammamutation, so dragging one slider will enqueue a large number of preference writes for temporary preview states. KeepapplyScreenEffectsConfig(renderer, config)live, but debouncepersistScreenEffectsConfig(config)or move persistence toonValueChangeFinished/ dismiss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/component/dialog/ScreenEffectDialog.kt` around lines 80 - 91, The LaunchedEffect currently calls persistScreenEffectsConfig on every brightness/contrast/gamma/enable... change which causes excessive writes; keep applyScreenEffectsConfig(renderer, config) immediate for live preview but debounce persistence or move it to user-final events: remove persistScreenEffectsConfig(config) from the LaunchedEffect and either (a) implement a debounced coroutine/handler that waits (e.g., 300–500ms) after the last change before calling persistScreenEffectsConfig(config) keyed to the same state values, or (b) call persistScreenEffectsConfig(config) only from slider onValueChangeFinished and dialog dismiss/confirm handlers so only final values are saved; ensure LaunchedEffect (and its keys: brightness, contrast, gamma, enableToon, enableFXAA, enableCRT, enableNTSC) still calls applyScreenEffectsConfig(renderer, config) immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/component/dialog/ScreenEffectDialog.kt`:
- Around line 80-91: The LaunchedEffect currently calls
persistScreenEffectsConfig on every brightness/contrast/gamma/enable... change
which causes excessive writes; keep applyScreenEffectsConfig(renderer, config)
immediate for live preview but debounce persistence or move it to user-final
events: remove persistScreenEffectsConfig(config) from the LaunchedEffect and
either (a) implement a debounced coroutine/handler that waits (e.g., 300–500ms)
after the last change before calling persistScreenEffectsConfig(config) keyed to
the same state values, or (b) call persistScreenEffectsConfig(config) only from
slider onValueChangeFinished and dialog dismiss/confirm handlers so only final
values are saved; ensure LaunchedEffect (and its keys: brightness, contrast,
gamma, enableToon, enableFXAA, enableCRT, enableNTSC) still calls
applyScreenEffectsConfig(renderer, config) immediately.
In `@app/src/main/java/app/gamenative/ui/component/ScreenEffectsPanel.kt`:
- Around line 86-132: Extract the duplicated state wiring for
ScreenEffectsConfig into a single reusable state holder (e.g., a data class or
stateful helper like ScreenEffectsState with fields brightness, contrast, gamma,
enableToon, enableFXAA, enableCRT, enableNTSC plus methods toConfig(), reset(),
and updateFromConfig()). Replace the repeated remember(...) blocks
(initialConfig, brightness, contrast, gamma, enableToon, enableFXAA, enableCRT,
enableNTSC) with a single remember/rememberSaveable call that returns
ScreenEffectsState, load initial values via loadScreenEffectsConfig() into that
state, and then use state.toConfig() in LaunchedEffect to call
persistScreenEffectsConfig(config) and applyScreenEffectsConfig(renderer,
config). Update both locations (the panel and the tab) to use the same helper so
resetEffects() becomes state.reset() and the LaunchedEffect observes state
fields through state.toConfig() or state snapshot.
In `@app/src/main/java/app/gamenative/ui/util/ScreenEffectsConfig.kt`:
- Around line 49-56: The ColorEffect is being recreated on each preview update;
change the block to reuse the existing instance via
composer.getEffect(ColorEffect::class.java) (or create only if null), then set
its properties (brightness = config.brightness / 100f, contrast =
config.contrast / 100f, gamma = config.gamma) and add that instance to effects
instead of instantiating a new ColorEffect(); ensure you still only add it when
the abs checks on config.brightness/contrast/gamma indicate a change so behavior
with setEffects() remains the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a52a5dbf-cfc3-4a29-8f41-5f943038f7e4
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/QuickMenu.ktapp/src/main/java/app/gamenative/ui/component/ScreenEffectsPanel.ktapp/src/main/java/app/gamenative/ui/component/dialog/ScreenEffectDialog.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/ui/util/ScreenEffectsConfig.kt
| PrefManager.screenEffectsEnableToon = config.enableToon | ||
| PrefManager.screenEffectsEnableFXAA = config.enableFXAA | ||
| PrefManager.screenEffectsEnableCRT = config.enableCRT | ||
| PrefManager.screenEffectsEnableNTSC = config.enableNTSC |
There was a problem hiding this comment.
Out of interest, what happens if you enable multiple of these effects?
There was a problem hiding this comment.
They stack, so like fxaa plus ntsc is nice for antialiasing as an example
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/component/QuickMenu.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/QuickMenu.kt:271">
P2: Direct `PrefManager` read in composable state initialization can crash when `PrefManager.init(...)` has not run (e.g., previews/tests).</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/component/QuickMenu.kt:542">
P2: Initial focus can fail when persisted tab is EFFECTS and renderer is null, leaving the quick menu without a valid focused target.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
xXJSONDeruloXx
force-pushed
the
fix/persist-screen-effects
branch
from
ba8d598 to
9bc3e94
Compare
phobos665
left a comment
phobos665
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing comments.
|
Feel like it actually might be more annoying to have these persist? What if you play two different games in parallel, one which needs effects? |
|
@utkarshdalal per game is a huge can of worms, and this just makes it consistent with perf hud behavior, plus there's a reset button at bottom |
|
Gonna push back a bit on this, why is per-container can of worms? Is it because the container isn't currently passed down into the ScreenEffects file? |
|
The reason for the pushback is that I don't think it's significantly more work to do this in the container, and that seems like the correct way to do it. |
|
I can look into it, just currently we have not tied state of quick menu content to per container, just the global prefmanager. I'll think through it tho, cause things like perf hud in steam deck land are globally defined |
|
Draft till rework as possibly per-game |
xXJSONDeruloXx
force-pushed
the
fix/persist-screen-effects
branch
from
1ecec5f to
48dbf7a
Compare
xXJSONDeruloXx
changed the title
4 participants
Description
previously, perf hid would persist its changes globally but screen effects did not do the same in PrefManager
this fixes so you dont have to re apply screen effects each time. as discussed in discord later we can explore per game profiles but out of scope for how quick menu currently functions
Recording
screen-20260405-113704-under10mb.mp4
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by CodeRabbit
New Features
UI Improvements