New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement exponentially scaled slider widget #14875

Merged
merged 1 commit into from Nov 25, 2018

Conversation

Projects
None yet
8 participants
@netnazgul
Copy link
Contributor

netnazgul commented Mar 3, 2018

Closes #14696.

rationale

Constant values a = 0.001 and b = 6.908 for f(x) = ae^(bx) rescale the curve to ~60dB span.

image

Custom mods that use different settings implementation will need to be updated with ExpSliderWidget for appropriate sliders.

@fruestueck

This comment has been minimized.

Copy link
Contributor

fruestueck commented Mar 4, 2018

I've tried it and it sounds good : )
Only the lower part (first quarter of the slider) felt like there was almost no change. Also could be my ears.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Mar 5, 2018

@fruestueck -60dB is pushing it on the dynamic range of some mainstream sound devices, especially if you do not wind them up at 100% output at the time. I didn't hear anything in my headphones as well on the lower settings. As we can't predict the system user uses in his sound path, we'd better have a bit more of excess control for that. But with this implemented there is at least greater control over more quiet parts of configuration, keeping close to the same scale for general use (which for me is around -20dB..-8dB)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 31, 2018

@netnazgul do you plan on getting back to this?

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Mar 31, 2018

Yes, sorry, couldn't find enough time to fix this. Will look through the next week.
One issue raised through the discussion above - I think it will be hard to apply the proper rescaling if MinValue/MaxValue are not set to 0/1.

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from dae94f3 to 235cf1c Apr 8, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Apr 8, 2018

  • Renamed ExpSliderWidget into ExponentialSliderWidget
  • Moved exponential coefficients to explicit variables
  • Removed ternary ops; simple (x <= 0) check is left to have f(0) = 0
  • Modified rescaling applications so that MinimumValue and MaximumValue are correctly applied
  • Humbly added myself to authors

Thing I'm not fully happy with is how ticks are displayed here (don't see a way to spread them more wisely).

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Apr 14, 2018

oh wow, tried to fix the AUTHORS tab instead of spaces using built-in github editor and it failed, will fix and squash it later today

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from 8956fda to ca31d60 Apr 14, 2018

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from ca31d60 to b034dbf Apr 22, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Apr 22, 2018

Done, forgot about this...

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from b034dbf to 3c49525 Jul 6, 2018

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from 3c49525 to 1aa0fcf Jul 28, 2018

@pchote pchote removed the PR: Rebase me! label Jul 28, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

This crashes when opening the music player tab saying the game can't convert (cast) SliderWidget to ExponentialSliderWidget.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 19, 2018

Does someone want to take this over, or shall we move it over to the Future milestone and close?

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Nov 21, 2018

I'll update it on friday/saturday if you still want to push it.

In fact I've come to conclusion, that implementing this with the exponential formula as is doesn't provide much benefit to what I've initially wanted here - expanding the control area around where the most volume tweaks are done. Basically, what it does now is moving the mentioned area from the left part of the slider to the right.

Also, it likely needs testing against different sound systems and users as maybe it's just me having a certain volume preference or my audio card that produced a dynamic range too big to become too sensitive to slider position changes.

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from 1aa0fcf to c29aff0 Nov 21, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Nov 21, 2018

Ok, now the new widget should be applied everywhere it is required.

@abcdefg30 @pchote safe to re-test it

Code changed

@pchote
Copy link
Member

pchote left a comment

A few code simplifications, and I think there may be something wrong with the scaling constants as I find the same control issues as you say.

public double ExpA = 1.0e-3;
public double ExpB = 6.908;

public ExponentialSliderWidget() { }

This comment has been minimized.

@pchote

pchote Nov 22, 2018

Member

I expect this should also chain through to base(), which sets the default GetValue binding.

public double ExpB = 6.908;

public ExponentialSliderWidget() { }
public ExponentialSliderWidget(ExponentialSliderWidget other) : base(other) { }

This comment has been minimized.

@pchote

pchote Nov 22, 2018

Member

Style nit:

public ExponentialSliderWidget(ExponentialSliderWidget other)
	: base(other) { }
return (int)(0.5f * RenderBounds.Height + (RenderBounds.Width - RenderBounds.Height) * LinearFromExp((x - MinimumValue) / (MaximumValue - MinimumValue)));
}

public override void Draw()

This comment has been minimized.

@pchote

pchote Nov 22, 2018

Member

This method looks to be identical to SliderWidget.Draw except for the tick positions.
Can you pull out a protected IEnumerable<float> GetTickPositions() in the base class then override it here, so that we don't need to override the drawing at all?

@@ -123,6 +123,17 @@ static void BindSliderPref(Widget parent, string id, object group, string pref)
ss.OnChange += x => field.SetValue(group, x);
}

static void BindExpSliderPref(Widget parent, string id, object group, string pref)

This comment has been minimized.

@pchote

pchote Nov 22, 2018

Member

This looks identical to BindSliderPref which I expect should already do what you need because ExponentialSliderWidget is a subclass. This method should be able to go away.

panel.Get<SliderWidget>("SOUND_VOLUME").OnChange += x => Game.Sound.SoundVolume = x;
panel.Get<SliderWidget>("MUSIC_VOLUME").OnChange += x => Game.Sound.MusicVolume = x;
panel.Get<SliderWidget>("VIDEO_VOLUME").OnChange += x => Game.Sound.VideoVolume = x;
panel.Get<ExponentialSliderWidget>("SOUND_VOLUME").OnChange += x => Game.Sound.SoundVolume = x;

This comment has been minimized.

@pchote

pchote Nov 22, 2018

Member

Likewise with all these: you're not actually using the subclass properties, so it is cleaner (not to mention a much smaller diff) to access these via the parent slider class.

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from c29aff0 to d110814 Nov 24, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Nov 24, 2018

Removed the unneeded binder duplicate and recasts to ExponentialSliderWidget, turned out those were overcommitments due to crashes appearing in other code.
Reverted exponential tick scaling back to linear ticks. 7 ticks represent the 60dB dynamic range (set by ExpA and ExpB values), with 1 tick span equal to 10dB.

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch 2 times, most recently from d04eb41 to e5ce62a Nov 24, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Nov 24, 2018

Aaand fixed the travis check fail.

@pchote
Copy link
Member

pchote left a comment

LGTM. Great job!

@pchote pchote added this to the Next Release milestone Nov 24, 2018

@obrakmann
Copy link
Contributor

obrakmann left a comment

👍, just one minor thing

public class ExponentialSliderWidget : SliderWidget
{
public double ExpA = 1.0e-3;
public double ExpB = 6.908;

This comment has been minimized.

@obrakmann

obrakmann Nov 24, 2018

Contributor

It would be nice to have a comment here with a link to https://www.dr-lex.be/info-stuff/volumecontrols.html that explains where those numbers come from.

@netnazgul netnazgul dismissed stale reviews from obrakmann and pchote via b4e520e Nov 25, 2018

@netnazgul netnazgul force-pushed the netnazgul:exp-sound-volume-control branch from e5ce62a to b4e520e Nov 25, 2018

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Nov 25, 2018

Comment line added.

@obrakmann @pchote

@pchote

pchote approved these changes Nov 25, 2018

@obrakmann obrakmann merged commit c195699 into OpenRA:bleed Nov 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@obrakmann

This comment has been minimized.

Copy link
Contributor

obrakmann commented Nov 25, 2018

@netnazgul netnazgul deleted the netnazgul:exp-sound-volume-control branch Nov 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment