PartCombine: Keep track of the state in the Part_combine_engraver

PartCombine: Keep track of the state in the Part_combine_engraver

-) Extend iterator to emit events also for apart and together states
-) Add a clear-partcombine-event and broadcast it to unused voices
   to reset the partcombine state

This can later on be used to make the engraver print out the texts
depending on the previous state, and even for e.g. line breaks or so.

[Valentin Villenave, 2010-11-26 23:05:51]

Hi Reinhold,
I really like where you're going with this! As always, I'm not a coder (yet) so
this is just my naive point of view.

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-engraver.cc
File lily/part-combine-engraver.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:52: bool item_to_create_;
if it's really a grob, then why are you calling it "item"?

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:69: Stream_event *unisono_event_;
How about declaring these variables on the same line? (I'm not sure this style
is used in Lily's source code, though.)

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:110: void broadcast_new_state (Music_iterator
*new_active, Stream_event *new_state_event);
I'm not familiar with C++ coding style, but that looks like an awfully long
line.

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:133: mmrest_event_ = 0;
Since all these variables are used together a lot, another possibility would be
to use an array? (At least that's what I've been reading, but I'm not sure it's
worth considering :)

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:37: (ApartEvent
I find this name a bit hard to understand. Is ApartEvent some kind of a
voice-agnostic solo-event, that can either be a solo-one- or solo-two-event? (in
which case "SoloEvent" could be a better name?)

(Now that I'm thinking about it, in the future it could be interesting to have a
single solo-event kind, that could take a "thread-number" event-property such as
'one or 'two, which would then be passed to the iterator so that any event could
be forced to be regarded as a solo-one- or solo-two-event...
Or maybe even make the iterator able to take any arbitrary number of music
"threads", not just two... Ok, I'm dreaming here.)

[Carl, 2010-11-26 23:07:29]

Looks like a good approach to improve the part combiner to me.

Thanks,

Carl

[Reinhold, 2011-01-25 17:11:19]

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:69: Stream_event *unisono_event_;
On 2010/11/26 23:05:51, Valentin Villenave wrote:
> How about declaring these variables on the same line?

I don't think that's our coding style.

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:110: void broadcast_new_state (Music_iterator
*new_active, Stream_event *new_state_event);
On 2010/11/26 23:05:51, Valentin Villenave wrote:
> I'm not familiar with C++ coding style, but that looks like an awfully long
> line.

I can shorten the argument names, that would save a few characters...

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:133: mmrest_event_ = 0;
On 2010/11/26 23:05:51, Valentin Villenave wrote:
> Since all these variables are used together a lot, another possibility would
be
> to use an array?

I had thought about that, too. But there are only two places where we are
looping through them (here and in derived_mark, which uses an array already).
Everywhere else, just one of those is used.

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:37: (ApartEvent
On 2010/11/26 23:05:51, Valentin Villenave wrote:
> I find this name a bit hard to understand. Is ApartEvent some kind of a
> voice-agnostic solo-event, that can either be a solo-one- or solo-two-event?

No, it indicates that the two part-combined voices cannot be combined and should
be displayed apart (i.e. with voiceOne/voiceTwo).
Part-combining has five different states:
-) Solo 1
-) Solo 2
-) Unisono (both play instruments play identical notes) and Unisilence (both are
quiet)
-) Chords (The two voices share the same rhythm/articulations and are not
crossing, so they can be written as chords)
-) Apart (It's not possible to combine the voices, so they have to be displayed
as two separate voices)


> Or maybe even make the iterator able to take any arbitrary number of music
> "threads", not just two... Ok, I'm dreaming here.)

Yes, that would be nice, but not that easy...

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:38: . ((description . "Indicate the part-combine
status @q{apart}.")
BTW, I can reformulate the description to make its purpose clearer. E.g.
something like "Incidate that the part-combined voices cannot be combined and
have to be displayed as separate voices."

[Graham Percival (old account), 2011-01-26 19:23:47]

Nothing obviously wrong in a regtest comparison, and the code style looks fine
to me.

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:69: Stream_event *unisono_event_;
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > How about declaring these variables on the same line?
> 
> I don't think that's our coding style.

I agree; let's keep the variable declarations separate.  They can be grouped by
adding extra newlines if necessary (as they are already).

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:110: void broadcast_new_state (Music_iterator
*new_active, Stream_event *new_state_event);
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > I'm not familiar with C++ coding style, but that looks like an awfully long
> > line.
> 
> I can shorten the argument names, that would save a few characters...

I think it's clearer to read them as is.  I wouldn't call that a really long
line.  87 chars is getting a bit uncomfortable, but it's not terrible.  Other
places in our code go over by more.

If any change were to be made here, I'd suggest a newline before "Stream_event".

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:110: void broadcast_new_state (Music_iterator
*new_active, Stream_event *new_state_event);
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > I'm not familiar with C++ coding style, but that looks like an awfully long
> > line.
> 
> I can shorten the argument names, that would save a few characters...

I think it's clearer to read them as is.  I wouldn't call that a really long
line.  87 chars is getting a bit uncomfortable, but it's not terrible.  Other
places in our code go over by more.

If any change were to be made here, I'd suggest a newline before "Stream_event".

[Neil Puttock, 2011-02-09 23:26:40]

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-engraver.cc
File lily/part-combine-engraver.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:62: if (ev->get_property ("class") ==
ly_symbol2scm ("clear-partcombine-event"))
ev->in_event_class ("clear-partcombine-event")

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:65: item_to_create_ = false;
item_to_create_ currently reflects the existence of active_event_; will you be
adding further code where they're set separately?

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:308: kill_mmrest (CONTEXT_ONE);
This doesn't do anything, as far as I can tell; kill_mmrest () broadcasts a
zero-length MultiMeasureRestEvent, which is ignored by the
Multi_measure_rest_engraver.

I think you can junk all the kill_mmrest () calls.

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#ne...
lily/part-combine-iterator.cc:386: for (int i = 0; i < NUM_OUTLETS-1; i++)
NUM_OUTLETS - 1

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/3334043/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:39: (part-combine-status . apart)
How will you be using this property?  It seems to be duplicating the name of the
event.

doc in define-music-properties.scm

[Keith, 2011-02-10 08:44:51]

I know you haven't added any new functionality yet, but I checked that the part
combiner continues to work (as well as it ever did) with this patch.  The
behavior looks good to me.

[Graham Percival (old account), 2011-06-21 09:27:28]

It would be nice if Neil's concerns could be addressed and we could move forward
with this patch.

[Reinhold, 2011-06-21 09:51:11]

On 2011/06/21 09:27:28, Graham Percival wrote:
> It would be nice if Neil's concerns could be addressed and we could move
forward
> with this patch.

Actually, that patch by itself it not that useful as it might first look. Its
main purpose is to make it possible to print the a2/Solo I/Solo II texts after a
linebreak again. Unfortunately, I don't know how to create those part combine
texts after a line break, so pushing this patch right now is not that useful.

What makes the problem so complicated is that the texts after a line break
should NOT in general be printed at the begin of each system, but on the first
note of the new system. I have no idea how this can be achieved.