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

Add stem controls #13086

Merged
merged 11 commits into from
Jul 30, 2024
Merged

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Apr 10, 2024

Depends on #13106

This adds basic COs and UI for stem volume control. Manuel PR is here

LateNight:

Kooha-2024-04-16-21-21-09-10MB.mp4

QML:

Kooha-2024-04-11-12-39-08.mp4

@JoergAtGithub
Copy link
Member

Looks really cool!

I think we need a table with all planed stems controls and all existing stems hardware to ensure, that we can cover all

@acolombier
Copy link
Member Author

I think we need a table with all planed stems controls and all existing stems hardware to ensure, that we can cover all

One of the reasons why I added the stem feature flag is so we can iterate on features and controls, definitely worth having them layed out for reference tho! Shall we create an epic like we did for S4 screens? Could you help with that since I believe it requires special permission?

  • Has small text displays, that show the name of the stem and the effect-chain assigned to the stem

That's the same with S4, which is part if the reason why I wanted to have the QML proxy in place. As it stands, you can already get the label for each stem.

  • Has stem-split button which connects one deck with multiple mixer channels

I'll have to look how that works and see if that something we can support.

@daschuer
Copy link
Member

I wonder where unity is on the stem knobs. My original expectation is to put it on center like EQs.
This allows to accentuate a stem, compared to others and allows switching between EQ and Stem knobs without changing scale.

Do we want to keep this switching in the mapping domain, or do we want to add four mixer meta knob that can be mapped and will switch between EQ and Stem.

By the way we had a similar discussion with the fourth EQ band, gain and filter knob.

@acolombier
Copy link
Member Author

acolombier commented Apr 11, 2024

  • Has stem-split button which connects one deck with multiple mixer channels

I'll have to look how that works and see if that something we can support.

I just watched the review - that piece of kit looks very good btw - the split button seems to perform a deck clone, and just mute the vocal, then mute everything else on the newly clone deck. IIRC, the NI stem standard suggests that the drum should always be first and the vocal always be last, so I believe this would be pretty trivial to do! We might need new CO to target deck when cloning tho (I believe at the moment, it pick a candidate out the last focus deck)

Do we want to keep this switching in the mapping domain, or do we want to add four mixer meta knob that can be mapped and will switch between EQ and Stem.

Perhaps I can add a setting in the "Interface" section? Something like "Display stem volume control instead of EQ for stem deck"? This way, user without controllers or stem support on them can still play with it? Otherwise, I think by default, we should keep stem control in the mapping domain, exception perhaps of the waveform, but that we come in a next chapter, where I start looking at analyser

Edit: I fixed the offset change, leading to un-centered crossfader and added a little animation

Kooha-2024-04-11-12-39-08.mp4

@acolombier acolombier force-pushed the feat/add-stem-controls branch from de099b9 to c9f2c2d Compare April 11, 2024 11:43
@vantaboard
Copy link

vantaboard commented Apr 11, 2024

Been thinking about this implementation a lot lately. What do we think about this design?
mixxx stem idea

Basically, you would select either the Main stem which would be all of the stems together, or you would pick one of them individually or group them together by holding down shift or something. Then, the EQ would apply to only the selected stems. Same with scratching, you could have a stem-sync mode where the stems are linked together, or you could scratch a stem independently, like if you wanted to scratch Vox. You could also put a way to re-sync the stems back to the main track which acts like the leader.

@acolombier
Copy link
Member Author

That's a good design, but perhaps this should come a second iteration, when the waveform also supports multiple stem layer? I can imagine the selection/hover of the stem button will "highlight" the stem signal on the waveform, which may be very handy to identify section of track with a specific feature on, say no vocals or instrumental solo

EQ would apply to only the selected stems.

Not sure this is something we will be able to do due to performance impact. EQ will likely remain applicable to the deck as whole. This means you can still spare a second deck for the voice and adjust the EQ there

Same with scratching

Scratching requires transport/playback control which would go against the logic that was implemented here, so definitely won't be possible. Here again tho, you can clone the deck and scratch just a stem (or set of them) on the individual deck, similar to the demo @JoergAtGithub shared above on Serato (?)

@vantaboard
Copy link

vantaboard commented Apr 11, 2024

That's a good design, but perhaps this should come a second iteration, when the waveform also supports multiple stem layer? I can imagine the selection/hover of the stem button will "highlight" the stem signal on the waveform, which may be very handy to identify section of track with a specific feature on, say no vocals or instrumental solo

Yeah, I agree. Definitely giving the ability to visualize the layered stems would be great, especially if we could have the waveform color in alignment with the stem color.

Scratching requires transport/playback control which would go against the logic that was implemented here, so definitely won't be possible. Here again tho, you can clone the deck and scratch just a stem (or set of them) on the individual deck, similar to the demo @JoergAtGithub shared above on Serato (?)

