Automatically unfold percent and tremolo repeats in MIDI output. (issue 769)

Automatically unfold percent and tremolo repeats in MIDI output. (issue 769)

Tremolo durations are now scaled when iterating, not when they're created,
so the original body is intact for unfolding. This also means unfold-repeats
and display methods no longer need to undo the scaling.

Tremolo spanners are now built in Scheme, not C++.

shift-duration-log now clears the cached length, since it changes the length.

\shiftDurations no longer duplicates shift-duration-log.

Open question: can we get rid of the tremolo-type property and just calculate it when making the spanner?

[Keith, 2013-12-21 06:43:25]

I think I like the approach.

Right now, we can
\score {
 \music
 \layout{} }
\score {
 \unfoldRepeats \music
 \midi{} }

And if I understand, when this is finished we can
\score {
 \music
 \layout{}
 \midi{midiUnfoldRepeats = #'(percent-repeated-music) } }

I did not before realize that the music is iterated *twice* when there is a
\layout and \midi in the same \score

I think we need the tremolo-type property to hold the 32 from input c2:32  From
what I can see, this type of tremolo will not be repeated (nor is it with
unfoldRepeats)

The other limitation of \unfoldRepeats is that it fails to see repeats in
parallel expressions << {\repeat volta 2 s1 } {c2 d2} >> while this approach
using the iterators will see them and be able to expand them when requested.

[Devon Schudy, 2013-12-21 07:10:35]

Tremolo cleanups removed to a separate patch. Remove unused midiUnfoldRepeats.

[dak, 2013-12-21 08:23:05]

Keith comments:
> The other limitation of \unfoldRepeats is that it fails to see repeats in
> parallel expressions << {\repeat volta 2 s1 } {c2 d2} >> while this approach
> using the iterators will see them and be able to expand them when requested.

I think he is wrong about that.  I don't see anything inherent in this code that
would work differently here.

https://codereview.appspot.com/40720060/diff/20001/lily/repeated-music.cc
File lily/repeated-music.cc (right):

https://codereview.appspot.com/40720060/diff/20001/lily/repeated-music.cc#new...
lily/repeated-music.cc:152: Repeated_music::should_unfold_repeat (Music *mus,
Context *context)
This does not belong in the Repeated_music class.  It's not a property of the
music, but of iteration.

It would probably make sense to derive the various repeat iterators from a
common base class.  This could then have this as a member function without
arguments.

I'm not happy with the hardcoding of midi, but until we can write
\new Staff \with { \midi { repeatTypes = #'(volta-repeated-music } }
namely until context mods can get more discriminatory, we are probably stuck
with it.

However, if the comment is "only unfold in midi" you should make it match the
code by checking is-midi.  Even if we can nowadays only iterate layouts and
midi, there is nothing precluding the iteration of MusicXML output or other.

https://codereview.appspot.com/40720060/diff/20001/lily/repeated-music.cc#new...
lily/repeated-music.cc:164: SCM *tail = &l;
It's usually simpler to just cons the list at the front and do an scm_reverse_x
on it before returning.

https://codereview.appspot.com/40720060/diff/20001/lily/repeated-music.cc#new...
lily/repeated-music.cc:182: if (i >= rep_count - alt_count)
That condition does not agree with the current implementation of
unfold-repeats-fully or what it was called.  After checking with the notation
manual, that would appear to be the mistake of unfold-repeats-fully while this
code gets it right.  (Actually, it should, stealing the code from
Unfolded_repeat_iterator).  Duh.

Seems like I should push a fix to that...

[Keith, 2013-12-22 07:01:10]

On Sat, 21 Dec 2013 00:23:05 -0800, <dak@gnu.org> wrote:

> Keith comments:
>> The other limitation of \unfoldRepeats is that it fails to see repeats
> in
>> parallel expressions << {\repeat volta 2 s1 } {c2 d2} >> while this
> approach
>> using the iterators will see them and be able to expand them when
> requested.
>
> I think he is wrong about that.  I don't see anything inherent in this
> code that would work differently here.

I meant the future of this approach, after there is the option to unfold volta
repeats.

I was thinking that by doing the unfolding in the iterators
(as opposed to working on music expressions as \unfoldRepeats does)
it would be at least possible to expand volta repeats.

It would not be easy, because there are nonsense cases like
   << {s1 \repeat volta 2 s1 } {c1~ c2 d2} >>
and probably some very similar things that actually make sense.

It would take significant work to communicate between various iterators,
similar to the efforts Mike made to break slurs at repeats.

[dak, 2013-12-22 08:07:00]

On 2013/12/22 07:01:10, Keith wrote:
> On Sat, 21 Dec 2013 00:23:05 -0800, <mailto:dak@gnu.org> wrote:
> 
> > Keith comments:
> >> The other limitation of \unfoldRepeats is that it fails to see repeats
> > in
> >> parallel expressions << {\repeat volta 2 s1 } {c2 d2} >> while this
> > approach
> >> using the iterators will see them and be able to expand them when
> > requested.
> >
> > I think he is wrong about that.  I don't see anything inherent in this
> > code that would work differently here.
> 
> I meant the future of this approach, after there is the option to unfold volta
> repeats.
> 
> I was thinking that by doing the unfolding in the iterators
> (as opposed to working on music expressions as \unfoldRepeats does)

But \unfoldRepeats does not do its actual unfolding work on music expressions. 
If it did, it would wreak havoc with \relative mode.  It merely replaces the
RepeatedMusic expression with an (still folded) UnfoldedRepeatedMusic expression
which is then actually expanded in its respective iterator.

Which is exactly the same thing Devon's patch does, except that the shortcircuit
to the UnfoldedRepeatedMusic code path is taken without changing the music, by
factoring out the respective code for use by both iterators.

> It would not be easy, because there are nonsense cases like
>    << {s1 \repeat volta 2 s1 } {c1~ c2 d2} >>
> and probably some very similar things that actually make sense.

Things break down anyway as soon as we are talking about more than one context.

[Devon Schudy, 2013-12-22 19:46:09]

Unfolding during iteration doesn't extend well to voltas, because it
changes their length. Even if the length is updated, the old length
has already been used by the parent iterator to compute moments for
subsequent events. So unless this can be changed, unfolding will have
to be done earlier, on music expressions. Unfortunately I think this
means unfold settings must come from the output def instead of the
context, so they can't be changed locally. This is not looking good
for voltas.

Keith wrote:
> I think we need the tremolo-type property to hold the 32 from input c2:32

On TremoloEvent, yes. But do we need it on TremoloRepeatedMusic? I
suppose it might be handy for tweaking.

dak wrote:
> This does not belong in the Repeated_music class.  It's not a property
> of the music, but of iteration.

Repeated_music isn't a music class — it should really be called
something like Repeat_utilities. (And it could be a namespace, not a
class.)

> I'm not happy with the hardcoding of midi, but until we can write
> \new Staff \with { \midi { repeatTypes = #'(volta-repeated-music } }
> namely until context mods can get more discriminatory, we are probably
> stuck with it.

\score { ... \layout { \context { \Score unfoldRepeats = #'(tremolo volta) } }
}
is a tolerable way to specify non-midi unfolding.

> That condition does not agree with the current implementation of
> unfold-repeats-fully [...] Seems like I should push a fix to that...

Or export Repeated_music::unfolded_music to Scheme and call it. This
part of the patch doesn't depend on tremolo cleanup or anything else.

[Keith, 2013-12-22 22:01:25]

On Sun, 22 Dec 2013 11:45:49 -0800, Devon Schudy <dschudy@gmail.com> wrote:

> Unfolding during iteration doesn't extend well to voltas, because it
> changes their length. Even if the length is updated, the old length
> has already been used by the parent iterator to compute moments for
> subsequent events. So unless this can be changed,

Okay.  That eager evaluation of lengths by the iterators also frustrated my
attempt at a \RestUntil function (to fill the tuba part, for example, with rests
until the next tempo change) so we might change that someday.

> Keith wrote:
>> I think we need the tremolo-type property to hold the 32 from input c2:32
>
> On TremoloEvent, yes. But do we need it on TremoloRepeatedMusic? I
> suppose it might be handy for tweaking.

I see.  It is not very likely that anyone refers to tremolo-type of the Music,
so possible tweaking does not seem enough reason to keep it.

[dak, 2013-12-22 22:41:00]

On 2013/12/22 22:01:25, Keith wrote:
> On Sun, 22 Dec 2013 11:45:49 -0800, Devon Schudy <mailto:dschudy@gmail.com>
wrote:
> 
> > Unfolding during iteration doesn't extend well to voltas, because it
> > changes their length. Even if the length is updated, the old length
> > has already been used by the parent iterator to compute moments for
> > subsequent events. So unless this can be changed,
> 
> Okay.  That eager evaluation of lengths by the iterators also frustrated my
> attempt at a \RestUntil function (to fill the tuba part, for example, with
rests
> until the next tempo change) so we might change that someday.

The lyric-combine-music-iterator seems to get along with that
preevaluation just fine, doesn't it?  Well, at least if one wants to
call what it does "just fine": no idea whether things like
http://code.google.com/p/lilypond/issues/detail?id=2010 could be due
to that.