Add a beats_translate_move? Like beats_translate_earlier and beats_translate_later, but can be used with a single CC SelectKnob style bind

[mxmilkiib]

Feature Description

My Akai MPD218 rotary encoders give out "ctrl 0x0E, val 0x01" when turned clockwise and "ctrl 0x0E, val 0x7F" when turned anti-clockwise.

Having looked at the code, this is two's complement, and is that the SelectKnob option is for, AFAIU.

But to move the beatgrid, there is only beats_translate_earlier and beats_translate_later, and it is required that these are bound to two separate/different CCs, so only one direction of movement can assigned to a rotary encoder as it stands.

If there were a generic beats_translate_move that could act upon a SelectKnob style input, that would be very handy.

(This relates to #7557 in that the two's complement SelectKnob system is using different values of the same CC, so this is arguably somewhat a subset of that issue)

[Holzhaus]

I think this is quite a specific case. Maybe this would better be mapped in JS?

[mxmilkiib]

Having to write a script to bridge between the way universal aspects of how a MIDI controller can work and the functions Mixxx allows MIDI controllers to affect seems detrimental as it adds a needless hurdle of complexity to the average user.

I mean, moving the grid left and right with a rotary encoder? That's imho really not that far out!

[daschuer]

Finally "source code wins". Since we already have a bunch of beats_translate_.... controls on more does not make a big difference. I would not object, if one contributes such a solution.

[mxmilkiib]

Would it be something like this?

[ronso0]

Yes, mxmilkiib@d4f5e90 would work.
But, to reduce code duplication in slotTranslateBeatsEarlier and slotTranslateBeatsLater, we can make them call slotTranslateBeats with the argument (+-) and do the sample / beats offset calculation there.

[daschuer]

Do we have a control type for it? It looks a bit hack to have the midi key value in our engine code.

[daschuer]

I think it is "ControlEncoder"

[ronso0]

Even though ControlEncoder should be used (to avoid any undesired behaviour of ControlPushButton), it does nothing special at all:

mixxx/src/control/controlbehavior.h

Lines 32 to 34 in 91c1869

	// ControlEncoderBehavior passes the midi value directly to the internal parameter value. It's
	// useful for selector knobs that pass +1 in one direction and -1 in the other.
	class ControlEncoderBehavior : public ControlNumericBehavior {

mixxx/src/control/controlbehavior.cpp

Lines 34 to 40 in 91c1869

	double ControlEncoderBehavior::midiToParameter(double midiValue) {
	return midiValue;
	}
	double ControlEncoderBehavior::valueToMidiParameter(double dValue) {
	return dValue;
	}

[mxmilkiib]

So move the code from slotTranslateBeatsEarlier and slotTranslateBeatsLater into slotTranslateBeatsMove and have an if based on the value there, and change https://github.com/mxmilkiib/mixxx/blob/beats_translate_move/src/engine/controls/bpmcontrol.cpp#L83 to point to slotTranslateBeatsMove (changing false to this)? And also change the control type of ControlPushButton to ControlEncoder on not just 90 but 80 and 85 also? Or leave 80 and 85, given it can still be bound to a button? And change 83 and 88 to just point to slotTranslateBeatsMove?

[ronso0]

So move the code from slotTranslateBeatsEarlier and slotTranslateBeatsLater into slotTranslateBeatsMove and have an if based on the value there

yes. Both COs are connected to their respective slot, both slots call slotTranslateBeatsMove(double v) with v being either positive or negative.

change https://github.com/mxmilkiib/mixxx/blob/beats_translate_move/src/engine/controls/bpmcontrol.cpp#L83 to point to slotTranslateBeatsMove

No.

(changing false to this)

This indicates you didn't build the sketch you posted ;) since this is not a valid argument for ControlPushButton.

[mxmilkiib]

Yeah, I just edted it in the Github editor. Now I've setup VSCodium, haven't had the time to get my Neovim setup as unannoying as my Vim setup yet..

I changed it to;

void BpmControl::slotTranslateBeatsLater(double v) {
    BpmControl::slotTranslateBeatsMove(v);
}

void BpmControl::slotTranslateBeatsEarlier(double v) {
    BpmControl::slotTranslateBeatsMove(v);
}

void BpmControl::slotTranslateBeatsMove(double v) {
    if (v <= 0) {
    return;
    }
    const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
    if (!pTrack) {
        return;
    }
    const mixxx::BeatsPointer pBeats = pTrack->getBeats();
    if (pBeats) {
        // TODO(rryan): Track::frameInfo is possibly inaccurate!
        if (v <= 63) {
            const double sampleOffset = frameInfo().sampleRate * -0.01;
        } else {
            const double sampleOffset = frameInfo().sampleRate * 0.01;
        }
        const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
        const auto translatedBeats = pBeats->tryTranslate(frameOffset);
        if (translatedBeats) {
            pTrack->trySetBeats(*translatedBeats);
        }
    }
}

but then I get;

[build] /home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp: In member function ‘void BpmControl::slotTranslateBeatsMove(double)’:
[build] /home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp:211:26: warning: unused variable ‘sampleOffset’ [-Wunused-variable]
[build]   211 |             const double sampleOffset = frameInfo().sampleRate * -0.01;
[build]       |                          ^~~~~~~~~~~~
[build] /home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp:213:26: warning: unused variable ‘sampleOffset’ [-Wunused-variable]
[build]   213 |             const double sampleOffset = frameInfo().sampleRate * 0.01;
[build]       |                          ^~~~~~~~~~~~
[build] /home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp:215:55: error: ‘sampleOffset’ was not declared in this scope; did you mean ‘frameOffset’?
[build]   215 |         const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
[build]       |                                                       ^~~~~~~~~~~~
[build]       |                                                       frameOffset
...

I'm not a programmer, I get emotionally (and physically) dysregulated when I can't get something to work, a symptom of some with ADHD+ASD (along with PoTS). I've done one C++ bit of work before, that was a copy and paste pattern matching hack PR on Carla to create a MIDI switch internal plugin, and it took a lot of handholding from falkTX.

I get the gist of the build error saying that later use of the variable is not good because it possibly would not be defined for that "lower"(?) scope (actually, no, I don't understand it, because it's either 63 or below or 64 and above), but moving the later uses inside of each side of the if just reproduces the duplicated code scenario, no?

[ronso0]

Alright, no worries, I can take over. I hoped that with a little guidance you get hooked and would become a regular : )
If it's too much stress (and c++ trial and error is stress / frustrating / disappointing for 'lateral entrants'), don't worry.

Re the compiler error:
sampleOffset is a scoped variable not a member variable, i.e. it exists only in the scope it was declared in. A scope might be a function, or just an if or else scope. Here, sampleOffset doesn't exist anymore when you want to use it to calculate frameOffset. It needs to be declared before the if/else scopes.

Anyway, I can take over if you like.

Also, we don't use the MIDI values as in controller scripts (btw 63 which you used wouldn't work for your encoder ;).
IIUC we're creating this control so it can be mapped directly to an encoder (with the MIDI Wizard) with the SelectKnob option (like [Library],MoveVertical for example). SelectKnob already transforms the absolute MIDI values to relative values, e.g. -n or +n, so we simply need to check if it's equal 0 or greater/less.

