[WiP] Traktor S4 MK2 controller updates#1562
Conversation
ywwg
left a comment
There was a problem hiding this comment.
There are a few things going on in this PR which makes it harder to know which parts relate to which other parts. If possible I'd prefer to split out the part that displays the loop size because I'd like to make sure that's written in a way that is easier to understand and reuse in the future.
| for (k = 0; k < 0x7F; k+=0x05) { | ||
| packets[0] = 0x82; | ||
| for (j = 0; j < sizesArray[beats].length; j++) { | ||
| var hid_msg = start_signal + sizesArray[beats][j] |
There was a problem hiding this comment.
hid_msg is only used on the next line so it's ok to inline there
| } | ||
| start_signal = LoopSizeIndicators[group] | ||
|
|
||
| var sizesArray = { |
There was a problem hiding this comment.
I don't suppose it's possible to mark this const somehow?
There was a problem hiding this comment.
QtScript 4.8 doesn't support such modern JavaScript keywords.
| var hid_msg = start_signal + sizesArray[beats][j] | ||
| packets[hid_msg] = k; | ||
| } | ||
| controller.send(packets, packets.length, 0); |
| 4: [9,10,11,13], | ||
| 2: [9,11,12,14,15], | ||
| } | ||
| var packets = Object(); |
There was a problem hiding this comment.
I think it would be safer to declare this object inside the loop so there's no risk of stale data getting reused
There was a problem hiding this comment.
IMO using the Object() constructor is odd. I'd prefer an empty object literal {} instead. But this is a really minor point. If you disagree you can leave it as is.
| start_signal = LoopSizeIndicators[group] | ||
|
|
||
| var sizesArray = { | ||
| 64: [1,2,4,5,6,7,9,10,11,13], |
There was a problem hiding this comment.
Are these arrays for lighting specific segments of the LED display? I'd prefer a general function that can display arbitrary integers for easier reading/testing and possible future reuse.
|
sweet! Can you take pictures or a short video of the new features in action? I don't have an S4 (I borrowed one to make the mapping) so I can't test it myself. |
Be-ing
left a comment
There was a problem hiding this comment.
Thanks for working on this! Could you rebase this on the 2.1 branch?
| TraktorS4MK2.controller.shift_pressed['deck1'] || | ||
| TraktorS4MK2.controller.shift_pressed['deck2'] | ||
| ) { | ||
| if(delta > 0){ |
There was a problem hiding this comment.
style nitpick: put space after if:
if (delta > 0) {
| ) { | ||
| if(delta > 0){ | ||
| engine.setValue(group, 'beats_translate_later', true); | ||
| } else if (delta < 0){ |
| engine.setValue(group, 'beats_translate_earlier', true); | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
combine with line above: } else {
| engine.setValue(field.group, "loop_halve", 0); | ||
| } | ||
| } | ||
| // TraktorS4MK2.sendLoopSizeMessage(group, engine.getValue(field.group, "beatloop_size")); |
There was a problem hiding this comment.
Connect changes in beatloop_size to an output callback function instead of sending the update to the controller when input is received. The former makes sure that the controller stays in sync even if the user changes beatloop_size by clicking on screen.
| engine.setValue("[Master]", "maximize_library", !library_maximized); | ||
| } | ||
|
|
||
| TraktorS4MK2.FXButtonHandler = function(field){ |
There was a problem hiding this comment.
In the future this should work the same as the Components EffectUnit, but that will be quite a bit of work and I don't know if you can do that in the next few days for 2.1. For now, just getting the enable buttons and metaknobs working would be good. Are you still having trouble with the metaknobs?
| } else if (value === 0.125){ | ||
| TraktorS4MK2.sendLoopSizeMessage(deck, '-', 8, false, false); | ||
| } else { | ||
| TraktorS4MK2.sendLoopSizeMessage(deck, '-', 'n', false, false); |
There was a problem hiding this comment.
It's great that you got this working! One of my main motivations for adding beatloop_size in #1187 was to support controllers like this, so I'm really glad this can be in the same Mixxx release.
If you use the dot instead of - to indicate a fraction, you could use both digits of the display for fractional sizes. This is how it is done on the Hercules P32. The code could also be more robust by showing the reciprocal of the loop size (1 / value) instead of checking for specific fractions. How does Traktor show fractional loop sizes?
There was a problem hiding this comment.
I'm not sure about Traktor, I haven't actually used it except for with the S2 (for a limited period of time) which doesn't have a loop counter.
Noted - although not sure how to make the code more robust with the fractions.
There was a problem hiding this comment.
Try checking if 1/value is an integer. If it is, show that on the LEDs with a dot.
|
Summary of changes:
|
| } | ||
| // deal with larger Loops | ||
| } else if (value.toString().length > 2){ | ||
| TraktorS4MK2.sendLoopSizeMessage(deck, value.toString().split("")[0], value.toString().split("")[1], true, true); |
There was a problem hiding this comment.
Maybe light up both dots in this case?
There was a problem hiding this comment.
That is what is happening here
There was a problem hiding this comment.
Oh, I see. Sorry, I didn't look at the code close enough before.
|
You may want to map the new |
|
@fayaaz I have rebased this branch on 2.1 and merged it with the 2.1 branch. If you have time to continue working on this before the 2.1 release, please open a new pull request targeting that branch. Otherwise, you can open a new PR targeted at the master branch after the 2.1 release. |
|
Could you update the wiki page for this controller? |
Pull request for updates to S4 MK2. WiP.