reimplement parameters reflecting metaknob when switching effects

[Be-ing]

This behavior was implemented in 5987290 in PR #1062 then broken by PR #1148. We are in feature freeze for 2.1 and the new feature proposed in #1148 has not been properly implemented. 2.1 is currently in a broken state, as reported by a new user. This reverts to the intended behavior from PR #1062, which extended the behavior of the superknob when switching effect chains in Mixxx 2.0 down to the metaknobs when switching effects.

[daschuer]

We cannot merge this like it is here, because it reintroduces an already fixed bug in master.

https://bugs.launchpad.net/mixxx/+bug/1335355

Remember that not all controllers have metaknob mapped.

I think all other arguments are outlined sufficient in #1148

So it is finally a question which bug is more important. For me as a user without a meta knob on the controller the behaviour in this PR is anoying.

[Be-ing]

The default of a parameter linked to the metaknob by default has no effective meaning. From the discussion in #1148 it seems that what you want is to couple changing the effect with setting it to parameters that make it go silent. That is redundant with the effect enable button and makes it cumbersome to test the sounds of different effects.

Most controllers that have controls for effects have finite range knobs. Controllers that use infinite encoders and touch strips are exceptions. The behavior in this PR works just as well with a mouse and keyboard as it does with controllers. That is not true for your proposed behavior.

[Be-ing]

I think we need more opinions here to break this deadlock. What do others think?

[daschuer]

The ideas outlined in #1148 are not too bad.

[ferranlala]

I think the user should have a preference option called "set default effect parameters on effect load".

If "set default effect parameters on effect load" is set to false:

• When a new effect is loaded all parameters including the metaknob remain at the current position

If "set default effect parameters on effect load" is set to true:

• All effect knobs are set to their default position. The meta knob is also set to a default position that is defined by each effect.

This is what I think the behaviour should be at the end. If this is a discussion about what the behaviour should be meanwhile we don't implement the definitive solution I cannot really help then :)

[ronso0]

I do agree with @ferranpujolcamins , an option would solve this issue. Not elegantly, but it's a solution.

For 99% of my Mixxx sessions I use a controller which has a Meta knob for each effect. In this scenario, it's useful to keep the Meta knobs position and adapt all linked parameters when loading an effect.

When loading a new effect without a controller attached (or with a controller not directly* providing Meta knobs) it's currently quite annoying to either do make sure the Meta 'has grip' on screen (since there's no default Meta knob position), or somehow toggle the controller mapping to do so.
Couldn't Mixxx detect whether a controller is attached, and if so, check if the mapping provides direct* Meta knobs?
*without having to go to a virtual second layer or using modifiers

[Be-ing]

Couldn't Mixxx detect whether a controller is attached, and if so, check if the mapping provides direct* Meta knobs?

No. I do not want the metaknob set to a random position when loading effects in any situation, whether I have a controller connected or not. The only use case I have for a default metaknob position feature would be right clicking the metaknob without a controller connected.

[Be-ing]

We need a decision on this for 2.1.

[ronso0]

The only use case I have for a default metaknob position feature would be right clicking the metaknob without a controller connected.

That's exactly what I mean.

[ronso0]

We need a decision on this for 2.1

I vote for making it optional.

[Be-ing]

The alternative option has not been implemented yet and we have a mountain of other things to take care of in the next two weeks for 2.1, so I think this should be merged as it is.

[ferranlala]

Traktor loads effect parameters defaults when loading a new effect. You can turn on/off this behaviour with a preferences option. Of course we don't have to do the same that Traktor does, but it means that a preference option is not a weird solution.

…

On Mon, 19 Mar 2018, 12:48 ronso0, ***@***.***> wrote: The only use case I have for a default metaknob position feature would be right clicking the metaknob without a controller connected. That's exactly what I mean. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1517 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGIcT7g6f_VqD_q7XoLj4bTg-LKuKfVmks5tf5sTgaJpZM4SPTib> .

[ferranlala]

If we agree to add a preference option in the future, I'm opening a launchpad bug for it not to fall in oblivion. Do we agree that a preference option is the way to go for post 2.1?

…

On Mon, 19 Mar 2018, 13:02 Be, ***@***.***> wrote: The alternative option has not been implemented yet and we have a mountain of other things to take care of in the next two weeks for 2.1, so I think this should be merged as it is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1517 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGIcT5bhzOtZu3BGko5bjPV7Tdb_r7K8ks5tf55hgaJpZM4SPTib> .

[Be-ing]

Yes, we already have a Launchpad bug for it.

[daschuer]

Since this is too controversial, let's move this from 2.1

[Be-ing]

No, we need to fix this for 2.1. The current behavior is nonsensical and trivially easy to fix.

[ronso0]

So far we have

• option to switch the behaviour: 2 votes
• keep as is: 1 vote
• change: 1 vote

If this decision can't wait for 2.2 (3 months from now) but has to be made for 2.1 (~2 weeks until release) it's clear, isn't it?

[Be-ing]

I agreed to have an option for this over a year ago after #1148 was prematurely merged, but that has not been implemented. It is now too late to do that for 2.1. The current state is broken, so we need a fix for 2.1.

[daschuer]

Is is not broken, it is just an unchangeable default setting that you do not like.
The option is a good solution for me, but it is to late to change it. I am also happy with the current state.
We have the current state now more than a year, without strong beta tester complains, so there is no neet to rush it in.

[Be-ing]

I think you are the only one happy with the current state. We have received complaints from myself, @ronso0, and @kshitij98. IIRC another user or two had complained on IRC as well and I directed them to the bug reports.

[Be-ing]

Honestly I am more upset about the bypass of our code review process with #1148 and then not fixing the situation for over a year than I am about the inconvenience of this broken behavior.

[Be-ing]

@ywwg any thoughts on this?

[Be-ing]

For me as a user without a meta knob on the controller the behaviour in this PR is anoying.

I am quite confused by this. If metaknobs are not mapped on your controller, then the problem is an out-of-date or incomplete controller mapping, not the effects system. It does not make sense to make metaknobs less intuitive to use because one controller mapping has not been updated to use them.

[daschuer]

This is nothing that should prevent a release. Moving by 6 month to the next release.

[Be-ing]

Please stop removing this from the 2.1 milestone. If you want alternate behavior, then when people raise objections, it is your responsibility to work with them to come to a solution and implement it. If you insist on making a release with the alternate behavior you proposed in #1148, then it is your responsibility to implement it in a way that does not break it for others. It is no problem for me to delay the release by a few days while you work on that. We still have a few other open issues anyway. If you don't want to work on that, then simply merging this PR as it is works for me. But leaving the situation as it is in the release does not work for me.

I have thought about the idea proposed in #1148 to calculate the default metaknob position from the linked parameters, but that does not feel like a good idea because it leaves open the possibility that multiple parameters will be linked in way that a good default cannot be calculated. Instead, I think Filter, Moog Filter, and Stereo Balance should specify their own default metaknob values with all other effects defaulting to 0. This will be forwards compatible in a nice way with custom per-effect defaults when we will be able to let users specify their own metaknob default.

[uklotzde]

Preamble: I didn't look at the code and just used Mixxx to explore the effects system. The following analysis is based on my own experience and describes both my expectations as a user as well as the viewpoint of a software developer.

I expect that the following common rule is respected when designing the integration of external hardware:

The internal state of the software should always follow and reflect the actual state of the corresponding hardware controls since we have no control on the physical counterparts.

Deviations from this rule should only be allowed when justified to avoid getting out of sync with the controller hardware.

The configuration of an effect consists of:

• Multiple parameter configurations
  • Knob position -> parameter value
  • Metaknob configuration: Range assignment + direction
• Metaknob position

Each effect provides a predefined default configuration. This default configuration might be replaced by a user-defined default configuration in the future. Assumption: The metaknob position of an effect configuration is consistent with the positions of all parameter knobs!

When loading an effect the metaknob configuration of each parameter is replaced by the default metaknob configuration. I assume that currently we don't have any corresponding hardware controls for these settings.

We have 2 options for the parameter values:

1. Adjust parameter values according to the current position of their corresponding knob
2. Discard all parameter knob positions and reset the corresponding parameters to default values

Since the effect parameters differ substantially between different effects we can justify to violate our common rule and choose option 2. We need to reset some of those parameter values anyway as we will see in a moment to avoid inconsistencies.

We have 2 options for the metaknob's position:

1. Adjust all parameter values (= parameter knob positions) according to the current metaknob's position
2. Discard the current metaknob's position and reset the metaknob's position to the default value

If we would choose option 2 then all parameters are consistent according to our assumption about the consistency of effect configurations. But then again we discard a precious knob position and violate our self-imposed rule. In this case I suggest to implement option 1 (i.e. keep the current position of the metaknob) and adjust all effect parameter values according to their current metaknob configuration that has just been loaded. We have already discarded their position while resetting to a default value, so it doesn't matter to adjust them again.

I expect the most common use case is to just show/use the metaknob and keep the parameter knobs/values hidden. The strategy I proposed will give you reasonable behaviour when switching effects and avoids unexpected surprises. The common rule for not discarding knob positions if possible is only violated for parameter knobs that may be invisible during performance. By introducing user-defined effect defaults users will be able to map the metaknob in a way that works best when switching between their favourite effects.

[daschuer]

@uklotzde: Thank you for your summarize. So we need an option to switch between 1 and 2 for the metaknob. 1. makes sense if you have a controller with mapped, non endless metaknobs 2. makes sense for all other cases. Right?
OK I will prepare a PR.

[daschuer]

See Be-ing#17

[Be-ing]

Oops, sorry I meant to press the merge button on
Be-ing#17
@daschuer could you reopen your PR targeted at the upstream 2.1 branch?

[Be-ing]

Nevermind, I'll open a new PR with a few small changes