Merge lp://qastaging/~mixxxdevelopers/mixxx/features_softtakeover into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Hey Sean,

I'm going to give you a hard time because you're a developer. :)

Two questions:
1) How is this useful in its current incarnation if you can't have
multiple <options> blocks?

2) Why does this have to go in 1.9? None of the mappings are using it. ??

3) What are the SCS.3d changes you made? Should we cherry-pick those
changes and put them in trunk before everything else?

Thanks,
Albert

On Tue, Nov 16, 2010 at 4:58 PM, Sean M. Pappalardo
<email address hidden> wrote:
> Sean M. Pappalardo has proposed merging lp:~mixxxdevelopers/mixxx/features_softtakeover into lp:mixxx.
>
> Requested reviews:
>  Mixxx Development Team (mixxxdevelopers)
> Related bugs:
>  #555547 Add soft-takeover for MIDI controllers (XML and script)
>  https://bugs.launchpad.net/bugs/555547
>
>
> This branch adds soft-takeover capabilities for MIDI controllers, both for simple XML mappings and MIDI scripting.
>
> I'm requesting that this be included in time for Mixxx v1.9.0.
>
> Note that the main goal was to add this for MIDI scripting. I did it for the XML mappings as well for completeness, but that part isn't totally useful until Mixxx can accept multiple <option>s for simple XML mappings. Adding that capability is beyond the scope of the feature itself since everything is hard-coded for a single <option>, and it would require modification of GUI classes as well, something I don't feel qualified to do, especially not with the hope of getting this merged in time for 1.9.0.
> --
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_softtakeover/+merge/41027
> Your team Mixxx Development Team is requested to review the proposed merge of lp:~mixxxdevelopers/mixxx/features_softtakeover into lp:mixxx.
>
> === modified file 'mixxx/res/midi/Stanton-SCS3d-scripts.js'
> --- mixxx/res/midi/Stanton-SCS3d-scripts.js     2010-06-29 11:58:44 +0000
> +++ mixxx/res/midi/Stanton-SCS3d-scripts.js     2010-11-17 00:57:57 +0000
> @@ -64,7 +64,7 @@
>  StantonSCS3d.triggerS4 = 0xFF;
>
>  // Signals to (dis)connect by mode: Group, Key, Function name
> -StantonSCS3d.modeSignals = {"fx":[       ["[Flanger]", "lfoDepth", "StantonSCS3d.FXDepthLEDs"],
> +StantonSCS3d.modeSignals = {"fx":[    ["[Flanger]", "lfoDepth", "StantonSCS3d.FXDepthLEDs"],
>                                       ["[Flanger]", "lfoDelay", "StantonSCS3d.FXDelayLEDs"],
>                                       ["[Flanger]", "lfoPeriod", "StantonSCS3d.FXPeriodLEDs"],
>                                       ["CurrentChannel", "reverse", "StantonSCS3d.B11LED"],
> @@ -80,77 +80,77 @@
>                             "loop2":[ ["CurrentChannel", "pfl", "StantonSCS3d.B11LED"] ],
>                             "loop3":[ ["CurrentChannel", "pfl", "StantonSCS3d.B11LED"] ],
>                             "trig":[  ["CurrentChannel", "pfl", "StantonSCS3d.B11LED"],
> -                                                                         ["CurrentChannel", "hotcue_1_enabled", "StantonSCS3d.BsALED"],
> -                                                                         ["CurrentChannel", "hotcue_2_enabled", "StantonSCS3d.BsBLED"],
> -                                                                         ["CurrentChannel...

Dude, you didn't need to quote the whole diff. :P

> 1) How is this useful in its current incarnation if you can't have
> multiple <options> blocks?

The XML soft-takeover option is independent of the MIDI scripting soft-takeover, so it's 100% useable from script now. And any XML controls that currently use the <normal> option can be used with <soft-takeover> right now.

> 2) Why does this have to go in 1.9? None of the mappings are using it. ??

I guess the urgency was from the discussion of ReplayGain auto-adjusting things (which Vittorio might add as an option anyway since I still want it.) But I'm going to update the SCS.1m mapping to use soft-takeover, and the EKS Otus mappings will use it.

> 3) What are the SCS.3d changes you made?

Sorry, that's just whitespace, tabs-to-spaces. (Soft-takeover would be senseless on most touch-only devices.)

This branch causes the following behavior:

If I turn a knob to one level (say 75%) and then whip it back (maybe to 25%) very fast, the knob stays at 75%. The effect is much more pronounced when I start at either 0% or 100% and whip it up or down. The problem is especially bad with knob controls, but I suppose that it's probably possible to have the same behavior occur for sliders if you move them fast enough.

> If I turn a knob to one level (say 75%) and then whip it back (maybe to 25%)
> very fast, the knob stays at 75%. The effect is much more pronounced when I
> start at either 0% or 100% and whip it up or down. The problem is especially
> bad with knob controls, but I suppose that it's probably possible to have the
> same behavior occur for sliders if you move them fast enough.

What's the refresh rate of your controller? That is, if you run Mixxx with --midiDebug and do the whipping, how great is the difference in successive values?

Is the control in question mapped via script? Which ControlObject is it working with that is exhibiting the problem?

(Of note, moving controls very quickly (with a slow controller refresh rate) is indistinguishable from the exact problem soft-takeover is trying to avoid. If you're The Flash, chances are you don't want soft-takeover.)

On Thu, Nov 18, 2010 at 12:57 AM, Sean M. Pappalardo
<email address hidden> wrote:
>
> Dude, you didn't need to quote the whole diff. :P
>
>> 1) How is this useful in its current incarnation if you can't have
>> multiple <options> blocks?
>
> The XML soft-takeover option is independent of the MIDI scripting soft-takeover, so it's 100% useable from script now. And any XML controls that currently use the <normal> option can be used with <soft-takeover> right now.

Then why is the XML soft-takeover code in this branch?

>
>> 2) Why does this have to go in 1.9? None of the mappings are using it. ??
>
> I guess the urgency was from the discussion of ReplayGain auto-adjusting things (which Vittorio might add as an option anyway since I still want it.) But I'm going to update the SCS.1m mapping to use soft-takeover, and the EKS Otus mappings will use it.
>

I hope you're not planning to do those for 1.9...

> Then why is the XML soft-takeover code in this branch?

The branch is called softTakeover. I added the XML and MIDI scripting implementations to this branch. If you're uneasy about using it before we have multiple <options> support, just don't tell anyone about the XML option. The version in MIDI script is fine to use now.

I was planning to update those mappings for a 1.9.x release, yes. It doesn't have to be 1.9.0 though.

If you don't feel comfortable merging this without the multiple <options> support, please tell me what I'd need to change and where in the GUI classes to support it. (I know what I need to do on the XML side.)

Revision 2559 improves responsiveness with controls mapped in scripts by an enormous amount. I really have to whip the control very hard to cause the behavior again. I think it's very suitable for use now.

Hey Sean,

More comments:
- Can you save the value of QDateTime::currentDateTime() in an intermediate variable in your calculations? You call that function an awful lot.
- is it possible to roll the actual soft takeover logic out into a function? Just get it out of setValue() because it makes that function rather hard to read.

- I still don't know what we should do with the MIDI XML part of this. It's not bad, besides the duplication of the soft takeover logic, but I don't think there's a use case for it yet unless we have multiple XML <option> blocks.
- The swath of QMaps in MidiScriptEngine suggests they should maybe be rolled into a class at some point. (I don't expect you to fix this now, just giving you a heads up.)

Thanks,
Albert

Re: your comments on IRC:

[15:16] <Pegasus_RPG_> I'm just going to factor out all the soft-takeover stuff into a class
[15:16] <Pegasus_RPG_> then instantiate that in the MSE
[15:16] <Pegasus_RPG_> and optionally the MIDI device

Yeah, sounds good. :)

Ok, all factored out. Applicable code in MidiDevice and the MSE is very clean now.
- Now storing the value of QDateTime::currentDateTime() when possible to cut down on the number of calls to it
- For the XML part, go ahead and include it since it works fine and will be useful for controls that otherwise have a <normal/> option tag. (For the future, there is a blueprint for extending the system to accept multiple <option/>s. That limitation is a general one; not a fault or result of soft-takeover specifically, so it shouldn't have any bearing on this merge since this is still useful without it. (It would just be more useful with it.))

Hey Sean, looking good. Here are my questions/comments:

* In SoftTakeover::currentTimeMSecs()
  - The string hack isn't pretty but it's fine. QElapsedTimer is also a good solution to what you're doing here because you don't actually care about the time itself, just the delta time. Unfortunately it's only part of 4.7 onwards as well. QElapsedTimer isn't affected by e.g. the user changing the system time while Mixxx is running (QTime::elapsed is also affected by that problem).

* In SoftTakeover::ignore(MixxxControl, float, bool)
  - Where did the '3' values in threshold calculations come from?
  - maxValue, minValue contain invalid data for scaleFactor calculation if cpo is NULL
  - scaleFactor calculation is a little strange. Why is it fabs(maxvalue) - fabs(minvalue)? Seems like maxValue - minValue is probably what you want instead?
  - Also, a little odd that 128 is factored in here. As I understand it that's mostly only used for MIDI XML mappings. Should MIDI scripts that deal with controls directly also have the 128 factor?
  - Check for NULL value of 'temp' (otherwise a MIDI script or mapping can segfault Mixxx!)
  - How was 50 determined as the right time delay for ignoring? Please pull it out of the expression and make it a const variable set to 50 w/ a comment about it. Does it work with all latencies?

* In MixxxControl
  - the # of MIDI_OPT_SOFT_TAKEOVER is a mystery to me .. why are SCRIPT and SOFT_TAKEOVER 40 and 50? Could they just be 11 and 12, following the options coming before it?

Thanks!
RJ

> * In SoftTakeover::currentTimeMSecs()
> - QElapsedTimer is also a good solution to what you're doing here because you don't actually
> care about the time itself, just the delta time.

True, but then I'd need to create and store pointers to QElapsedTimer objects for each MixxxControl that had soft-takeover enabled instead of just uints as currently. And run-time dynamic memory allocation is a no-no in a RT app, since enable/disable can happen at any time. (Granted QHash also dynamically allocates memory, but not anywhere near as much as another whole object.)

> * In SoftTakeover::ignore(MixxxControl, float, bool)
> - Where did the '3' values in threshold calculations come from?

Experimentation. 3/128 units away from the current is enough to catch fast non-sequential moves but not cause an audially noticeable jump.

> - maxValue, minValue contain invalid data for scaleFactor calculation if cpo
> is NULL

I know, but there are no sensible defaults since CO ranges can vary widely. Just hurry up with Control 2.0. ;) For now, I've set max to something really high and min to 0 so that the if(diff>threshold) always fails, effectively disabling soft-takeover for that pass.

> - scaleFactor calculation is a little strange. Why is it fabs(maxvalue) -
> fabs(minvalue)? Seems like maxValue - minValue is probably what you want
> instead?

Hmm, you're right. Logic error I guess. Fixed.

> - Also, a little odd that 128 is factored in here. As I understand it that's
> mostly only used for MIDI XML mappings. Should MIDI scripts that deal with
> controls directly also have the 128 factor?

No, no. I just did that for consistency. See above. We could just as easily do threshold=difference*(3/128). I've changed the code to do this so it's clear.

> - Check for NULL value of 'temp' (otherwise a MIDI script or mapping can
> segfault Mixxx!)

Done.

> - How was 50 determined as the right time delay for ignoring?

Experimentally. Unless there exists a controller with a really slow control sampling rate, it's just enough to catch wide swings in a short amount of time without defeating the purpose of soft-takeover. Tested and approved by "fast fingers" Joe (JJWC.) :)

> Please pull it
> out of the expression and make it a const variable set to 50 w/ a comment
> about it. Does it work with all latencies?

Done. I would expect that it works with all latencies since MIDI data input speed is not tied to latency. It tests good for me at 1ms and 42ms anyway.

> * In MixxxControl
> - the # of MIDI_OPT_SOFT_TAKEOVER is a mystery to me .. why are SCRIPT and
> SOFT_TAKEOVER 40 and 50? Could they just be 11 and 12, following the options
> coming before it?

Sure they could but I spaced them out since they're conceptually different from the others in that they don't directly modify values the same way rot64, jog, etc. do, leaving room for additional direct-modify options.

Looks good, thanks Sean

Minor nitpick in midiscriptengine.cpp

 if(cot != NULL) {
- cot->slotSet(newValue);
+ if (!m_st.ignore(group,name,newValue)) cot->slotSet(newValue);
     }

You should probably fold the two if statements into one.

Also one line if statements, while they are concise are often problematic 1) because it's easy to overlook that it's even there and 2) because there are no braces to protect against someone adding another statement mistakenly. I have a slight preference to not have one-line or brace-less if's anywhere in Mixxx if possible since they can be a source of bugs.

Go ahead and merge to trunk -- we haven't been strict at all about the feature freeze this time around and this only affects controllers with it explicitly enabled so I don't see a problem.

thanks again,
rj