Add support for S-Series MK2 speed-sensitive knobs

[keithhand]

What does this PR change?

Adds support to use the speed-sensitive knobs on S-Series keyboards.

Links to Issues PR addresses or fixes

• Fixes Support S-Series MK2 speed-sensitive knobs #19

How was this PR tested?

Tested with a Komplete Kontrol Mk2 S49. I can't test with other keyboards to verify that adding the sensitivity to the adjustMixer() function will not cause issues. But, following the logic inside mixer.py, I don't think there should be any problems.

[hobyst]

First of all, thanks for contributing!
The issue you have been experiencing with much bigger increment values than expected during your testing is that you haven't applied the modification in the right place. Notice that all the code for the knobs and the mixer is located at line 300 in controller_definition.py regardless of the controller, which in combination with your changes make it so that if a S-Series keyboard is used then the script will adjust the same value twice (one as a part of Core.OnMidiMsg() and another one as a part of S_SeriesMK2.OnMidiMsgAdd()). Solving that should make the adjustments act the same way as before, so no need for the behavior divergence on the code that normalizes the speed by dividing by 15 or 10 depending on the action.

With that said, there's no need whatsoever to fragment the code for the knobs so that it acts differently depending on the controller used. Instead of that, you could change the declaration of adjustMixer() so that sensitivity: int = None becomes speed: int (making it obligatory) in and feed the argument right the function calls used in Core.OnMidiMsg() with:

# Increase
event.data2

# Decrease
nihia.mixer.KNOB_DECREASE_MIN_SPEED - event.data2 + 1

And then, to convert it to a 0-1 range inside the function you can just do speed / 63. But even with all of that it wouldn't "feel" right, as people tend to be more conscious of their movements when they try to do more meticulous movements than when doing faster or more general ones. This would make a linear value mapping to seem inconsistent and unresponsive to the way people turn their knob. In order to get an increment/decrement offset value that properly responds to the way people are turning the knob, you need a numeric expression that treats changes in the lower spectrum of possible values as more relevant that changes in the higher spectrum, being logarithms the perfect thing use here.

# Volume
config.KNOB_INCREMENTS_VOL * log(speed, 63)

# Panning
config.KNOB_INCREMENTS_PAN * log(speed, 63)

In the expressions above, log(speed, 63) will result in 1 if speed = 63 and make speed changes in the lower end much significant that in the higher end of the 0-63 range. config.KNOB_INCREMENTS_<param> will define the maximum value the offset can be.

Your last two commits are irrelevant and you can delete them by using a Git interactive rebase as all the changes can be done in the first one.

I'll be reviewing the commits using the GitHub revision UI for pull requests so I can mark all the changes more clearly, but that should be everything! 😄

[hobyst]

sensitivity: float = None should be speed: int instead. Also, please comment the parameter in the docstring of the function.

[keithhand]

I pushed 9094dd1, which should resolve this. Let me know if I can elaborate more on the description I put.

[hobyst]

No need to do any of this to normalize the value. Try first with the suggestion I wrote in my review and tweak the logarithmic function before falling back the implementation to a conditional block again.

[hobyst]

Offset should be config.KNOB_INCREMENTS_VOL * log(speed, 63) instead of config.KNOB_INCREMENTS_VOL * sensitivity. Same thing goes for decrement.

[hobyst]

Offset should be config.KNOB_INCREMENTS_PAN * log(speed, 63) instead of config.KNOB_INCREMENTS_PAN * sensitivity. Same thing goes for decrement.

[keithhand]

I will add the documentation annotations and remove that leftover parameter default. If you are interested in keeping the commits short, could we squash when merging rather than rewriting the git history?

I would love to dive into the "sensitivity" subject, as that problem took me a while to tackle, and I'm open to discussing my solution. While I think something log-based works on the surface, consider how clockwise and counterclockwise rotations get calculated. More specifically, with the counterclockwise spins, just going off of log by itself became tricky. After a bit of tinkering with different graphs, I came up with, y = abs(-1/x-64):

I tested those changes here 7a91a03#diff-b08e51789bb8c1a5a312a819c3fbf2bdf8413f40d7549cac4d60cc4493e3c5a5L439-R440, and I found that when calculating sensitivity this way, I get fine-tune controls when I want them, but I also don't feel held back by the knob, either. Side note, once I finally got this calculation working, I could tell when my "Shift" button was kicking in, as the precision dropped down to what I think are only sensitivity values 0-9 and felt VERY smooth.

You made an excellent point about other models and how they will use the sensitivity value. The calculation sensitivity = abs(-1 / (speed - 64)) where the keyboard sends a value speed = 63 or speed = 65, will result with sensitivity = 1. So think that this solution will work for these models.

[hobyst]

If you are interested in keeping the commits short, could we squash when merging rather than rewriting the git history?

Being honest, I could talk for weeks about all the possible policies when it comes to commit history organization 😂. I personally prefer to keep commits as independent code attributions that can be reversed and/or dropped independently so in case regressions happen debugging becomes more easy. In this case

When drafting and implementing code every single person has different approaches to it, so while I don't really care about how others integrate the concept of commits on their workflow, it does matter when merging pull requests. In this case doing a squash and merging would do it since you already amended the issues from the first commits on the last ones and would make sense to make all of this a single commit since the code modification isn't that big, but in other cases I might consider it better to keep some things separated from each other in different commits.

I would love to dive into the "sensitivity" subject, as that problem took me a while to tackle, and I'm open to discussing my solution.

Yeah, that's exactly the main reason why I haven't implemented it myself yet. The whole sensitivity calculation is rather complex to tackle since it does not only involve getting to pick the right calculation algorithm but also comparing it to what it actually feels like. I don't have an S-Series MK2 keyboard so for me this whole thing would have been me constantly sending mathematical functions to someone else that actually has the keyboard to get their feedback, everything but seamless 😅.

The function looks good, although to get more feedback I'll be sending the patch to someone else I know that has the keyboard to get their opinion on the overall feel of the knobs and reply you back with their thoughts.

[hobyst]

After testing it for a while with with some people, I've now uploaded the updated controller_definition.py file to the thread for the script in the Image-Line forum to get a more general feedback before merging the pull request.

[hobyst]

Sorry for taking so long to manage the PR, but I completely forgot about this one after waiting for people to share feedback on the FL Studio forums.

After uploading the patched controller_definition.py to the FL Studio forums, it wasn't until July 2023 that someone tried the patch and gave their approval. Because of the lack of feedback on the forums, I'll assume either too little people have tested it and/or those who tried it didn't have anything negative to say about it so I'll merge the PR as it is right now.

Thanks for the contribution!