add per-effect meta knobs by Be-ing · Pull Request #1062 · mixxxdj/mixxx

Be-ing · 2016-12-04T22:36:46Z

Add a new ControlPotmeter for each Effect within an EffectUnit called "meta". This acts just like the EffectUnit super1 CO for each effect and shares the linking with the superknob. It will enable skins and controller mappings to implement a practical way to make use of effect chains. A new ControlPushbutton for each EffectUnit, called "expanded", is also being added to signal to skins and controller mappings whether to control the meta knobs of each effect in the chain (expanded = 0, default) or individual parameters of one effect at a time (expanded = 1). I have not yet reworked the skins' effects UIs; this PR is just for the underlying COs. For now, if you want to test it, you can map knobs on a controller to the new meta CO.

The superknob for the whole chain still exists for controllers that only only have one knob/encoder for effects and for users who only use a keyboard & mouse. The superknob now acts to adjust the meta knobs of each of the effects in the chain.

I'm not particularly tied to the name "meta". If you have a better suggestion for the name, please share.

fixing https://bugs.launchpad.net/mixxx/+bug/1575973

…ing effects

daschuer · 2016-12-06T22:05:47Z

            this, SLOT(slotChannelStatusChanged(const QString&)));
+
+    // It is up to skins and controller mappings to do anything useful with this.
+    m_pControlExpanded = new ControlPushButton(ConfigKey(m_group, "expanded"));


I am not sure if this CO fits quite well here. If it is pure GUI "expanded" control, it can (should) be implemented inside the GUI.

But it looks like you want to do more with it. It can be uses to switch the effect mappings as well.

I am not sure if, and how we can achieve this, but I thing it is more a "view_mode" or "knob_layout" enum type.
The controller might be able to tell Mixxx which mode it supports, or switch between these modes.
This can be growing by design to be future proof.

What are typical Controller Effect Layouts?

Hercules RMX2: Four pressure detecting buttons + one endless knob.

Denon HS5500: three toggle buttons + one endless knob

vestax_vci-400: Trakor optimized fx region with mode switch.

....

The point of this CO is to have a way for skins and controller mappings to stay in sync with what controls they are exposing. I have made a generic EffectUnit ControlContainer for the P32 mapping that makes use of this. It provides a framework for the 3 knobs + 3 buttons + 4th knob/encoder + additional button layout common on most mid-grade and high-grade controllers (such as the Hercules P32, Denon MC6000 (ping @uklotzde) & MC4000 (ping @timrae), Pioneer DDJ-SR & RR, Traktor Kontrol S2/S4/X1, Vestax VCI-400).

For controllers with fewer controls like the Pioneer DDJ-SB(2), it will may make most sense to not use the metaknobs and just use the superknob for the whole EffectUnit with enable buttons for the individual effects within the unit, regardless of the status of the EffectUnit's expanded CO. On the other hand, for the Numark Mixtrack (Pro) 3 (see PR #1014 , ping @djsteph), the metaknobs could be adjusted by holding an effect button down and manipulating the touch strip, with the buttons acting as enable/disable buttons when not held down.

Ah OK I understand. Than this is basically the "mode" switch on a vestax vci-400 and the "FX Mode" switch on Pioneer DDJ-SR & RR right?

In addition, I think we need a focus control, that allows to highlight the effect in the GUI that is currently controlled in "single mode". On the Pioneer DDJ-SR & RR it is called "FX Select"

Ah OK I understand. Than this is basically the "mode" switch on a vestax vci-400 and the "FX Mode" switch on Pioneer DDJ-SR & RR right?

Correct.

In addition, I think we need a focus control, that allows to highlight the effect in the GUI that is currently controlled in "single mode". On the Pioneer DDJ-SR & RR it is called "FX Select"

That would be great, but AFAIK there isn't any way to really make use of that in skins yet. It would be great to be able to show a box around the selected effect like Ableton can do with the active areas in its Live View.

I think we already can. At least we can switch between to widgets, the focused one and the normal.

But it should also be possible to us a property binding to set the background or border property. If all this does not work, It is no deal to invent a new "focusableWidgetGroup" or something.

Also related is allowing controllers to create controls:
https://bugs.launchpad.net/mixxx/+bug/374239

