Metaknob soft takeover by Be-ing · Pull Request #1111 · mixxxdj/mixxx

Be-ing · 2017-01-07T18:42:59Z

Follow-up from discussion on #1062:

When an effect is on, do not make parameters audibly jump when metaknob changes. If the superknob is turned, do not make metaknobs jump for effects that are on.
When an effect is off, do not do any automatic soft takeover. Snap parameters to position of metaknob when the metaknob turns (either directly or by the superknob). With Effect enable switches changes #1103, this does not interfere with loading initial states of knobs from controllers when Mixxx starts. It also makes it easier to experiment with different metaknob linkings.

daschuer · 2017-01-08T20:47:04Z

This works as advertised, and seams to be nice. I there are only some remaining worries, if the conditional disabled oversoft will leads to confusion because it may kick in or out when not expected.
A third voice would be nice.
@timrae: Could you have a look?

daschuer · 2017-01-08T20:48:29Z

I wonder why we see commits here that are currently in master.
Did you debase the previous branch? Than this need to be rebase on master as well.

timrae · 2017-01-08T23:41:11Z

@timrae: Could you have a look?

Sure

Be-ing · 2017-01-09T01:16:51Z

Yikes I tried to rebase and it lost the new commit >.<

Be-ing · 2017-01-09T01:27:10Z

Alright, looks good now. I think that happened because I branched off my per_effect_metaknobs branch, added a commit to this branch, then added another commit to per_effect_metaknobs before that was merged.

timrae · 2017-01-09T02:01:18Z

    connect(m_pControlMetaParameter, SIGNAL(valueChanged(double)),
            this, SLOT(slotEffectMetaParameter(double)));

+    m_pSoftTakeover = new SoftTakeover();


Does this need a delete or it owned by the object tree? (If the former, consider using unique_ptr as per the discussion in #953)

timrae · 2017-01-09T02:17:50Z

I'm testing it now... With the current master branch, metaknob itself has soft takeover already (I only checked flanger which only has one parameters assigned to meta, it may be different for other effects). However, the super knob and the meta knob do not have any soft takeover in terms of the interaction between them. Is that the objective of this PR?

The sync between the two is still not perfect, as demonstrated by the following steps:

Enable flanger effect
Turn metaknob all the way down to zero on your controller
Turn super knob all the way up to max in the GUI
Turn metaknob on your controller

--> Audible jump

timrae · 2017-01-09T02:32:29Z

the conditional disabled oversoft will leads to confusion because it may kick in or out when not expected

Do you mean people will be confused that the soft takeover doesn't occur when the effect is disabled? The only situation where I think it might be confusing is if the user maps both super knob and meta knobs on their controller. We should tell them not to do this.

Otherwise, in general I feel that soft takeover being disabled is more intuitive than being enabled, so only enabling soft takeover when the effect itself is enabled (with the priority of preventing audible jumps) makes sense to me.

daschuer · 2017-01-09T07:16:47Z

@timrae Thank, you. This means we are on the right track. :-)

Be-ing · 2017-01-09T18:42:09Z

The sync between the two is still not perfect, as demonstrated by the following steps:
Enable flanger effect
Turn metaknob all the way down to zero on your controller
Turn super knob all the way up to max in the GUI
Turn metaknob on your controller
--> Audible jump

I cannot reproduce that with this branch. Are you sure you tested the correct branch?

timrae · 2017-01-09T21:02:51Z

I forked master branch and cherry picked this commit.

…

On 10 Jan. 2017 03:42, "Be" ***@***.***> wrote: The sync between the two is still not perfect, as demonstrated by the following steps: Enable flanger effect Turn metaknob all the way down to zero on your controller Turn super knob all the way up to max in the GUI Turn metaknob on your controller --> Audible jump I cannot reproduce that with this branch. Are you sure you tested the correct branch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4nmQL6H4a11c4Ocv6iY5CvVDRHH8ks5rQn-DgaJpZM4Ldffr> .

Be-ing · 2017-01-10T05:22:38Z

Could you try checking out this branch and building it?

timrae · 2017-01-10T05:24:46Z

The way I was checking it is more indicative of the effect of merging this PR, so I prefer you rebase this into master and check that. Plus I might not have time today

Be-ing · 2017-01-10T23:57:19Z

