MIDI: intelligently combine overlapping notes.

MIDI: intelligently combine overlapping notes.

Fix issue 1647
A patch from Micheal Duggan

[md5i, 2011-05-11 13:41:04]

Small variable nit upon rereading this patch.  No change in semantics.

http://codereview.appspot.com/4520050/diff/1/flower/include/pqueue.hh
File flower/include/pqueue.hh (right):

http://codereview.appspot.com/4520050/diff/1/flower/include/pqueue.hh#newcode121
flower/include/pqueue.hh:121: vsize next = i / 2;
This should really be tgt / 2, for consistency.

[Keith, 2011-05-11 23:12:08]

This looks good overall.
I will test out my own suggestions, because if we can make a smaller change, it
will be easier to follow through this complicated stretch of history in the
MIDI-output code.

http://codereview.appspot.com/4520050/diff/1/flower/include/pqueue.hh
File flower/include/pqueue.hh (right):

http://codereview.appspot.com/4520050/diff/1/flower/include/pqueue.hh#newcode121
flower/include/pqueue.hh:121: vsize next = i / 2;
On 2011/05/11 13:41:04, md5i wrote:
> This should really be tgt / 2, for consistency.

Done

http://codereview.appspot.com/4520050/diff/9001/flower/include/pqueue.hh#newc...
flower/include/pqueue.hh:110: void del (vsize i)
Rather than change the 'flower' library to support deletion from the heap, I
suggest we continue to use the ignore_ flag on items we would delete.  
The formerly-used "lazy deletion" technique seems appropriate for the note-of
queue, because deletions are rare.

http://codereview.appspot.com/4520050/diff/9001/lily/midi-walker.cc
File lily/midi-walker.cc (right):

http://codereview.appspot.com/4520050/diff/9001/lily/midi-walker.cc#newcode94
lily/midi-walker.cc:94: if (now_ticks == stop_note_queue[i].val.first
The use of the .first field to cache the note-on-time was difficult for me to
understand.

I suggest following pointers as was done to find now_ticks:
int queued_ticks = 
 stop_note_queue[i].val.second->audio_->audio_column_->ticks ()

[md5i, 2011-05-12 00:01:08]

On 2011/05/11 23:12:08, Keith wrote:
>
http://codereview.appspot.com/4520050/diff/9001/flower/include/pqueue.hh#newc...
> flower/include/pqueue.hh:110: void del (vsize i)
> Rather than change the 'flower' library to support deletion from the heap, I
> suggest we continue to use the ignore_ flag on items we would delete.  
> The formerly-used "lazy deletion" technique seems appropriate for the note-of
> queue, because deletions are rare.

Makes sense to me. I don't recall if "_ignore" existed when I originally wrote
the patch, several years ago now.

> http://codereview.appspot.com/4520050/diff/9001/lily/midi-walker.cc
> File lily/midi-walker.cc (right):
> 
> http://codereview.appspot.com/4520050/diff/9001/lily/midi-walker.cc#newcode94
> lily/midi-walker.cc:94: if (now_ticks == stop_note_queue[i].val.first
> The use of the .first field to cache the note-on-time was difficult for me to
> understand.
> 
> I suggest following pointers as was done to find now_ticks:
> int queued_ticks = 
>  stop_note_queue[i].val.second->audio_->audio_column_->ticks ()

Good call, in my opinion.  I was never happy with the Pair.

[Keith, 2011-05-25 06:17:24]

I have been successfully testing this patch for
<http://code.google.com/p/lilypond/issues/detail?id=1647>
and want to push it within a couple of days.

Now is a good time to review (even though Michael Duggan wrote the core of this
patch seven years ago) because it gets us thinking about how to dispose of the
similar 
<http://code.google.com/p/lilypond/issues/detail?id=1648>

[Graham Percival (old account), 2011-05-25 10:32:55]

LGTM

http://codereview.appspot.com/4520050/diff/16001/input/regression/midi-unison...
File input/regression/midi-unisons.ly (right):

http://codereview.appspot.com/4520050/diff/16001/input/regression/midi-unison...
input/regression/midi-unisons.ly:6: @code{timidity -idvvv file.midi |grep Midi}
to see midi events."
I could kiss you for giving this exact command-line way to test midi events!

http://codereview.appspot.com/4520050/diff/16001/input/regression/midi-unison...
input/regression/midi-unisons.ly:18: \midi {}
technically these two lines should be indented with two spaces, but it doesn't
really matter in the regtests.

[Graham Percival (old account), 2011-05-28 19:13:45]

I did an extra regtest comparison, and it's fine.  Please push whenever's
convenient.