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.

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

Fix for #242 program changes handling, program number as parameter #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chabanenk0
Copy link

@chabanenk0 chabanenk0 commented Mar 26, 2017

Hello! Could you please review my PR. It fixed the bug #242 for me, because early fix didn't work for me.

I need to change my loop when I change program number (sound) on my midi keyboard. I need to take the program number into consideration, for example for sound 1 (grand piano) I want to switch to loop number 3 etc. It needs to save setting for all program numbers, like note numbers or controller changes.

@jdekozak
Copy link
Contributor

jdekozak commented Apr 1, 2017

Hi,

Quote from #242 :

But it will only solve half of your problem, you won't be able to toggle the start/stop with PROGRAM_CHANGE 8, Hydrogen has no built-in "mapping by value" feature. Here I won't say this is a feature for Hydrogen, you can use qmidiroute as you did, or mididings as I do with my Korg ES-1.

Have you tried a midi router as described above ?

Regards,

@chabanenk0
Copy link
Author

chabanenk0 commented Apr 2, 2017

Hello, @jdekozak !
I've tried to check if I can start-stop Hydrogen with program change and it worked ok. Please see the screenshot:

hydrogen_config

Hope I understand your question correctly. I was changing the program on my midi keyboard and Hydrogen was successfully changed patterns, and stopped, started playing.

I didn't checked the mms_play or mms_stop events, I don't use them and I don't have it on my keyboard, it's easier to start playing by pressing space key on Hydrogen. And I think my patch should not affect it. If you consider it can cause any issues, please tell the details.
Could you please explain what do you mean by telling "Hydrogen has no built-in "mapping by value" feature." ?

P.S. yes, I'm using qmidiroute for other purposes. I need a useful event type to switch my hydrogen loops. Program changes are the most useful for me. Of course I could create one more additional output channel and transform my program changes to some note or controller events and connect it to Hydrogen for loops changing, but it is more useful to send the same midi events to the sampler and to the Hydrogen.

@jdekozak
Copy link
Contributor

jdekozak commented Apr 8, 2017

Hi,

First of all, a short disclaimer, I am not a maintainer of Hydrogen, I am giving my opinion on this topic because I have changed a little bit Hydrogen midi code in the past. And I was involved in #242 Also, I am not saying you are wrong doing this change, as always, if there is a need, it reveals a missing piece of software somewhere...

That being said, let me explain deeper "mapping by value". I will compare the CONTROL_CHANGE (aka CC) and the PROGRAM_CHANGE (aka PC). According to MIDI specs, CC can be many while PC is unique, and both events can carry a value, eg. :
CC X VALUE Y
PC _ VALUE Y

You have coded a "mapping by value" for PC event in Hydrogen. It is visible in midi_map.h file, you have put side by side
MidiAction* __cc_array[ 128 ];
MidiAction* __pc_array[ 128 ];
But there is a difference between those arrays, CC is indexed by X while PC is indexed by Y.
This is first thing that makes me think Hydrogen is not ready to hold a mapping by value feature, and your need must be outsourced.

Next in your pull request, in midi_input.cpp
MidiAction *pAction = mM->getPCAction( programNumber );
pAction->setParameter2( QString::number( programNumber ) );
The program_number is used in 2 different lines of code but with a different semantic, first is an index, second is a value, you have coded a "mapping by value" but still carry the value. Repeating things in code often reveals that something is not clean.
This is second thing that makes me think Hydrogen is not ready to hold a mapping by value feature, and your need must be outsourced.

Hope things are clearer now.

Regards

@chabanenk0 chabanenk0 force-pushed the bugfix-242-program-change-number-fix branch 2 times, most recently from 9f0b8b2 to 49d0eaf Compare June 11, 2017 14:56
@mauser
Copy link
Member

mauser commented Jun 28, 2017

Hey everyone!

I suppose the most confusion comes from the fact that you're using the parameter of the program change message as the second parameter to the midi action and at the same time in other places as the first parameter (via lastMidiEventParameter). I haven't tried the code yet nor looked into this deeper, but this design looks a litttle bit weird to me..

PS: Will have a closer look soon, sorry for long delay!

@chabanenk0 chabanenk0 force-pushed the bugfix-242-program-change-number-fix branch from 49d0eaf to f48a397 Compare October 13, 2017 16:28
@jeremyz jeremyz force-pushed the master branch 2 times, most recently from 42293e2 to b26ebd8 Compare July 18, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants