search

Tyrrrz / LightBulb

Reduces eye strain by adjusting screen gamma based on the current time
MIT License
2.23k stars 141 forks source link

Add customizable max transition duration for settings changes #291

Closed LilithSilver closed 4 months ago

LilithSilver commented 4 months ago

Closes #290 by implementing a new setting, a max duration for settings config changes. If the duration of a transition would exceed the maximum duration allowed, it speeds up the step size of the transition to reach the target duration instead. This applies to all sliders, improving their smooth behavior to make them more responsive without sacrificing the smoothing.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.35%. Comparing base (a9eb2f7) to head (427a280). Report is 9 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #291 +/- ## ======================================= Coverage 96.35% 96.35% ======================================= Files 7 7 Lines 192 192 Branches 15 15 ======================================= Hits 185 185 Misses 6 6 Partials 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Tyrrrz commented 4 months ago

Hi, thanks for the contribution.

I'm apprehensive about adding this as a fully supported setting in the app due to maintainability reasons, but I'm okay with making it a "configurable constant" that you can change by editing the settings file directly.

Is that fine with you? Otherwise you're always welcome to keep using the fork with your own changes as well.

LilithSilver commented 4 months ago

Yep, that's totally fine by me, makes sense! I'll update the code later today.

What's a reasonable default value? I can restore the old behavior by making it 15s, or use a lower value to make sliders and configs more responsive by default.

Tyrrrz commented 4 months ago

What's a reasonable default value? I can restore the old behavior by making it 15s, or use a lower value to make sliders and configs more responsive by default.

I'd keep the current value as the default.

LilithSilver commented 4 months ago

Fixed and pushed.

LilithSilver commented 4 months ago

Hi, any updates on this? Happy to make any additional changes as needed.

Tyrrrz commented 4 months ago

Hi, any updates on this? Happy to make any additional changes as needed.

Hi, sorry, was away for a while. I'm still ruminating on the extra complexity in the UpdateConfiguration() method, more specifically the additional state it now has to maintain in _brightnessMaxStep, _temperatureMaxStep, _lastTarget. I was trying to think of how to avoid it, but can't keep up with a good solution.

Looking back, your original issue was that the settings sliders were not responsive enough. If that's the case, we may be able to solve this by disabling smoothing when the gamma changes are caused by settings changes. Or even simpler, disable gamma smoothing when the settings dialog is open.

What that solve the issue?

Sorry for the back-and-forth, I'm trying to find how to achieve the desired result in the smallest amount of changes possible.

LilithSilver commented 4 months ago

Looking back, your original issue was that the settings sliders were not responsive enough. If that's the case, we may be able to solve this by disabling smoothing when the gamma changes are caused by settings changes. Or even simpler, disable gamma smoothing when the settings dialog is open.

My biggest issue is actually that, when I disable the filter entirely, it takes forever. If I want to jump into a game, I have to wait 15s for it to change. The sliders are a secondary issue; they also take a while for large changes, but I'm not adjusting them frequently. (Some users may, however).

A quick fix is to disable gamma smoothing entirely, but that causes instant changes, which doesn't give time for the eyes to adjust at all.

So ideally, any solution would hit these points:

The max duration limit for changes does hit these points, but the code is indeed more complex.

Sorry for the back-and-forth, I'm trying to find how to achieve the desired result in the smallest amount of changes possible.

No problem, we should work to find the best solution!

Tyrrrz commented 4 months ago

I wonder if we track _lastConfigurationBeforeSmoothing (or something) instead of _lastTarget, we'd be able to re-calculate the steps every time and avoid carrying that stuff as state?

LilithSilver commented 4 months ago

I wonder if we track _lastConfigurationBeforeSmoothing (or something) instead of _lastTarget, we'd be able to re-calculate the steps every time and avoid carrying that stuff as state?

Sure, that works; pushed!