Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#4039 closed enhancement (fixed)

[PATCH] Volume control slider

Reported by: Damien PIQUET Owned by: Damien PIQUET
Priority: Should Have Milestone: Alpha 22
Component: Core engine Keywords: patch
Cc: Patch:

Description

Use a slider to control the sound volume.

Attachments (6)

4039.patch (11.8 KB ) - added by Damien PIQUET 8 years ago.
4039-2.patch (12.4 KB ) - added by Damien PIQUET 8 years ago.
Resize & revert changes fixes; corrected class description (reported by Imarok and s0600204)
slider.png (160.2 KB ) - added by Vladislav Belov 7 years ago.
slider2.png (131.3 KB ) - added by Vladislav Belov 7 years ago.
4039_slider.patch (11.9 KB ) - added by Vladislav Belov 7 years ago.
Improved patch by dpiquet
4039_slider.2.patch (12.9 KB ) - added by Vladislav Belov 7 years ago.
Added missed width, used the first design, buttons move to ends of both sides

Download all attachments as: .zip

Change History (25)

by Damien PIQUET, 8 years ago

Attachment: 4039.patch added

comment:1 by Damien PIQUET, 8 years ago

Keywords: review patch added
Owner: set to Damien PIQUET

by Damien PIQUET, 8 years ago

Attachment: 4039-2.patch added

Resize & revert changes fixes; corrected class description (reported by Imarok and s0600204)

comment:2 by mimo, 8 years ago

nice work! Here are a few quick comments (without proper review of the code):

  • you must add a way to display the selected value of the slider (maybe in a tooltip ?), and the min and max could be displayed too.
  • it seems that your slider implementation is a ratio between 0 and 1? but that's not always wanted: the default gains were all between 0 and 1, but I'm not sure that these were supposed to be relative gain vs the maximum gain (in particular the master gain, but someone knowing the sound system should comment), and there are other places where we could use a slider not necessarily between 0 and 1. Ideally, we should have min/max values of the slider in options.json, and these values would be used to define the min/max of the slider or to make a conversion between the value between 0 and 1 returned by the slider, and the output of the slider which should be between min and max.
  • futhermore, as the slider should never return a value outside its min/max range, lines 166-169 seem useless.

comment:3 by elexis, 8 years ago

Keywords: review removed

#2593 points out other desired features, but I wouldn't mind if we get basic slider support now and add more in that ticket if you can't implement all things easily.

Before comitting you should IMO at least address these points:

  • Agreeing with mimo that one should be able to pass min and max to the slider, so that it's versatile and easily usable. If that computation isn't done in C++ it would have to be done in JS which would likely end in code duplication.
  • Reuse the caption field for the value like other GUI object types do.
  • Those lines mimo mentioned indeed some redundant.
  • Displaying the min/max/current value is likely irrelevant for this ticket, if it's going to be a 0-100% thing anyway. It would become more important when one can select values from a given range.

Coding style:

  • L99 first check MouseOver then !MouseOver in the else part, also that one pair of braces is not needed
  • Draw a sprite at the % position comment not really needed as it states the same as the code
  • // Cursor should 5% of it's parent with a max ? didn't understand that one

comment:4 by Stan, 8 years ago

Summary: Volume control slider[PATCH] Volume control slider

comment:5 by elexis, 8 years ago

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:6 by Vladislav Belov, 7 years ago

Any progress?

by Vladislav Belov, 7 years ago

Attachment: slider.png added

comment:7 by Vladislav Belov, 7 years ago

http://trac.wildfiregames.com/raw-attachment/ticket/4039/slider.png

comment:8 by fabio, 7 years ago

(refs #3737)

comment:9 by Vladislav Belov, 7 years ago

Keywords: rfc added

comment:10 by Vladislav Belov, 7 years ago

Current slider works with max/min values, uses the checkbox button image.

by Vladislav Belov, 7 years ago

Attachment: slider2.png added

comment:11 by Vladislav Belov, 7 years ago

Another style: http://trac.wildfiregames.com/raw-attachment/ticket/4039/slider2.png

by Vladislav Belov, 7 years ago

Attachment: 4039_slider.patch added

Improved patch by dpiquet

comment:12 by wraitii, 7 years ago

Looks good. I don't think the "another style" works for our interface, but the first is OK. However the knobs don't go all the way to the left and I get an error when I open the options screen (something about width).

by Vladislav Belov, 7 years ago

Attachment: 4039_slider.2.patch added

Added missed width, used the first design, buttons move to ends of both sides

comment:13 by Vladislav Belov, 7 years ago

Currently the upper bound of sound gain is 10.0: http://trac.wildfiregames.com/wiki/Manual_Settings. But 1.0 is normal sound gain (not increased, not decreased), so probably, make the upper bound in [1.0, 2.0], because user with normal settings will have a problem to make gain in [0.5, 1.0], also the default gain value is 0.5.

Last edited 7 years ago by Vladislav Belov (previous) (diff)

comment:14 by echotangoecho, 7 years ago

While you're at it, IMO, the "Gain" part of the sound settings names should be renamed "volume".

in reply to:  14 comment:15 by Vladislav Belov, 7 years ago

Replying to echotangoecho:

While you're at it, IMO, the "Gain" part of the sound settings names should be renamed "volume".

I agree, but in a separated commit.

comment:16 by wraitii, 7 years ago

Resolution: fixed
Status: newclosed

In 19479:

Add sliders to the GUI. Use them for the sound/music volume controls.

Patch by vladislavbelov, based on a patch by dpiquet.

Fixes #4039.
Differential Revision: https://code.wildfiregames.com/D325

comment:17 by elexis, 7 years ago

Component: UI & SimulationCore engine
Keywords: rfc removed
Milestone: BacklogAlpha 22
Priority: Nice to HaveShould Have

Thanks for the patch guys!

comment:18 by elexis, 7 years ago

In 19482:

Use the slider for the number of shader graphics setting.
Thereby remove the only hardcoded reference of options.json from options.js.
Rename Gain to Volume, refs #4039.

Differential Revision: https://code.wildfiregames.com/D398
Reviewed By: Vladislav

comment:19 by elexis, 5 years ago

In 23005:

Change GUI Object Setting values to be members of the IGUIObject inheriting class, rename AddSetting to RegisterSetting.

Improves performance for Draw calls by 3-5% according to a shady benchmark.
Improves memory layout, since the values are not on the heap anymore but in the using class.
Reduces complexity of the implementation and increases type safety.
Allows specifying default values at setting value construction time, refs D2242.
Inspired by Vladislav introducing members that cached GetSetting values in rP19479/D325, refs #4039, rP20075/D763, refs 4225, rP21429/D474, D406, which were formerly proposed to be removed in D2241.

Differential Revision: https://code.wildfiregames.com/D2313
Tested on: clang 8.0.1, gcc 9.1.0, Jenkins vs2015
Comments By: Vladislav

Note: See TracTickets for help on using tickets.