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

Effect Button Parameters #281

Merged
merged 8 commits into from
Jul 21, 2014
Merged

Conversation

badescunicu
Copy link
Contributor

This PR adds button support for Effect Parameters.


////////////////////////////////////////////////////////////////////////////////
// Controls exposed to the rest of Mixxx
////////////////////////////////////////////////////////////////////////////////
Copy link
Member

Choose a reason for hiding this comment

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

We do not use ASCII-arts in Mixxx code

@daschuer
Copy link
Member

daschuer commented Jul 1, 2014

LGTM! Thanks.
This is required for kill buttons of EQ effects.

It would be nice to have EQ effects with kill buttons in 1.12.
So we may merge this. Any objections?

@ywwg
Copy link
Member

ywwg commented Jul 9, 2014

I'm going to require that @rryan look at this because he's got Big Plans for the effects framework.

@daschuer
Copy link
Member

This is kind of blocking for the other EQ patches. @rryan what about merge this now and fixing issues in master if you find any?

@rryan
Copy link
Member

rryan commented Jul 15, 2014

:( I think most of this code is unnecessary. The manifest already has support for control types:
https://github.com/mixxxdj/mixxx/blob/master/src/effects/effectmanifestparameter.h#L21
All that should be needed for this is handling in EffectParameter / ControlEffectKnob (which should be renamed ControlEffect probably if it grows to support button/toggle handling).

@daschuer
Copy link
Member

Hi @RJ

Nicu has careful considered both ways and has actually tried to go the outlined way see: http://sourceforge.net/p/mixxx/mailman/message/32418908/

But this has lead to unmaintainable code with a monster effect parameter CO.
On the other hand with this solution we are able to reuse the normal pushbutton button CO.
There are also advantages in the effect GUI, because you can line up buttons and knobs as the design requires, since you know the appearance before.

Here you find a sample implementation, testable with shade_dev in the developer skins.
https://github.com/badescunicu/mixxx/blob/b64f3cb14868d01b5b42a212f6435dff638a8a4f/src/effects/native/eqdefault.cpp

I am not saying that we have no room to improvements here. But at least it works well.
Since a lot of work is already done, it would be nice to give hints to improve this.

Thank you,

Daniel

@badescunicu
Copy link
Contributor Author

As @daschuer said, I considered that way of adding button support. Since ControlEffectKnob inherits from ControlPotmeter we can't use that class unless we rewrite it to be a hybrid class and support both Potmeter and PushButton behaviour, which I'm pretty sure introduces additional complexity. IMHO it is not recommended to have a class which is responsible for multiple behaviours.

I need this type of parameters for the LV2 support I'm currently working at. This way I can easily check the manifest's ControlHint and create the proper parameter for LV2 effects.

Thanks,
Nicu Badescu

@daschuer
Copy link
Member

I will merge this now to allow Nicu continue based on this. In the unlikely case, we find out this was a bad Idea, it is no problem to fix this in another commit.

daschuer added a commit that referenced this pull request Jul 21, 2014
@daschuer daschuer merged commit b6c4384 into mixxxdj:master Jul 21, 2014
@daschuer
Copy link
Member

daschuer commented Sep 9, 2014

@rryan: your post from mailing list:

My main complaint hasn't changed -- that change bakes UI decisions (grouping the parameters by type) into the data model / class hierarchy. It complicates everything to have multiple sets of parameters (as the enum PR shows we will have 3 sets of widgets, 3 sets of parameter slots, etc.) when they're all just copypasta. I don't think the decision there has any philosophical significance -- we can still do whatever we want in the UI layer.
The specific implementation is just too much technical debt for us to carry forward.

@daschuer
Copy link
Member

daschuer commented Sep 9, 2014

My main complaint hasn't changed -- that change bakes UI decisions (grouping the parameters by type) into the data model / class hierarchy.

It is not required to group the controls by type. If you set up pairs of knobs and buttons
in the skin, they will line up as they are defined. But we still have a simple knob widget and a
Simple multi state button widget. (already introduced in the LV2 branch)

But there is an other strong demand of allowing this, because of the fixed layout of the controllers.
Please look at the VCI-400 http://www.vestax.de/produkte/controller/vci-400.html
It actually has a row of knobs and a row ob buttons in its effect rack.
Mapping these (for single effect mode) to a effect with dedicated buttons is quite simple.
Because the buttons can be mapped to button 1 .. 4 and the knobs to parameter 1 .. 4
This is possible to map these in a common list of button and parameters, but this requires an
additional mapping layer.

The same problem appease on every controller with Kill buttons, once we merge the EQ effect Rack. It is quite easy to load an EQ with or without a kill feature if we have separate list for knobs and buttons.

I hope you agree that we need a data model for this in any case.
So lets discuss what is the best solution for this. We may put it in a separate mapping controller, but I like the current solution more and it is already there.

It complicates everything to have multiple sets of parameters (as the enum PR shows we will have 3 sets of widgets, 3 sets of parameter slots, etc.)

We need only buttons and knobs, nothing more. The current solution aims to be the simple one.

when they're all just copypasta.

Yes, you are right there is still a notable grade of code duplication. Once we have a common sense how continue with this, I will try to remove these duplications.

I don't think the decision there has any philosophical significance -- we can still do whatever we want in the UI layer. The specific implementation is just too much technical debt for us to carry forward.

Do not get you point here. Please explain.

What is your favorite solution for
a) the button mapping issue and
b) how to avoid a Monster CO and Monster Widget featuring all parameter appearance.

@daschuer daschuer mentioned this pull request Sep 12, 2014
@daschuer daschuer mentioned this pull request Oct 7, 2014
m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants