Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Always give own channel MIDI number 0 #3419

Closed
wants to merge 2 commits into from
Closed

Always give own channel MIDI number 0 #3419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46f7567
Always give own channel MIDI number 0
softins Nov 5, 2024
072b2af
Save my channel ID in local variable for efficiency
softins Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions src/audiomixerboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ void CChannelFader::SetGUIDesign ( const EGUIDesign eNewDesign )
UpdateGroupIDDependencies();

// the instrument picture might need scaling after a style change
SetChannelInfos ( cReceivedChanInfo );
SetChannelInfos ( cReceivedChanInfo, cReceivedMIDIID );
}

void CChannelFader::SetMeterStyle ( const EMeterStyle eNewMeterStyle )
Expand Down Expand Up @@ -436,6 +436,7 @@ void CChannelFader::Reset()
plblCountryFlag->setVisible ( false );
plblCountryFlag->setToolTip ( "" );
cReceivedChanInfo = CChannelInfo();
cReceivedMIDIID = INVALID_INDEX;
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

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.

SetupFaderTag ( SL_NOT_SET );

// set a defined tool tip time out
Expand Down Expand Up @@ -666,10 +667,11 @@ void CChannelFader::UpdateSoloState ( const bool bNewOtherSoloState )

void CChannelFader::SetChannelLevel ( const uint16_t iLevel ) { plbrChannelLevel->SetValue ( iLevel ); }

void CChannelFader::SetChannelInfos ( const CChannelInfo& cChanInfo )
void CChannelFader::SetChannelInfos ( const CChannelInfo& cChanInfo, int iMIDIID )
{
// store received channel info
cReceivedChanInfo = cChanInfo;
cReceivedMIDIID = iMIDIID;

// init properties for the tool tip
int iTTInstrument = CInstPictures::GetNotUsedInstrument();
Expand All @@ -682,7 +684,7 @@ void CChannelFader::SetChannelInfos ( const CChannelInfo& cChanInfo )
// show channel numbers if --ctrlmidich is used (#241, #95)
if ( bMIDICtrlUsed )
{
strModText.prepend ( QString().setNum ( cChanInfo.iChanID ) + ":" );
strModText.prepend ( QString().setNum ( iMIDIID ) + ":" );
}

QTextBoundaryFinder tbfName ( QTextBoundaryFinder::Grapheme, cChanInfo.strName );
Expand Down Expand Up @@ -886,7 +888,6 @@ CAudioMixerBoard::CAudioMixerBoard ( QWidget* parent ) :
bDisplayPans ( false ),
bIsPanSupported ( false ),
bNoFaderVisible ( true ),
iMyChannelID ( INVALID_INDEX ),
iRunningNewClientCnt ( 0 ),
iNumMixerPanelRows ( 1 ), // pSettings->iNumMixerPanelRows is not yet available
strServerName ( "" ),
Expand Down Expand Up @@ -1049,7 +1050,6 @@ void CAudioMixerBoard::HideAll()
bIsPanSupported = false;
bNoFaderVisible = true;
eRecorderState = RS_UNDEFINED;
iMyChannelID = INVALID_INDEX;
iRunningNewClientCnt = 0; // reset running counter on new server connection

// use original order of channel (by server ID)
Expand Down Expand Up @@ -1123,7 +1123,7 @@ void CAudioMixerBoard::ChangeFaderOrder ( const EChSortType eChSortType )
}
break;
case ST_BY_SERVER_CHANNEL:
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

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.

PairList << QPair<QString, size_t> ( QString ( "%1" ).arg ( vecpChanFader[i]->GetReceivedChID(), 3, 10, QLatin1Char ( '0' ) ) +
PairList << QPair<QString, size_t> ( QString ( "%1" ).arg ( vecpChanFader[i]->GetReceivedMIDIID(), 3, 10, QLatin1Char ( '0' ) ) +
vecpChanFader[i]->GetReceivedName().toLower(),
i );
break;
Expand Down Expand Up @@ -1246,6 +1246,8 @@ void CAudioMixerBoard::ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInf
iFaderNumber[vecChanInfo[iFader].iChanID] = static_cast<int> ( iFader );
}

const int iMyChannelID = pClient->GetMyChannelID();

// Hide all unused faders and initialize used ones
for ( size_t iChanID = 0; iChanID < MAX_NUM_CHANNELS; iChanID++ )
{
Expand Down Expand Up @@ -1322,7 +1324,7 @@ void CAudioMixerBoard::ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInf
}

// set the channel infos
vecpChanFader[iChanID]->SetChannelInfos ( vecChanInfo[idxVecpChan] );
vecpChanFader[iChanID]->SetChannelInfos ( vecChanInfo[idxVecpChan], pClient->ChanToMIDI ( iChanID ) );
}

// update the solo states since if any channel was on solo and a new client
Expand Down Expand Up @@ -1394,6 +1396,8 @@ void CAudioMixerBoard::SetAllFaderLevelsToNewClientLevel()
{
QMutexLocker locker ( &Mutex );

const int iMyChannelID = pClient->GetMyChannelID();

for ( size_t i = 0; i < MAX_NUM_CHANNELS; i++ )
{
// only apply to visible faders and not to my own channel fader
Expand Down Expand Up @@ -1422,6 +1426,8 @@ void CAudioMixerBoard::AutoAdjustAllFaderLevels()
CVector<CVector<float>> levels;
levels.resize ( MAX_NUM_FADER_GROUPS + 1 );

const int iMyChannelID = pClient->GetMyChannelID();

// compute min/max level per group and number of channels per group
for ( size_t i = 0; i < MAX_NUM_CHANNELS; ++i )
{
Expand Down Expand Up @@ -1778,4 +1784,4 @@ void CAudioMixerBoard::SetChannelLevels ( const CVector<uint16_t>& vecChannelLev
}
}

void CAudioMixerBoard::MuteMyChannel() { SetFaderIsMute ( iMyChannelID, true ); }
void CAudioMixerBoard::MuteMyChannel() { SetFaderIsMute ( pClient->GetMyChannelID(), true ); }
9 changes: 5 additions & 4 deletions src/audiomixerboard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ class CChannelFader : public QObject
int GetReceivedInstrument() { return cReceivedChanInfo.iInstrument; }
QString GetReceivedCity() { return cReceivedChanInfo.strCity; }
int GetReceivedChID() { return cReceivedChanInfo.iChanID; }
void SetChannelInfos ( const CChannelInfo& cChanInfo );
int GetReceivedMIDIID() { return cReceivedMIDIID; }
void SetChannelInfos ( const CChannelInfo& cChanInfo, int iMIDIID );
void Show() { pFrame->show(); }
void Hide() { pFrame->hide(); }
bool IsVisible() { return !pFrame->isHidden(); }
Expand Down Expand Up @@ -120,6 +121,7 @@ class CChannelFader : public QObject
QLabel* plblCountryFlag;

CChannelInfo cReceivedChanInfo;
int cReceivedMIDIID;

bool bOtherChannelIsSolo;
bool bIsMyOwnFader;
Expand Down Expand Up @@ -190,6 +192,7 @@ class CAudioMixerBoard : public QGroupBox, public CAudioMixerBoardSlots<MAX_NUM_

virtual ~CAudioMixerBoard();

void SetClientPointer ( CClient* pNClient ) { pClient = pNClient; }
void SetSettingsPointer ( CClientSettings* pNSet ) { pSettings = pNSet; }
void HideAll();
void ApplyNewConClientList ( CVector<CChannelInfo>& vecChanInfo );
Expand All @@ -200,8 +203,6 @@ class CAudioMixerBoard : public QGroupBox, public CAudioMixerBoardSlots<MAX_NUM_
void SetDisplayPans ( const bool eNDP );
void SetPanIsSupported();
void SetRemoteFaderIsMute ( const int iChannelIdx, const bool bIsMute );
void SetMyChannelID ( const int iChannelIdx ) { iMyChannelID = iChannelIdx; }
int GetMyChannelID() const { return iMyChannelID; }

void SetFaderLevel ( const int iChannelIdx, const int iValue );

Expand Down Expand Up @@ -260,14 +261,14 @@ class CAudioMixerBoard : public QGroupBox, public CAudioMixerBoardSlots<MAX_NUM_
void UpdateSoloStates();
void UpdateTitle();

CClient* pClient;
CClientSettings* pSettings;
CVector<CChannelFader*> vecpChanFader;
CMixerBoardScrollArea* pScrollArea;
QGridLayout* pMainLayout;
bool bDisplayPans;
bool bIsPanSupported;
bool bNoFaderVisible;
int iMyChannelID; // must use int (not size_t) so INVALID_INDEX can be stored
int iRunningNewClientCnt; // integer type is sufficient, will never overrun for its purpose
int iNumMixerPanelRows;
QString strServerName;
Expand Down
60 changes: 56 additions & 4 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ CClient::CClient ( const quint16 iPortNumber,
ChannelInfo(),
strClientName ( strNClientName ),
Channel ( false ), /* we need a client channel -> "false" */
iMyChannelID ( INVALID_INDEX ),
CurOpusEncoder ( nullptr ),
CurOpusDecoder ( nullptr ),
eAudioCompressionType ( CT_OPUS ),
Expand Down Expand Up @@ -789,8 +790,44 @@ void CClient::OnHandledSignal ( int sigNum )
#endif
}

void CClient::OnControllerInFaderLevel ( int iChannelIdx, int iValue )
// Ensure user's own channel is always given MIDI offset 0, so it is the first physical fader
// All channels with a client ID less than user's own will use ID+1 as their MIDI offset

int CClient::ChanToMIDI ( const int iChannelIdx )
{
if ( iMyChannelID == INVALID_INDEX )
Copy link
Collaborator

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.

Copy link
Member Author

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my PR.

return iChannelIdx;

if ( iChannelIdx == iMyChannelID )
return 0;

if ( iChannelIdx < iMyChannelID )
return iChannelIdx + 1;

return iChannelIdx;
}

int CClient::MIDIToChan ( const int iMIDIIdx )
{
if ( iMyChannelID == INVALID_INDEX )
return iMIDIIdx;

if ( iMIDIIdx == 0 )
return iMyChannelID;

if ( iMIDIIdx <= iMyChannelID )
return iMIDIIdx - 1;

return iMIDIIdx;
}

// Handle received MIDI controls

void CClient::OnControllerInFaderLevel ( int iMIDIIdx, int iValue )
{
// map the MIDI index to the real channel number
const int iChannelIdx = MIDIToChan ( iMIDIIdx );
Copy link
Collaborator

@pljones pljones Nov 6, 2024 •
edited
Loading

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.


// in case of a headless client the faders cannot be moved so we need
// to send the controller information directly to the server
#ifdef HEADLESS
Expand All @@ -804,8 +841,11 @@ void CClient::OnControllerInFaderLevel ( int iChannelIdx, int iValue )
emit ControllerInFaderLevel ( iChannelIdx, iValue );
}

void CClient::OnControllerInPanValue ( int iChannelIdx, int iValue )
void CClient::OnControllerInPanValue ( int iMIDIIdx, int iValue )
{
// map the MIDI index to the real channel number
const int iChannelIdx = MIDIToChan ( iMIDIIdx );

// in case of a headless client the panners cannot be moved so we need
// to send the controller information directly to the server
#ifdef HEADLESS
Expand All @@ -816,8 +856,11 @@ void CClient::OnControllerInPanValue ( int iChannelIdx, int iValue )
emit ControllerInPanValue ( iChannelIdx, iValue );
}

void CClient::OnControllerInFaderIsSolo ( int iChannelIdx, bool bIsSolo )
void CClient::OnControllerInFaderIsSolo ( int iMIDIIdx, bool bIsSolo )
{
// map the MIDI index to the real channel number
const int iChannelIdx = MIDIToChan ( iMIDIIdx );

// in case of a headless client the buttons are not displayed so we need
// to send the controller information directly to the server
#ifdef HEADLESS
Expand All @@ -827,8 +870,11 @@ void CClient::OnControllerInFaderIsSolo ( int iChannelIdx, bool bIsSolo )
emit ControllerInFaderIsSolo ( iChannelIdx, bIsSolo );
}

void CClient::OnControllerInFaderIsMute ( int iChannelIdx, bool bIsMute )
void CClient::OnControllerInFaderIsMute ( int iMIDIIdx, bool bIsMute )
{
// map the MIDI index to the real channel number
const int iChannelIdx = MIDIToChan ( iMIDIIdx );

// in case of a headless client the buttons are not displayed so we need
// to send the controller information directly to the server
#ifdef HEADLESS
Expand All @@ -851,6 +897,9 @@ void CClient::OnControllerInMuteMyself ( bool bMute )

void CClient::OnClientIDReceived ( int iChanID )
{
// remember our client ID received from the server
iMyChannelID = iChanID;

// for headless mode we support to mute our own signal in the personal mix
// (note that the check for headless is done in the main.cpp and must not
// be checked here)
Expand Down Expand Up @@ -903,6 +952,9 @@ void CClient::Stop()
// disconnects the connection anyway).
ConnLessProtocol.CreateCLDisconnection ( Channel.GetAddress() );

// forget our own channel ID
iMyChannelID = INVALID_INDEX;

// reset current signal level and LEDs
bJitterBufferOK = true;
SignalLevelMeter.Reset();
Expand Down
18 changes: 14 additions & 4 deletions src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ class CClient : public QObject

bool IsConnected() { return Channel.IsConnected(); }

int GetMyChannelID() { return iMyChannelID; }

EGUIDesign GetGUIDesign() const { return eGUIDesign; }
void SetGUIDesign ( const EGUIDesign eNGD ) { eGUIDesign = eNGD; }

Expand Down Expand Up @@ -272,6 +274,10 @@ class CClient : public QObject
Channel.GetBufErrorRates ( vecErrRates, dLimit, dMaxUpLimit );
}

// convert between MIDI index and real channel index
Copy link
Collaborator

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.

int ChanToMIDI ( const int iChanIdx );
int MIDIToChan ( const int iMIDIIdx );

// settings
CChannelCoreInfo ChannelInfo;
QString strClientName;
Expand All @@ -292,6 +298,9 @@ class CClient : public QObject
CChannel Channel;
CProtocol ConnLessProtocol;

// this client's channel ID sent by the server
int iMyChannelID; // must use int (not size_t) so INVALID_INDEX can be stored

// audio encoder/decoder
OpusCustomMode* Opus64Mode;
OpusCustomEncoder* Opus64EncoderMono;
Expand Down Expand Up @@ -398,10 +407,11 @@ protected slots:
void OnCLPingWithNumClientsReceived ( CHostAddress InetAddr, int iMs, int iNumClients );

void OnSndCrdReinitRequest ( int iSndCrdResetType );
void OnControllerInFaderLevel ( int iChannelIdx, int iValue );
void OnControllerInPanValue ( int iChannelIdx, int iValue );
void OnControllerInFaderIsSolo ( int iChannelIdx, bool bIsSolo );
void OnControllerInFaderIsMute ( int iChannelIdx, bool bIsMute );

void OnControllerInFaderLevel ( int iMIDIIdx, int iValue );
void OnControllerInPanValue ( int iMIDIIdx, int iValue );
void OnControllerInFaderIsSolo ( int iMIDIIdx, bool bIsSolo );
void OnControllerInFaderIsMute ( int iMIDIIdx, bool bIsMute );
void OnControllerInMuteMyself ( bool bMute );
void OnClientIDReceived ( int iChanID );
void OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo );
Expand Down
5 changes: 2 additions & 3 deletions src/clientdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
// MeterStyle init
SetMeterStyle ( pClient->GetMeterStyle() );

// set the settings pointer to the mixer board (must be done early)
// pass the client and settings pointers to the mixer board (must be done early)
MainMixerBoard->SetClientPointer ( pClient );
MainMixerBoard->SetSettingsPointer ( pSettings );
MainMixerBoard->SetNumMixerPanelRows ( pSettings->iNumMixerPanelRows );

Expand Down Expand Up @@ -487,8 +488,6 @@ CClientDlg::CClientDlg ( CClient* pNCliP,

QObject::connect ( pClient, &CClient::ChatTextReceived, this, &CClientDlg::OnChatTextReceived );

QObject::connect ( pClient, &CClient::ClientIDReceived, this, &CClientDlg::OnClientIDReceived );

QObject::connect ( pClient, &CClient::MuteStateHasChangedReceived, this, &CClientDlg::OnMuteStateHasChangedReceived );

QObject::connect ( pClient, &CClient::RecorderStateReceived, this, &CClientDlg::OnRecorderStateReceived );
Expand Down
2 changes: 0 additions & 2 deletions src/clientdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,6 @@ public slots:
ConnectDlg.SetConnClientsList ( InetAddr, vecChanInfo );
}

void OnClientIDReceived ( int iChanID ) { MainMixerBoard->SetMyChannelID ( iChanID ); }

void OnMuteStateHasChangedReceived ( int iChanID, bool bIsMuted ) { MainMixerBoard->SetRemoteFaderIsMute ( iChanID, bIsMuted ); }

void OnCLChannelLevelListReceived ( CHostAddress /* unused */, CVector<uint16_t> vecLevelList )
Expand Down
8 changes: 4 additions & 4 deletions src/sound/soundbase.h
View file
Open in desktop
Copy link
Collaborator

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.

Copy link
Member Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,9 @@ class CSoundBase : public QThread

signals:
void ReinitRequest ( int iSndCrdResetType );
void ControllerInFaderLevel ( int iChannelIdx, int iValue );
void ControllerInPanValue ( int iChannelIdx, int iValue );
void ControllerInFaderIsSolo ( int iChannelIdx, bool bIsSolo );
void ControllerInFaderIsMute ( int iChannelIdx, bool bIsMute );
void ControllerInFaderLevel ( int iMIDIIdx, int iValue );
Copy link
Collaborator

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...

Copy link
Member Author

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.

Copy link
Collaborator

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).

Copy link
Member Author

@softins softins Nov 8, 2024 •
edited
Loading

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.

void ControllerInPanValue ( int iMIDIIdx, int iValue );
void ControllerInFaderIsSolo ( int iMIDIIdx, bool bIsSolo );
void ControllerInFaderIsMute ( int iMIDIIdx, bool bIsMute );
void ControllerInMuteMyself ( bool bMute );
};
Loading