Okay, rebased it onto master.

timrae · 2017-01-11T00:33:24Z

Did you try to reproduce on the rebased branch?

…

On 11 Jan. 2017 08:57, "Be" ***@***.***> wrote: Okay, rebased it onto master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4vL4NknFWdYsATVs4SeVLQOv9Azhks5rRBrhgaJpZM4Ldffr> .

Be-ing · 2017-01-11T00:49:40Z

Yes. I cannot reproduce the issue. It's working as intended as far as I can tell.

timrae · 2017-01-11T00:52:16Z

Ok I'll take a video when I get home after work

…

On 11 Jan. 2017 9:49 am, "Be" ***@***.***> wrote: Yes. I cannot reproduce the issue. It's working as intended as far as I can tell. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4jtaPLXZ3moLjjw_g9sXibRqKzoEks5rRCcngaJpZM4Ldffr> .

timrae · 2017-01-11T12:51:21Z

It's still happening...

timrae · 2017-01-11T12:53:45Z

See around 25s
recording.zip

timrae · 2017-01-11T12:55:32Z

This is with 42d3262 and gcc 6.2 on Ubuntu x64, Qt version 4.8.7

Be-ing · 2017-01-11T17:06:39Z

Oh, I was testing with my controller which had soft takeover enabled by its script. >.<

daschuer · 2017-01-11T21:35:45Z

I have watched the recording, and I actually think this behavior is correct if you do not have softtakover enabled on your controller. you most likely see an out of sync situation between GUI meta knob and controller meta knob.

You turn the meta knob fully left.
Than you turn the super knob fully right.
-> the hidden GUI Metaknob follows the super knob, the controller knob is unchanged
now you touch the Metaknob on the controller, the GUI knob jumps back.
This is IMHO correct for no controller softtakover.

The issue targeted here is just the other way round.

Turn the super knob
-> meta follows
Turn the meta knob
Turn the super knob again
Without this PR meta jumps to super without the chance to fix this in the controller mapping
Now we get a out of sync and snap in behavior.

Does this work for you?
An alternative would be to let meta not follow super. But then we loose the nice GUI feedback.

Be-ing · 2017-01-12T00:02:03Z

The issue targeted here is just the other way round.

Yes, this PR fixes the issue you outlined. Together with soft takeover activated by the controller mapping it solves the issue @timrae reported.

An alternative would be to let meta not follow super.

That would be overcomplicated. I don't think we need to introduce any more ControlObjects for the new effects interface.

Be-ing · 2017-01-12T00:06:58Z

    QList<EffectParameterSlotPointer> m_parameters;
    QList<EffectButtonParameterSlotPointer> m_buttonParameters;

+    SoftTakeover* m_pSoftTakeover;


Does this need to be deleted in the destructor?

Yes, though you can make it a member variable so you don't have to allocate it at all.

Fixed in rebased and force pushed commit.

timrae · 2017-01-12T00:16:02Z

Sounds good to me

…

On 12 Jan. 2017 9:07 am, "Be" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/effects/effectslot.h <#1111 (review)>: > @@ -108,6 +109,8 @@ class EffectSlot : public QObject { QList<EffectParameterSlotPointer> m_parameters; QList<EffectButtonParameterSlotPointer> m_buttonParameters; + SoftTakeover* m_pSoftTakeover; Does this need to be deleted in the destructor? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1111 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4gSJF4LspUMv_-hE5XtPtjvGcH8aks5rRW6kgaJpZM4Ldffr> .

Be-ing · 2017-01-12T00:18:53Z

The other issue this PR solves is:

Turn the metaknob
Adjust a parameter knob linked to the metaknob
Turn the metaknob

If the effect is enabled, the manually adjusted parameter should not jump.

If an effect is on, avoid making parameters audibly jump. If effect is off, do not do soft takeover, just snap parameters to position of metaknob. Controller mappings still must enable soft takeover for the super/metaknobs as appropriate.

Be-ing · 2017-01-12T16:15:18Z

Ready for merge?

rryan · 2017-01-12T17:40:13Z

Looks good to me. SuperLinkTest.Softtakeover is failing because the test does not enable the effect. Could you split SuperLinkTest.SoftTakeover into SuperLinkTest.SoftTakeover_EffectEnabled and SuperLinkTest.SoftTakeover_EffectDisabled to cover both cases?

