Effect enable switches changes by Be-ing · Pull Request #1103 · mixxxdj/mixxx

Be-ing · 2017-01-02T08:27:11Z

This PR disables the per-effect switches when Mixxx starts and makes the switches to enable EffectUnits on decks (and other sources) persistent.

The motivation for this came from my experience setting up for my set last night using the persistent effects in PR #1092. I was worried because the level meters on my sound card showed no signal was coming out. I had to ask the previous DJ to play another track while I figured out what was going wrong. I thought something was wrong with my sound card. I tried restarting Mixxx and replugging my sound card several times and restarted my computer. Finally, I noticed that the level meters on Mixxx were not showing any signal coming through when the deck was playing. I realized that the dry/wet knob for the EffectUnit enabled on that deck was all the way wet and the filter effect's metaknob was all the way right, completely silencing the deck. I have my mapping set up to set the initial position of the software knobs to the position of the hardware knobs on startup, so the knobs may have accidentally been turned that way during transit to my gig. With this PR, that situation would not happen because each effect would have to be explicitly enabled by the user when they want to use it.

Making the deck enable switches persistent allows controllers lacking buttons to control which effect units are active on which decks to use effects conveniently. This approach is better than requiring the controller's mapping to set the enable switches in its init() function because the user may want to change their defaults and change the switches throughout a set. The user would have to enable the effect units on their preferred decks once and could leave them that way indefinitely if they choose, but users who want otherwise won't have their preferences overriden by the controller mapping's init() function.

daschuer · 2017-01-02T13:54:14Z

I like the idea. Unfortunately we cannot merge it without changing the way our default LateNight skin works.

Be-ing · 2017-01-02T19:36:21Z

The last commit should hold over LateNight until someone takes up the work of redesign its effects UI.

Be-ing · 2017-01-03T02:34:59Z

-        // Enabled is a persistent property of the effect slot, not of the
-        // effect. Propagate the current setting to the effect.
-        pEffect->setEnabled(m_pControlEnabled->get() > 0.0);
-


This is a no-op with the initial value of m_pControlEnabled equal to the initial value of Effect's m_bEnabled.

…d/disabled state

Be-ing · 2017-01-03T23:46:52Z

@timrae : Could you test this? I have merged the recent updates into my personal branch. I think it's ready for merging to master.

timrae · 2017-01-04T01:07:39Z

Sure I'm already back to work but I'll try test it over the next few days.

timrae · 2017-01-04T19:32:15Z

Did you figure out the steps to reproduce? I'm not exactly sure how to test it

Be-ing · 2017-01-04T19:36:01Z

The steps to reproduce were simply starting Mixxx after shutting it down with effects loaded. The workaround is described here and it should not be necessary anymore.

timrae · 2017-01-04T19:58:32Z

Thanks, I can confirm that yes that was the bug that I reported, and yes it is fixed here. I'm also 100% in support of disabling the individual FX enable switches and making the assignments persistent. It looks good for the merge to me.

rryan · 2017-01-04T20:12:36Z

    m_channelStatusMapper.setMapping(pEnableControl, handle_group.name());
    connect(pEnableControl, SIGNAL(valueChanged(double)),
            &m_channelStatusMapper, SLOT(map()));
+    slotChannelStatusChanged(handle_group.name());


We already have pInfo here so you could avoid the QMap lookup by doing a

if (pEnableControl->toBool()) { m_pEffectChain->enableForChannel(pInfo->handle_group); } else { m_pEffectChain->disableForChannel(pInfo->handle_group); }

rryan · 2017-01-04T20:38:17Z

-        // Enabled is a persistent property of the effect slot, not of the
-        // effect. Propagate the current setting to the effect.
-        pEffect->setEnabled(m_pControlEnabled->get() > 0.0);
+        pEffect->updateEngineState();


Couldn't the effect's enabled state be out of sync with the EffectSlot's enabled status? That's what the original line was doing here.

No, the original line was a no-op because setEnabled() checks if the parameter passed to it is equal to its private enabled boolean.

If the effect slot is enabled then the effect will be disabled but the effect slot will be enabled.

It should be easy to see -- enable an effect, then change the effect to something else. The enabled button will be lit but the effect will be disabled.

The problem I was fixing was that when the effect slot was disabled, loading a new effect would load the EngineEffect enabled but the slot would still be disabled.

Ah, looks like you also need to make EngineEffect default to disabled so Effect/EngineEffect don't start life out of sync. I think this line should stay as it was.

rryan · 2017-01-04T20:38:57Z

-    m_pControlEnabled->set(true);
+    // Default to disabled to prevent accidental activation of effects
+    // at the beginning of a set.
+    m_pControlEnabled->setDefaultValue(false);


These are both no-ops since they're the default. Maybe drop them and move the comment above the "new ControlPushButton".