[mxmilkiib]

Maybe I can manage. I've been suffering from fatigue the last 48 hours, sleeping lots and finding it hard to keep my eyes open. My genetic constitution and its expressions can be quite frustratin! Thanks for the feedback.

So I've moved the logic and sorted definitions, etc.

But after building and binding to an encoder, it only goes one way and not the other. It was going the wrong way too, so I flipped the 0.01 and -0.01. Now it's not going either way! Time to rest.

Also, I get this warning on build;

[5/7] Building CXX object CMakeFiles/mixxx-lib.dir/src/engine/controls/bpmcontrol.cpp.o
/home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp: In constructor ‘BpmControl::BpmControl(const QString&, UserSettingsPointer)’:
/home/milk/pkgs/mixxx-git-debug/src/mixxx-git/src/engine/controls/bpmcontrol.cpp:90:94: warning: ‘nonnull’ argument ‘this’ compared to NULL [-Wnonnull-compare]
   90 |     m_pTranslateBeatsMove = new ControlEncoder(ConfigKey(group, "beats_translate_move"), this);
      |                                                                                              ^

[ronso0]

Okay, great.

I suggest you open a (draft) PR against the mixxx repo so we can discuss the code changes much easier (inline comments etc.)

[ronso0]

I also suggest to set up some IDE. This simplifies looking up definitions, code paths etc.
E.g. how ControlEncoder is constructed

mixxx/src/control/controlencoder.h

Line 9 in 2c2e706

ControlEncoder(const ConfigKey& key, bool bIgnoreNops = true);

This indicates the second argument (if present) must be a bool (defaults to true if absent).

[mxmilkiib]

I setup VSCodium with various plugins, but my problem is that, when I run the Arch makepkg util to create a package to install the build, it creates a makepkg branch, and I'm not sure how to avoid this, or at least cleanly undo this. I made all the edits three times before giving up and going back to the GitHub editor. Also not sure why it didn't flag up the incorrect type in the IDE itself.

N ah yes, the argument type was mentioned previously.

[ronso0]

I don't think you need to install, you can run the build directly from the build directory. Or is this different on Arch? I'm not familiar with it.

it creates a makepkg branch, and I'm not sure how to avoid this, or at least cleanly undo this.

A directory? If yes, you can add stuff like that to your private .gitignore file (not the one in the mixxx source directory)

[mxmilkiib]

I just prefer to have the debug version as the one that's in $PATH, cos I'm never going to find bugs if I don't dogfood use the bleeding edge, so to do that I create a package using the makepkg system, but that creates a makepkg branch, and then I accidently commit the next thing to it or such, and I've not consistently used git enough to remember how to avoid being tripped up by or reverse that (I forget a lot of things).

Maybe I should indeed just not install and run separately, just the autistic part of me doesn't like change if it can be helped.

[ronso0]

Fixed by #12376