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

[ht . lilypond . development]

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#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.

Ok.


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.

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#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.

Ok.


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.

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


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.

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.


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