IMO if we have skins and and controllers creating COs to communicate it is probably because we have failed at designing an interface that serves their purposes. We already have a mess with controller scripts including user-configurable options to change their behavior to correspond better with different skins. Let's not dig deeper into that hole.

We do need a way for controller mappings to communicate with each other, but that's a separate issue.

Right, it's the blessing and curse of having a skin system -- everything is convention. One thing we've learned is that baking UI-concerns into the backend is a form of technical debt.

If you define these COs in the skin attributes section, I think the outcome would be the same as if we have the CO here, would it not? Skins we ship with would support it, skins we don't ship with won't (same as if we did it in C++). The difference would be the C++ implementation of the effects system doesn't have to worry about this skin/controller interaction.

We could start a wiki page describing a skin/controller CO interface with some well-defined controls for visual indicators (like focus and expansion state). Then controllers and skin authors could have a common standard to build against.

Okay, that makes sense. I can move it into each of the official skins. I think good documentation for skin authors would solve the issue (see also https://bugs.launchpad.net/mixxx/+bug/1647221 ).

Cool -- thanks. Indeed, better documentation is needed!

Be-ing · 2016-12-07T04:15:29Z

Any other suggestions for the name other than "metaknob"?

rryan · 2016-12-07T18:09:33Z

Any other suggestions for the name other than "metaknob"?

I don't have anything better.. though maybe it'd be less confusing in a way to call it an "effect superknob" so we have a chain superknob and effect superknob. That way you kind of understand that they perform similar roles since they have a similar name (whereas with super vs meta you might wonder if there was some difference between them other than controlling the whole chain vs effect). And it wouldn't be confusing that turning the chain superknob would also turn the effect superknobs. Or maybe that's a terrible idea, I can't tell :).

Be-ing · 2016-12-07T18:29:38Z

Hm, interesting idea. I think either way is going to be somewhat confusing. I'll mull on the naming a bit before this is merged...

Be-ing · 2016-12-07T18:36:20Z

Does the code otherwise look good?

Be-ing · 2016-12-07T18:49:46Z

Here's a thought for naming: call them all superknobs. In the UI, label the chain superknob as "CHAIN" and the effect superknobs as "SUPER".

A downside to this approach is that it may confuse users who are used to Mixxx 2.0. I think the growing pain would be worth it though.

daschuer · 2016-12-07T19:46:11Z

I think we should think it all over ... I will write a longer comment.

daschuer · 2016-12-07T20:58:27Z

Here it is:

The new "expanded" CO not persist over a skin change, this means that the skin <-> controller interaction will be broken after the user changes a skin. So if we consider that "expanded" CO is a skin CO, we need some code, that it outlives a skin change. But lets discuss the skin <-> controller interface fist, since the new "expanded" CO is only a fraction of the whole and the purpose is not that clear from its name:

The main goal of this PR it to get good support of "Pioneer DDJ-SR" effect regions, which finally allows to map many other controllers nicely.

One question is: Should we define the interface for the controller model or the skin model?

The skin interaction is required, because we want to make sure that the single parameters of an effect are shown when the controller is in "single parameter mode". This could be probably express by these controls:
[EffectRack1_EffectUnit1],controller_single_mode
[EffectRack1_EffectUnit1_EffectX],controller_focus

On the other hand we have th Skin, with a controllable layout:
[EffectRack1_EffectUnit1],show_effect_parameter
[EffectRack1_EffectUnit1_EffectX],highlighted
However it is no requirement that all effect parameters of all effects are shown. It would be sufficient to show the parameters of the focused effect. It is also noting against always showing all parameters.

In general we have much more freedom in skinning, that makes a standard CO interface for the skin model hard to define.

So we should probably switch over to a CO interface for the controller model.

How about that:

[EffectRack1_EffectUnit1],controller_single_mode
0.0:

skin may show the "effect super knobs"
the controller knobs are controlling the "effect super knobs"
1.0:
skin may show the" parameter knobs" of the focused effect
the controller knobs are controlling the "parameter knobs" of the focused effect
a single knob controller is controlling the "effect super knob" of the focused effect

[EffectRack1_EffectUnit1_EffectX],controller_focus
all 0.0:

No effects highlighted
one 1.0:
One effect highlighted

IMHO these controls should be defined in C++ to avoid issues when changing skins or midi mappings.
We may also consider to move some of the supporting js code to the C++ domain as well.

In Jonas branch, we have the code to focus and style a widget using a QObject property.
This can be reused here for highlight the focused effect. I will prepare a separate PR for it.

What do you think?

Be-ing · 2016-12-07T22:26:01Z

[EffectRack1_EffectUnit1],controller_single_mode

I don't think the CO should specifically refer to controllers or skins in its name. How about [EffectRack1_EffectUnit1],single_effect_focused?

[EffectRack1_EffectUnit1_EffectX],controller_focus

I think the focus CO should be moved up to the EffectUnit level, rather than being a binary CO on each effect. This means only one effect could be focused, but if more than one effect was focused, well, it wouldn't be focus. :P

However it is no requirement that all effect parameters of all effects are shown.

Good point. I had not thought of that. It would save vertical space in the skins if only the parameters for the one focused effect was shown, with the meta/superknobs for each effect shown like in condensed view.

a single knob controller is controlling the "effect super knob" of the focused effect

Ah, I understand your idea now. With that, a single push encoder could provide pretty good control over an effect chain (map pushing it to shifting the focus).

daschuer · 2016-12-08T09:27:08Z

[EffectRack1_EffectUnit1],single_effect_focused

nice!

I think the focus CO should be moved up to the EffectUnit level, rather than being a binary CO on each effect. This means only one effect could be focused, but if more than one effect was focused, well, it wouldn't be focus. :P

This comes at the cost that skinning will be a bit harder, because we cannot just bind the CO to a widget group we need to explicit assign it to an index. But we can do it.
This will allow us to use the value 0 for group mode.
If you consider to merge the new concept "learnable", it would be better to have binary COs.

Ah, I understand your idea now. With that, a single push encoder could provide pretty good control over an effect chain (map pushing it to shifting the focus).

The same here, If we like to have it learn-able, we need a kind of Focus controller in the C++ domain.
But since it is software we can implement is on demand and implement here just a single
[EffectRack1_EffectUnit1],single_effect_focused per Unit which does not clutter the interface too much.

Be-ing · 2016-12-08T16:32:27Z

This comes at the cost that skinning will be a bit harder, because we cannot just bind the CO to a widget group we need to explicit assign it to an index. But we can do it.

Putting the CO at the effect level comes at the cost of controller scripting being a little harder. Scripts would have to iterate over each effect to set a different focus. This would be a little cumbersome, but easier to handle in JS than in skins. Or, we could have COs both at the EffectUnit (range 0-3) and Effect (binary) levels that the C++ keeps in sync with each other to spare scripts the trouble of iterating over each Effect.

For example, suppose effect button 2 is pressed on a controller when EffectUnit,single_effect_focused is 1. The mapping would set EffectUnit,focused_effect to 2. focused_effect's valueChanged signal would be connected to a function that sets the Effect,focused CO of the previously focused effect to 0 and the new one's Effect,focused CO to 1. The MIDI output callback functions of the mapping would be connected to the Effect,focused COs.

daschuer · 2016-12-09T22:32:35Z

I have just published my highlight widget branch, which can help to continue here.

Lets go for "[EffectRack1_EffectUnit1],single_effect_focused" per EffectUnit. This reflects the underlying problem the best. Th GUI issue is solved now and this Unit level CO should fit best th the controller script. For now this variable has to become again a C++ viable to work as desired.

Be-ing · 2016-12-15T23:39:49Z

This comes at the cost that skinning will be a bit harder, because we cannot just bind the CO to a widget group we need to explicit assign it to an index. But we can do it.
This will allow us to use the value 0 for group mode.

Thinking about this more, it will not provide a good user experience if a single CO is used to indicate both whether the EffectUnit is in expanded or collapsed mode and which effect is focused if it is in expanded mode. If an index of 0 indicates collapsed mode, there will be no way to tell which effect to focus when switching into expanded mode again. For example, if Effect2 is focused, then the EffectUnit is switched to collapsed mode, then back to expanded mode again, skins and controllers will not know to select Effect2 when going back to expanded mode. Also, it would require an effect to be focused to be in expanded mode, which would be odd for users without controllers for whom the effect focus is meaningless. There should be

a binary CO at the EffectUnit level indicating expanded or collapsed mode,
a numeric CO at the EffectUnit level indicating which effect is focused, and
a binary CO at the Effect level indicating if that effect is focused.

Be-ing · 2016-12-19T00:16:20Z

a binary CO at the EffectUnit level indicating expanded or collapsed mode,

a numeric CO at the EffectUnit level indicating which effect is focused, and

a binary CO at the Effect level indicating if that effect is focused.

I realized 2 is unnecessary with the C++ side automatically managing 3. 1 can stay in skins.

as per discussion on PR mixxxdj#1062

rryan · 2017-01-07T20:37:46Z

This LGTM -- @daschuer meta soft takeover is now in #1111, ok?

daschuer · 2017-01-07T23:28:44Z

I will test it again.

daschuer · 2017-01-08T01:14:18Z

Meta and Super should share the same default value, to avoid the out of sync state after start up.

daschuer · 2017-01-08T20:41:33Z

There is the pending issue with using the focused_effect as GUI button, but since it works currently nice as read only button, we can merge fix this "feature" later.
Thank you for all the effort! LGTM.

Be-ing · 2017-01-26T18:49:02Z

I documented the new ControlObjects on the wiki.

as discussed in PR mixxxdj#1062

ronso0 · 2017-03-08T17:39:20Z

One question on show_focus & show_parameters:
Why are show_focus & show_parameters linked when using midi-components.js, even though an effect still has focus from expanded view?
I can still control single parameters but I have no on-screen indicator which effect is focused. This just doesn't make sense to me...

Be-ing · 2017-03-08T18:09:51Z

That is a bug in Components. Thanks for reporting that.

ronso0 · 2017-03-08T18:26:12Z

Puuuh..okay
And I need some (quick) help on mapping the shift button with Components JS. I only want to map the FX units with it, buttons & knobs are already working :) Where's the best place to do that?

