\partcombine mmrest improvements (Issue 4599 etc.)

This review covers three commits.  The latter two depend on the first.

I. Carry multi-measure rests across voices in \partcombine

The Part_combine_iterator no longer just stops a multi-measure rest
when a part moves out of a context.  Now it recreates a rest of the
correct remaining duration in the part's new context.

The remaining duration of a busy multi-measure rest passes from
Multi_measure_rest_engraver to Part_combine_iterator in a context
property.

II. part combiner: prefer rests to multi-measure rests
    
This change fixes the case originally reported as issue 1677; however,
the regression test included with this change shows that it does not
solve the root problem.

III. Issue 4599: end solo passage at end of part
    
Also select silence from the remaining part.
This fixes regression test part-combine-solo-end.ly.

[Dan Eble, 2015-10-04 00:09:06]

Prefer rests to mmrests

[Dan Eble, 2015-10-04 00:09:56]

End solo at end of part

[Dan Eble, 2015-10-04 13:46:22]

Remove vestiges of an intermediate failed solution

[Trevor Daniels, 2015-10-08 12:07:42]

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:146: // get_event_length(), which reads "length"?
Perhaps this question should be resolved before pushing.

[Dan Eble, 2015-10-08 22:09:32]

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:146: // get_event_length(), which reads "length"?
On 2015/10/08 12:07:42, Trevor Daniels wrote:
> Perhaps this question should be resolved before pushing.

That'll teach me to raise questions about the existing implementation.

If someone can help answer this and explain that a specific and simple change is
desirable, I'd be happy to take action.  Otherwise, I'd be willing to retract
the comment before pushing.  Otherwise, this patch is going to be delayed until
I have more time.

[dak, 2015-10-09 06:23:09]

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:144: // What is the relationship between
"duration" and "length"?  Why does
Length is a Moment, duration is a Duration.  get_event_length converts a
Duration into a Moment and caches the result.  Moments are what the general
timing works with.

So there is really nothing to be seen here.

