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

Show "passthrough" on Waveform Overviews if enabled #2575

Merged
merged 4 commits into from
Mar 31, 2020

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Mar 20, 2020

This has already bitten me at the start of a gig when Mixxx "did not work properly". I actually had to delay my set 10 minutes until I figured out that passthrough was enabled because I have a physical 3-state switch (PC/LINE/PHONO) on my controller and I must have accidently switched to LINE (i.e. passthrough) while carrying it around.

@Holzhaus Holzhaus added the ui label Mar 20, 2020
@ronso0
Copy link
Member

ronso0 commented Mar 21, 2020

@ronso0
Copy link
Member

ronso0 commented Mar 21, 2020

I'll test this soonish. I'm curious if we can style the text so it stands out appropriately.

@esbrandt
Copy link
Contributor

@ronso0
Copy link
Member

ronso0 commented Mar 22, 2020

Could you draw the text on the v-center line so that it's not cutoff when we make it big:
image

Also can we use a QLabel or whatever is needed to give the text a distinct ObjectName so we can style it independently from hotcue labels Edit? and "Analyzing..." text (uppercase and color)

@ronso0
Copy link
Member

ronso0 commented Mar 22, 2020

The overview is shown after changing the skin with Passthrough enabled

@ronso0
Copy link
Member

ronso0 commented Mar 22, 2020

Also can we use a QLabel or whatever is needed to give the text a distinct ObjectName so we can style it independently from hotcue labels Edit? and "Analyzing..." text (uppercase and color)

Instead of using the current paintText() function which is also used for Analyzing overlay and others, I think actually we should simply hide all track widgets (overview, controls, waveform etc.) and show a separate <Label> when passthrough is enabled. This would allow styling the Passthrough label much easier, though requires much more changes in skin xmls.
What do you think?

@daschuer
Copy link
Member

At least the current PR is a goods step forward. :-)
Is there a use case to load a new track while passthrough is enabled? We should at keep his option.

So I think dropping a track should not be disabled.

Unfortunately the overlay text disappears after changing the skin.

src/widget/woverview.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

I think actually we should simply hide all track widgets (overview, controls, waveform etc.) and show a separate <Label> when passthrough is enabled. This would allow styling the Passthrough label much easier, though requires much more changes in skin xmls.
What do you think?

I think that's a good idea. Do you have time to do that? I'm not really a skin designer 🤷‍♂️

I'll still try to fix this PR as a fallback for custom skins that don't implement widget hiding.

@ronso0
Copy link
Member

ronso0 commented Mar 22, 2020

I think actually we should simply hide all track widgets (overview, controls, waveform etc.) and show a separate <Label> when passthrough is enabled. This would allow styling the Passthrough label much easier, though requires much more changes in skin xmls.
What do you think?

I think that's a good idea. Do you have time to do that? I'm not really a skin designer man_shrugging

I put that on my ToDo list, right after the Play/Cue fix and that vinyl control thing.

I'll still try to fix this PR as a fallback for custom skins that don't implement widget hiding.

Yup, great!

@Swiftb0y
Copy link
Member

Kinda offtopic question: In the code, you are binding to the passthrough CO. This assumes the controller is able to send external input signals to mixxx (and that mixxx is set up to route them accordingly). Some controllers still have the option to take an external analog input signal and mix that in hardware, but not stream that back to Mixxx. In that case, one would use the mute CO to mute Mixxx decks from playing. This could lead to similar problems so it would make sense have a similar feature like this PR proposes that also takes care of the mute CO.

@Holzhaus
Copy link
Member Author

Why use mute? This would keep the deck playing, and mute might be used for other purposes so we can't just disable the waveform overview for muted decks.

You could also just stop the deck playback, right?

@Holzhaus Holzhaus force-pushed the passthrough-overview branch from 4634ac6 to 9624e5c Compare March 23, 2020 10:20
@Holzhaus Holzhaus requested review from daschuer and ronso0 March 23, 2020 10:21
@Swiftb0y
Copy link
Member

Why use mute? This would keep the deck playing, and mute might be used for other purposes so we can't just disable the waveform overview for muted decks.
You could also just stop the deck playback, right?

Yes, but the way the controller works is that it just takes the audio from mixxx blindly and mixes it with the external input sources. You can switch each channel between PC and Line. If its set to PC, it mutes the external input on that channel, so a user would expect that when it is set to Line, that the deck in the software would be muted as well. This is also how the controller behaves with the proprietary software.

@Holzhaus
Copy link
Member Author

If the controller does not send any MIDI signal when switching that button, we could add a separate Mute indicator somewhere in the skins. But we can't just disable the overview as we do here.

@Swiftb0y
Copy link
Member

Swiftb0y commented Mar 23, 2020

If the controller does not send any MIDI signal when switching that button,

It does send it though. So mixxx has to take care of muting that deck since the controller can't do it in hardware itself.

@Holzhaus
Copy link
Member Author

Holzhaus commented Mar 23, 2020

If the controller does not send any MIDI signal when switching that button,

It does send it though. So mixxx has to take care of muting that deck since the controller can't do it in hardware itself.

Does not even make a difference though. We could add a mute indicator in the mixxx section or gray out the gain knob and the volume slider or something like that.

Or what are you suggesting?

@Swiftb0y
Copy link
Member

Exactly. I'm suggesting to add some indicator that a deck is muted, though I'm not sure where to either.

@Holzhaus
Copy link
Member Author

This is unrelated to this PR though, so we should continue the discussion on Zulip. Can you open a new topic for it?

@Swiftb0y
Copy link
Member

Well it was kinda related. Here's the Zulip stream: https://mixxx.zulipchat.com/#narrow/stream/109122-general/topic/Deck.20mute.20indicator

@Holzhaus
Copy link
Member Author

I fixed a little issue that prevented the waveform from being repainted when disabling passthrough while no track is loaded. IMHO this is ready now.

@ronso0
Copy link
Member

ronso0 commented Mar 30, 2020

related but also present in master: the Passthrough functionality seems to be broken:
OK deck playing, enable passthrough > deck stops, replaygain is disabled
FAIL enable passthrough, press Play > deck starts (with replayGain disabled)

My build from 2020-01-31 is okay in that regard, and from the log I can't find an obvious cause.
Shouldn't EngineBuffer::updateIndicatorsAndModifyPlay also test for passthrough?

@ronso0
Copy link
Member

ronso0 commented Mar 30, 2020

In 6160a63 I replaced the painter string with an QLabel. That way we can customize the warning overlay label and maintain the styles for "Anaylyzing..." etc.

image

@Holzhaus
Copy link
Member Author

In 6160a63 I replaced the painter string with an QLabel. That way we can customize the warning overlay label and maintain the styles for "Anaylyzing..." etc.

image

Wasn't the plan to customize it in the Skin XML code?

@ronso0
Copy link
Member

ronso0 commented Mar 30, 2020

Wasn't the plan to customize it in the Skin XML code?

Yup, but from a what I understood here and at the related bug report, we still want to drop tracks on the overview widget. With widgets on top of the overview that wouldn't be possible aynmore.
Much less work this way, only some qss lines for each skin : )

@Holzhaus
Copy link
Member Author

Alright, nice. I suggest we merge this PR as-is and do the styling in a separate PR too keep our PRs small and reviewable.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants