Re: Add Staff.midiCC context property, refactor handling of MIDI control changes (issue 284280043 by address@hidden)

[dak]

On 2016/01/16 18:08:08, ht wrote:
> Hi,

> This patch introduces an interface for adjusting the values of all
MIDI
> controllers responding to Control Change events from within LilyPond
input
> using context properties.  Instead of adding a new context property
for
> every controller, however, this change adds a generic Staff.midiCC
context
> property, which accepts a list of (control number, value) pairs, where
the
> control number and the value are integers between 0 and 127
(inclusive).
> Setting the context property will generate the corresponding "raw"
control
> change events in the MIDI output.

> In hindsight, the generic context property would have provided (from a
> purely technical standpoint) an interface sufficient for making
changes to
> all of the MIDI controls for whose adjustment I have previously added
> support using controller-specific context properties
(Staff.midiBalance,
> Staff.midiExpression, Staff.midiPanPosition, Staff.midiReverbLevel and
> Staff.midiChorusLevel).  I realize that the introduction of another
context
> property whose value syntax differs from that of the existing ones,
but
> whose behavior still overlaps with them, will reduce the overall
> consistency of the design, but in my opinion there are the following
> arguments for making this change:

> * The main single benefit of the new context property is that it
enables
>    making changes to the values of any MIDI controllers adjustable
using
>    MIDI control change events from within LilyPond input files without
>    polluting the LilyPond context property namespace.  (Even after this
>    change, there still remain MIDI controls whose values cannot be
modified
>    from within LilyPond input, such as Channel Pressure or Pitch Wheel
-
>    however, the current patch concerns only enhancing support for MIDI
CC
>    events.)

> * There are no changes added which would _require_ setting the context
>    property in any existing LilyPond files: just as with the other MIDI
>    context properties, users need to concern themselves with the new
>    property only if they need it.

> * This patch simplifies the implementation of handling the existing
MIDI
>    context properties by reducing their implementation to use the same
data
>    structures for outputting MIDI control changes.  Adding support for
even
>    new controller-specific context properties should be as easy (or
even
>    easier) as before because the definitions of the controller-specific
>    context properties are now located in a single file
>    (lily/midi-cc-announcer.cc) instead of being scattered over several
>    Audio_item and Midi_item subclasses.


> In my opinion, the alternative to adding a generic context property
for
> generating MIDI control changes - that is, adding new context
properties
> separately each controller - would only lead to a burden of having to
write
> a lot of new technical documentation (which would have more to do with
the
> MIDI standard, not LilyPond) because:

> - the names of MIDI controllers do not appear to be very strictly
defined,
>    so the context properties could not be named without documenting
also
>    their associated MIDI controller numbers (unless the numbers were
embedded
>    in the property names, of course - however, this option would
probably
>    make the names so generic that having separate context properties
for
>    every controller would bring little benefit over the Staff.midiCC
>    property);

> - on the other hand, due to the genericity of many of the names of
>    controllers (<http://www.midi.org/techspecs/midimessages.php>), and
the
>    freedom given by the standard as regards their function, choosing
too
>    specific names for the context properties would likely result in the
names
>    not matching the actual function of the corresponding controllers
well
>    across different MIDI devices; and

> - each context property would need a decision on how to represent its
value
>    in LilyPond input.  However, any value representation scheme that
does not
>    use "raw" MIDI values (integers between 0 and 127), could make it
harder
>    for advanced MIDI users who wish to use particular integer values of
their
>    own choosing for MIDI output: with custom schemes for specifying
values
>    for the context properties, having a particular integer value
outputted in
>    MIDI would not be possible without precise knowledge about how
LilyPond
>    handles the values of the context properties, in order to do reverse
>    calculations.  (I do not claim that custom value types would
necessarily
>    be bad for all controllers: for example, a context property which
accepts
>    Boolean values could be a natural choice for controller types that
only
>    have an "on" or "off" setting.)


> As this patch will not remove any existing functionality, but only
extends
> it, I see no reason to remove support for the controller-specific
context
> properties, even though the behavior of the new context property
overlaps
> with that of the existing ones.  The controller-specific properties
could be
> seen just as a convenience interface for adjusting the values of
"common"
> controllers, and even new controller-specific properties could be
defined
> later if necessary, whereas the generic Staff.midiCC property is
provided to
> allow making direct adjustments even to more rarely used (or MIDI
device
> specific) controllers if necessary.

> A few small additional notes about the design of the changes in the
patch:

> * The current implementation of the Staff.midiCC context property
always
>    requires a list as the context property value.  It would be possible
to
>    add support for accepting single (control number, value) pairs as
>    themselves, but I think this would require extending scm/c++.scm and
>    scm/lily.scm with an "integer-pair-or-integer-pair-list" predicate,
which
>    to me feels like too specific a predicate to warrant being added to
the
>    public definitions.  (I guess one cannot use lambda expressions as
>    context property predicates in scm/define-context-properties.scm?)

> * When updating the Changes document, I did not put the description of
the
>    new context property to the top of the file, but added it after the
>    description of the Staff.midiExpression context property - I think
it is
>    more logical to describe these context properties together.

I am not happy about the increasing abuse of context properties for not
actually setting context properties but rather sending Midi messages.
The previous interfaces at least actually set a queryable property.
This one takes a pair/value pair instead and only sets a property in the
Midi controller but not in the context and one cannot actually query
this property again.

I think that this is not a good idea.  Suggestions how we could rather
do this?  Possible mechanisms would include Midi-specific music events,
or getting the equivalent of override/revert for Midi (mobs then instead
of grobs).  Of course, if we had mobs, it would make sense to map a
_lot_ of the current Midi generation through them eventually because it
would make programming the Midi generation much more transparent.

https://codereview.appspot.com/284280043/