[dak, 2015-10-09 06:55:02]

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:53: void start_mmrest (Context *c, const Moment
&length);
We don't pass Moment by reference.  It's a simple scalar type and references
reached via unsmob are rarely guaranteed to survive for long.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:132: ev->set_property ("length",
length.smobbed_copy());
A multi-measure rest event having a length (which is a cache of the "duration"
converted to a Moment) but not a duration is very, very, weird.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:156: {
Setting "duration" to SCM_EOL is just wrong.  Instead of setting lengths
(assuming that a Moment is all you have available), you should use
make-duration-of-length or similar in order to get a useful "duration" value.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
lily/part-combine-iterator.cc:172: Moment *rem = unsmob<Moment> (c->get_property
("multiMeasureRestBusy"));
Please use robust_scm2moment here.

https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly#newco...
ly/engraver-init.ly:841: %% by the part combiner.
Ugh.  It would appear that multi-measure rest splitting (or other rest
splitting) is only needed in the part combiner so why does it make sense to add
all this complication to the Multi_measure_rest_engraver and let it idle in
NullVoice?

Why not keep the code for dealing with this in the part combiner and make the
Multi_measure_rest_engraver just do what is expected from it?

https://codereview.appspot.com/265410043/diff/60001/scm/define-context-proper...
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/265410043/diff/60001/scm/define-context-proper...
scm/define-context-properties.scm:480: (multiMeasureRestBusy ,ly:moment?
"Remaining multi-measure rest duration.")
It seems like a strange deal to maintain an engraver-specific property in a
context property.  Most of the time, such variables are kept inside the
engraver.

Stupid question: would the partcombiner work better by having a context
PartCombinePart accepted by Voice, and then we use \change Voice = "soloII" and
similar in the two original parts?  Or would it make more sense to put something
like PartCombinePart between Staff and Voice and run the original stuff in
Voice, using \change PartCombinePart = "soloII" and so on?  One of those might
lead to a reasonably natural way of dealing with slurs and stuff.

The reason I am asking in this context is that I think that you use
multiMeasureRestBusy for transplanting the internal state of a
Multi_measure_rest_engraver, and it may be expedient to instead transplant the
whole engraver by an appropriate use of context hierarchy.

Just some food for thought.

[Dan Eble, 2015-10-10 00:36:04]

On 2015/10/09 06:55:02, dak wrote:

> We don't pass Moment by reference.  It's a simple scalar type and references
> reached via unsmob are rarely guaranteed to survive for long.

I'll change it to pass by value, though I don't understand why passing
by reference is a problem.  When passed by value, the Moment will only
survive for the duration of the call.  Do you mean that a Moment
passed by reference might not survive even that long?

Moments are 40 bytes (in Lilydev 3) which seems a bit large to pass by
value, but I'm speaking from experience that mostly involves limited
memory or high performance.


> Why not keep the code for dealing with this in the part combiner and make the
> Multi_measure_rest_engraver just do what is expected from it?

Can you be more specific?  The first interpretation that comes to my
mind is that you want the part combiner to rewrite multi-measure rest
events in the input so that they are divided at the moments where the
parts need to be swapped to other voice contexts.  Then the MMR
engraver would need to do nothing special.  The PC iterator would not
even need to "kill" MMRs because they would already be the right
length.  But rewriting the input parts would be a significant change.


> It seems like a strange deal to maintain an engraver-specific property in a
> context property.  Most of the time, such variables are kept inside the
> engraver.

Can you provide an example of an iterator getting state from an engraver?


> Stupid question: would the partcombiner work better by having a context
> PartCombinePart accepted by Voice, and then we use \change Voice = "soloII"
and
> similar in the two original parts?  Or would it make more sense to put
something
> like PartCombinePart between Staff and Voice and run the original stuff in
> Voice, using \change PartCombinePart = "soloII" and so on?  One of those might
> lead to a reasonably natural way of dealing with slurs and stuff.

I experimented with those ideas several months ago and could not find
a reasonable solution.  (That's not to say one doesn't exist.)  A
problem with having a new context below Voice is that things
expecting Bottom to be Voice would work differently.  A problem with
having a new context above Voice is that you start wanting to move
engravers from Voice to it (as I recall), and it basically becomes
what Voice was, and Voice serves a different purpose inside
\partcombine than outside.  I don't remember any of the other issues;
I'd have to review my files and the mail archives.


> The reason I am asking in this context is that I think that you use
> multiMeasureRestBusy for transplanting the internal state of a
> Multi_measure_rest_engraver, and it may be expedient to instead transplant the
> whole engraver by an appropriate use of context hierarchy.

I'll accept it as food for thought.  I am not optimistic that it will
lead to a solution that is easier to understand.

Thanks for the review.

[Dan Eble, 2015-10-10 01:50:28]

On 2015/10/09 06:23:09, dak wrote:

>
https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
> lily/part-combine-iterator.cc:144: // What is the relationship between
> "duration" and "length"?  Why does
> Length is a Moment, duration is a Duration.  get_event_length converts a
> Duration into a Moment and caches the result.  Moments are what the general
> timing works with.
> 
> So there is really nothing to be seen here.

By what process should get_event_length() convert a duration to a moment?  All I
see in get_event_length() is a call to get the "length" property.

When I change my start_mmrest() to set "duration" instead of "length", the bug
in part-combine-solo-end.ly reappears.  I am using
Duration().compressed(length.main_part_).smobbed_copy() as the duration.

[dak, 2015-10-10 07:16:42]

On 2015/10/10 00:36:04, Dan Eble wrote:
> On 2015/10/09 06:55:02, dak wrote:
> 
> > We don't pass Moment by reference.  It's a simple scalar type and references
> > reached via unsmob are rarely guaranteed to survive for long.
> 
> I'll change it to pass by value, though I don't understand why passing
> by reference is a problem.  When passed by value, the Moment will only
> survive for the duration of the call.  Do you mean that a Moment
> passed by reference might not survive even that long?

If garbage collection happens because of an allocation in that call, yes that
may happen.  With get_property there is a good chance that a callback call will
be _cached_ in which case the caching will provide protection from garbage
protection.  But that's rather hand-wavy.

> Moments are 40 bytes (in Lilydev 3) which seems a bit large to pass by
> value, but I'm speaking from experience that mostly involves limited
> memory or high performance.
> 
> 
> > Why not keep the code for dealing with this in the part combiner and make
the
> > Multi_measure_rest_engraver just do what is expected from it?
> 
> Can you be more specific?  The first interpretation that comes to my
> mind is that you want the part combiner to rewrite multi-measure rest
> events in the input so that they are divided at the moments where the
> parts need to be swapped to other voice contexts.  Then the MMR
> engraver would need to do nothing special.  The PC iterator would not
> even need to "kill" MMRs because they would already be the right
> length.  But rewriting the input parts would be a significant change.

But a change happening at an obvious place and not relying on some
behind-the-scenes magic in a place where it's sort of unexpected.

> > It seems like a strange deal to maintain an engraver-specific property in a
> > context property.  Most of the time, such variables are kept inside the
> > engraver.
> 
> Can you provide an example of an iterator getting state from an engraver?

Should it be getting state from an engraver?  To answer your question literally
(I doubt that this example makes a lot of sense for your application)
Grace_engraver is an example of an engraver talking with an iterator though in
that case the purpose is to synchronize engraver action to iterator action
(iterators fire in well-defined order even when at the same time step, and
that's needed here).

Having an iterator look at context properties is a so-so communication path, and
part of the reason is that changes and inspection happening at the same time
step render things tricky.

> > Stupid question: would the partcombiner work better by having a context
> > PartCombinePart accepted by Voice, and then we use \change Voice = "soloII"
> and
> > similar in the two original parts?  Or would it make more sense to put
> something
> > like PartCombinePart between Staff and Voice and run the original stuff in
> > Voice, using \change PartCombinePart = "soloII" and so on?  One of those
might
> > lead to a reasonably natural way of dealing with slurs and stuff.
> 
> I experimented with those ideas several months ago and could not find
> a reasonable solution.

I have to think about what you wrote here.  This was just labelled "food for
thought" and if there are only some "oh, all that would be needed to make this
work would be if LilyPond did $x" impediments, it might be worth in the long run
to let it do $x.  However, this is not immediately pertinent to this review, it
just made me think of it.

[dak, 2015-10-10 07:35:59]

On 2015/10/10 01:50:28, Dan Eble wrote:
> On 2015/10/09 06:23:09, dak wrote:
> 
> >
>
https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterato...
> > lily/part-combine-iterator.cc:144: // What is the relationship between
> > "duration" and "length"?  Why does
> > Length is a Moment, duration is a Duration.  get_event_length converts a
> > Duration into a Moment and caches the result.  Moments are what the general
> > timing works with.
> > 
> > So there is really nothing to be seen here.
> 
> By what process should get_event_length() convert a duration to a moment?  All
I
> see in get_event_length() is a call to get the "length" property.

Right.  It gets there by "precompute-music-length" during scorification where
the real job is done by Music::get_length.  So obviously if we are creating a
Stream_event from scratch, this mechanism can no longer be used since the
respective callbacks don't work with a Stream_event.  Still, omitting duration
altogether is asking for trouble since the Stream_event is also seen by other
engravers.

And it's not like Duration (mom.main_part_, true) (who wrote _that_? It's scary)
or Duration ().compressed (mom.main_part_) is that hard to write.

> When I change my start_mmrest() to set "duration" instead of "length", the bug
> in part-combine-solo-end.ly reappears.  I am using
> Duration().compressed(length.main_part_).smobbed_copy() as the duration.

Well, here I see SCM_EOL.

[Dan Eble, 2015-10-11 01:49:13]

On 2015/10/10 07:35:59, dak wrote:
> On 2015/10/10 01:50:28, Dan Eble wrote:
> > When I change my start_mmrest() to set "duration" instead of "length", the
bug
> > in part-combine-solo-end.ly reappears.  I am using
> > Duration().compressed(length.main_part_).smobbed_copy() as the duration.
> 
> Well, here I see SCM_EOL.

I was talking about experimenting in my working copy; sorry for the confusion.