[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Add support for setting MIDI balance, pan position, reverb and choru
|
From: |
k-ohara5a5a |
|
Subject: |
Re: Add support for setting MIDI balance, pan position, reverb and chorus levels (issue 14434043) |
|
Date: |
Sun, 06 Oct 2013 22:13:10 +0000 |
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#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.
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.
https://codereview.appspot.com/14434043/