Add support for setting MIDI balance, pan position, reverb and chorus levels

Add support for setting MIDI balance, pan position, reverb and chorus levels


Allow overriding Translator::{connect_to,disconnect_from}_context


New data types for controlling balance, pan position, reverb and chorus levels

[ht, 2013-10-05 16:46:17]

Fix range checking for MIDI property values

[Keith, 2013-10-06 02:23:00]

Thanks, Heikki; this looks very good.
Panning will help people to proof-read complex scores by listening to the MIDI
output.  I added an item to the bug tracker to add some documentation when this
is done.

Did you consider adding the functionality to Staff_performer, rather than the
separate Midi_effect_performer ?  
They both control settings that apply to a MIDI channel.  If we move one
_performer to the Voice context and not the other, the results seem more
potentially confusing, than potentially useful.

https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc
File lily/midi-item.cc (right):

https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc#newcode367
lily/midi-item.cc:367: // Audio_control_function_value_change::Control.
Is it appropriate, then, to make this array a public static member of
Audio_control_function_value_change and still access it from here?
(I find the spaghetti of object-oriented obfuscation in the MIDI-output code to
be overwhelming, so I'm happy that you seem to find your way through it.)

https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc#newcode398
lily/midi-item.cc:398: 
Implementing fine as well as coarse seems to be more trouble than it is worth.

https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc
File lily/staff-performer.cc (right):

https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc#new...
lily/staff-performer.cc:71: Control_function_value_map
control_function_value_map_;
Better to move up a few lines, outside of the group of mappings indexed by voice
names, and to define directly without typedef so it can be compared with the
others.

https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc#new...
lily/staff-performer.cc:231: if (!ins.second && ai->value_ == value)
Why bother removing duplicate settings of midiPanPosition, etc. ?

The bookkeeping is complicated, and a duplicate setting would come from a second
explicit \set Staff.midiPanPosition, so the user might expect to see the
duplicate control-change-event.
Users often have applications that are surprising; someone might use the MIDI
output as input to another program that does not remember control settings
properly.

https://codereview.appspot.com/14434043/diff/8001/ly/performer-init.ly
File ly/performer-init.ly (right):

https://codereview.appspot.com/14434043/diff/8001/ly/performer-init.ly#newcod...
ly/performer-init.ly:200: instrumentName = #"bright acoustic"
Answering your earlier question on the bug-tracker, I would suggest simply
omitting this initialization.  
The current state with the erroneous name has no effect, and people have not
complained about a missing initial programChange.  If you fix it, and start
sending an initial programChange, someone might have a problem.

https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
scm/define-context-properties.scm:435: (midiChannelMapping ,symbol? "How to map
MIDI channels: per @code{instrument} (default), @code{staff} or @code{voice}.")
Oops.  I changed the default to be 'staff, so that it works with the default
arrangement of performers in the Staff-like contexts, but forgot to change this
line of documentation.  I'll fix it later if you don't.  I hope that didn't
cause confusion.

https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
scm/define-context-properties.scm:438: @code{midiChannelMapping}).  Ranges
from@tie{}@w{-1} to@tie{}1,
I would skip the text in "(...)" because "MIDI channel associated with the
context" says it all.

[ht, 2013-10-06 18:58:33]

On 2013/10/06 02:23:00, Keith wrote:
> Thanks, Heikki; this looks very good.
> Panning will help people to proof-read complex scores by listening to the MIDI
> output.  I added an item to the bug tracker to add some documentation when
this
> is done.

Thanks. If there's any additional information or help needed from me for
writing documentation for the new properties, just ask.

> Did you consider adding the functionality to Staff_performer, rather than the
> separate Midi_effect_performer ?  
> They both control settings that apply to a MIDI channel.  If we move one
> _performer to the Voice context and not the other, the results seem more
> potentially confusing, than potentially useful.

As a matter of fact, not introducing a new performer is exactly what I
would've also preferred myself (not really considering the scenario of moving
performers to different contexts, but just for keeping all related changes
better localized, since the only purpose of the new performer is to announce
audio items to Staff_performer which does the "real" work of adding the
items to Audio_staff objects).

