Add master volume slider and checkbox #7848

vijfhoek · 2018-08-02T11:21:21Z

Fixes #6720 and the issues discussed in PR #3001.

Also fixes a stubborn bug with widget_scroll_get_part not supporting more than two scroll widgets.

This PR includes some rewriting of rct_scroll code that I did while debugging the issue with more than two rct_scrolls in one window. I decided to leave it in.

AaronVanGeffen · 2018-08-02T11:24:33Z

Thanks for your patch! Assuming it works correctly, I have no objections.

I do think using the scrollbars as sliders is a bit of an ugly hack, though. In the future, I'd like us to port the slider widgets from OpenLoco, and use those instead:

Broxzier

Scrolling feels off. The mouse seems to move faster than the scrollbar, and you can scroll past the left arrow button.

Broxzier · 2018-08-02T14:06:28Z

src/openrct2-ui/windows/Options.cpp

            widget_set_checkbox_value(w, WIDX_MUSIC_CHECKBOX, gConfigSound.ride_music_enabled);
            widget_set_checkbox_value(w, WIDX_AUDIO_FOCUS_CHECKBOX, gConfigSound.audio_focus);

            // Initialize only on first frame, otherwise the scrollbars wont be able to be modified
            if (w->frame_no == 0)
            {
+                widget = &window_options_audio_widgets[WIDX_MASTER_VOLUME];
+                w->scrolls[0].h_left = ceil(
+                    (gConfigSound.master_volume / 100.0f) * (w->scrolls[0].h_right - ((widget->right - widget->left) - 1)));


Please split this calculation in two parts and store them, so make it more readable and make debugging easier.
(gConfigSound.master_volume / 100.0f) and (w->scrolls[0].h_right - ((widget->right - widget->left) - 1)).

I left this in the same style as the other two scrolls. I will refactor this, but there are many other similar cases in the existing code that are near unreadable - a full refactor would be more appropriate sometime.

That's definitely true, and small refactors like this makes it much easier in the long run.

vijfhoek · 2018-08-02T15:29:01Z

The scrolling feeling off is an existing issue (it happens in develop too), and not my fault. That'd be a separate issue.

vijfhoek · 2018-08-02T16:36:02Z

The issue with the scrollbar moving to the left too much doesn't happen with other horizontal scroll bars, like the map. Guess I'll take a quick look

vijfhoek · 2018-08-02T20:33:07Z

I found the cause for the slider going too far left:

        if (scroll->h_thumb_right - scroll->h_thumb_left < 20)
        {
            double barPosition = (scroll->h_thumb_right * 1.0) / view_size;

            scroll->h_thumb_left = (uint16_t)std::lround(scroll->h_thumb_left - (20 * barPosition));
            scroll->h_thumb_right = (uint16_t)std::lround(scroll->h_thumb_right + (20 * (1 - barPosition)));
        }

(https://github.com/OpenRCT2/OpenRCT2/blob/develop/src/openrct2/interface/Window.cpp#L2238)

That if statement ensures that the thumb is at least 20 pixels wide. This does cause some issues with the thumb disappearing behind the arrows though. Removing the if statement fixes the problem entirely.

Another way around the problem, which I'm going to suggest for now, is setting the width window_options_scrollgetsize returns to 500. This causes the scroll bar to be more than 20 pixels wide. This makes the if statement not trigger at all, while looking about the same width, at the additional advantage of the scrollbar actually matching the mouse speed.

This does not solve the issue with the logic inside the if statement being incorrect, but that's a problem for a future issue.

AaronVanGeffen · 2018-08-12T08:33:07Z

@Broxzier Have your concerns been addressed?

AaronVanGeffen

Took the code for a quick spin, and it all seems to be working fine.

I'd really like the scroll widgets to be disabled when the boxes next to them have not been checked, but our widget code doesn't seem to support disabled scrollbars at all yet. Something for another PR, then. :-)

Broxzier

I just noticed that the master checkbox isn't linked with the mute button in the top-toolbar and mute-hotkey. They do share their intent (though muting doesn't get saved), so it'd make sense to have them work together.

Right now you can check the box with the game not visibly being muted, and the other way around: the game can be muted without the checkbox in front of the master slider being unticked.

