Add customizable max transition duration for settings changes #291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
|
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. |
|
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. |
I'd keep the current value as the default. |
|
Fixed and pushed. |
| var isSmooth = | ||
| _settingsService.IsConfigurationSmoothingEnabled | ||
| && !IsCyclePreviewEnabled | ||
| && _settingsService.MaxSettingsTransitionDuration.TotalSeconds >= 0.1; |
There was a problem hiding this comment.
Is this check required if we already have IsConfigurationSmoothingEnabled?
There was a problem hiding this comment.
Yes -- this fixes an edge case where, if the calculated transition duration is very small, it can lead to 0s or NaNs being produced by the calculation and fed into the smoothing function (which causes my monitors to go black, so it's near impossible to debug).
| if (_lastTarget != TargetConfiguration && isSmooth) | ||
| { | ||
| _brightnessMaxStep = _brightnessDefaultStep; | ||
| _temperatureMaxStep = _temperatureDefaultStep; | ||
| _lastTarget = TargetConfiguration; | ||
|
|
||
| var tempDelta = Math.Abs( | ||
| TargetConfiguration.Temperature - CurrentConfiguration.Temperature | ||
| ); | ||
| var brightnessDelta = Math.Abs( | ||
| TargetConfiguration.Brightness - CurrentConfiguration.Brightness | ||
| ); | ||
| var expectedTemperatureDuration = tempDelta / (_temperatureMaxStep * stepsPerSecond); | ||
| var expectedBrightnessDuration = | ||
| brightnessDelta / (_brightnessMaxStep * stepsPerSecond); | ||
|
|
||
| // If the expected durations are longer than our duration limit, we adjust the step amount to stay at the max duration. | ||
| var goalDuration = Math.Max(expectedTemperatureDuration, expectedBrightnessDuration); | ||
| goalDuration = Math.Min( | ||
| goalDuration, | ||
| _settingsService.MaxSettingsTransitionDuration.TotalSeconds | ||
| ); | ||
|
|
||
| // Calculate the step-rate needed to reach the goal. | ||
| _brightnessMaxStep = brightnessDelta / (goalDuration * stepsPerSecond); | ||
| _temperatureMaxStep = tempDelta / (goalDuration * stepsPerSecond); | ||
|
|
||
| // If we ended up slower on either of the durations, speed us up. | ||
| _brightnessMaxStep = Math.Max(_brightnessMaxStep, _brightnessDefaultStep); | ||
| _temperatureMaxStep = Math.Max(_temperatureMaxStep, _temperatureDefaultStep); | ||
| } | ||
|
|
||
| CurrentConfiguration = isSmooth | ||
| ? CurrentConfiguration.StepTo(TargetConfiguration, 30, 0.008) | ||
| ? CurrentConfiguration.StepTo( | ||
| TargetConfiguration, | ||
| _temperatureMaxStep, | ||
| _brightnessMaxStep | ||
| ) | ||
| : TargetConfiguration; |
There was a problem hiding this comment.
What about instead of configuring smoothing duration, you could configure the steps instead? You'd then be able to set arbitrary values instead of 30 and 0.008, which would let you speed up the transitions, but the code would end up a lot simpler (basically same as before).
There was a problem hiding this comment.
I tried that, but it ended up leading to strobing in the case of small transitions at high speed. If a small configuration change was made, such as sliding a slider, it led to instant changes similar to gamma smoothing being disabled entirely. But if that's preferable behavior, I can change it to that.
|
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 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. |
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.
No problem, we should work to find the best solution! |
|
I wonder if we track |
Sure, that works; pushed! |
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.