Be-ing · 2017-03-08T18:36:41Z

I fixed the bug in #1211. You could pull that into a personal branch or just copy and paste the single line that changed into your copy of midi-components-0.0.js. FYI I may end up changing the name of EffectUnit.showParametersButton to EffectUnit.effectFocusButton, but other than that I don't plan on changing the public API of EffectUnit.

And I need some (quick) help on mapping the shift button with Components JS. I only want to map the FX units with it, buttons & knobs are already working :) Where's the best place to do that?

In your function that handles your shift button, call the EffectUnit object's shift and unshift methods (no arguments are necessary) on button down/up.

ronso0 · 2017-03-08T19:44:10Z

I have Effect unit pretty much set up like in your docu:
TerminalMix.effectUnit1 = new components.EffectUnit(1);
TerminalMix.effectUnit1.enableButtons[1].midi = [0x90, 0x07];
...
and I have
TerminalMix.shiftButton = function (channel, control, value, status, group) {
if (value === 127) {
[what goes here?].shift();
} else
...

In the P32 example it's a child of P32.Deck but stand-alone I have no clue.
If you plan to update the docu, then there's no need to escalate this here.

Be-ing · 2017-03-08T19:45:17Z

TerminalMix.effectUnit1 would go in [what goes here?]

ronso0 · 2017-03-08T20:15:51Z

Tried that already, doesn't work.
Those two blocks are the only additions I made to the default script.
Maybe the forum is a better place to work this out, IDK?

Be-ing · 2017-03-08T20:20:17Z

Yes, please post your whole script on the forum. It will be easier for others to find there.

ronso0 · 2017-03-08T20:42:48Z

Done. http://www.mixxx.org/forums/viewtopic.php?f=7&t=9015

This behavior was implemented in 5987290 in PR mixxxdj#1062 then broken by PR mixxxdj#1148.

add per-effect meta knobs

bea347d

Be-ing force-pushed the per_effect_metaknobs branch from c87d170 to bea347d Compare December 4, 2016 22:55

make EffectUnitX,expand a toggle button

4d9bce6

Be-ing mentioned this pull request Dec 5, 2016

new Deere effects units with meta knobs #1063

Merged

Be-ing force-pushed the per_effect_metaknobs branch from 5c1ed5d to db97501 Compare December 5, 2016 23:53

make meta knobs update when super knob is turned

070838f

Be-ing force-pushed the per_effect_metaknobs branch from db97501 to 070838f Compare December 5, 2016 23:54

Be-ing mentioned this pull request Dec 6, 2016

Hercules P32 mapping and Controls JS library #1054

Merged

make effect parameters reflect current position of metaknob when load…

5987290

…ing effects

daschuer reviewed Dec 6, 2016

View reviewed changes

move EffectUnits' expanded CO to skin.xml files

d755e50

Be-ing force-pushed the per_effect_metaknobs branch from cca91d9 to d755e50 Compare December 7, 2016 16:07

daschuer mentioned this pull request Dec 9, 2016

Highlight feature for WidgetGroup #1068

Merged

This was referenced Dec 11, 2016

Dj compact #918

Merged

Reloop terminal mix2 cleanup #1076

Merged

Be-ing force-pushed the per_effect_metaknobs branch 2 times, most recently from c4943b1 to 04c4f9f Compare December 19, 2016 01:25

Be-ing added a commit to Be-ing/mixxx that referenced this pull request Jan 6, 2017

remove use of meta_force_next_update

3ef1298

as per discussion on PR mixxxdj#1062

small code cleanup

101b6cd

Be-ing mentioned this pull request Jan 7, 2017

Metaknob soft takeover #1111

Merged

set metaknob default value to same as superknob

6089239

daschuer merged commit 227ed80 into mixxxdj:master Jan 8, 2017

Be-ing deleted the per_effect_metaknobs branch January 27, 2017 00:08

This was referenced Jan 27, 2017

Remove adopting Meta knob after effect load #1148

Merged

SELECTMULTI ControlPushButtonBehavior #1160

Closed

Be-ing added a commit to Be-ing/mixxx that referenced this pull request Feb 3, 2017

Deere: implement new effect interface

2d842d4

as discussed in PR mixxxdj#1062

Be-ing mentioned this pull request Mar 8, 2017

Components EffectUnit rework #1211

Merged

2 tasks

Be-ing mentioned this pull request Mar 22, 2017

M-Audio Xponent, advanced mapping #1194

Closed

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

Be-ing added a commit to Be-ing/mixxx that referenced this pull request Feb 22, 2018

reimplement parameters reflecting metaknob when switching effects

723f062

This behavior was implemented in 5987290 in PR mixxxdj#1062 then broken by PR mixxxdj#1148.

Be-ing mentioned this pull request Feb 22, 2018

reimplement parameters reflecting metaknob when switching effects #1517

Merged

This was referenced Aug 22, 2022

Add focus function for the effect superknob #7391

Closed

per-effect superknobs #8525

Closed

Uh oh!

Conversation

Be-ing commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rryan Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Dec 7, 2016

Uh oh!

rryan commented Dec 7, 2016

Uh oh!

Be-ing commented Dec 7, 2016

Uh oh!

Be-ing commented Dec 7, 2016

Uh oh!

Be-ing commented Dec 7, 2016

Uh oh!

daschuer commented Dec 7, 2016

Uh oh!

daschuer commented Dec 7, 2016

Uh oh!

Be-ing commented Dec 7, 2016

Uh oh!

daschuer commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Dec 9, 2016

Uh oh!

Be-ing commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rryan commented Jan 7, 2017

Uh oh!

daschuer commented Jan 7, 2017

Uh oh!

daschuer commented Jan 8, 2017

Uh oh!

daschuer commented Jan 8, 2017

Uh oh!

Be-ing commented Jan 26, 2017

Uh oh!

ronso0 commented Mar 8, 2017

Uh oh!

Be-ing commented Mar 8, 2017

Uh oh!

ronso0 commented Mar 8, 2017

Uh oh!

Be-ing commented Mar 8, 2017

Uh oh!

ronso0 commented Mar 8, 2017

Uh oh!

Be-ing commented Mar 8, 2017

Be-ing commented Dec 4, 2016 •

edited

Loading

Be-ing Dec 6, 2016 •

edited

Loading

rryan Dec 6, 2016 •

edited

Loading

daschuer commented Dec 8, 2016 •

edited

Loading

Be-ing commented Dec 8, 2016 •

edited

Loading

Be-ing commented Dec 15, 2016 •

edited

Loading

Be-ing commented Dec 19, 2016 •

edited

Loading

Be-ing commented Mar 8, 2017 •

edited

Loading