Initially, I even tried to make Staff_performer listen to the SetProperty
events directly, but failed because I couldn't then get the performer to
acknowledge the new audio elements that it had announced itself.  (The
goal was to have the new events acknowledged just like notes and
dynamics in Staff_performer::acknowledge_audio_element, which already
handles resolving the MIDI channel associated with an audio element, and
adding the element's audio item to an Audio_staff, which is what I needed
to have done also to the new audio items).

Tracking through the code to see how the
acknowledge_audio_element function gets called, I got the impression
that a performer will (probably for a valid reason) explicitly _skip_
acknowledging any audio elements announced by itself.  Since
announcing the events in a new performer worked, I settled for this
solution without giving any alternatives much further thought.  I admit
that I have only a very vague understanding about contexts and the
way the translators' callback functions get called in general – if
there's a better way to have the new events added to Audio_staff
objects without using a new performer, I'm happy to improve the patch,
but I'll probably need some advice on how to do this.  Also, there is
probably no good reason why the new performer needs to be added
specifically to the various Staff (instead of Voice) contexts in
ly/performer-init.ly – this is again just a first solution that happened to
work (and I wasn't really thinking about moving the performers
between contexts).
 
> https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc
> File lily/midi-item.cc (right):
> 
> https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc#newcode367
> lily/midi-item.cc:367: // Audio_control_function_value_change::Control.
> Is it appropriate, then, to make this array a public static member of
> Audio_control_function_value_change and still access it from here?

The note about the ordering of the array elements here was only
intended as a reminder when adding new control functions (this will
in general require extending all of
Audio_control_function_value_change::Control,
Audio_control_function_value_change::context_properties_, and
Midi_control_function_value_change.:control_functions_). Requiring
a particular order for the elements in
Midi_control_function_value_change::control_functions_ was just to
make it slightly easier to look up elements of the array without
searching.

The information in the
Midi_control_function_value_change::control_functions_ array could
easily be attached directly to the
Audio_control_function_value_change::context_properties_ array, but
then this array would mix "abstract" audio item information with
MIDI related information, and so far I've seen these two types of
information usually kept separate (in the various Audio_items and
the corresponding Midi_items).

The Midi_control_function_value_change::control_functions_ array
could probably well be made local to the
Midi_control_function_value_change::to_string function (there's no
real reason why it needs to be declared on the class level as it's not
used elsewhere), but the
Audio_control_function_value_change::context_properties_ array
still needs to be public because it's referenced also from
Staff_performer and Midi_effect_performer.

> https://codereview.appspot.com/14434043/diff/8001/lily/midi-item.cc#newcode398
> lily/midi-item.cc:398: 
> Implementing fine as well as coarse seems to be more trouble than it is worth.

Well, fine resolution was one of the things that were considered
missing from the original patchset when I posted it on the mailing list.
I can certainly remove the support if it's not really needed.

> https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc
> File lily/staff-performer.cc (right):
> 
>
https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc#new...
> lily/staff-performer.cc:71: Control_function_value_map
> control_function_value_map_;
> Better to move up a few lines, outside of the group of mappings indexed by
voice
> names, and to define directly without typedef so it can be compared with the
> others.

Ok.

>
https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc#new...
> lily/staff-performer.cc:231: if (!ins.second && ai->value_ == value)
> Why bother removing duplicate settings of midiPanPosition, etc. ?
> 
> The bookkeeping is complicated, and a duplicate setting would come from a
second
> explicit \set Staff.midiPanPosition, so the user might expect to see the
> duplicate control-change-event.
> Users often have applications that are surprising; someone might use the MIDI
> output as input to another program that does not remember control settings
> properly.

You're right in that keeping track of the control function values for each
MIDI channel makes the code more complicated just for purpose of
optimizing away duplicate "set property" events on the same channel.

