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

citra_qt: Add a volume slider #3831

Merged
merged 2 commits into from Jul 18, 2018

Conversation

Projects
None yet
@FearlessTobi
Contributor

FearlessTobi commented Jun 10, 2018

This PR adds a volume slider to Citra, which uses a logarithmic scale.
It also implements Hid:GetSoundVolume.

UI Screenshot:
slider


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jun 10, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-10T14:40:49Z: cf9bfe0...FearlessTobi:0d09cd84de660b0e2a5fb7d034823bedebc14bee

@cluezbot

This comment has been minimized.

cluezbot commented Jun 10, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-10T18:46:43Z: cf9bfe0...FearlessTobi:981dbc27c2cc7e14a2abcb334c66270e99e002a8

@FearlessTobi FearlessTobi force-pushed the FearlessTobi:add-volume-slider branch from 0d09cd8 to 981dbc2 Jun 10, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 10, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-10T18:58:36Z: cf9bfe0...FearlessTobi:207478ae95ddf23ea854e33d3e60aae6f9fa7824

@lioncash

There should be a percentage label beside the slider that numerically indicates what the current volume level is.

cc @MerryMage for the DSP-related changes

if (!sink)
return;
for (int i = 0; i < frame.size(); i++) {

This comment has been minimized.

@lioncash

lioncash Jun 10, 2018

Member

this should be a std::size_t instead of an int.

This comment has been minimized.

@MerryMage

MerryMage Jun 11, 2018

Member

Preferably also add a comment before this stating it's implementing the hardware volume slider.

if (!sink)
return;
for (int i = 0; i < frame.size(); i++) {
frame[i][0] =
static_cast<s16>(frame[i][0] * std::exp(6.908 * Settings::values.volume) * 0.001);

This comment has been minimized.

@MerryMage

MerryMage Jun 11, 2018

Member
  1. Good, nice exponential.
  2. While it may be obvious this has a dynamic range of 60 dB, a comment would be nice.
  3. Please pre-compute the scale factor outside the loop. Compilers are not smart enough to hoist this out of the loop automatically (thanks to the std::exp call).

Also, perhaps use 6.90775 instead. It is much preferable to use a slight underestimate, because your current overestimate would overflow a 16-bit number. (e.g.: currently at volume = 1.0, 65535 -> 65551.)

@adityaruplaha

IMO, this should be more easily accessible. Like a control panel as @valentinvanelIslande (sorry if I didn't get your name right) did in a previous PR.

@mailwl

This comment has been minimized.

Contributor

mailwl commented Jun 12, 2018

for me, audio settings is good

@Dragios

This comment has been minimized.

Contributor

Dragios commented Jun 13, 2018

Or this design is preferable? I know this is from an old PR

@adityaruplaha

This comment has been minimized.

Contributor

adityaruplaha commented Jun 13, 2018

Idk why use a group box.
And why not make it a vertical slider that runs along the right edge? That would be hardware-accurate, as the 3DS slider is vertical too, and would save space.

@Dragios

This comment has been minimized.

Contributor

Dragios commented Jun 13, 2018

Vertical slider will look weird in a window configuration imho. Also how are you going to fit in text in vertical layout? Make it tiny or vertical as well? Not reliable at all. Vertical slider only looks good on its own window.

@cluezbot

This comment has been minimized.

cluezbot commented Jun 29, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-29T15:02:48Z: 1eeccdd...FearlessTobi:6d319e7da964cff45dde7349c544d6b582ad7f1b

@FearlessTobi FearlessTobi force-pushed the FearlessTobi:add-volume-slider branch from 207478a to 6d319e7 Jun 29, 2018

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Jun 29, 2018

Updated screenshot:
unbenannt

@@ -19,6 +19,8 @@ ConfigureAudio::ConfigureAudio(QWidget* parent)
ui->output_sink_combo_box->addItem(sink_detail.id);
}
connect(ui->volume_slider, &QSlider::valueChanged, [&]() { emit OnVolumeChanged(); });

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

You can omit the (), and probably you needn't catch & here (I'm not very sure)

This comment has been minimized.

@FearlessTobi

FearlessTobi Jun 29, 2018

Contributor

I just tried to follow the style we are you using in places like here:

connect(open_save_location, &QAction::triggered, [this, program_id] {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

No, that's not the same.
GameList uses this because it has to spread the signal further.
See:

connect(game_list, &GameList::OpenFolderRequested, this, &GMainWindow::OnGameListOpenFolder);

And, you shouldn't provide definition for a signal. Definitely not.

@@ -76,6 +83,10 @@ void ConfigureAudio::updateAudioDevices(int sink_index) {
}
}
void ConfigureAudio::OnVolumeChanged() {
ui->volume_indicator->setText(tr("%1 %").arg(ui->volume_slider->sliderPosition()));
}

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

Likely you are not doing things right;

  1. This one should be a slot (so you can connect to it properly without lambda&emit)
  2. You can actually put the code to the lambda above. (thus catch [this])
@cluezbot

This comment has been minimized.

cluezbot commented Jun 30, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-30T12:18:31Z: ffae5be...FearlessTobi:620ed454c67026017f5771be5120e96f41f08d2a

@FearlessTobi FearlessTobi force-pushed the FearlessTobi:add-volume-slider branch from 6d319e7 to 620ed45 Jun 30, 2018

@FearlessTobi FearlessTobi force-pushed the FearlessTobi:add-volume-slider branch from 620ed45 to 0c449cb Jun 30, 2018

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Jun 30, 2018

Replaced [&] with [this] and squashed my commits.

PR should be (hopefully) good to merge now.

@zhaowenlan1779

@FearlessTobi
The UI has a minor problem.
The position of the slider moves with label size (eg. 0%-9%, 10%-99%, 100%). That is not a good experience, I think.

@FearlessTobi FearlessTobi force-pushed the FearlessTobi:add-volume-slider branch from 0c449cb to a780f38 Jul 3, 2018

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Jul 3, 2018

The UI problem should be fixed now. @zhaowenlan1779

@KPJoshi

This comment has been minimized.

KPJoshi commented Jul 4, 2018

I have an issue with the canary build that includes this volume slider PR (Canary 617). Issue is not present in the latest nightly (Nightly 789).

Issue: No sound

Steps: Start a game. There will be no sound.

Workaround: After starting game, open the configurations window and press OK to close it (Cancel won't work). Sound starts working again.

Build: Ryzen 3, Win 10 w/ latest updates, motherboard soundcard (AsRock AB350M Pro 4)

Don't know if issue tracker is for PR issues, so posting here.

@legoj15

This comment has been minimized.

legoj15 commented Jul 5, 2018

I think I may have encountered the same issue as described above after fiddling with the audio volume then closing and then opening Citra canary again (where then I have to open the configurations to fix it).

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Jul 5, 2018

Strange... I will try to look into this.

@wwylele

This comment has been minimized.

Member

wwylele commented Jul 17, 2018

What's the status of all issues reported above? Can some one retest these to make sure they are actually still there?

@KPJoshi

This comment has been minimized.

KPJoshi commented Jul 17, 2018

@wwylele Let me check tonight to see if I still have this issue with the latest drivers and canary/nightly versions.

Also note that I am not sure if this is the cause of the aforementioned issue; I just assumed it is because of the related nature of the issue.

@legoj15

This comment has been minimized.

legoj15 commented Jul 17, 2018

Citra Canary seems to be set at the volume I have it at, and has not changed, so I think the issue disappeared somehow.... (at least for me)

@KPJoshi

This comment has been minimized.

KPJoshi commented Jul 17, 2018

@wwylele @FearlessTobi I have good news and bad news

Earlier, when I tested with Canary 617 and Nightly 789, there were no problems with the Nightly, but problems with the Canary

Now, when I test with Canary 635 and Nightly 802, both have the same problem I mentioned earlier.

Which means the problem is probably not in this new code, but rather in other code that has been added to Nightly between versions 789 and 802.

@Dragios

This comment has been minimized.

Contributor

Dragios commented Jul 18, 2018

@KPJoshi why not you try to bisect which Nightly version that cause the aforementioned issue. This will speed up the process for the developer to fix this issue.

@jroweboy jroweboy merged commit 2f8c9c8 into citra-emu:master Jul 18, 2018

2 checks passed

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

@FearlessTobi FearlessTobi deleted the FearlessTobi:add-volume-slider branch Oct 26, 2018

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