Same with scratching and EQ, I agree. I guess I just find cloning a bit cumbersome at the moment. Perhaps in another iteration we can cue stems for cloning, so that a clone from deck A to deck B with a cue on vocals would mute the vocals on deck A and solo them onto B with seamless playback. This would be ideal because there are some transitions where I want to continue some portion of the music into a new track, but cloning feels clunky at the moment.

@vantaboard
Copy link

I think I have a tendency to want to avoid creating a similar design to what @JoergAtGithub posted https://youtu.be/Pf3GQdkWByk?t=1555 because the doubling up can be really annoying and it isn't optional even with cloning in the current state.

@acolombier
Copy link
Member Author

Yeah, I agree. Definitely giving the ability to visualize the layered stems would be great, especially if we could have the waveform color in alignment with the stem color.

That will come in future PR tho! I've been trying to keep PR shot to help ease and speed up the review.

I guess I just find cloning a bit cumbersome at the moment. Perhaps in another iteration we can cue stems for cloning, so that a clone from deck A to deck B with a cue on vocals would mute the vocals on deck A and solo them onto B with seamless playback.

Yes, that's the plan, very similar to the "split stem" feature from that demo. This way you easily "dive" in the track stem and have complete freedom on your mixing, without convoluting the deck option and control

the doubling up can be really annoying and it isn't optional even with cloning in the current state.

Agreed. When we would look into this, I think we will make sure that no doubling up gets hearable - decks get cloned but stems are exclusive to avoid that gain issue (say A plays 0 1 2, B plays 3).

@vantaboard
Copy link

EQ would apply to only the selected stems.

Not sure this is something we will be able to do due to performance impact. EQ will likely remain applicable to the deck as whole. This means you can still spare a second deck for the voice and adjust the EQ there

Is EQ for the whole deck still accessible somewhere in your design? I'm wondering if it even makes sense to be able to control volume for each stem in that case. Maybe my design would be mute/unmute for the different stems rather than selection.

mixxx stem idea

@acolombier acolombier force-pushed the feat/add-stem-controls branch from c9f2c2d to eaefb08 Compare April 11, 2024 22:22
@vantaboard
Copy link

Is the skin you're using available anywhere? I have built the branch, but I don't have any options to load a stem.

@JoergAtGithub
Copy link
Member

What the Rane Four (and other Serato stem controllers) do is bassically a mapping between stem channels playing in any deck and mixer channels. Similar, to what we do with effect units. But exposing this in the UI similar to the FX1,FX2 buttons might be overwhelming.
Another approach would be to visually merge 2 decks if the user presses the Stem Split button on the controller. If there is only one deck, there can't be a conflict of two decks outputs connected to the same mixer channel.

@daschuer
Copy link
Member

Does the Rane Four use transport controls of deck one for deck three in split mode? That sounds kind of reasonable if you want to scratch the whole Track. Maybe we can consider two split modes like coupled and Independent. That is almost like our sync scratch mode we have abandoned for Mixxx 2.4

@acolombier
Copy link
Member Author

acolombier commented Apr 12, 2024

Is the skin you're using available anywhere? I have built the branch, but I don't have any options to load a stem.

This is the new UI which isn't yet production ready and is lacking some basic features. You can see it by building with QT6 and QML, then run mixxx with --qml

@JoergAtGithub
Copy link
Member

Does the Rane Four use transport controls of deck one for deck three in split mode?

In Sertao DJ this is not the case, in the Rane Four video, I posted already above, you can see scratching of the deck with the vocal stem, while the deck with the instrumental stems is playing at normal speed: https://www.youtube.com/watch?v=Pf3GQdkWByk&t=1060s

Maybe we can consider two split modes like coupled and Independent.

Might make sense, we should write down the use cases first.

@acolombier
Copy link
Member Author

So I guess my suggestion on creating a variant of clone deck might make sense, right? I assume that the split stem feature only work if the sibling deck is free, so we could do something similar.

Might make sense, we should write down the use cases first.

That's a fair point. Could you put this into an issue maybe so we can gather all different usecase to expect around stem? I've been trying to keep the stem PRs fairly atomic - one step at a time - so I won't be doing any "advanced" controls in this PR. That "split stem" is definitely something I will look into as that definitely would work well on my S4 Mk3

@JoergAtGithub
Copy link
Member

We should clarify in general, where to put the documentation of the GSoC projects.

Maybe a STEM-Controls proposal at https://github.com/mixxxdj/proposals might be also suitable here?

