Skip to content
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

UI: Add headphone checkbox to mixer #1253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cg2121
Copy link
Contributor

@cg2121 cg2121 commented Apr 6, 2018

This adds a headphone icon to the mixer area. It replaces the audio monitor selection in the advanced audio properties.

Screenshot from 2023-02-28 06-09-01

@cg2121 cg2121 force-pushed the headphone-checkbox branch 3 times, most recently from 6958643 to adae112 Compare April 11, 2018 02:19
@Himura2la
Copy link

Himura2la commented Apr 24, 2018

OMG, I was totally ready to do the same. I'm glad I checked the existing PRs first.
I'm going to PR into this PR to fix the left margin and try to make the obs_monitoring_type boolean.

UPD: The screenshot lies, margins are totally OK

image

@cg2121
Copy link
Contributor Author

cg2121 commented Apr 24, 2018

I fixed the margin, just didn't update the image. I added the output to monitor only in the right click menu of the mixer.

@RytoEX
Copy link
Member

RytoEX commented Apr 25, 2018

@cg2121
This appears to require a rebase.

@SuslikV
Copy link
Contributor

SuslikV commented Apr 27, 2018

I don't think that this is good idea. Is this "Add headphone checkbox to mixer" or Move monitoring controls to the mixer?

Also, converting three (and more) states UI elements into two becomes feature of the application...

Edit: For example, I don't understand is this normal to have headphones sign always in OFF state or I did something wrong and it should be fixed.

@Himura2la
Copy link

@SuslikV Call it as you like, but monitoring control Must be easily accessible, I'm very new to OBSS and I was shocked how inconvenient the monitoring is now. I was forced to google how to do it and swear each time I add a new source.

Two buttons determines four states, three of which are defined in the monitoring_type enum. It's way more intuitive than the drop-down in Advanced Configuration. Monitoring is an essential feature, why Advanced?

(Sorry, I really hate the way it's done now)

@jp9000
Copy link
Member

jp9000 commented Apr 27, 2018

We could have it a three-state widget or something. I kind of wish we could communicate to the user more easily what mode it's in so they don't get confused by the "monitor but mute output" mode. Tooltip? Status bar message? Context menu?

Additionally, this could be put as three menu items in the "configure" context menu instead, and the options and what they do would become more clear.

Just throwing out ideas.

@Himura2la
Copy link

Himura2la commented Apr 27, 2018

There's no need in the third state, all states are easily covered by two Boolean widgets. Check my PR into this PR cg2121#2
Monitor-and-mute is easily recognizable by red speaker icon with cross

@jp9000
Copy link
Member

jp9000 commented Apr 27, 2018

I suppose that's true, if you monitor then mute then that would basically do the same thing as the third state.

@Himura2la
Copy link

Himura2la commented Apr 27, 2018

I built it for Windows to send my friend for testing. This build is made from the cg2121@3a4d175 commit merged with the current master (dcbad4a).

You guys may also test it to ensure it's convenient!
MinSizeRel.zip

@Alex-Dash
Copy link

Pretty useful button for live performances of any kind) Love it)

@SuslikV
Copy link
Contributor

SuslikV commented Apr 27, 2018

@Himura2la Users struggling to select right tracks to record them all at once... Two and more switches makes any task harder to complete. For example, when I see headphones sign - I think that headphones are connected. If only this button could bring up window to select the device.

I think, speaker - OFF should mute the audio and monitoring should reflect the current behavior of the sound (muted) and shouldn't change its own type.

You want to place this feature (that itself has config at Advanced section of the Settings) to the main UI - then add tool tip - it is unknown where to find the monitoring device settings. At least, when it was at advanced properties, the corresponding part was in advanced settings.

My view.

I think of three options:

  • audio monitoring disabled = Off (default)
  • monitor audio but do not output it = Listen
  • monitor audio and output it = Listen + Output

The button: listen-obs may act like gear icon button - bring up menu with 3 radio boxes mentioned above.

I think, there is no need to remove the monitoring controls from Advanced Audio Properties.

P.S. listen-obs-svg-source.zip if needed.

@Himura2la
Copy link

Himura2la commented Apr 27, 2018

@SuslikV
I agree we need a tooltip.

