Add chord range to make-part-combine-music

Issue 4112: Add internal chord-range option to part combiner
    
Add a number-pair parameter to make-part-combine-music to replace the
previously hard-coded minimum and maximum intervals that may be
combined into a chord or unison in the shared voice.
    
There is no user interface for this enhancement because it is not easy
to agree on one, but there is a regression test showing how it might
be used at one's own risk.

[Dan Eble, 2014-09-19 02:38:12]

I hope this is acceptable for now without a music function.  I have other
partcombiner contributions planned and I think it makes sense to work from the
inside out.

https://code.google.com/p/lilypond/issues/detail?id=4112

[dak, 2014-09-19 11:20:55]

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

https://codereview.appspot.com/144170043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:1134: #(define partcombine-chord-range '(0 . 8))
I think that's the kind of thing much better put into a context property.  It
can change over the course of one partcombine, and it is the kind of arbitrary
parameter that we do not want to pass up and down all the time since it is just
one of many possibly interesting parameters for which we do not want to redesign
interfaces any time a new one gets added.

[Dan Eble, 2014-09-19 13:20:41]

On 2014/09/19 11:20:55, dak wrote:
> I think that's the kind of thing much better put into a context property.  It

I see that recording-group-emulate calls (ly:context-property child
'instrumentTransposition)).  Should I look to that as an example?

> can change over the course of one partcombine

I would never use that feature.  It would also complicate implementation and
testing.

> parameter that we do not want to pass up and down all the time since it is
just
> one of many possibly interesting parameters for which we do not want to
redesign
> interfaces any time a new one gets added.

I agree.  Would you say the same about the "direction" parameter or is it
exceptional?

[dak, 2014-09-19 13:51:55]

On 2014/09/19 13:20:41, Dan Eble wrote:
> On 2014/09/19 11:20:55, dak wrote:
> > I think that's the kind of thing much better put into a context property. 
It
> 
> I see that recording-group-emulate calls (ly:context-property child
> 'instrumentTransposition)).  Should I look to that as an example?
> 
> > can change over the course of one partcombine
> 
> I would never use that feature.  It would also complicate implementation and
> testing.
> 
> > parameter that we do not want to pass up and down all the time since it is
> just
> > one of many possibly interesting parameters for which we do not want to
> redesign
> > interfaces any time a new one gets added.
> 
> I agree.  Would you say the same about the "direction" parameter or is it
> exceptional?

I would say the same about the direction parameter.  And the key, and the clef. 
And particular forced partcombiner decisions.  I have several unfinished
branches in my repository that are concerned with ripping out the ridiculous
amount of \partCombine commands that still don't result in flexible ways of
combining parts (like using \voiceThree and \voiceFour instead of \voiceOne and
\voiceTwo) and replace them with optional context modifications and context
properties.  Before I parked work on
<URL:https://code.google.com/p/lilypond/issues/detail?id=1321>, it was going in
that direction as well.

If you want to, I can rebase the respective branches on current master and push
them into the central repository, and you can see whether you want to pick up
work on them.

[Dan Eble, 2014-09-19 22:16:33]

On 2014/09/19 13:51:55, dak wrote:
> If you want to, I can rebase the respective branches on current master and
push
> them into the central repository, and you can see whether you want to pick up
> work on them.

My first choice is to implement this change in a way that is acceptable for 2.19
and easy for you to rebase to.

If that is not feasible and you expect your branch to make it into 2.19, I would
be willing to base this change on your branch.

If there is no way to get this change (and related partcombine changes) into
2.19, I will postpone it indefinitely.  I am trying to make my changes available
to some friends in the near future.  If I can't get them into the next stable
release, I will end up just sharing my files with them.  Once that is done, my
main incentive for contributing them here will be gone.

[Keith, 2014-10-29 05:06:28]

This seems to allow the style of partcombine where unisons are double-stemmed
which would be very nice.
You can, if you like, put a small image (like a 2kB png) on the lilypond issue
tracker showing the desired output; then people can understand the purpose of
the patch.

> I see that recording-group-emulate calls (ly:context-property child
> 'instrumentTransposition)).  Should I look to that as an example?
>
No.  We could not find any way in which this was functioning.  
 (http://code.google.com/p/lilypond/issues/detail?id=645#c11)
recording-group-emulate works before the Property_engraver gets a chance to set
instrumentTransposition on the output Staff.

It would be nice to be able to change the chord-range in the middle of the
music, but in general, context properties seem a poor choice, because the output
contexts are not even determined when \partcombine executes.

If you used a context, the way David is trying for partCombineChords, etc., you
could say
  combined = \displayMusic \transpose c c' \partcombineUp
    {c4 c c c  \set partcombineChordRange = #'(2 . 8)
       | c c c c}
    {c4 c c e | c c c c }
but then user would be understandably confused when
  \new Staff \with { partcombineChordRange = #'(4 . 5) } \combined
has no effect.

You see what Reinhold did when he created \partCombineChords and so on; he made
a new event type, for the sole purpose of being iterated by
recording-group-emulate to get the forced-state messages to \partcombine.

I think that an optional argument to \partcombine music function is the most
appropriate way to set chord-range.

https://codereview.appspot.com/144170043/diff/1/input/regression/part-combine...
File input/regression/part-combine-chord-range.ly (right):

https://codereview.appspot.com/144170043/diff/1/input/regression/part-combine...
input/regression/part-combine-chord-range.ly:21: (make-part-combine-music parser
(list part1 part2) #f '(2 . 12)))
you can make a friendlier interface; see below.

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

https://codereview.appspot.com/144170043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:1137: #(define-music-function (parser location part1
part2) (ly:music? ly:music?)
Here, an optional argument might be appropriate, instead of the global variable
above,
 #(define-music-function (parser location part1 part2) 
   (number-pair? '(0 . 8))(ly:music? ly:music?)
Then you can say
  \partCombine '(1 . 7) {...} {...}

https://codereview.appspot.com/144170043/diff/1/scm/part-combiner.scm
File scm/part-combiner.scm (right):

https://codereview.appspot.com/144170043/diff/1/scm/part-combiner.scm#newcode275
scm/part-combiner.scm:275: "@var{evl1} and @var{evl2} should be ascending.
@var{chord-range} is a pair (min . max) defining the distance (in steps) between
notes that may share a stem."
"..that may share a stem, i.e., that may be set as a single chords or as a
unison."
(Reading the code, I /think/ unisons are affected in the consistent way.)

[Keith, 2014-10-29 05:07:58]

>  #(define-music-function (parser location part1 part2) 
>    (number-pair? '(0 . 8))(ly:music? ly:music?)

  #(define-music-function (parser location chord-range part1 part2) 
    (number-pair? '(0 . 8))(ly:music? ly:music?)

[Keith, 2014-10-29 05:09:12]

On 2014/10/29 05:07:58, Keith wrote:
> >  #(define-music-function (parser location part1 part2) 
> >    (number-pair? '(0 . 8))(ly:music? ly:music?)

or maybe
 
   #(define-music-function (parser location chord-range part1 part2) 
     ((number-pair? '(0 . 8)) ly:music? ly:music?)

Scheme: just add parentheses until it compiles

[Dan Eble, 2014-10-31 03:19:01]

On 2014/10/29 05:06:28, Keith wrote:
> This seems to allow the style of partcombine where unisons are double-stemmed
> which would be very nice.

Exactly.  I use it to keep both unisons and seconds apart in vocal music.

> I think that an optional argument to \partcombine music function is the most
> appropriate way to set chord-range.

I'm glad you said that, because I was thinking along those lines too; however,
this is not the only new option in the works.  I thought it would be wise to
keep it internal for now, write some regression tests that exercise it as an
expert might, and then work on the other features before finally choosing a UI.

On the other hand, if you are willing to discuss it now, maybe you can ease my
mind.  This is the beginning of the function I've actually been using:

(define-public (dfe-determine-split-list evl1 evl2 chord-range enable-solo)
  "@var{evl1} and @var{evl2} should be ascending. @var{chord-range} is a pair
(min . max) defining the distance (in steps) between notes that may share a
stem. If @var{enable-solo} is false, skip solo analysis and display the rests of
both voices."
  (let* ((pc-debug #f)
         ; For solo analysis, all silence is equivalent.  Otherwise,
         ; voices that rest together must rest for the same duration
         ; to be considered in unisilence.
         (div-rest-state (if enable-solo 'unisilence 'apart-silence))
         (voice-state-vec1 (make-voice-states evl1))
         (voice-state-vec2 (make-voice-states evl2))
         (result (make-split-state voice-state-vec1 voice-state-vec2)))

Setting enable-solo to #f allows a style in which the partcombiner never selects
the solo state, but it still chooses between other states.  Most of my work with
Lilypond is arrangement or transcription of four-part hymns in which there isn't
much silence in the first place, but when a voice is silent, rests are printed
for that voice rather than marking it "solo."  (Hopefully I'll have time to
attach an image in the tracker tonight, but I might not.)

When solo analysis is disabled, I've improved (sez me) the analysis of silence
so that rests are combined in roughly the same way as notes.  If the voices
begin and end a rest simultaneously, then that rest is shared.  I think this
works around some issues with rests and solo analysis, but please forgive me for
not going into more detail; it's been a long time since I wrote this code, and
it's in Scheme, so I'm going to have to write it again to understand it.  :-)

Based on the feedback I've received so far, it seems that reimplementing this
solo control like the forced partcombine states would be the best approach for
that, leaving me free to add the chord-range as an optional parameter to
\partcombine without fear that the options will get out of hand.  What do you
say?

[Keith, 2014-10-31 06:39:45]

On Thu, 30 Oct 2014 20:19:01 -0700, <nine.fierce.ballads@gmail.com> wrote:

> This is the beginning of the function I've actually been
> using:
>
> (define-public (dfe-determine-split-list evl1 evl2 chord-range
> enable-solo)
>    "@var{evl1} and @var{evl2} should be ascending. @var{chord-range} is a
> pair (min . max) defining the distance (in steps) between notes that may
> share a stem. If @var{enable-solo} is false, skip solo analysis and
> display the rests of both voices."

If I understand, this is the style where if one part plays solo, you show the
rests of the resting part on the score.  J.F.Lucarelli used that style for
Dvorák Symph #7 at
  http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1901
which is a good test-case for partcombine and its override functions.

"skip solo analysis" doesn't make sense unless you read the code.  I guess the
idea is "instead of using SoloI/II, display the rests of the resting part."

> Based on the feedback I've received so far, it seems that reimplementing
> this solo control like the forced partcombine states would be the best
> approach for that, leaving me free to add the chord-range as an optional
> parameter to \partcombine without fear that the options will get out of
> hand.  What do you say?

A reasonable choice, given that a no-solo override is related to the other
PartCombineForceEvents (and given the fact that there is no context shared by
the inputs to \partcombine, in which to set context properties like we would
normally prefer).

> https://codereview.appspot.com/144170043/
 

[Dan Eble, 2014-10-31 13:20:01]

On 2014/10/31 06:39:45, Keith wrote:
> If I understand, this is the style where if one part plays solo, you show the
> rests of the resting part on the score.  J.F.Lucarelli used that style for
> Dvorák Symph #7 at
>   http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1901
> which is a good test-case for partcombine and its override functions.

Judging from the previews, yes, for example the clarinets.  I also notice that
there is an a2 section, so it's a case of preventing solo I/II without changing
the chord range from the default (the feature this ticket is actually about).

[dak, 2014-10-31 13:36:31]

On 2014/10/31 13:20:01, Dan Eble wrote:
> On 2014/10/31 06:39:45, Keith wrote:
> > If I understand, this is the style where if one part plays solo, you show
the
> > rests of the resting part on the score.  J.F.Lucarelli used that style for
> > Dvorák Symph #7 at
> >   http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1901
> > which is a good test-case for partcombine and its override functions.
> 
> Judging from the previews, yes, for example the clarinets.  I also notice that
> there is an a2 section, so it's a case of preventing solo I/II without
changing
> the chord range from the default (the feature this ticket is actually about).

Do you have a good suggestion how we could separate out the part engraving
forced decision into settings that operate per-voice rather than
per-part-engraver?  It would not likely help all that much with a "chord range"
setting which indeed always applies to the combination of both voices, but it
would seem like it would put the information where one would look for it.

Maybe something like \forceAutomatic \forceChord \forceApart \forceSolo with
increasing priority (Solo in one voice beats Chord in the other)?

[Dan Eble, 2014-10-31 22:50:35]

On 2014/10/31 13:36:31, dak wrote:
> Do you have a good suggestion how we could separate out the part engraving
> forced decision into settings that operate per-voice rather than
> per-part-engraver?

> Maybe something like \forceAutomatic \forceChord \forceApart \forceSolo with
> increasing priority (Solo in one voice beats Chord in the other)?

Rock crushes scissors, jazz beats rock.  I'll think about it when I get into
solo inhibition, but first I'll redo the chord-range option properly.

One thing just occurred to me.  If this option is going into the UI, do you
think the numbers should be 1-based for easier comprehension by musicians?  For
example, use '(1 . 9) to allow chording unisons to ninths, rather than '(0 . 8)?

[Keith, 2014-11-01 04:18:41]

On Fri, 31 Oct 2014 15:50:35 -0700, <nine.fierce.ballads@gmail.com> wrote:

> One thing just occurred to me.  If this option is going into the UI, do
> you think the numbers should be 1-based for easier comprehension by
> musicians?  For example, use '(1 . 9) to allow chording unisons to
> ninths, rather than '(0 . 8)?
>

Good to think of this now, because it is awkward to change later.

I think either convention will be fine, if the doc strings are clear.

I understood the zero-based system without hesitation, because your docstring
said the numbers were  "the distance (in steps) between notes".    There are
probably cases where negative values are appropriate, so as to set two parts as
chords even if they cross, on the conductor's score.  Negative values make more
sense on the zero-based scheme.


> https://codereview.appspot.com/144170043/

[dak, 2014-11-01 06:37:11]

On 2014/10/31 22:50:35, Dan Eble wrote:

> One thing just occurred to me.  If this option is going into the UI, do you
> think the numbers should be 1-based for easier comprehension by musicians? 
For
> example, use '(1 . 9) to allow chording unisons to ninths, rather than '(0 .
8)?

As a UI, numbers instead of intervals (possibly represented only by a pitch in
relation to c' like \transposition does) do not make much sense.

The sole motivation for numbers would be to make them distinguishable from
music, and as we are piling options on, things will become increasingly awkward
to tell apart by type since we are indeed talking about musical concepts all the
time.

Regarding such user interface extensions, I think we should rather go in the
direction of <URL:https://code.google.com/p/lilypond/issues/detail?id=1321#c30>.
 Using context modifications makes it reasonably straightforward to add
arbitrary named settings in arbitrary order on demand.

I can rebase that patch if you want to see whether fitting a user interface
there feels better.

[Dan Eble, 2014-11-01 18:04:31]

On 2014/11/01 06:37:11, dak wrote:
> I can rebase that patch if you want to see whether fitting a user interface
> there feels better.

Would you settle for my original approach of adding an internal argument but not
exposing any UI for it until after more thought is put into the many
interrelated issues that have been mentioned?

[dak, 2014-11-01 18:32:49]

On 2014/11/01 18:04:31, Dan Eble wrote:
> On 2014/11/01 06:37:11, dak wrote:
> > I can rebase that patch if you want to see whether fitting a user interface
> > there feels better.
> 
> Would you settle for my original approach of adding an internal argument but
not
> exposing any UI for it until after more thought is put into the many
> interrelated issues that have been mentioned?

Well, you may have to rework once we have progressed on how we organize
internals.  But that beats getting nowhere because of deadlocking on everything
that might change.

With the user interface, it's a bit more icky.  At the very least, what we get
in 2.20 should either stay around or be fully upgradable using convert-ly.  I am
skeptical that we can really guarantee this for even the current forced
partcombine decisions yet.

I'm not getting half of the stuff done I want to.  And it *is* sort of
ridiculous that for every proposal regarding the part combiner I pull an old
issue out of the hat where I did not ultimately commit anything because I was
not really happy with it either.

But again: maybe I should just commit those and accept that we'll get nowhere
fast, and likely have interfaces in 2.20 that we cannot really save or convert
into 2.22.

[Dan Eble, 2014-11-01 19:07:36]

On 2014/11/01 18:32:49, dak wrote:
> On 2014/11/01 18:04:31, Dan Eble wrote:
> > Would you settle for my original approach of adding an internal argument but
> > not exposing any UI for it until after more thought is put into the many
> > interrelated issues that have been mentioned?
> 
> Well, you may have to rework once we have progressed on how we organize
> internals.  But that beats getting nowhere because of deadlocking on
everything
> that might change.

What I am using now is a monolithic alternative part combiner.  A small
contribution that reduces the risk and extent of conflicts with new work is
still valuable to me.

After this, I think there might be one or two tiny bug-fixes that I can extract,
but the main thing that remains is solo inhibition. When I get to that, I will
try to consider the whole problem including forced decisions, output voice
selection, and context modification.  You have shown me a lot in this review. 
It strikes me as a project worthy of two months of full-time work, but maybe
I'll stumble upon some ideas that eventually contribute to progress.

> And it *is* sort of ridiculous that for every proposal regarding the part
> combiner I pull an old issue out of the hat where I did not ultimately
> commit anything because I was not really happy with it either.

You said it. :-)

I expect to post another patch set with minor revisions soon.  (Hopefully it
will show the deltas; this review was originally created with my problematic
LilyDev 2 installation.)

[Dan Eble, 2014-11-03 13:19:22]

Improve readability

[Keith, 2014-11-06 05:44:50]

Still good, still useful.

The hesitation on the user interface seems foolish.  Without a stable user
interface this becomes a function for just one user.

Changing the current global variable 'partcombine-chord-range' will not become
the user interface.  There is no context in which to set that range, because
\parcombine does its job outside any context:
  variable = \partcombine \violinOne \violinTwo
So the choices are 
1) a variant \partcombineSelectChordRange that takes an extra argument
2) an optional argument to \partcombine

If you push this I'll submit the patch for option (2)

[Dan Eble, 2014-11-07 12:37:41]

On 2014/11/06 05:44:50, Keith wrote:
> Changing the current global variable 'partcombine-chord-range' will not become
> the user interface.

All I meant for that variable was to avoid repeating the same magic numbers
three times.  Is there a more idiomatic way to do that in Scheme?  Should I have
commented it?

[Keith, 2014-11-08 04:51:57]

On Fri, 07 Nov 2014 04:37:41 -0800, <nine.fierce.ballads@gmail.com> wrote:

> On 2014/11/06 05:44:50, Keith wrote:
>> Changing the current global variable 'partcombine-chord-range' will
> not become
>> the user interface.
>
> All I meant for that variable was to avoid repeating the same magic
> numbers three times.  Is there a more idiomatic way to do that in
> Scheme?  Should I have commented it?
>

No, that seems fine as it is.
I was trying to go through the possible user-interfaces systematically, and
exposing that global variable was one possible interface.  I had trouble
articulating exactly /why/ simply changing that global variable wouldn't become
the user-interface.

I forgot the originally-suggested possible interface for the user to change the
chord-range, by setting some context property of the temporary Voice contexts,
the ones used by \partcombine to iterate through its inputs.  Then we could
change chord-range in the middle of the music, but I'm not sure the \partcombine
logic can handle mid-music changes.  The analysis is global, so if you
double-stem the e' below, probably the slurs should separate and all slurred
become double-stemmed (like \partcombine already does if there is a
voice-crossing).
   \partcombine
    {e'( e' b' f')}
    {c'( e' g' c')}
Changing chord-range just before the e' might (or might not) fool \partcombine.