Opened 3 years ago

Closed 2 years ago

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

Download all attachments as: .zip

Change History (24)

Changed 3 years ago by Damien PIQUET

Attachment: 4039.patch added

comment:1 Changed 3 years ago by Damien PIQUET

Keywords: review patch added
Owner: set to Damien PIQUET

Changed 3 years ago by Damien PIQUET

Attachment: 4039-2.patch added

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

comment:2 Changed 3 years ago by mimo

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 Changed 3 years ago by elexis

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 Changed 3 years ago by stanislas69

Summary: Volume control slider[PATCH] Volume control slider

comment:5 Changed 3 years ago by elexis

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:6 Changed 3 years ago by Vladislav Belov

Any progress?

Changed 3 years ago by Vladislav Belov

Attachment: slider.png added

comment:7 Changed 3 years ago by Vladislav Belov

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

comment:8 Changed 3 years ago by fabio

(refs #3737)

comment:9 Changed 3 years ago by Vladislav Belov

Keywords: rfc added

comment:10 Changed 3 years ago by Vladislav Belov

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

Changed 3 years ago by Vladislav Belov

Attachment: slider2.png added

comment:11 Changed 3 years ago by Vladislav Belov

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

Changed 3 years ago by Vladislav Belov

Attachment: 4039_slider.patch added

Improved patch by dpiquet

comment:12 Changed 3 years ago by wraitii

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).

Changed 3 years ago by Vladislav Belov

Attachment: 4039_slider.2.patch added

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

comment:13 Changed 3 years ago by Vladislav Belov

Currently the upper bound of 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 the gain in [0.5, 1.0], also the default gain value is 0.5.

Version 0, edited 3 years ago by Vladislav Belov (next)

comment:14 Changed 3 years ago by echotangoecho

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

comment:15 in reply to:  14 Changed 3 years ago by Vladislav Belov

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 Changed 2 years ago by wraitii

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 Changed 2 years ago by elexis

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

Thanks for the patch guys!

comment:18 Changed 2 years ago by elexis

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

Note: See TracTickets for help on using tickets.