I don't agree that muting should disable monitoring. It's a comon task to listen to the output before sending it live. And monitoring should not be linked with stream, 'cause it's just two different audio targets, why the heck to link them? I want to monitor -- I start monitoring, I'm ready to stream -- I unmute... I just don't get it, why stream muting should prevent monitoring, and we should overcomplicate the UI to enable monitoring-only as a separate state... It's not a special state, it's just stream==off && monitor==on (users never know how it's made in the code [it's strange, I tried to fix this])

I believe monitoring feature must be one click away, because it's essential. Hiding it in context menus and/or doubling in advanced settings does not feel good for me.

And we need more opinions, especially from the decision-making guys ))

@SuslikV
Copy link
Contributor

SuslikV commented Apr 27, 2018

@Himura2la Users will decide. And, to be clear, I said that "mute" shouldn't change monitoring type.

@Himura2la
Copy link

Himura2la commented Apr 27, 2018

@SuslikV

I think, speaker - OFF should mute the audio and monitoring....

Sorry, I stopped understanding this paragraph on ellipsis, and the meaning I got was probably incorrect. (you can explain it in Russian if it's more convenient for you). Anyway, your idea of three states involves the Listen and Listen + Output options, but the output is controled by the Mute button. How are they supposed to live together? Like how it is now? I strongly disagree to control output in two places, it's dangerous and may easily cause errors. I've got to be sure that the sound goes to stream when unmuting and does not when muting. No other hidden options should interfere this.

Users struggling to select right tracks to record them all at once...
If only this button could bring up window to select the device.

OBS Studio is widely used for live streaming where the timing is EXTREMELY important. It's very easy to learn what two bottons does and very difficult to fight tons of windows menus and dialogs when the timing is on fire. I think my next contribution would be putting the Target Bitrate setting to the main window, because I should never need to invoke settings while straming.

As for the icon, your option seems way less elegant from the design point of view, I'd better stick with the headphones....

As for monitoring device, I was also wondering about this, seems like it's only possible to use the default output now.

Aaaaand, the good news!
I added the tooltip in cg2121@2494859
image

@SuslikV
Copy link
Contributor

SuslikV commented Apr 28, 2018

@Himura2la In Russian, it will be too rude to understand, I think. I will try in English.

Look,

if (mute_checked) {
		obs_source_set_monitoring_type(source,
				OBS_MONITORING_TYPE_MONITOR_ONLY);
...
} else {
		obs_source_set_monitoring_type(source,
				OBS_MONITORING_TYPE_MONITOR_AND_OUTPUT);
...

This part of code changes monitoring type, isn't it?

What about the volume meter and mute? User had explicit control over the "mute" sound by single click or by hotkey. Now, it is mess of the controls and visuals. Simply add "mute" hotkey to your monitoring source and click the speaker icon while you monitoring or not. You'll notice the difference. And you said that 2+3 logic easily fits into two boolean variables?

The streamers is casual users, it is desirable to make intuitive interface. While two buttons solution is not intuitive at all. The icon disabled/inactive state is bad idea too, because it is unknown what it does when it enabled (the "eye" icon and "lock" icon, for example, has difference in drawing - this is two completely different pictures) and what to choose (I prefer to enable it). Also, when I first saw the OBS Studio's mixer UI, I didn't knew that the "speaker" icon - interactive, and you may on/off the sound (it's because icon doesn't changes on hover).

@Himura2la
Copy link

Himura2la commented Apr 28, 2018

Please see the code from my PR into this PR. I still not sure I understand what's wrong, because two botton logic is the only possible intuitive way here, the context menu item used in this PR is a mess, I removed it. There's no 2+3 logic, it's four separate states.

  1. No stream no monitor
  2. Stream no monitor
  3. No stream monitor
  4. Stream monitor

And these states are perfectly controlled by two switches

Volume meter does not work only in state 1, because I may need it to fix the levels before going live.

If you say that there's some problem with shortcuts, we should research it, because personally I didn't test shortcuts.

The crossed out icon is a nice idea, but I don't think it's a showstopper if everything else works.

@cg2121
Copy link
Contributor Author

cg2121 commented Apr 28, 2018

@Himura2la idea IMO, doesn't seem very intuitive to me. I think it might confuse some users.
I don't really like the right click menu for the 'output to monitory only' either.
Another idea:
Single click on the icon: toggle monitor on and off
Double click on icon: toggle the 'output to monitor only', turning the icon red when on

@Himura2la
Copy link

Not bad, but I still don't understand how the 'make Mute button useless' state may help, sorry

@cg2121
Copy link
Contributor Author

cg2121 commented Apr 28, 2018

Now that I thinking about it more, your idea currently makes the most sense. There is not really an ideal solution for this.

@cg2121
Copy link
Contributor Author

cg2121 commented Apr 28, 2018

I have now merged @Himura2la PR into this.
32b071c

@Andersama
Copy link
Contributor

Used to do broadcast work, @Himura2la 's approach is definitely what I'd go w/ and for all the reasons he laid out. And I'll definitely play around w/ this pr when I get back home. I'd throw in check-boxes in the right click drop down as well, but that's no deal breaker.

Another thought*, if this pr does eventually get added,
chrome_2018-06-11_14-45-21
monitoring device selection through right click menus? In a new pr of course.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cg2121 It seems this PR needs a re-roll. I've also made a couple of notes in a quick review.

Could we get some updated screenshots of the feature? Maybe add them to the first post (with a note indicating the date added)?

UI/data/themes/Acri.qss Outdated Show resolved Hide resolved
UI/volume-control.cpp Outdated Show resolved Hide resolved
@WizardCM
Copy link
Member

After experimenting with this a little, I agree removing the existing options in Advanced Audio Properties is a bad idea. What happens to the users who have already configured their monitoring settings? What happens to the third option that it used to provide?

@cg2121 cg2121 requested a review from Warchamp7 April 18, 2022 18:01
@GeorgesStavracas GeorgesStavracas added the UI/UX Anything to do with changes or additions to UI/UX elements. label May 24, 2022
@cg2121 cg2121 added this to the OBS Studio 28.0 milestone Jun 11, 2022
@cg2121
Copy link
Contributor Author

cg2121 commented Jul 19, 2022

Updated to fix merge conflicts.

@Himura2la
Copy link

Himura2la commented Jul 19, 2022

Wow, that's great news!
@WizardCM, looks like a good time for merge!

@ryantheleach
Copy link

ryantheleach commented Jul 21, 2022

@cg2121 Considering the history of this PR, can you update the original post with updated screenshots / justifications / what was decided on (noting I'm just some random user)?

It would make it much easier to follow considering (apparent) controversial replies.

@cg2121 cg2121 force-pushed the headphone-checkbox branch 2 times, most recently from 9089c9f to dc63a25 Compare July 25, 2022 19:58
@cg2121 cg2121 force-pushed the headphone-checkbox branch 3 times, most recently from 578bb7d to 2c62f29 Compare February 28, 2023 05:15
@pkviet pkviet self-requested a review February 28, 2023 08:47
Copy link
Member

@pkviet pkviet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly checked volume-control.cpp. LGTM. I'm in favour of merging this sorely needed feature sooner than later.

@Himura2la

This comment was marked as spam.

@pkviet
Copy link
Member

pkviet commented Feb 28, 2023

@Himura2la To be fair I was not personally satisfied with the initial implementation which is why I came up with an alternative, which I use in my own fork. The monitoring API is quite intricate because it unnecessarily mixes monitoring and output functions.
The main difficulty has been to untangle both functions.
What's changed since the initial work is that mute audio is now displayed as greyed out. This is a game changer for me. So the mute state can be used in an intuitive manner to output or not the audio. A mute audio is then assigned an OBS_MONITORING_TYPE_MONITOR_ONLY state if the monitor button is active.
The current state of the PR is now in my opinion very satisfactory.

libobs/obs.h Show resolved Hide resolved
Co-authored-by: Himura Kazuto <glagol15@gmail.com>
@cg2121
Copy link
Contributor Author

cg2121 commented Feb 28, 2023

Here is a updated screenshot of what it looks like.

Screenshot from 2023-02-28 06-09-01

@ghost
Copy link

ghost commented Jun 11, 2023

Could you remove Basic.AdvAudio.Monitoring.None, Basic.AdvAudio.Monitoring.MonitorOnly, Basic.AdvAudio.Monitoring.Both & Basic.AdvAudio.MonitoringSource, they are now unused.

@cg2121 cg2121 mentioned this pull request Oct 4, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality Seeking Testers Build artifacts on CI UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.