Conversation
midiplayer.cpp -> volumechanged event is treated separately in order to include the maxVolume threshold midievent.cpp -> added a check isVolumeChange()
cameronwhite
left a comment
cameronwhite
left a comment
There was a problem hiding this comment.
This looks good, thanks for working on this!
Just a couple minor things noted in the comments, and a possible alternative to getPlayerFromChannel() that I'm unsure about.
| return myIsPlaying; | ||
| } | ||
|
|
||
| int MidiPlayer::getPlayerFromChannel(const int channel) |
There was a problem hiding this comment.
An alternative to this function would be to record the player number on the MidiEvent, which avoids having to duplicate the logic for how players are assigned to channels. (There's actually an unused myPlayer member variable in MidiEvent that was intended for this.purpose, but the myInstrument member should probably be removed).
What are your thoughts on that?
If we stick with the current approach, those unused members should be removed, and there's also a compiler warning (below) where the compiler incorrectly thinks the result could be uninitialized:
../source/audio/midiplayer.cpp:314:14: warning: variable 'player' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
else if (channel > METRONOME_CHANNEL)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
../source/audio/midiplayer.cpp:319:12: note: uninitialized use occurs here
return player;
^~~~~~
../source/audio/midiplayer.cpp:314:10: note: remove the 'if' if its condition is always true
else if (channel > METRONOME_CHANNEL)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../source/audio/midiplayer.cpp:303:15: note: initialize the variable 'player' to silence this warning
int player;
^
= 0
1 warning generated.
| // handle volume change events | ||
| // using device.setVolume() ensures that the maximum volume threshold | ||
| // is taken into consideration | ||
| if(event -> isVolumeChange()) |
There was a problem hiding this comment.
The formatting is a bit inconsistent here - should be if (event->isVolumeChange())
| // is taken into consideration | ||
| if(event -> isVolumeChange()) | ||
| { | ||
| device.setVolume(channel, event->getData()[2]); |
There was a problem hiding this comment.
Could you wrap the event->getData()[2] in a function similar to MidiEvent::getTempo()? It's nice to encapsulate the MIDI details in that class.
2 participants
Description of Change(s)
The changes fixes the issue that each individual player volume in the mixer cannot be set/has no effect. The change involves handling volume change midi events separate from other event, as well as a small modification in the midievent class
Fixes Issue(s)
#276