@acolombier acolombier force-pushed the feat/add-stem-controls branch from eed35f2 to a17c578 Compare July 25, 2024 18:54
Co-authored-by: JoergAtGithub <[email protected]>
Co-authored-by: Daniel Schürmann <[email protected]>
@acolombier acolombier force-pushed the feat/add-stem-controls branch from a17c578 to de5bfaf Compare July 25, 2024 19:16
@acolombier
Copy link
Member Author

@daschuer @JoergAtGithub I took the freedom to remove the legacy UI wiget as it looks like we are getting closer to merge this PR. If you need to do more testing, the widget is available here.

I have kept the QML part as this one is pretty functional. Arguably, we may want to change the layout in the future, but the fact that nothing happens in QMl for the last 6 months makes me sad so I'd like to have stem in there, if that's okay! :)

@acolombier acolombier requested a review from JoergAtGithub July 28, 2024 10:40
@JoergAtGithub
Copy link
Member

The code in this PR is LGTM now, but I miss test cases for the new COs. could you please add some basic tests for them.

@acolombier acolombier force-pushed the feat/add-stem-controls branch from f5f6dd9 to ffeca7e Compare July 30, 2024 07:56
@acolombier
Copy link
Member Author

@JoergAtGithub I added some tests now that should cover all new stem COs

@daschuer
Copy link
Member

Thank you for the next great step.

Do we have plans regarding a widget based skin?
I am afraid without it, it will take a while until stem support is usable for all users.

@acolombier
Copy link
Member Author

We've been discussing that in the draft PR with the widget based skin feature. The TL;DR is no easy way to support all themes correctly, but we could release the stem control under some kind of "opt-in" alpha and address some of the common issues with it

@daschuer
Copy link
Member

Ah Ok. Yes and even a single Stem skin would be reasonable. Maybe it attracts other skin designers to help with the rest.

Comment on lines 23 to 35
m_pStem1Volume = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_1_volume");
m_pStem2Volume = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_2_volume");
m_pStem3Volume = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_3_volume");
m_pStem4Volume = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_4_volume");
m_pStem1Mute = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_1_mute");
m_pStem2Mute = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_2_mute");
m_pStem3Mute = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_3_mute");
m_pStem4Mute = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_4_mute");
m_pStem1Color = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_1_color");
m_pStem2Color = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_2_color");
m_pStem3Color = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_3_color");
m_pStem4Color = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_4_color");
m_pStemCount = std::make_unique<PollingControlProxy>(m_sGroup1, "stem_count");
Copy link
Member

Choose a reason for hiding this comment

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

Before I merge this PR, which introduces new COs, I would like to hear your opinion about the naming scheme of the stem COs ( the full list can be found in the description of #13116 ).
Do you prefer the style:
[Channel1],stem_1_volume or [Channel1Stem1],volume
The first has the benefit, that the group name is similar to other COs in the mappings,
the second would have the benefit, that we seperate channel and function clearer and we could add it as just another group syntax to the documentation of some existing COs.
The first syntax would be in line with [Channel1],hotcue_X_color, the second with [QuickEffectRack1_[ChannelXStemY]],super1.
If we go with the first syntax, than I think [QuickEffectRack1_[Channe1Stem1]],super1 should become [QuickEffectRack1_[Channel1]],stem1_super1 for consistency.
What's your opinion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I think using [ChannelXStemY] globally would make a lot of sense, but I remember stumbling across something and settling for [ChannelX],stem_Y_....
Now that my Mixxx knowledge has grew, I will retry and see if I can get ChannelXStemY working for consistency.

Copy link
Member Author

@acolombier acolombier Jul 30, 2024

Choose a reason for hiding this comment

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

Ok, that was surprisingly easy finally, I do not know how come I didn't do this before 😅
Also updated the manual and will update the epic right away.
Note that I have opted for track_color so it matches 1-to-1 with the other channels

@acolombier acolombier force-pushed the feat/add-stem-controls branch from 2a3b142 to 0a4ba6d Compare July 30, 2024 19:03
@JoergAtGithub
Copy link
Member

Thank you very much!

@JoergAtGithub JoergAtGithub merged commit 9b31059 into mixxxdj:main Jul 30, 2024
13 checks passed
@acolombier
Copy link
Member Author

Thanks for your review! Next stop is either #13268 or #13123

"StemVolumeControlFull");
}

TEST_F(StemControlTest, VolumeResetOnLoad) {
Copy link
Member

Choose a reason for hiding this comment

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

For some reason this test failed here
https://github.com/mixxxdj/mixxx/actions/runs/10981245295/job/30488031679?pr=13077#step:11:1823
(in case the link doesn't work, the PR is #13077)

any idea why @acolombier ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. It looks like it is failing with an unrelated assertion, not quite sure why this would happen

Copy link
Member

Choose a reason for hiding this comment

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

oh, I see it's loading one of the dummy tracks. sorry for the noise then

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

Successfully merging this pull request may close these issues.

6 participants