AaronVanGeffen · 2018-08-17T17:54:21Z

@Broxzier Should be good now?

Broxzier · 2018-08-17T18:36:56Z

Partly, unfortunately. It toggles with both the 'master volume' and 'sound effects' now, with the later muting all sounds including ride music.

Idealy the sound and ride music checkboxes get disabled when the master volume checkbox is not set (checkmark and label drawn inset), but I don't think checkboxes have that ability yet.

AaronVanGeffen · 2018-08-17T18:39:10Z

We can disable checkboxes (cf. weather and lighting effects), just not the scrollbars. I agree, though, it'd be nice if they were disabled when appropriate.

Broxzier · 2018-08-17T20:06:28Z

Updated:

Also includes some rewriting of rct_scroll code that I did while debugging the issue with more than two rct_scrolls in one window. I decided to leave it in.

Clicking mute from the top toolbar or from with the hotkey will toggle the master volume instead of the game volume now, and invalidates the options window to immediately make the checkbox visible.

- Feature: [#5993] Ride window prices can now be set via text input. - Feature: [#6998] Guests now wait for passing vehicles before crossing railway tracks. - Feature: [#7658] Add option to always use system file browsing window. - Feature: [#7694] Debug option to visualize paths that the game detects as wide. - Feature: [#7713] The virtual floor now takes land ownership rights into account. - Feature: [#7771] Danish translation. - Feature: [#7797, #7802, #7821, #7830] Add sprite font glyphs for Danish, Norwegian, Russian, Turkish, Catalan and Romanian. - Feature: [#7848] Add a master volume slider to audio options screen. - Feature: [#7868] Placing scenery while holding shift now scales appropriately with zoom levels. - Feature: [#7882] Auto-detect Steam and GOG installations of RCT1. - Feature: [#7885] Turkish translation. - Fix: [#3177] Wrong keys displayed in shortcut menu. - Fix: [#4039] No sprite font glyph for German opening quotation mark. - Fix: [#5548] platform_get_locale_date_format is not implemented for Linux. - Fix: [#7204] Object source filters do not work for RCT1, AA and LL. - Fix: [#7440] Memory leak. All system memory used. - Fix: [#7462] Guest window goes beyond the map edge on a spiral slide. - Fix: [#7533] Screenshot is incorrectly named/file is not generated in CJK language. - Fix: [#7628] Always-researched items can be modified in the inventory list. - Fix: [#7643] No Money scenarios with funding set to zero. - Fix: [#7653] Finances money spinner is too narrow for big loans. - Fix: [#7673] Vehicle names are cut off in invention list. - Fix: [#7674] Rides show up as random numbers in guest's ride list. - Fix: [#7678] Crash when loading or starting a new game while having object selection window open. - Fix: [#7683] 'Arbitrary ride type' dropdown state is shared between windows. - Fix: [#7697] Some scenery groups in RCT1 saves are never invented. - Fix: [#7711] Inverted Hairpin Coaster allows building invisible banked pieces. - Fix: [#7734] Title sequence not included in macOS builds as of 0.2.0 release. - Fix: [#7756] Steam RCT2 path not correctly checked on macOS and Linux. - Fix: [#7765] Crash when opening ride list window on Windows Vista. - Fix: [#7773] Once research has been completed, player is still charged for research. - Fix: [#7786] Crash when importing a track design. - Fix: [#7793] Duplicate private keys generated. - Fix: [#7817] No sprite font glyph for interpunct. - Fix: [#7823] You can build mazes in pause mode. - Fix: [#7804] Russian ride descriptions are cut off. - Fix: [#7872] CJK tooltips are often cut off. - Fix: [#7895] Import of Mega Park and the RCT1 title music do not work on some RCT1 sources. - Improved: [#7899] Timestamps in the load/save screen are now displayed using local timezone instead of GMT. - Improved: [#7918] Better RCT2 detection if both disc and GOG/Steam versions are installed.

Broxzier requested changes Aug 2, 2018

View reviewed changes

Broxzier approved these changes Aug 12, 2018

View reviewed changes

Broxzier added the changelog This issue/PR deserves a changelog entry. label Aug 12, 2018

AaronVanGeffen approved these changes Aug 12, 2018

View reviewed changes

Broxzier requested changes Aug 12, 2018

View reviewed changes

Broxzier force-pushed the feature/master-volume-slider branch from d5ce22b to c6bb9ec Compare August 17, 2018 20:04

Broxzier approved these changes Aug 17, 2018

View reviewed changes

vijfhoek and others added 12 commits August 17, 2018 22:14

Fix issue with more than two rct_scrolls not working

2e95c64

Add master sound checkbox and master volume slider

0cdd7c8

Also includes some rewriting of rct_scroll code that I did while debugging the issue with more than two rct_scrolls in one window. I decided to leave it in.

clang-format Options.cpp

9583368

Refactor volume slider initialization

1a1ad56

Fix widget_index type and scroll_index -> _id

a7d17a6

Refactor widget_hscrollbar_draw to be more readable

5f8c7ef

Change window_options_scrollgetsize width to 500px

1b805d9

Pause/unpause audio when disabling channels.

69d76a3

Fix muting game sounds instead of master sound

c99d281

Clicking mute from the top toolbar or from with the hotkey will toggle the master volume instead of the game volume now, and invalidates the options window to immediately make the checkbox visible.

Draw disabled checkboxes inset.

d4591a1

Draw sound effects and music checkboxes as disabled when appropirate

01608ba

Add a changelog entry for OpenRCT2#7848.

c47b77b

AaronVanGeffen force-pushed the feature/master-volume-slider branch from c6bb9ec to c47b77b Compare August 17, 2018 20:18

AaronVanGeffen removed the changelog This issue/PR deserves a changelog entry. label Aug 17, 2018

AaronVanGeffen merged commit 35ac24a into OpenRCT2:develop Aug 17, 2018

AaronVanGeffen mentioned this pull request Aug 17, 2018

Strings 6262–6263, 1777 OpenRCT2/Localisation#1542

Closed

17 tasks

qqkookie pushed a commit to qqkookie/OpenRCT2 that referenced this pull request Aug 19, 2018

Add a changelog entry for OpenRCT2#7848.

647449f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add master volume slider and checkbox #7848

Add master volume slider and checkbox #7848

vijfhoek commented Aug 2, 2018

AaronVanGeffen commented Aug 2, 2018

Broxzier left a comment

Broxzier Aug 2, 2018

vijfhoek Aug 2, 2018

Broxzier Aug 2, 2018

vijfhoek commented Aug 2, 2018 •

edited

Loading

vijfhoek commented Aug 2, 2018

vijfhoek commented Aug 2, 2018 •

edited

Loading

AaronVanGeffen commented Aug 12, 2018

AaronVanGeffen left a comment

Broxzier left a comment •

edited

Loading

AaronVanGeffen commented Aug 17, 2018

Broxzier commented Aug 17, 2018

AaronVanGeffen commented Aug 17, 2018

Broxzier commented Aug 17, 2018

Add master volume slider and checkbox #7848

Add master volume slider and checkbox #7848

Conversation

vijfhoek commented Aug 2, 2018

AaronVanGeffen commented Aug 2, 2018

Broxzier left a comment

Choose a reason for hiding this comment

Broxzier Aug 2, 2018

Choose a reason for hiding this comment

vijfhoek Aug 2, 2018

Choose a reason for hiding this comment

Broxzier Aug 2, 2018

Choose a reason for hiding this comment

vijfhoek commented Aug 2, 2018 • edited Loading

vijfhoek commented Aug 2, 2018

vijfhoek commented Aug 2, 2018 • edited Loading

AaronVanGeffen commented Aug 12, 2018

AaronVanGeffen left a comment

Choose a reason for hiding this comment

Broxzier left a comment • edited Loading

Choose a reason for hiding this comment

AaronVanGeffen commented Aug 17, 2018

Broxzier commented Aug 17, 2018

AaronVanGeffen commented Aug 17, 2018

Broxzier commented Aug 17, 2018

vijfhoek commented Aug 2, 2018 •

edited

Loading

vijfhoek commented Aug 2, 2018 •

edited

Loading

Broxzier left a comment •

edited

Loading