VCI-100mkII: rewrite with new loop, effect and navigation CO by sohet · Pull Request #1293 · mixxxdj/mixxx

sohet · 2017-06-25T08:20:17Z

rewrite with new loop CO (no change in the functionality)
rewrite with new effect CO (add meta knob control and focus move)
rewrite with new navigation CO (use MoveUp/Down instead of SelectPrev/Next*)
https://www.mixxx.org/wiki/doku.php/vestax_vci-100mkii#mapping_for_mixxx_development

Great thanks to Be and Ronso for preparing the new environment!

Be-ing

Thanks for continuing to take care of this old controller. :) The use of the new loop, beatjump, and effects controls seems odd in a handful of ways as detailed in comments in the code.

I suggest mapping the bottom row of buttons like the Pioneer DDJ-SB2 mapping. That would require moving or removing the functionality of button 26. I mapped keylock on my first controller mapping but I found that I never actually used it while mixing; I just keep keylock on. Is it really useful to you to have this mapped on your controller? Similarly, the waveform zoom levels are not something I want to adjust while mixing; I have them set how I like then leave them. If you want to keep keylock on the controller, you could move it to shift + button 25 if you remove the waveform zooming.

I have some ideas about how to adjust the effects mapping for this controller. You could use buttons 1 & 2 to switch the top row of buttons (currently only used for hotcues) between different modes. One mode could control the effect enable switches and another could allow choosing the focused effect. You could get the buttons back to hotcue mode by pressing button 1 or 2 again (depending on which was last pressed to activate a layer).

Be-ing · 2017-06-25T17:09:13Z