The only reason for the extra bookkeeping was to keep consistent with
the way that (I think) MIDI instrument and tempo changes have been
handled before: to my understanding, a change in the MIDI instrument or
tempo is also checked against the value that is currently in effect, and
the change is ignored if it isn't a "real" one (in Tempo_performer the logic
seems clearer, I may well be mistaken with the MIDI instrument changes
in Staff_performer).

Should I nevertheless remove the extra bookkeeping even though the
behaviour will then differ from how the other MIDI related properties
are handled?
 
> https://codereview.appspot.com/14434043/diff/8001/ly/performer-init.ly
> File ly/performer-init.ly (right):
> 
>
https://codereview.appspot.com/14434043/diff/8001/ly/performer-init.ly#newcod...
> ly/performer-init.ly:200: instrumentName = #"bright acoustic"
> Answering your earlier question on the bug-tracker, I would suggest simply
> omitting this initialization.  
> The current state with the erroneous name has no effect, and people have not
> complained about a missing initial programChange.  If you fix it, and start
> sending an initial programChange, someone might have a problem.

Ok.

>
https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
> File scm/define-context-properties.scm (right):
> 
>
https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
> scm/define-context-properties.scm:435: (midiChannelMapping ,symbol? "How to
map
> MIDI channels: per @code{instrument} (default), @code{staff} or
@code{voice}.")
> Oops.  I changed the default to be 'staff, so that it works with the default
> arrangement of performers in the Staff-like contexts, but forgot to change
this
> line of documentation.  I'll fix it later if you don't.  I hope that didn't
> cause confusion.

Not really, I noticed the inconsistency before I got to testing my
changes.

>
https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properti...
> scm/define-context-properties.scm:438: @code{midiChannelMapping}).  Ranges
> from@tie{}@w{-1} to@tie{}1,
> I would skip the text in "(...)" because "MIDI channel associated with the
> context" says it all.

Ok, I'll remove the explicit references to midiChannelMapping here.

Thanks for the review, I'll try to send a new patchset in the next few
days.

[Keith, 2013-10-06 22:13:10]

On 2013/10/06 18:58:33, ht.lilypond.development wrote:
> 
> Initially, I even tried to make Staff_performer listen to the SetProperty
> events directly, but failed because I couldn't then get the performer to
> acknowledge the new audio elements that it had announced itself. 

I see.
What I was expecting, was that midiPanPosition could go through the
Staff_performer just like midiInstrument does, but now I see that
Staff_performer::new_instrument_string() repeatedly polls to check
midiInstrument, rather than listening for an announced change.

The only concern I had with the separate Midi_effect_performer was the extra
complexity.

>
https://codereview.appspot.com/14434043/diff/8001/lily/staff-performer.cc#new...
> > lily/staff-performer.cc:231: if (!ins.second && ai->value_ == value)
> > Why bother removing duplicate settings of midiPanPosition, etc. ?
> > 
> > The bookkeeping is complicated, and a duplicate setting would come from a
> second
> > explicit \set Staff.midiPanPosition, so the user might expect to see the
> > duplicate control-change-event.

> The only reason for the extra bookkeeping was to keep consistent with
> the way that (I think) MIDI instrument and tempo changes have been
> handled before: to my understanding, a change in the MIDI instrument or
> tempo is also checked against the value that is currently in effect, and
> the change is ignored if it isn't a "real" one (in Tempo_performer the logic
> seems clearer, I may well be mistaken with the MIDI instrument changes
> in Staff_performer).
>

It is true that duplicate changes to tempo and midiInstrument are suppressed. 
Other changes are repeated even if they repeat the same value, such as dynamics
{c4\p c c c c\p c c c} where both 'p's are printed and generate events to set
the volume (which is redundant now that we use note-on velocity).  The new
features to set midiPanPosition, etc., are more analogous to setting a volume
level. 

The bookkeeping code is done and looks correct, so you can keep it or simplify,
whichever you think wisest.

[ht, 2013-10-09 04:36:46]

Simplify MIDI control function handling as per the review suggestions

[ht, 2013-10-09 04:45:39]

Fix a small typo