Re: PartCombine: Keep track of the state in the Part_combine_engraver (issue3334043)

[reinhold . kainhofer]

Reviewers: Valentin Villenave, carl.d.sorensen_gmail.com,


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#newcode69
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#newcode110
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#newcode133
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#newcode37
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#newcode38
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."

Description:
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.

Please review this at http://codereview.appspot.com/3334043/

Affected files:
  M lily/part-combine-engraver.cc
  M lily/part-combine-iterator.cc
  M scm/define-event-classes.scm
  M scm/define-music-types.scm