Feature midi pan issue 95#945
Conversation
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
78abed8 to
0e26069
Compare
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
0e26069 to
da57c59
Compare
|
Here is to the followup work: it's actually a bit more tricky. Mute and Solo keys may be on toggle-type or push-button controllers. One could just indicate those with different letters ('M' for toggle, 'm' for push-button). They have lights inside. Obviously the cleanest way is to have them as push buttons (which is the default I think) and let the program decide whether to light them (since only that way the light can react to the user changing the state in the GUI). However, that requires an actual Midi-out channel. It also requires keeping track of Mute/Solo in the program, and as far as I understand the code, the program does not keep track itself but only lets the toggle buttons in the GUI (which are absent in HEADLESS compilation) do that. Obviously one purpose of using a Midi controller is to support headless operation, like with a Raspberry Pi. So I am not overly clear with the sensible default strategy. The only thing that works reasonably without bidirectional Midi is "toggle button, decides on its own lighting". For the Korg nanoKONTROL2, you can use the delivered app to put it in this mode, and if you use the GUI for changing the buttons, they may go out of sync with the button lights and state on the controller and may require two presses there to resynchronise (like with the non-motorised faders). So I'll likely follow up with options for that behavior: that allows one to configure and use the nanoKONTROL2 sensibly in this regard, even if it's not the nicest config imaginable. |
|
Is this typical for Jamulus development? This merge request was made a week ago. There was no substantial comment, only after 3 days a request to change the formatting to the "standard". This being done, there continues to be no substantial comment, no review. Without 2 approving reviews, the patch is dead. This is not exactly encouraging to contributors. Why invest effort in a project that chooses to play dead to would-be contributors and has set its rules such that they have no chance to actual get anywhere? |
|
Ah, I wasn't clear on
I thought you meant as part of this PR. Do you consider this now complete? |
The work on the buttons is more complicated because the relation between headless and headed does not work in the same way, with no explicit scorekeeping at least of the solo buttons but rather stuff getting collected via the GUI. So that followup work is a lot fuzzier, needs a separate discussion. Particularly so as there are different semantics for toggle controllers and push controllers to be implemented (there may be some point in using This PR is not marked as "draft" and I have addressed the only feedback I got, namely that it needed to be formatted differently. I'll probably do some followup work that works with my particular controller, but given the complete lack of interest or feedback it is not clear that there is a point in submitting something that will likely benefit few people other than myself. |
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
2 times, most recently
from
ce03446 to
6e69590
Compare
|
Unfortunately I can't comment much on this (I don't own a midi controller and don't necessarily the C++ code since I'm not a C++ dev). The only thing I can say is that it seems to compile... I can have a look at the code and try to understand it of course. I do support this PR: controlling Jamulus especially on a headless Raspberry Pi is a feature I strongly support (I think I even commented something like this in #472 (comment),
Is probably a bit more complicated. I really appreciate every contribution and this keeps the community alive. A lot was going on since corrados left and I think we must sort some things out... |
ann0see
left a comment
ann0see
left a comment
There was a problem hiding this comment.
Unfortunately, I can't comment much here. But I assume it's ok.
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
6e69590 to
c084cbc
Compare
softins
left a comment
softins
left a comment
There was a problem hiding this comment.
OK, I've read the changes and the comments already made, and am happy to approve the PR.
On a general note, the syntax for the argument to --ctrlmidich is quite arcane, and we need some additions to the documentation to:
- describe the syntax, and
- give some example recipes for devices
|
Tony Mountifield <[email protected]> writes:
@softins approved this pull request.
OK, I've read the changes and the comments already made, and am happy
to approve the PR.
On a general note, the syntax for the argument to `--ctrlmidich` is
quite arcane, and we need some additions to the documentation to:
* describe the syntax, and
* give some example recipes for devices
Sure. It's just that previous to my patch there was no documentation of
the current behavior to be found anywhere except in some Wiki pages, so
it was not clear where and how the new behavior should be documented.
So it's not really a degression but an ongoing disgrace.
…--
David Kastrup
|
Indeed, it wasn't intended as a criticism of the PR, but just highlighting the need for more info to be put somewhere. The only info I've found so far is at https://jamulus.io/wiki/Tips-Tricks-More#Using-ctrlmidich-for-MIDI-controllers |
|
Probably we need to add something in the documentation, but I'm quite sure @ignotus666 already added them. It'll be part of the new documentation release ;-). @dakhubgit since It's your first PR, could you please add yourself to the contributor list and document this change in the Changelog? |
|
I haven't documented this latest change yet because I'm not really sure what the commands should actually look like. To document the previous change (CC offset) and provide the right syntax I had to trawl through PRs and do repeated tests to check what worked and what didn't... If you could just give me a brief summary of what has changed and what the real-life commands should look like I'll be happy to add it to the documentation. |
|
BTW @dakhubgit your work on MIDI is much appreciated - I'm one of the (apparently few) people who find it really useful. |
|
Given the syntax is backwards-compatible, we could go ahead and merge this separately from the docs change. We don't lose anything. |
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
c084cbc to
db88019
Compare
I put something into the ChangeLog, and since is seems weird to put some half-feature in, I did add an implementation for mute/solo that is enough to use with a Korg nanoKONTROL2 after configuring all the respective buttons as toggles with Korg's utility (it even works under Wine once you set the device properly: actually configuring the device and having it autodetected are mutually exclusive, so you need to ignore and override the initial autodetection "success"), and preferably turn controller LED access to 'internal' so that the controller itself lights and unlights the buttons according to the state they are in. In that case, This will not work (pretty sure) for headless regarding the solo/mute buttons. Also I can find no "contributors' list" that would be manually maintained: it would appear that just an automatically generated list is referenced in the README file. |
|
Sorry. I forgot to link the file/commit. See this commit on how to add yourself: 8fa8581
Can you test if it works on the headless build? It would be great if you were wrong/could find a workaround. All in all this sounds like a great feature! |
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
db88019 to
3b758ed
Compare
I've added myself there, but since I don't want to see my name associated with bad HTML (or at least invalid XHTML) code, I am inconsistent with the other contributors in that I actually quote the href link references. If they were intentionally unquoted, I find it offensive to the reader to pretend otherwise by inserting "" (which is ignored in C++ strings) gratuitously. So I have no idea whether this is allowed to stand. |
|
@dakhubgit Thanks for spotting and bringing up this mistake. @softins is already working on fixing it for all contributors: #945 (comment) |
softins
left a comment
softins
left a comment
There was a problem hiding this comment.
I think this is mainly fine, except for the typo and needing to add the headless support for Solo and Mute.
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
2 times, most recently
from
aff3176 to
a3628f8
Compare
@dakhubgit I think it was a genuine mistake from long ago, thinking that a quote could be embedded in a string by doubling it, which works in some languages, but not C++, although it is not a syntax error, just adjacent string concatenation. #989 fixes it in all places in the code where it occurs. This was not the only occurrence. |
softins
left a comment
softins
left a comment
There was a problem hiding this comment.
OK, I'm happy to approve this. Headless clients can be addressed in the future.
This is preparation for continuing work on issue jamulussoftware#95. While it does not yet introduce functional differences, MIDI controller information is organised in a manner where reacting to more than just fader control messages becomes easy. The preparation right now is for controllers of type fader, pan, solo, mute but can be easily extended for other messages arriving on a single channel.
As part of issue jamulussoftware#95, this implements a new way of specifying various controllers. The previous possibilities are retained. This leaves --ctrlmidich <n> receives on channel n (if 0, on all channels), offset to first fader is 70 (Behringer X-Touch) --ctrlmidich <n>;<off> for specifying a different offset --ctrlmidich <n>;f<off>*<channels>;p<off>*<channels> specified offsets for fader controllers and pan controllers, respectively. There are also s<off> and m<off> specs for Solo and Mute buttons, respectively. This only concerns the command line parsing: the actual implementation is only there for the volume faders.
This actually implements the --ctrlmidich options for panning with a MIDI controller for issue jamulussoftware#95. For example, in native mode a KORG nanoKONTROL2 can be used with Jamulus --ctrlmidich '0;f0*8;p16*8' since its colume faders start at controller 0 and its pan pots start at controller 16.
This implements the --ctrlmidich controller specifications 's' and 'm'. Since the MIDI connection is onedirectional and it is not clear how controller state may be maintained for headless operation, this currently relies on the controller maintaining and displaying state (and accepting that changing the state via the GUI will let the states of controller and GUI diverge). It's not clear to me what this implies for headless operation and what kind of code might need to get added to make that work.
Also add David Kastrup to contributor list.
dakhubgit
force-pushed
the
feature_midi_pan_issue_95
branch
from
a3628f8 to
1131be7
Compare
|
Didn't follow the last steps, but is this now ready to be merged and part of the next release? |
Well, it's backwards compatible and as good as I manage to get it without considerably more involvement for the headless case. If someone wants to do headless, that would apparently involve some refactoring of what gets done where that is not really a reasonable match to the acquaintance with the code I have. It may or may not involve further changes on panners/faders in order to make for a coherent whole. So from my point of view, that's where I can get it. Let's see if it inspires further feature/extension requests or not once it is part of the code base. |
|
Ok. So I've merged this now. |
|
Sorry if I'm asking the obvious, but I want to document this change and would like to better understand what's going on. In, e.g.:
Cheers. |
|
ignotus <[email protected]> writes:
Sorry if I'm asking the obvious, but I want to document this change
and would like to better understand what's going on.
In, e.g.: `--ctrlmidich <n>;f<off>*<channels>;p<off>*<channels>`
- What does `<channels>` refer to? Is that the MIDI channel (1-16)? Is
it `0` for all channels too?
It's sloppy wording. It means the number of consecutive controllers in
MIDI terminology (rather typically 8, but large devices may have 12 or
16).
- Does the offset default to 70 (like for the fader) if none is
specified for p, s and m?
There is no default offset in that syntax. You have to specify a
number. Including for f. The default of 70 only applies if you use the
old, letterless syntax.
Thanks for catering to this!
…--
David Kastrup
|
|
Ok, thanks. Again, sorry, but my experience with MIDI controllers is limited - so would |
|
ignotus <[email protected]> writes:
Ok, thanks. Again, sorry, but my experience with MIDI controllers is
limited - so would `f30*8` mean "CC numbers for faders start at 30 and
end at 37"?
Yes, exactly. I may have had more exposure, but still was too fuzzy
about it to think of the "CC" word. You got that right.
…--
David Kastrup
|
5 participants
This implements the command line parsing for my last proposal on issue #95 . It provides for specifying controllers for fader, pan, solo and mute buttons, but the implementation only retains the functionality for faders (though using a different framework) and in a final commit adds support for panning via MIDI.
I don't have an actual clue what I am doing, so the implementation may well be mistaken. Also the code style seems designed to thwart all conventions and it not supported by any editor or project I know, so I had to constantly fight my editor and may have gotten parts wrong.