Re: Add support for setting MIDI balance, pan position, reverb and chorus levels (issue 14434043)

[k-ohara5a5a]

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#newcode71
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#newcode231
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#newcode200
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-properties.scm
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/14434043/diff/8001/scm/define-context-properties.scm#newcode435
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-properties.scm#newcode438
scm/define-context-properties.scm:438: @code{midiChannelMapping}).
Ranges address@hidden@w{-1} address@hidden,
I would skip the text in "(...)" because "MIDI channel associated with
the context" says it all.

https://codereview.appspot.com/14434043/