Basic support for MIDI input note range limiting

[kelytha]

A very simple solution to limit MIDI input note ranges on
instruments. Using this function it is possible to create an
arbitrary number of zones on a MIDI keyboard, assigning each zone
to a different instrument. Relates to issues #1792 and #1381

[zonkmachine]

Cool! Testing it right now... 8)

[zonkmachine]

Especially the people, and there are a bunch of them, who call for more live oriented features will love this.

I found an issue. If you press keys, as one might, to find the range and scroll the midi min/max so you suddenly are out of range, the key is now stuck. You un-stick it by hitting the same key again. Either on the midi keyboard after adjusting it back into range or by pressing the keyboard on the instrument gui.

Emit note off for every note you just left out of the range?

[zonkmachine]

Thought no.2
I think you should put a limit on the setting so Min can't be bigger than Max and vice versa.

[kelytha]

@zonkmachine Thank you very much for the feedback.

I have tried to implement your suggestions, but I am a little lost on what would be the best solution there. It seems to me, that the InstrumentMidiIOView class is modifying the IntModel members of MidiPort directly, instead of going through the getter/setter functions.

I tried adding MidiPort::set[Lower|Upper]InputNoteLimit() methods with the suggested checks and emiting of a MIDI note off for the closed out note (current value of the model, before setting the new one), but it seems like the setters don't get invoked when I change the values on the GUI.

[zonkmachine]

I have tried to implement your suggestions,

I'm in no way an authority around here but I found those changes reasonable. When I check the commit history the latest work with the word MIDI in it was done by @Wallacoloo and he's more qualified to give you advice here.

It seems to me, that the InstrumentMidiIOView class is modifying the IntModel members of MidiPort directly, instead of going through the getter/setter functions.

I said 'emit note off' but I just made that up on the fly in lack of a proper term. I think vaguely that you need to 'talk' to the noteplayhandler and not to MIdiPort. However, I don't know my way around those parts of the code.

In the case of the spinbox you should be able to lock the max/min values to the current setting of the other spinbox. I've done this with knobs so it should work with spinboxes too.
So something along the lines of:
setMaximum( otherSpinbox.value() )

http://doc.qt.io/qt-4.8/qspinbox.html#maximum-prop
http://doc.qt.io/qt-4.8/qspinbox.html

[kelytha]

I'm in no way an authority around here but I found those changes reasonable.

I agree with that, that is why I wanted to implement them.

I said 'emit note off' but I just made that up on the fly in lack of a proper term.

Yes, it is also not the term that matters here. My problem is that I couldn't figure out how to add any code that runs when the value of the spinbox is changed.

[zonkmachine]

I don't have time to look into this further right now but this looked interesting and maybe what you need.
According to http://doc.qt.io/qt-4.8/qspinbox.html#value-prop
setValue() will emit valueChanged() if the new value is different from the old one.

[curlymorphic]

@kelytha Thanks for your contribution. Ive not really had time to look over this yet, but I do like the idea :)

My problem is that I couldn't figure out how to add any code that runs when the value of the spinbox is changed.

The model class emits a DataChanged() QtSignal signal you could utilize.
https://github.com/LMMS/lmms/blob/master/src/core/AutomatableModel.cpp#L233

For an example of it being used
https://github.com/LMMS/lmms/blob/master/plugins/triple_oscillator/TripleOscillator.cpp#L109

[softrabbit]

TBH, I'd like to see this feature implemented closer to the Instrument class, to be useful also when playing back notes from the sequencer. I'm thinking towards possible future features where this might be useful, like #2134 maybe?

[kelytha]

@softrabbit I think the two use-cases should be separated somehow. My intention here is only to limit the MIDI input notes, but I still want to allow all sounds for the instrument and for the MIDI output. This way for example I can have only 4 keys of a MIDI keyboard assigned to a bass synthesizer and play a progression of arpeggios on it, while the rest of the keyboard plays the lead part.

[zonkmachine]

It makes perfect sense to have it work in a similar way with notes from the sequencer but I think it's better to fix this one and have it merged and then further down the line change/expand upon it.

[tresf]

@zonkmachine @softrabbit are we asking that this is closed, fixed, or merged? Your comments read like a mix between fixed and closed. :)

[softrabbit]

@tresf, my opinion is still the same: there's a better place for this feature. But I won't lose any sleep if this gets merged either.

[tresf]

Ok, so we have two issues:

1. Do we want this in its current place described here Basic support for MIDI input note range limiting #2185 (comment)
2. Fix the note range bug described here Basic support for MIDI input note range limiting #2185 (comment)

In regards to the first request, @softrabbit can you elaborate a bit more on what "Closer to the instrument class" means? Are you saying you'd rather we improve the note range visually from an instrument perspective (such as placing markers on the piano roll?). If so, do you find the features mutually exclusive? Also, @kelytha in the current design is there a visual indicator when a midi range on one instrument overlaps that on another instrument?

In regards to the note range bug, I think @curlymorphic explained best how to use the signals and slots. Notes stuck on are terrible from a user experience perspective, and that has to be fixed prior to merging.

[softrabbit]

can you elaborate a bit more on what "Closer to the instrument class" means?

Sure. It means that this limiting should happen somewhere closer to the actual sound generation, if possible. Maybe somewhere like https://github.com/LMMS/lmms/blob/master/src/tracks/InstrumentTrack.cpp#L456. That way it would work for all kinds of notes, both ones coming in over MIDI and ones from piano roll tracks or the internal piano widget.

Visually, it could be spin boxes, knobs, drop down menus, dots over the piano keyboard or something else.

In regards to the note range bug, I think @curlymorphic explained best how to use the signals and slots.

Or it could be handled through filtering only note on events and passing note offs even if they're out of range:

• If a note off matching a playing note arrives, the note must've started before the range changed, so turn it off.
• If a note off comes in not matching a playing note, I'm pretty sure it's a situation any MIDI synth should be prepared to handle.

[tresf]

@softrabbit I guess what I'm trying to determine is whether or not this is best left as a separate PR. If this is to ideally be implemented in a better fashion, adding this functionality now would be a mistake, no?

[tresf]

@softrabbit I'm leaving the decision on this one up to you.

[softrabbit]

@softrabbit I'm leaving the decision on this one up to you.

Ok, I'll have to look closer at the code first to see if I missed any finer details.

[Reaper10]

This is a question on the note min & max readout boxes why do they use numbers instead of note names like this ----------------> Min Note A#4 Max Note C5?

[tresf]

@softrabbit @kelytha shall we keep this PR open a bit longer?

[kelytha]

@tresf Sorry for being silent. I probably won't be able to return working on this. I don't really have the required knowledge, and to be honest I'm not using LMMS anymore.

[tresf]

@kelytha no hard feelings. We'll reference this PR sometime down the road when the feature comes to fruition. 👍