�[0;32m[ RUN      ] �[mSuperLinkTest.Softtakeover
Loading resources from  "/home/travis/build/mixxxdj/mixxx/res/" 
src/test/super_link_test.cpp:120: Failure
Value of: m_pControlValue->get()
  Actual: 0.25
Expected: 1.0
Which is: 1
src/test/super_link_test.cpp:128: Failure
Value of: m_pControlValue->get()
  Actual: 0.55
Expected: 0.1
�[0;31m[  FAILED  ] �[mSuperLinkTest.Softtakeover (9 ms)

Test for new metaknob -> parameter and superknob -> metaknob behavior both when effect is enabled and disabled.

rryan · 2017-01-13T05:59:12Z

Nice, thanks for the extra tests!

Be-ing · 2017-01-13T07:30:06Z

Is there a such thing as extra tests? :P

rryan mentioned this pull request Jan 7, 2017

add per-effect meta knobs #1062

Merged

Be-ing closed this Jan 9, 2017

Be-ing force-pushed the metaknob_soft_takeover branch from 0701908 to 836db6a Compare January 9, 2017 01:14

Be-ing reopened this Jan 9, 2017

Be-ing force-pushed the metaknob_soft_takeover branch from 6c672de to 0701908 Compare January 9, 2017 01:26

timrae reviewed Jan 9, 2017

View reviewed changes

Be-ing force-pushed the metaknob_soft_takeover branch from 0701908 to 42d3262 Compare January 10, 2017 23:56

Be-ing commented Jan 12, 2017

View reviewed changes

Be-ing force-pushed the metaknob_soft_takeover branch from 42d3262 to 8c06c3e Compare January 12, 2017 01:06

update metaknob link soft takeover tests

391c025

Test for new metaknob -> parameter and superknob -> metaknob behavior both when effect is enabled and disabled.

rryan merged commit 220be8a into mixxxdj:master Jan 13, 2017

This was referenced Jan 23, 2017

improve effects defaults #1118

Merged

Remove adopting Meta knob after effect load #1148

Merged

Be-ing deleted the metaknob_soft_takeover branch February 1, 2017 01:15

Uh oh!

Conversation

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

Uh oh!

Uh oh!

daschuer commented Jan 8, 2017

Uh oh!

daschuer commented Jan 8, 2017

Uh oh!

timrae commented Jan 8, 2017

Uh oh!

Be-ing commented Jan 9, 2017

Uh oh!

Be-ing commented Jan 9, 2017

Uh oh!

timrae Jan 9, 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!

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

Uh oh!

Uh oh!

timrae commented Jan 9, 2017

Uh oh!

daschuer commented Jan 9, 2017

Uh oh!

Be-ing commented Jan 9, 2017

Uh oh!

timrae commented Jan 9, 2017 via email

Uh oh!

Be-ing commented Jan 10, 2017

Uh oh!

timrae commented Jan 10, 2017

Uh oh!

Be-ing commented Jan 10, 2017

Uh oh!

timrae commented Jan 11, 2017 via email

Uh oh!

Be-ing commented Jan 11, 2017

Uh oh!

timrae commented Jan 11, 2017 via email

Uh oh!

timrae commented Jan 11, 2017

Uh oh!

timrae commented Jan 11, 2017

Uh oh!

timrae commented Jan 11, 2017

Uh oh!

Be-ing commented Jan 11, 2017

Uh oh!

daschuer commented Jan 11, 2017

Uh oh!

Be-ing commented Jan 12, 2017

Uh oh!

Be-ing Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

daschuer Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

rryan Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

Be-ing Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

timrae commented Jan 12, 2017 via email

Uh oh!

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

Uh oh!

Uh oh!

Be-ing commented Jan 12, 2017

Uh oh!

rryan commented Jan 12, 2017

Uh oh!

rryan commented Jan 13, 2017

Uh oh!

Be-ing commented Jan 13, 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 •

edited

Loading

timrae Jan 9, 2017 •

edited

Loading

timrae commented Jan 9, 2017 •

edited

Loading

Be-ing commented Jan 12, 2017 •

edited

Loading

Be-ing commented Jan 13, 2017 •

edited

Loading