-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always give own channel MIDI number 0 #3419
Conversation
1. Moved my channel ID from the Mixer to the Client, so it is available when headless too. 2. Client's own channel, sent by the server, is always assigned to MIDI number 0, for left-most fader. 3. Translate between server channel and MIDI number at the client level.
@AndersGoran - I would be interested in your feedback on this solution. Thanks! |
I'm happy with this for my purposes! I can confirm I get my own fader mapped to the first CC I give in I tested on one server with two other people - I saw channel numbers 0 (me), 1, 2, and with midi string "0;f100*4", my first three physical controls map to channels 0,1,2 in order - perfect. No duplicate control for me. That said, on another server, there was me (0) and someone else (9) and then I could only control my own fader, since 9 is out of range. That's not an issue for me but rather a general problem with I tried the macOS build at https://github.com/jamulussoftware/jamulus/actions/runs/11702443604/artifacts/2151703050 |
Great, thanks for the quick feedback!
Yes, that could happen if there had been many people on a server, and most depart leaving one of the last joiners. Addressing that kind of issue would require a considerably more complex design, and still probably wouldn't cater for the above scenario without fader positions jumping about. I think the solution in this PR is worth having now for the majority of scenarios, without keeping it on hold awaiting a completely new design structure that might take ages to materialize! |
@@ -436,6 +436,7 @@ void CChannelFader::Reset() | |||
plblCountryFlag->setVisible ( false ); | |||
plblCountryFlag->setToolTip ( "" ); | |||
cReceivedChanInfo = CChannelInfo(); | |||
cReceivedMIDIID = INVALID_INDEX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using INVALID_INDEX
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it just needs to be something like -1
that couldn't match a real MIDI Controller offset from 0
upwards. I don't mind whether we use that symbol or define another symbol for it, although I don't see the point of changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I said. I was told it was confusing so I changed it. I'm raising here because it was one of the first objections on my pull request, so I'm assuming it was something important.
@@ -1123,7 +1123,7 @@ void CAudioMixerBoard::ChangeFaderOrder ( const EChSortType eChSortType ) | |||
} | |||
break; | |||
case ST_BY_SERVER_CHANNEL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this get renamed if it's always the received MIDI ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did wonder about that, and even started to change the name, but then had second thoughts, as it implied the menu option should change to "Sort by MIDI Channel". I think this could be more confusing to some users.
The actual server channel numbers and the channel assigned to the particular client are never actually visible to the user, except when used for MIDI channels.
What we are doing here by mapping is presenting to the user a view that always uses 0
for the user's own channel, and maps the rest of the channels up by one. As far as the user is concerned, they are server channels (even if virtual); we just guarantee that the user themselves is channel zero. So whether or not a MIDI controller is used, a "Sort by Server Channel" will always put the user's own channel first, whether or not "Show Own Fader First" is enabled. I think that is a worthwhile improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a definite source of potential confusion, to be honest. I'd only want my fader sorted first if I asked for it, if there's an option in the menu. I'd be wondering why the option was there, otherwise.
Certainly for "No sorting", always putting own fader first is wrong unless requested.
src/audiomixerboard.cpp
Outdated
@@ -1267,7 +1267,7 @@ void CAudioMixerBoard::ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInf | |||
vecpChanFader[iChanID]->Reset(); | |||
vecAvgLevels[iChanID] = 0.0f; | |||
|
|||
if ( static_cast<int> ( iChanID ) == iMyChannelID ) | |||
if ( static_cast<int> ( iChanID ) == pClient->GetMyChannelID() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe stick pClient->GetMyChannelID()
into a variable as it's used in a few places here. (The call is crossing threads, I think.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value doesn't change while connected, and it is just a simple Get method, so I don't think threads are relevant in this case. However, I see most of these uses are within a loop, so I'm happy to fetch once to a local variable within each function and use that within the loop. I'll add a commit for that shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 072b2af
|
||
int CClient::ChanToMIDI ( const int iChannelIdx ) | ||
{ | ||
if ( iMyChannelID == INVALID_INDEX ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, it was suggested using INVALID_INDEX
for something that isn't an index into a vector wasn't a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can choose an additional name for a -1
value if you prefer. I see src/global.h
has both INVALID_PORT
and INVALID_INDEX
for -1
, so we could also add something like INVALID_ID
, INVALID_CHANNEL
or INVALID_VALUE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my PR.
@@ -272,6 +274,10 @@ class CClient : public QObject | |||
Channel.GetBufErrorRates ( vecErrRates, dLimit, dMaxUpLimit ); | |||
} | |||
|
|||
// convert between MIDI index and real channel index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be done my the MIDI-handling code? Otherwise, it could do with a longer comment explaining why it's here.
{ | ||
// map the MIDI index to the real channel number | ||
const int iChannelIdx = MIDIToChan ( iMIDIIdx ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client shouldn't need to worry about this. It should have been handled in the MIDI code. The MIDI code should signal this slot.
void ControllerInPanValue ( int iChannelIdx, int iValue ); | ||
void ControllerInFaderIsSolo ( int iChannelIdx, bool bIsSolo ); | ||
void ControllerInFaderIsMute ( int iChannelIdx, bool bIsMute ); | ||
void ControllerInFaderLevel ( int iMIDIIdx, int iValue ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMidiCtlEntry
should have the first entry renamed to match if it's not iChannel
any more. Or else rename it to iChannel
-- it's the value of CMidiCtlEntry::iChannel
, after all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really a "virtual channel", where 0 is our fader, and the others are allocated in server order.
In MIDI-speak it's not a channel anyway, it's a controller offset. We are just using it to access a Jamulus channel.
I'm happy to rename items if you have suggestions, but I still think the architecture is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the offset from the base MIDI Controller number. It's the Jamulus channel numbers that get re-ordered so that the user's own Jamulus channel is at offset 0 when the user uses z
(and should only be when they use z
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, whatever. My problem with the z
solution is the duplicate fader. That's a show-stopper for me, and why I put in a lot of work to come up with an improved solution, instead of just complaining about it.
I'm going to leave this for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think CSoundBase::ParseMIDIMessage
is where the mapping from MIDI Controller Number to Jamulus Channel Number should happen. Keep the MIDI out of CClient entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my general comment in the main thread.
I'm not particularly happy with putting MIDI knowledge into CClient - we don't (maybe I should say "shouldn't") put JSON RPC knowledge in or UI widget knowledge in. MIDI control of the client should happen through signals to slots and the client shouldn't really know what caused the slot to be called. Ideally, MIDI control, JSON RPC control and UI control would use the same slots on the client - i.e. they'd emit a signal that complied with the interface defined by the slot and they'd connect themselves to the slot (because the client doesn't know about them). But all three should use the same slot for the same purpose with the same arguments, without CClient having to know what called it. |
I can see where you're coming from, but I'm not convinced. At the moment, Soundbase doesn't know anything about Jamulus channels, and particularly not about which is our channel. All it does is pass MIDI numbers (CC offsets) to the client. It's the client that know what our channel number is, and decides how to use and map those MIDI numbers. In fact before this PR, it was only the Audio Mixer Board that knew about our channel. That meant decisions based on it could only be made at the GUI level, and precluded headless operation. I needed to move You mentioned both MIDI and JSON-RPC. Should they all be responsible for knowing what our current channel number is, so that they can each do their own mapping? I'm not convinced. They can both just rest in the knowledge that virtual channel number 0 will always control our own channel, and rely on So I would still argue in favour of what I put in this PR. |
If you look at my implementation, you'll see that soundbase.cpp continues to know nothing about Jamulus channels - all that changes is the parsing of the I think you're pulling into CClient concerns that shouldn't be brought in. My implementation also removes CClientDlg needing to be involved in knowing about MIDI controls, so improving the separation rather than making it worse, like this. |
Except that you end up with two separate faders controlling the "own" channel, rather than having the "own" channel only being fader 0 and the others being arranged to suit. And audiomixerboard is way to high up the hierarchy, as I found, which means the headless code can't use it.
I still disagree. I'm effectively virtualising the channel numbers so that own channel is always 0.
|
This shouldn't be such hard work :( |
Yes, that's to be expected:
I don't see that as confusing or a problem. EDIT: if the sort order is by Jamulus channel number. I've my "special" fader plus whoever (including me) is assigned to the normal channel order. Upsetting that means the visual cues between what I see on screen and what I've set up on my MIDI controller aren't congruent. |
Agreed. The "model-view-controller" concept torn into shreds. Only CClient should know about state (whether transient or persistent). The UI controls, JSON-RPC controls, MIDI controls, etc, shouldn't have to know anything - just send signals for "what just happened". Maybe that should be what Jamulus 4 does...
Agreed, too. |
If that's the intent, discussing "MIDI" throughout the approach is highly misleading. |
Well, yes, but that concept only came to mind when you started also relating JSON-RPC to it. |
OK -- maybe this approach to |
I still think there is a better approach. I'm just writing up some notes to post in a while. |
OK, let's take this concept and run with it, although without the "virtual" term. Currently, there is no distinction between "client-side" channel and "server-side" channel. The Let's decouple the client-side and server-side channel numbers, and make "client-side channel" a concept local to an individual client, mapping it within the client at the lowest level possible to the "server-side channel". Something like this within
Maybe we put other items in there that are currently just arrays indexed by server channel index. Or maybe not, but they would now be indexed by client-side channel ID.
In view of recent experience, I'm not going to waste a lot of time implementing the above until there is agreement about the approach. |
OK, from a GUI perspective - all the existing sort options would continue to work as is, except "Sort by Channel", where "My Channel" would automatically be first (and thus adding "Own channel first" would have no visible effect; still useful on the other options). A server with one person on where they got server channel 10 allocated when they joined.
Now two different scenarios - not problems, just to think through:
OK, I think that makes sense, too -- if you leave and rejoin, the likelihood is you'll get the same EDIT:
|
From there, GUI, JSON-RPC and MIDI control of CClient messaging to the server to adjust the mix:
So I think this is a good first step (and a discrete change from the above). |
I'm wondering if we need to avoid making That would mean the existing "GUI-only" mixing desk would be split into a class that was independent of I'd almost not mind all of that in one file with a bunch of |
I was honestly also thinking about this for a while. From the perspective of good separation it would make sense:
The biggest downside would be that this may be hard to get through a review. |
These are all good thoughts. But I think we should go incrementally, rather than for a big bang which might take ages to finish or not even get around to being completed. So I would think of implementing what I suggested above within I'm aware too that it will be necessary to translate from server channel IDs to client channel IDs within |
Yep, not suggesting doing it in one go. But having these goals laid out clearly as incremental planned steps would be useful, I think, so we can assess progress to a better implementation. |
Short description of changes
Ensure the user's own channel is always given MIDI channel 0, so that the user is always the leftmost fader when using an external MIDI fader bank.
CHANGELOG: Client: always assign MIDI channel 0 to the user's own channel.
Context: Fixes an issue?
Raised in https://github.com/orgs/jamulussoftware/discussions/2220#discussioncomment-10811813
This supersedes the solution proposed in #3394, and in particular avoids the use of a duplicate fader. It also needs no change to the
--ctrlmidich
parameter.It is an implementation of my proposal in #3394 (comment)
Does this change need documentation? What needs to be documented and how?
Only to document that the user's own channel will always be given fader channel 0, and that this is only relevant
when using
--ctrlmidich
to assign an external fader bank.Status of this Pull Request
Tested and ready for review
What is missing until this pull request can be merged?
Consensus on the approach and testing on other platforms. Tested on Pi Linux.
Checklist