rryan · 2017-01-04T20:43:12Z

Failing test:

[ RUN      ] EffectSlotTest.ControlsReflectSlotState

Loading resources from  "/Users/travis/build/mixxxdj/mixxx/res/"

src/test/effectslottest.cpp:62: Failure

Value of: ControlObject::get(ConfigKey(group, "enabled"))

  Actual: 0

Expected: 1.0

Which is: 1

src/test/effectslottest.cpp:68: Failure

Value of: pEffect->enabled()

  Actual: false

Expected: true

"[EffectRack1_EffectUnit1_Effect1]" , "loaded" is read-only. Ignoring set of value: 0

"[EffectRack1_EffectUnit1_Effect1]" , "num_parameters" is read-only. Ignoring set of value: 2

[  FAILED  ] EffectSlotTest.ControlsReflectSlotState (72 ms)

rryan · 2017-01-04T21:09:15Z

I think it's inconsistent to default the EffectChainSlot to enabled but the EffectSlot to disabled or vice versa. They were both enabled by default to support skins that don't show either one of them with little hassle. Let's disable them both by default and then put the work onto the skins to set things up according to the paradigm the skin offers.

rryan · 2017-01-04T21:10:43Z

                  <Text>ON</Text>
                </State>
                <Connection>
-                  <ConfigKey persist="true"><Variable name="group"/>,enabled</ConfigKey>


I'm boggling a bit as to why there were two "enabled" connections here. @ywwg do you remember?

O.o I don't understand either. This whole <Connection> element should probably be removed.

Yea, I think it probably can be.

Since this is empty now could you remove it?

Be-ing · 2017-01-04T21:16:01Z

Let's stop providing an inconsistent interface in different skins. Skins should conform to a standard interface or be expected to not work properly. I'm leaning towards removing the chain enable switches from skins. It's one more layer of complexity to get confused about while mixing. Effect enable and deck assignment switches is already pretty complicated.

Be-ing · 2017-01-04T21:16:58Z

What is the use case for chain enable switches separate from deck assignment switches? AFAIK no controllers are designed for such a concept.

rryan · 2017-01-04T21:21:15Z

What is the use case for chain enable switches separate from deck assignment switches?

They let you drop an effect unit in and out of the mix with one click -- as LateNight does.

Be-ing · 2017-01-04T21:22:50Z

They let you drop an effect unit in and out of the mix with one click -- as LateNight does.

So do effect enable switches and deck assignment switches. But, confusingly, those won't work if the whole chain is disabled.

timrae · 2017-01-04T21:24:36Z

I think it's inconsistent to default the EffectChainSlot to enabled but the EffectSlot to disabled or vice versa.

Having spent time mixing with the new branch, I strongly disagree with this. Having the states of the individual enabled switches to "off" makes sure that the unit doesn't affect the sound by default. Going further to disable the unit itself would make it such that users who are experimenting with effects for the first time have to press THREE buttons before they can use the FX:

channel assignment, effect enable, effect unit enable.

I would go further to say that by default only ONE button press should be required by default: the effect enable. I.e. channel assignment should default to 1 and 2 for the two units respectively

rryan · 2017-01-04T21:25:48Z

They let you drop an effect unit in and out of the mix with one click -- as LateNight does.
So do effect enable switches and deck assignment switches. But, confusingly, those won't work if the whole chain is disabled.

Right -- in practice the skin supports either enabling at the chain level or the effect level.

Be-ing · 2017-01-04T21:26:15Z

I would go further to say that by default only ONE button press should be required by default: the effect enable. I.e. channel assignment should default to 1 and 2 for the two units respectively

That's a good idea.

rryan · 2017-01-04T21:26:58Z

Going further to disable the unit itself would make it such that users who are experimenting with effects for the first time have to press THREE buttons before they can use the FX:

I think you misunderstood the proposal -- we would default all of them to off in C++ and then put it on the skin to set one of the two to on via an attributes section.

Be-ing · 2017-01-04T21:28:46Z

Skins shouldn't have to worry about a ControlObject that they do not show any visual indicator for.

and make changes to default persistent

Be-ing · 2017-01-07T07:17:54Z

Effects, EQs, and QuickEffects are working now, but one of the EffectSlot tests is failing. I'll fix that tomorrow.

rryan · 2017-01-07T19:13:32Z

-        // Enabled is a persistent property of the effect slot, not of the
-        // effect. Propagate the current setting to the effect.
-        pEffect->setEnabled(m_pControlEnabled->get() > 0.0);
+        pEffect->setEnabled(m_pControlEnabled->toBool());


Could you keep the comment?

I don't really understand the comment as it was. The slot enabled status has never been persistent, unless I'm misunderstanding the context of the comment.

Oh, I think I understand. Persistent across calls to loadEffect() with different EffectPointers, not persistent across restarts of Mixxx.

Ah, yea I think this comment predates the concept of a persistent CO :). Feel free to reword.

as per discussion on PR mixxxdj#1103

I thought this would be useful before when working on mixxxdj#1103, but experience has shown it is annoying. I want effect unit 1 assigned to deck 1, unit 2 to deck 2, unit 3 to deck 3, and unit 4 to deck 4 on startup regardless of how the routing switches happened to be when I last shut down Mixxx.

…1103)

Be-ing added 2 commits January 2, 2017 02:11

turn off effects by default

f7464e4

make EffectUnit deck assignment switches persistent

41ed69a

LateNight: quick hack to not break effects

64b001d

Be-ing added 2 commits January 2, 2017 17:28

fix effect enable buttons on startup

038d4cd

fix EffectUnit channel asignment buttons on startup

8019197

Be-ing mentioned this pull request Jan 2, 2017

add per-effect meta knobs #1062

Merged

Be-ing commented Jan 3, 2017

View reviewed changes

make effects loaded after initialization load with the correct enable…

c6c5b3c

…d/disabled state

rryan reviewed Jan 4, 2017

View reviewed changes

Be-ing added 2 commits January 7, 2017 01:14

assign EffectUnits to corresponding decks by default

7629cb5

and make changes to default persistent

fix effects being disabled by default

476e76e

Be-ing added 3 commits January 7, 2017 11:00

fix EQs and QuickEffects

224a621

fix EffectSlotTest ControlsReflectSlotState

d757a5f

small code cleanup

e6924ee

Be-ing mentioned this pull request Jan 7, 2017

Metaknob soft takeover #1111

Merged

rryan reviewed Jan 7, 2017

View reviewed changes

Be-ing added 2 commits January 7, 2017 14:09

add clarifying comment

6be7800

remove empty <Connection>

2218f55

rryan merged commit 75f3c1b into mixxxdj:master Jan 7, 2017

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

Deere: remove effect chain enable button

78cff2e

as per discussion on PR mixxxdj#1103

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

Deere: remove effect chain enable button

59246bf

as per discussion on PR mixxxdj#1103

Be-ing deleted the effect_enable_switches branch January 29, 2017 18:06

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

Deere: remove effect chain enable button

9468e45

as per discussion on PR mixxxdj#1103

This was referenced Feb 7, 2017

Deere 2.1 #940

Merged

persistent effects #1092

Merged

Be-ing mentioned this pull request Mar 7, 2017

Update Mixtrack (Pro) 3 mappings for Mixxx 2.1 #1180

Merged

4 tasks

Be-ing mentioned this pull request Nov 6, 2017

Tango skin :: update #1330

Merged

19 tasks

Be-ing mentioned this pull request Nov 20, 2017

remove persistence from EffectUnit routing switches #1385

Closed

Be-ing mentioned this pull request Mar 23, 2018

Update Reloop Beatmix 2/4 mapping to reflect new effects UI #1371

Closed

Be-ing mentioned this pull request Apr 1, 2018

Advanced Mapping for the Behringer CMD MM-1 #1329

Closed

Swiftb0y pushed a commit to Swiftb0y/mixxx that referenced this pull request Apr 1, 2018

adds function that sets fxunit states to "neutral" (refer to mixxxdj#…

c64621e

…1103)

Be-ing pushed a commit that referenced this pull request Apr 1, 2018

adds function that sets fxunit states to "neutral" (refer to #1103)

63de4b9

Uh oh!

Conversation

Be-ing commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Jan 2, 2017

Uh oh!

Be-ing commented Jan 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Jan 4, 2017

Uh oh!

timrae commented Jan 4, 2017

Uh oh!

Be-ing commented Jan 4, 2017

Uh oh!

timrae commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Jan 6, 2017 • 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!

rryan commented Jan 4, 2017

Uh oh!

rryan commented Jan 4, 2017

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!

Be-ing commented Jan 4, 2017

Uh oh!

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

Uh oh!

Uh oh!

rryan commented Jan 4, 2017

Uh oh!

Be-ing commented Jan 4, 2017

Uh oh!

timrae commented Jan 4, 2017

Uh oh!

rryan commented Jan 4, 2017

Uh oh!

Be-ing commented Jan 4, 2017

Uh oh!

rryan commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Be-ing commented Jan 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Jan 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Be-ing commented Jan 2, 2017 •

edited

Loading

Be-ing commented Jan 3, 2017 •

edited

Loading

Be-ing Jan 6, 2017 •

edited

Loading

Be-ing commented Jan 4, 2017 •

edited

Loading

rryan commented Jan 4, 2017 •

edited

Loading

Be-ing commented Jan 4, 2017 •

edited

Loading

Be-ing Jan 7, 2017 •

edited

Loading