update all scene launch buttons

[georgkrause]

I filed this before, but now its clean and i will describe the exact behavior to be sure there is no error in thinking. This adresses #196

The old behavior: scene launch is triggered Loop over all specified midi bindings if the current binding is a scene launch loop from 0 to 5 if i is not the scene which is activated send note off to 5 adresses after the first found scene launch binding

This has the problem that it only works for 5 scenes and only if the midi adresses of the used buttons are following each other, eg 0,1,2,3,4. If the adresses are in an other order, it wont work since the function returns after the first found scene launch binding.

The new behavior is: scene launch is triggered Loop over all specified midi bindings if the current binding is a scene launch if it is the triggered scene launch turn it on else turn it off

in my opinion this is much easier, more flexible and allows more than 5 scene launch buttons on any controller. I use this code since over a year now and didnt had any problems. Would be happy if this could be merged into Luppp.

[harryhaaren]

Hi Georg, I remember testing this change with the APC40 and it didn't work as expected. I had a quick look for the previous discussion / issue on the tracker but didn't find it - do you remember the name that you reported it with? I'd like to look back through the previous discussion

[georgkrause]

144

Can you tell me what does not worked as expected?

[harryhaaren]

If a particular binding file doesn't have all scenes mapped, this code change breaks that controller map. Previously all scenes would be written - but that inner for-loop has been removed. Assuming the .ctlr map does have all scenes mapped, then the new approach is valid.

[georgkrause]

well, only in one case are all scenes written: when the adresses have an offset of 1: 0,1,2,3,4,5

On the other side, it makes no sense from my point of view to update buttons which are not mapped. And now imagine one button has a midi note number which is directly after one of a scene launch button (eg scene launch: 56 and other button: 57). In this case the other button, which might be mapped to any other function will turn off because of this inner loop. This is simply wrong.

[harryhaaren]

Agreed that the current behaviour isn't ideal - I'm aware of that - but breaking it in other ways isn't an improvement: so lets get this right. I'm OK with requiring all feedback items to be mapped - we can add an item in the release notes saying "beaware that the mapping semantics have changed".

[georgkrause]

I dont see why removing the behavior of updating unmapped buttons is "braking it in other ways". Please explain this to me. In fact, Luppp is doing crazy stuff at the moment. Please try it, map only one scene launch buttons because you want use only one and what happens is simply wrong. it is not breaking to remove this.

[georgkrause]

In general, if we wont get to an common point, it might be an good idea to ask @geraldmwangi for his opinion.

[harryhaaren]

Looks a lot better now - I'll test with a MIDI controller later.

[georgkrause]

@harryhaaren any news on this?

[coderkun]

Hey @harryhaaren and @georgkrause, I don’t know about the history of this PR but the lastest code works fine for my AKAI APC mini and fixes the lights for scene buttons 6 – 8. The logic of the code (method GenericMIDI::launchScene()) also looks fine to me.

I would fix line 615 causing a warning though (comparison of signed and unsigned int).

[georgkrause]

We could fix this by making the the i in the for loop a signed one. much better would be to change "scene" to unsigned, but since this needs to be done on quite a lot places in the project this should be another PR. If this gets merged, i will create an issue (wanted to look at the warnings anyway)

[harryhaaren]

Ok - found the root cause of this issue/bug that was causing certain controllers to mis-behave. Fixing in a new commit, and mergeing...