Issue 4622: create a <Part-combiner> class

<Part-combiner> collects in one place the options for routing parts to Voice contexts and generating marks.
    
Scheme functions combine-two-parts-center, combine-two-parts-up, and combine-two-parts-down form the core of \partcombine, \partcombineUp, and \partCombineDown.  The scheme functions allow customization with a <Part-combiner> other than the default.

[dak, 2015-09-26 07:42:37]

https://codereview.appspot.com/265260043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (left):

https://codereview.appspot.com/265260043/diff/1/ly/music-functions-init.ly#ol...
ly/music-functions-init.ly:1265: #(define-music-function (chord-range part1
part2)
No, just no.  Putting some new Scheme type without any LilyPond representation
into a user command interface is not the way to go.  There is nothing here that
you could not express using a context mod.

[Dan Eble, 2015-09-26 13:39:55]

restore \partcombine interface

[dak, 2015-09-30 11:23:11]

On 2015/09/26 13:39:55, Dan Eble wrote:
> restore \partcombine interface

What is the problem you are trying to solve here?  I've just resurrected issue
4131 since it's now (as of issue 4609) possible to tell \set and \once \set
apart reliably and thus work with context properties.  Context mods are a good
representation for starting configurations, and using \set/\unset also allows
changing things like the chord range on the fly.

I don't see where you want to be going with this patch.  What is it supposed to
make easier for the programmer or the user?

[Dan Eble, 2015-09-30 12:18:07]

On 2015/09/30 11:23:11, dak wrote:
>
> I don't see where you want to be going with this patch.  What is it supposed
to
> make easier for the programmer or the user?

Well, for the typical user, nothing.  For people like me, it reduces the amount
of internal Lilypond code that must be duplicated to combine parts with
non-default settings (e.g. without solo passages).  With master, I need to
duplicate make-directed-part-combine-music.  After this patch, I will be able to
create my own <Part-combiner> and say just this:

dfeVoiceCombine =
#(define-music-function (part1 part2)
   (ly:music? ly:music?)
   (combine-two-parts-center dfe-voice-part-combiner part1 part2))

Also the internal code is better organized (IMO).  The process (parts -> event
lists -> split list ...) is more obvious.  The dependence on specific context
names has been pushed outward a bit.

[dak, 2015-09-30 12:58:38]

On 2015/09/30 12:18:07, Dan Eble wrote:
> On 2015/09/30 11:23:11, dak wrote:
> >
> > I don't see where you want to be going with this patch.  What is it supposed
> to
> > make easier for the programmer or the user?
> 
> Well, for the typical user, nothing.  For people like me, it reduces the
amount
> of internal Lilypond code that must be duplicated to combine parts with
> non-default settings (e.g. without solo passages).  With master, I need to
> duplicate make-directed-part-combine-music.  After this patch, I will be able
to
> create my own <Part-combiner> and say just this:
> 
> dfeVoiceCombine =
> #(define-music-function (part1 part2)
>    (ly:music? ly:music?)
>    (combine-two-parts-center dfe-voice-part-combiner part1 part2))
> 
> Also the internal code is better organized (IMO).  The process (parts -> event
> lists -> split list ...) is more obvious.  The dependence on specific context
> names has been pushed outward a bit.

Well, my problem with non-default settings like "without solo passages" is the
standard one: changing such settings may well be desirable during part combiner
operation, so it seems like making any such settings actual context properties
would be the more flexible path.  And instead of creating your own
<Part-combiner> you'll be able to create your own context mod with a particular
set of initial settings.

Now make no mistake: the internals of the part combiner and its state management
are rather messy and non-modular.  If it would be two additional lines to have
another property like partCombineForced tracked and extracted, that would make
things nicer.  But that concerns more how the part combiner is organized and
controlled internally.  I don't think that externally we have much to gain from
having it fall apart into different implementations rather than using different
parameters for driving a single partcombiner.

[Dan Eble, 2015-09-30 20:50:55]

On 2015/09/30 12:58:38, dak wrote:
> I don't think that externally we have much to gain from
> having it fall apart into different implementations rather than using
different
> parameters for driving a single partcombiner.

I'm not sure I understand you clearly.  Which one of these describes this patch,
and are you opposing these changes or just stating observations?

[dak, 2015-10-01 08:33:11]

On 2015/09/30 20:50:55, Dan Eble wrote:
> On 2015/09/30 12:58:38, dak wrote:
> > I don't think that externally we have much to gain from
> > having it fall apart into different implementations rather than using
> different
> > parameters for driving a single partcombiner.
> 
> I'm not sure I understand you clearly.  Which one of these describes this
patch,
> and are you opposing these changes or just stating observations?

I have a bad feeling about these changes because they look like they are
painting us into a corner, making it harder to add generally accessible
functionality in the style of \partcombineForce, both with regard to adding them
into the user interface as well as extending the code.  Let's assume that we
make chord extent changeable on the fly via setting a context property.  How
would you implement that using your part combiner structure?

[Dan Eble, 2015-10-01 22:07:28]

On 2015/10/01 08:33:11, dak wrote:
> into the user interface as well as extending the code.  Let's assume that we
> make chord extent changeable on the fly via setting a context property.  How
> would you implement that using your part combiner structure?

Then I would remove it from the structure.

For the force commands, it would be good to survey where they have been used so
far.
Some of them are obsolete now that the chord range is configurable.
Some of them could be blamed on bugs that should be fixed.
Some of them would be unnecessary with different state machines.
Some of them might be easily replaced with other work-arounds like manual beams
or invisible phrasing slurs.

I don't mean to say that there shouldn't be a method of working around unwanted
decisions, but that the number of situations in which they are necessary
probably does not justify a high level of elegance.  Really, I couldn't care
less how the force commands are implemented.

But I am skeptical about converting the chord range to a context property
because from what I have seen so far, it seems to involve having the
property-set commands within the parts being combined, and that doesn't make
sense.  Configuring the part combiner's decisions from within the parts breaks
down cases where a part might be used in different ways in multiple situations:

  % flute.ly
  \include "hymn.ly"
  ... \partcombine \transpose c c' \tenor \soprano

  % violin.ly
   \include "hymn.ly"
   ... \partcombine \soprano \alto ...

  % viola.ly
  \include "hymn.ly"
  ... \partcombine \alto \tenor ...

I have collections that do that kind of thing.

Putting all that aside, what I really want in the near term is to reduce the
amount of Lilypond code I have duplicated with my compositions.  Without
defining a <Part-combiner> class, I could still do that by passing the chord
range and state machines as individual arguments.  <Part-combiner> was a nice
way to help me organize things on the way there, but I could still achieve this
goal without it.  Would you feel any better about that?  Or do you think that it
is proper for me to have to make my own copy of make-directed-part-combine-music
in order to use the part combiner with different state machines?

Alternatively, if exposing the state machines as options to public scheme
methods is itself objectionable, what would be the reaction to my submitting a
patch with a new command that does exactly what I want (no a2/solo passages) out
of the box?

[dak, 2015-10-02 12:43:39]

On 2015/10/01 22:07:28, Dan Eble wrote:
> On 2015/10/01 08:33:11, dak wrote:
> > into the user interface as well as extending the code.  Let's assume that we
> > make chord extent changeable on the fly via setting a context property.  How
> > would you implement that using your part combiner structure?
> 
> Then I would remove it from the structure.

I think that's the wrong way to do things.  I'm open to using a GUILE structure
for keeping the governing state of the partcombiner in.  But then it should not
be restricted to the initial state but should reflect the current state.  One
can have a separate copy of the state to reflect "\once" settings.  And there
likely should be a function that initializes the current state from current
context properties: that way one can make any parameters to the part combiner a
context mod, and then one just initializes the voices from the recording group
emulator with that context mod and generates the GUILE structure from the
resulting state of the context.

That way you can just store specific settings in context mods and pass them to
the part combiner in that manner.  How the part combiner maintains its internal
state is then its own business.

> For the force commands, it would be good to survey where they have been used
so
> far.
> Some of them are obsolete now that the chord range is configurable.
> Some of them could be blamed on bugs that should be fixed.
> Some of them would be unnecessary with different state machines.
> Some of them might be easily replaced with other work-arounds like manual
beams
> or invisible phrasing slurs.

I don't see that the part combiner will ever get along satisfactorily without
getting additional, mostly localized hints, and predicting the form all of them
may take seems audacious.

> I don't mean to say that there shouldn't be a method of working around
unwanted
> decisions, but that the number of situations in which they are necessary
> probably does not justify a high level of elegance.  Really, I couldn't care
> less how the force commands are implemented.

Sure, but having to rewrite your internals whenever one item should become
configurable on the fly seems just like shortsighted design.  I don't see that
it should significantly complicate matters to keep current state rather than
just initial state in your structure.

> But I am skeptical about converting the chord range to a context property
> because from what I have seen so far, it seems to involve having the
> property-set commands within the parts being combined, and that doesn't make
> sense.  Configuring the part combiner's decisions from within the parts breaks
> down cases where a part might be used in different ways in multiple
situations:
> 
>   % flute.ly
>   \include "hymn.ly"
>   ... \partcombine \transpose c c' \tenor \soprano
> 
>   % violin.ly
>    \include "hymn.ly"
>    ... \partcombine \soprano \alto ...
> 
>   % viola.ly
>   \include "hymn.ly"
>   ... \partcombine \alto \tenor ...
> 
> I have collections that do that kind of thing.

So?  We have \tag for doing that sort of thing conditionally, and of course you
can just put the whole timed information for one partcombining pass into a
separate variable and add it to one particular use of the music by using << ...
>>.

Once we are dealing with standard LilyPond entities, we have the tools for
working with those entities.

> Putting all that aside, what I really want in the near term is to reduce the
> amount of Lilypond code I have duplicated with my compositions.  Without
> defining a <Part-combiner> class, I could still do that by passing the chord
> range and state machines as individual arguments.  <Part-combiner> was a nice
> way to help me organize things on the way there, but I could still achieve
this
> goal without it.  Would you feel any better about that?  Or do you think that
it
> is proper for me to have to make my own copy of
make-directed-part-combine-music
> in order to use the part combiner with different state machines?

How the Scheme functions are fed is more or less an implementation detail.  I'd
try to keep any interfaces that have been along for a reasonably long time, but
refactoring behind that sort-of-established interface can of course be done in
manners that are nicer to juggle with.  One should just make sure that the
factoring is reasonably straightforward, for example having a single function
that can convert a context into such a GUILE structure by looking at the
respective context properties.

> Alternatively, if exposing the state machines as options to public scheme
> methods is itself objectionable, what would be the reaction to my submitting a
> patch with a new command that does exactly what I want (no a2/solo passages)
out
> of the box?

I don't want to have too many different commands: we already have too many of
them.  Customization of the behavior should mostly be possible using a context
mod.  The partcombiner is less cluttered than the cue commands currently I
think: it's ridiculous that up/down and transposition and clef all have separate
commands with different names: all of that can perfectly well be stuffed into a
single context mod when required.  That way one can be reasonably sure that one
did not overlook something to configure, and further extensions are painless and
don't require thinking about just what permutation of variations one wants to
creep into the command names.

[Dan Eble, 2015-10-03 12:42:19]

I've marked the ticket as "needs work" for now.  I still have questions.  It
*would* be nice to be able to say

   \override Staff.partCombineParameter = #x
   \partcombine \one \two ...

and have the part combiner get its initial settings from the context.  The
current implementation runs the combination algorithm as the part-combine music
is constructed.  But there is a way to defer evaluating the elements of music
until they are requested, isn't there?  Would it be safe to run the combination
algorithm from a callback that is not called until the part-combine music is
able to look up its initial parameters in context?  I'm not very clear on the
sequencing of things; I would worry that the elements might be needed earlier
than that, or that there might be a lot of recalculating, etc.

(I appreciate the time you're putting into this discussion.)

[dak, 2015-10-03 12:56:50]

On 2015/10/03 12:42:19, Dan Eble wrote:
> I've marked the ticket as "needs work" for now.  I still have questions.  It
> *would* be nice to be able to say
> 
>    \override Staff.partCombineParameter = #x
>    \partcombine \one \two ...
> 
> and have the part combiner get its initial settings from the context.

You bet.

> The
> current implementation runs the combination algorithm as the part-combine
music
> is constructed.  But there is a way to defer evaluating the elements of music
> until they are requested, isn't there?

elements-callback, but that's mostly for sequential-music.  But it is mostly up
to the iterator how it does things.  The partcombine-iterator currently combines
a split-list with the running stuff to get things.  The disadvantage of
on-the-fly operation might be that creating the split-list might be expensive so
reusing the not-yet-partcombined music might get slower.

> Would it be safe to run the combination
> algorithm from a callback that is not called until the part-combine music is
> able to look up its initial parameters in context?

I think so.  For better or worse, the split list generator runs in a sandbox
independent from the original contexts, so if you want to see the part combining
properties in their original, you'd have to pick them off and transfer them
over.  However, the stream events generated for set, unset, and once set, once
unset, no longer contain any reference to actual context data so it does not
really matter all that much whether the "real" context properties are out of
whack with the "virtual" ones for the purpose of the split list generator.

>I'm not very clear on the
> sequencing of things; I would worry that the elements might be needed earlier
> than that, or that there might be a lot of recalculating, etc.

I think the recalculation is the principal worry but it should be quite faster
than any actual typesetting of the resulting material.

[Dan Eble, 2015-10-04 13:53:05]

On 2015/10/03 12:56:50, dak wrote:
> On 2015/10/03 12:42:19, Dan Eble wrote:
> > I've marked the ticket as "needs work" for now.  I still have questions.  It
> > *would* be nice to be able to say
> > 
> >    \override Staff.partCombineParameter = #x
> >    \partcombine \one \two ...
> > 
> > and have the part combiner get its initial settings from the context.
> 
> You bet.
> 

Oh, or even better....

    \override Staff.partCombineParameter = #x
    << \one \\ \two >>

Or are there pits to fall into there?

[dak, 2015-10-04 14:54:12]

On 2015/10/04 13:53:05, Dan Eble wrote:
> On 2015/10/03 12:56:50, dak wrote:
> > On 2015/10/03 12:42:19, Dan Eble wrote:
> > > I've marked the ticket as "needs work" for now.  I still have questions. 
It
> > > *would* be nice to be able to say
> > > 
> > >    \override Staff.partCombineParameter = #x
> > >    \partcombine \one \two ...
> > > 
> > > and have the part combiner get its initial settings from the context.
> > 
> > You bet.
> > 
> 
> Oh, or even better....
> 
>     \override Staff.partCombineParameter = #x
>     << \one \\ \two >>
> 
> Or are there pits to fall into there?

Uh, I don't see how the latter could work.