+    if (n) {
+        VCI102.parameter(ch, value, VCI102.slot(group, n), "parameter4");
+    } else {
+        VCI102.meta(ch, value, group, "super1");


Why did you change this from "mix" (dry/wet)? The superknob is redundant with access to each metaknob.

Since "send" is added to effects that need it (as reverb) by you, I feel dry/wet mix is not necessary any more.

The dry/wet knob is necessary for controlling the whole chain together. You may want to keep the metaknob of one effect like Filter or Reverb at one position while using the chain and manipulate the metaknobs of other effects. Without the dry/wet knob, you'd have to turn the metaknobs of every effect when mixing the effects in and out, but that does not work well when you want to leave one or more metaknobs in the same position. It also doesn't work well when you are using all 3 effects in a chain, unless you happen to have 3 hands.

You may want to keep the metaknob of one effect like Filter or Reverb at one position while using the chain and manipulate the metaknobs of other effects. Without the dry/wet knob, you'd have to turn the metaknobs of every effect when mixing the effects in and out, but that does not work well when you want to leave one or more metaknobs in the same position.

For this purpose, we have multiple effect units. For example, using one effect unit for variable effects and another unit for constant effects.

It also doesn't work well when you are using all 3 effects in a chain, unless you happen to have 3 hands.

"super1" can also be used for changing 3 effects simultaneously.

I think "mix" and "super1" have pros and cons in controlling the whole effect unit. "mix" can control it with ease and acceptable sound quality, but "super1" can control it with fine adjustments (by changing links from prameters) and better sound quality (mixing wet and dry may degrade the sound).

Be-ing · 2017-06-25T17:11:55Z

+        if (engine.getValue(group, "loop_enabled")) {
+            engine.setValue(group, "reloop_toggle", 1);
+            // restore beatloop_size
+            engine.setValue(group, "beatloop_size", VCI102.loopSize[ch]);


Why are you storing this in a script variable? Mixxx keeps track of this.

I don't want to change beatloop_size (used as default value) while temporary changing loop size on loop. So I save the size at the beginning and restore the size on exit. But now I find "loop_scale" control for temporary change, so rewrite the script.

loop_scale is not intended for use with beatloops. It only takes into consideration the size of the loop, not the beat positions, so it will create off beat loops with non constant beatgrids.

If you would like to keep this nonstandard behavior, then make it easy for users to change with a boolean variable at the top of the script file. IMO it should default to false.

Be-ing · 2017-06-25T21:56:09Z

        ["loop_enabled", "play", "reverse"].forEach(function(key) {
            engine.connectControl(deck, key, VCI102.slip);
        });
+        engine.connectControl(deck, "beatloop_size", beatjumpSize);


Why are you tying these together? They are intentionally separate values. Even if there are not enough controls to manipulate beatjump_size from the controller, users should still be able to set it via mouse or keyboard.

Ok, separate current loop size, beatloop_size and beatjump_size.

Be-ing · 2017-06-25T22:01:22Z

+            engine.setValue(group, "beatloop_size", VCI102.loopSize[ch]);
+        } else {
+            VCI102.loopSize[ch] = engine.getValue(group, "beatloop_size");
+            engine.setValue(group, "beatloop_activate", 1);


beatloop_activate should be reset to 0 or the skin button for it will stay lit.

Add engine.setValue(group, "beatloop_activate", 0). Also add engine.setValue(group, "loop_out", 1) for future possible changes.

Do not add engine.setValue(group, "loop_out", 1) when releasing this button. When a loop is active and loop_out is 1, the loop out point will be continuously adjusted to the play position until loop_out is reset to 0. This is is meant so you can hold down loop_in/loop_out buttons while moving a jog wheel to finely adjust manual loops.

Be-ing · 2017-06-25T22:13:49Z

+    var key, n = engine.getValue(group, "focused_effect");
+    if (n) {
+        group = VCI102.slot(group, n);
+        key = "prev_effect";


prev/next_chain aren't very useful at the moment. In the future (after Mixxx 2.1) I think they'll be revived to cycle through user defined chain presets.

I think effect chain and super knob have been central ideas of Mixxx effect system. Linking parameters of effects various way to one knob is like making an complicated instrument on synthesizer, and controlling such complicated effects by one knob is of great fun. I miss user defined chain presets and want to remain effect chain and super knob till then.

Be-ing · 2017-06-25T22:19:28Z

-            VCI102.fxKnob[ch] = group.slice(0, -1) + "_Effect1]";
+VCI102.parameter = function(ch, value, group, key) {
+    if (VCI102.shift[ch % 2]) {
+        // link to meta, inverse variants -> none -> direct variants


It doesn't really hurt to have this, but I don't think there's a need to manipulate metaknob linking from a controller. That's meant to be set-and-forget. Mixxx now saves and restores changes the state of metaknob linkings on shutdown/startup.

I know some people using compact controllers like this for preparing tracks at home. For those (including me) mapping editing controls (link to meta, waveform zoom, beats translate, ...) is useful.

Be-ing · 2017-06-25T23:08:56Z

It is easier to review changes if you split them into smaller commits. The easiest way for you to do this is to work on one change at a time then commit it. Unlike older version control systems, Git works offline, so you can make multiple commits on your computer before pushing them to GitHub for a pull request. If you have modified your code to work on a few unrelated things, you can pick individual changes to stage for a commit, then pick the other changes for the next commit. Using a graphical Git client helps a lot for that. Personally I like Git Cola, but there are others available that you may prefer.

Be-ing · 2017-07-01T16:25:54Z

 VCI102.reloop = function(ch, midino, value, status, group) {
    if (value) {
        if (engine.getValue(group, "loop_enabled")) {
+            engine.setValue(group, "loop_out", 1);


I don't understand why this button is using loop_out. It is unrelated to reloop_toggle. reloop_toggle should be accessible when loops are disabled.

If you want to use a single button for both beatloop_activate and reloop_toggle, I suggest mapping it like I have mapped pushing the loop encoder on the Hercules P32:

Push with no loop active: beatloop_activate

Push with loop active: reloop_toggle

Shift + push with no loop active: reloop_toggle

Shift + push with loop active: reloop_andstop

This controller sends different messages depending on the shift button. I map VCI102.loop (without shift) and VCI102.reloop (with shift) to the same button 27. So total behavior is,

Push with no loop active: beatloop_activate

Push with loop active: reloop_toggle

Shift + push with no loop active: reloop_toggle

Shift + push with loop active: loop_out

Of course I have realized that "loop_out" is intended to be used with "loop_in" for manual loop, but it can be used with "beatloop_activate" (or other beatloop starters) for manual loop. For example,

Begin beatloop by "beatloop_activate" with size 8

After 5 beats, activate-deactivate "loop_out"

Continue to loop with size 5

Though it is not intended use of "loop_out", but works fine at least from Mixxx 1.11 till now. This use of "loop_out" can add manual loop to usual beatloop mapping with minimal cost, I think.

Be-ing · 2017-07-01T16:51:28Z

+        engine.setValue(group, "beatjump_size",
+                        engine.getValue(group, "beatjump_size") * scale);
+    } else if (engine.getValue(group, "loop_enabled")) {
+        engine.setValue(group, "loop_scale", scale);


loop_scale is only for manual loops set with loop_in and loop_out which are not mapped on this controller.

sohet · 2017-08-10T11:48:27Z

Drastic change of the layout.

map effect enabled to button 30-33 (these buttons are originally for effect, but I mapped hotcue to them historically for there was no effect system except flanger)
map hotcue to button 23-24 (only hotcue 1 and 2 sorry)
map sync to button 25 (just as labeled on the face)

https://www.mixxx.org/wiki/doku.php/vestax_vci-100mkii#mapping_for_mixxx_development

Be-ing · 2017-09-26T16:25:15Z

FYI, I haven't forgotten about this, but I'm prioritizing getting things together for 2.1 beta. Controller mappings can be merged during the beta period after feature freeze.

Be-ing · 2017-11-19T22:13:22Z

In case you missed it, we've decided to release Mixxx 2.1 beta on 2017-12-22. I'll get back to reviewing controller mappings to be included in the 2.1 release after the beta is released.

Be-ing · 2017-12-23T18:56:49Z

I will get to reviewing this soon, but for now I will be taking a break from development and code review for a few days.

…overy

Be-ing

Thanks for updating this. Sorry for the delay reviewing it. Overall it looks good. I made a few comments below. Will you have time to address those comments in the next few days before the 2.1 release and rebase this on the 2.1 branch? If not, we can merge it as it is.

Be-ing · 2018-03-23T01:40:19Z

+        VCI102.parameter(ch, VCI102.slot(group, n), "parameter3", value);
+    } else {
+        VCI102.set(VCI102.slot(group, 3), "meta", value);
+    }


These parameter functions duplicate code. You could have one function and get the parameter number from the MIDI note number. Alternatively, you could create 3 different functions with a for loop.

Be-ing · 2018-03-23T01:43:04Z

+            value -= 27;
+        } else if (value > 50) {
+            value = 50;
        }


I'm confused what this is doing. Could you explain?

Be-ing · 2018-03-23T02:36:00Z

+    if (value) {
+        if (engine.getValue(group, "loop_enabled")) {
+            engine.setValue(group, "loop_out", 1);
+            engine.setValue(group, "loop_out", 0);


Having loop_out mapped without loop_in seems strange. Maybe you could use reloop_andstop here instead? If you prefer it how it is that is okay though.

loop_out should be set to 1 on button press and set to 0 on button release. That way, when the user holds the button down, the loop out point will move with the play position until they release the button. This allows for setting a new loop out point outside of a loop that is active.

I have just now realized the exact function of "loop_out". Then beginning loop size is irrelevant to manual loop! I will change the code.

This ability to hold down loop_out buttons is new in 2.1 (it was added in #1187).

Be-ing · 2018-03-26T00:39:51Z

+    }
+};
+
+VCI102.reloop = function(ch, midino, value, status, group) {


I got quite confused figuring out how this is working. Could you add a comment explaining that this is the loop button with shift?

Some old controllers including VCI-100mkII send messages with different midi numbers depending on shift. So not only VCI102.loop(without shift)-VCI102.reloop(with shift), but also other controls, play-reverse, sync_enabled-quantize, ... can be mapped to same buttons without scripts. You can see them in "Vestax VCI-100MKII.midi.xml".

sohet · 2018-03-26T11:20:28Z

Thank you for reviewing. I don't want to change codes at this moment, so only add some comments. How to rebase it on the 2.1 branch?

Be-ing · 2018-03-26T11:53:46Z

If you haven't done so already, add the upstream repository as a remote:

git remote add upstream https://github.com/mixxxdj/mixxx.git
git fetch upstream

then

git rebase upstream/2.1

Be-ing · 2018-03-26T22:48:15Z

LGTM, thank you!

@daschuer, I tried rebasing this branch on 2.1 but there are some conflicts. Could you take care of that? I'm not super familiar with resolving rebasing conflicts and don't want to mess up the Git history somehow.

sohet · 2018-03-26T22:50:17Z

Sometimes I rebased this branch to Master during this long Pull. And now when I try to rebase it to 2.1, following warnings are shown. How can I resolve it?

First, rewinding head to replay your work on top of it...
Applying: Happy New Year 2018!
Using index info to reconstruct a base tree...
M COPYING
M LICENSE
M build/debian/copyright
M build/wix/LICENSE.rtf
.git/rebase-apply/patch:43: trailing whitespace.
\fs22 Copyright (C) 2001-2018 Mixxx Development Team\par
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: remove NSIS files as we now have completely switched to wix
Applying: also remove nsis patch
Applying: properly update LICENCE and README.md for 2.2
Using index info to reconstruct a base tree...
M LICENSE
M README.md
.git/rebase-apply/patch:44: trailing whitespace.
\pard\qj\ul\b\f0\fs22\lang1036 Mixxx version 2.2, Digital DJ'ing software.\ulnone\b0\fs24\par
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging README.md
Auto-merging LICENSE
Applying: Remove visual studio version from LICENCE
Using index info to reconstruct a base tree...
M LICENSE
.git/rebase-apply/patch:25: trailing whitespace.
On Windows you may receive a copy of the Microsoft Visual Studio runtime libraries. These are distributed only for the purpose of allowing Mixxx to run as per the license agreement for Visual Studio and are copyright of Microsoft.\par
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LICENSE
CONFLICT (content): Merge conflict in LICENSE
error: Failed to merge in the changes.
Patch failed at 0005 Remove visual studio version from LICENCE
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Be-ing · 2018-03-28T22:10:28Z

I rebased this on 2.1 and pushed it directly to that branch. I had to use git rebase -i upstream/2.1 to exclude the commits from the master branch.

Be-ing suggested changes Jun 25, 2017

View reviewed changes

sohet force-pushed the master branch from 2098d85 to 0f8cc54 Compare July 1, 2017 12:49

Be-ing reviewed Jul 1, 2017

View reviewed changes

sohet force-pushed the master branch from 0f8cc54 to 8c54b89 Compare July 5, 2017 09:44

sohet force-pushed the master branch from 8c54b89 to c8aff5e Compare July 28, 2017 22:23

sohet force-pushed the master branch 2 times, most recently from a435b87 to 563d7c8 Compare August 10, 2017 11:07

sohet force-pushed the master branch from f45b67d to eed6e73 Compare September 18, 2017 05:28

sohet force-pushed the master branch 2 times, most recently from 10678b7 to fed20b1 Compare September 29, 2017 23:39

sohet force-pushed the master branch from fed20b1 to 94d0f0d Compare October 9, 2017 23:19

sohet force-pushed the master branch from 94d0f0d to 40db745 Compare October 20, 2017 23:06

uklotzde added the controller mappings label Oct 29, 2017

sohet force-pushed the master branch from 40db745 to a941e18 Compare November 1, 2017 02:57

sohet force-pushed the master branch 2 times, most recently from 68a406d to 0c6eed1 Compare November 13, 2017 10:22

sohet force-pushed the master branch from 0c6eed1 to 759ee4b Compare November 21, 2017 00:10

Be-ing added this to the 2.1.0 milestone Dec 27, 2017

sohet force-pushed the master branch from 759ee4b to 3595c40 Compare February 19, 2018 23:27

sohet added 5 commits March 23, 2018 08:12

VCI-100mkII: rewrite with new loop, effect and navigation CO

1d3d97a

VCI-100mkII: separate current loop size, beatloop_size and beatjump_size

527bb2d

VCI-100mkII: move beatjump_size control to the other Deck

ee6a5c7

VCI-100mkII: show beatjump_size

d6f2b4b

VCI-100mkII: separate refreshLED function

6031fb9

sohet added 17 commits March 23, 2018 08:12

VCI-100mkII: map effect enabled to button 30-33

1e312e8

VCI-100mkII: map hotcue to button 23-24

f39727a

VCI-100mkII: fix VCI102.focusEffect*

8922e3b

VCI-100mkII: change velocity of disabling scratch

319b826

VCI-100mkII: even in scratch send value to jog on play for better rec…

660ca2c

…overy

VCI-100mkII: longer delay before reenabling slip

b94cd91

VCI-100mkII: fix missing hotcue-slip connection

39fe787

VCI-100mkII: inhibit wheel in slip reenabling wait

7372a58

VCI-100mkII: resume slip after all slip-source controls are disabled

501f10c

VCI-100mkII: clean up around effect script

cb9ffb5

VCI-100mkII: loose center for filter knob

bf0fadb

VCI-100mkII: add effect mix selector

ad4437c

VCI-100mkII: update control names

acb115b

VCI-100mkII: change navigation mapping

d16a9c3

VCI-100mkII: change navigation mapping again

77776e6

VCI-100mkII: exchange effect select and focus

c551139

VCI-100mkII: add unfocus effect buttons

b9b4516

Be-ing suggested changes Mar 26, 2018

View reviewed changes

VCI-100mkII: add some comments

38018d3

sohet force-pushed the master branch from 3595c40 to 38018d3 Compare March 26, 2018 10:58

Be-ing changed the base branch from master to 2.1 March 26, 2018 12:07

Be-ing changed the base branch from 2.1 to master March 26, 2018 12:07

VCI-100mkII: set loop_out to 0 when released

d91899d

Be-ing closed this Mar 28, 2018

Uh oh!

Conversation

sohet commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing left a comment • 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!

Be-ing Jul 1, 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!

Be-ing Jul 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohet Jul 2, 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!

sohet commented Aug 10, 2017

Uh oh!

Be-ing commented Sep 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

Be-ing commented Dec 23, 2017

Uh oh!

Be-ing left a comment

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!

sohet commented Jun 25, 2017 •

edited

Loading

Be-ing left a comment •

edited

Loading

Be-ing Jul 1, 2017 •

edited

Loading

Be-ing Jul 1, 2017 •

edited

Loading

Be-ing commented Jun 25, 2017 •

edited

Loading

sohet Jul 2, 2017 •

edited

Loading

Be-ing commented Nov 19, 2017 •

edited

Loading

Be-ing commented Mar 26, 2018 •

edited

Loading

Be-ing commented Mar 26, 2018 •

edited

Loading