Added vowel transitions for lyrics

A vowel transition may be drawn as an arrow from one syllable to another, and indicates a gradual change of vowel (see Gould pp. 452--453).

Input with the command \vowelTransition between lyric syllables.

[davidsg, 2020-03-08 14:38:25]

I've been working on this feature I would like to propose for LilyPond, and I
wonder how I should proceed (request feedback on users group, discussion here,
or something else?). I'm aware of being very much a beginner here, but I'm
tentatively posting this hoping for feedback either on the process or the code
itself.

I do have some questions:
- Copyright notices at the top of new files: Should I always be adding my own
name (as I have done so far), even though the contents is based on existing
code? I'm assuming the fact that there _is_ a notice in the files is the main
goal, rather than the contents itself. Of course, I don't want to be taking
'credit' for code written by others, but I also don't want to be attributing
anything to them that they wouldn't want to touch with a ten foot pole.
- Everything is bundled together here - the feature itself, regtests and docs.
Should this be broken apart into separate commits?

Thanks!

[lemzwerg, 2020-03-08 14:58:24]

Looks very nice, thanks!

I must admit that I've never seen such a feature before, so I can't really
comment on the actual implementation; my nits are just to improve the
documentation.

However, I wonder why you call this transition *line* and not transition *arrow*
...

https://codereview.appspot.com/565750043/diff/553650043/Documentation/notatio...
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/565750043/diff/553650043/Documentation/notatio...
Documentation/notation/vocal.itely:871: (drawn as an arrows), which are entered
as @samp{ -> } between
Please use @samp{->}.  The additional white space doesn't make sense in a
paragraph.

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
File input/regression/lyric-transition-broken.ly (right):

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-broken.ly:5: texidoc = "Lyric transitions run
to the end of the line if it
s/Lyric transitions run/A lyric transition runs/

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-broken.ly:7: the note on the next line.
Transition lines are printed at the
Two spaces after the full stop, please.

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-broken.ly:17: \new Voice =A  {
s/=A/= "A"/

I think it's better to always put identifiers into double quotes.

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-broken.ly:22: \context Lyrics \lyricsto A { a
-> a -> ha }
s/A/"A"/

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
File input/regression/lyric-transition-padding.ly (right):

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-padding.ly:3: texidoc = "Padding does not
cause LyricTransitions to become
@code{LyricTransition}s

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-padding.ly:5: leaving the transition line at
minimum-length."
@code{minimum-length}

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
File input/regression/lyric-transition-right-margin.ly (right):

https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyri...
input/regression/lyric-transition-right-margin.ly:5: that the transition can be
drawn at minimum-length."
@code{minimum-length}

[hanwenn, 2020-03-08 15:21:22]

I think it would be better to do this as something that doesn't require special
syntax at first, ie. some identifier. 

We can always add special syntax later, if this becomes a very popular feature.

Also, it looks like the transition-engraver is almost a literal copy of the
hyphen engraver. Wouldn't this be much simpler if you'd implement the transition
as a layout tweak to a hyphen? You'd get something like:

  vowelTransition = \once \override LyricVoice.LyricHyphen.style = #'transition

  a \vowelTranstion -- b

[hanwenn, 2020-03-08 15:25:08]

https://codereview.appspot.com/565750043/diff/553650043/lily/lyric-transition.cc
File lily/lyric-transition.cc (right):

https://codereview.appspot.com/565750043/diff/553650043/lily/lyric-transition...
lily/lyric-transition.cc:103: As r is a fresh rod, we can set distance_ with no
complication.
this looks like a copy & paste from spanner.cc ? Do you really need this?

[davidsg, 2020-03-08 20:09:22]

Renamed 'transition line' to 'transition arrow' and documentation fixes.

[davidsg, 2020-03-08 20:32:03]

On 2020/03/08 15:21:22, hanwenn wrote:
> I think it would be better to do this as something that doesn't require
special
> syntax at first, ie. some identifier. 
> 

One of my motivations for introducing '->' is that it keeps the lyrics clean and
easy to read. Music that uses vowel transitions can have a _lot_ of them.

> Also, it looks like the transition-engraver is almost a literal copy of the
> hyphen engraver. Wouldn't this be much simpler if you'd implement the
transition
> as a layout tweak to a hyphen?

I thought it might be best to separate transitions from hyphens, and use the
line-spanner-interface to draw the arrow. The requirements for spacing are
slightly different (transitions should never be omitted even in tight spacing),
and also some other properties differ (e.g. minimum-space). So even though
transitions are similar in many ways to hyphens I wonder if they are distinct
enough to be separated. But I will certainly look into tweaking hyphens instead,
as -- as you have pointed out -- there is currently a lot of duplicated code.

Thank you both for the feedback! I'll have another go at this.

[dan_faithful.be, 2020-03-08 23:44:43]

On Mar 8, 2020, at 11:21, hanwenn@gmail.com wrote:
> 
> I think it would be better to do this as something that doesn't require
> special syntax at first, ie. some identifier. 
> 
> We can always add special syntax later, if this becomes a very popular
> feature.

+1.  In the meantime, the minority of users who want an abbreviation can define
one, right?

> Also, it looks like the transition-engraver is almost a literal copy of
> the hyphen engraver. Wouldn't this be much simpler if you'd implement
> the transition as a layout tweak to a hyphen? You'd get something like:
> 
>  vowelTransition = \once \override LyricVoice.LyricHyphen.style =
> #'transition
> 
>  a \vowelTranstion — b

I agree that lots of duplication is something that should be avoided, but so is
the conflation of style and meaning.  A lyric hyphen separates syllables; this
arrow thing means something more.

It reminds me of the abuse of rehearsal marks for several categories of
instructions.  You build up a body of work full expedient little tricks; then
one day you want to move all your "D.C." marks, but not your rehearsal marks, to
the other side of the staff in certain scores; and then you learn what a mess it
really was from the beginning.

I haven't looked at the code, but maybe factoring out shared functions would
help reduce duplication.
— 
Dan

[hanwenn, 2020-03-09 08:42:43]

On Mon, Mar 9, 2020 at 12:44 AM Dan Eble <dan@faithful.be> wrote:
> I agree that lots of duplication is something that should be avoided, but so
is the conflation of style and meaning.  A lyric hyphen separates syllables;
this arrow thing means something more.

Can you have a hyphen and a transition between two syllables at the
same time? If not, that suggests that they are two variations of
essentially the same thing.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[dan_faithful.be, 2020-03-10 00:41:50]

On Mar 9, 2020, at 04:42, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> 
> On Mon, Mar 9, 2020 at 12:44 AM Dan Eble <dan@faithful.be> wrote:
>> I agree that lots of duplication is something that should be avoided, but so
is the conflation of style and meaning.  A lyric hyphen separates syllables;
this arrow thing means something more.
> 
> Can you have a hyphen and a transition between two syllables at the
> same time? If not, that suggests that they are two variations of
> essentially the same thing.

I would agree that a hyphen and a "transition line" are mutually exclusive ways
of demarcating syllables; but that doesn't make a "transition line" a kind of
hyphen any more than it makes a staccato mark a kind of legato mark or blue a
kind of red.
— 
Dan

[hanwenn, 2020-03-10 08:14:03]

On Tue, Mar 10, 2020 at 1:41 AM Dan Eble <dan@faithful.be> wrote:
>
> On Mar 9, 2020, at 04:42, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> >
> > On Mon, Mar 9, 2020 at 12:44 AM Dan Eble <dan@faithful.be> wrote:
> >> I agree that lots of duplication is something that should be avoided, but
so is the conflation of style and meaning.  A lyric hyphen separates syllables;
this arrow thing means something more.
> >
> > Can you have a hyphen and a transition between two syllables at the
> > same time? If not, that suggests that they are two variations of
> > essentially the same thing.
>
> I would agree that a hyphen and a "transition line" are mutually exclusive
ways of demarcating syllables; but that doesn't make a "transition line" a kind
of hyphen any more than it makes a staccato mark a kind of legato mark or blue a
kind of red.

In LilyPond All articulation marks (staccato, portato, staccatissimo)
are Script grobs, and they use identical code, both in the engravers
and the grob formatting.

I never said that blue is a kind of red. I said "two variations of
essentially the same thing." Blue and red are both colors, so they
could be implemented in terms of a generic 'color' type.

From a music-semantical perspective, hyphens and transitions may be
quite different, but from the typographical perspective, they really
seem quite similar, which means that they can share a lot of code, up
to and including the Grob name and the engraver instance producing
them.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[dak, 2020-03-10 09:33:45]

Han-Wen Nienhuys <hanwenn@gmail.com> writes:

> On Tue, Mar 10, 2020 at 1:41 AM Dan Eble <dan@faithful.be> wrote:
>>
>> On Mar 9, 2020, at 04:42, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>> >
>> > On Mon, Mar 9, 2020 at 12:44 AM Dan Eble <dan@faithful.be> wrote:
>> >> I agree that lots of duplication is something that should be
>> >> avoided, but so is the conflation of style and meaning.  A lyric
>> >> hyphen separates syllables; this arrow thing means something
>> >> more.
>> >
>> > Can you have a hyphen and a transition between two syllables at the
>> > same time? If not, that suggests that they are two variations of
>> > essentially the same thing.
>>
>> I would agree that a hyphen and a "transition line" are mutually
>> exclusive ways of demarcating syllables; but that doesn't make a
>> "transition line" a kind of hyphen any more than it makes a staccato
>> mark a kind of legato mark or blue a kind of red.
>
> In LilyPond All articulation marks (staccato, portato, staccatissimo)
> are Script grobs, and they use identical code, both in the engravers
> and the grob formatting.

Which can end up a nuisance if you want to change some, but not all.

> I never said that blue is a kind of red. I said "two variations of
> essentially the same thing." Blue and red are both colors, so they
> could be implemented in terms of a generic 'color' type.
>
> From a music-semantical perspective, hyphens and transitions may be
> quite different, but from the typographical perspective, they really
> seem quite similar, which means that they can share a lot of code, up
> to and including the Grob name and the engraver instance producing
> them.

It's worth noting that sharing the engraver does not necessitate sharing
the Grob name (and respective defaults): different grobs can share an
interface, and it is interfaces that an engraver triggers on with regard
to the typesetting.  In a similar vein, engravers react to event
classes rather than event types.

So code sharing does not necessitate item sharing.

-- 
David Kastrup

[davidsg, 2020-03-10 09:59:29]

Revision following review

[davidsg, 2020-03-10 10:05:46]

On 2020/03/10 09:59:29, davidsg wrote:
> Revision following review

Removed -> as special syntax, and uses instead the command \vowelTransition.
    
Transitions are still separated from hyphens, which leaves transition-engraver
_almost_ a duplicate of hyphen-engraver. As transitions are printed with
line-spanner, the properties are quite different to hyphens. Will it get messy
if the properties are merged into LyricHyphens, or doesn't this matter?
    
Uses ly:spanner::set-spacing-rods rather than
ly:lyric-transition::set-spacing-rods, so the Lyric_transition struct has been
removed completely.
    
Added (somewhat clumsily named - any suggestions?) properties to spanner, for
spacing required for transitions: minimum-length-add-bounds and
minimum-length-add-padding. If true, the extent of bounds protrusion into the
spacing rod, or padding, is in effect added to minimum-length for spacing.

[hanwenn, 2020-03-10 18:12:13]

On 2020/03/10 10:05:46, davidsg wrote:
> On 2020/03/10 09:59:29, davidsg wrote:
> > Revision following review
> 
> Removed -> as special syntax, and uses instead the command \vowelTransition.
>     
> Transitions are still separated from hyphens, which leaves transition-engraver
> _almost_ a duplicate of hyphen-engraver. As transitions are printed with
> line-spanner, the properties are quite different to hyphens. Will it get messy
> if the properties are merged into LyricHyphens, or doesn't this matter?

You don't have to merge into LyricHyphen. You could do something like

if (ev_)
  if (is_transition (ev_)) {
    hyphen_ = make_spanner ("LyricTransition", ev_->self_scm ());
  } else {
    hyphen_ = make_spanner ("LyricHyphen", ev_->self_scm ());
  }
  

> Uses ly:spanner::set-spacing-rods rather than
> ly:lyric-transition::set-spacing-rods, so the Lyric_transition struct has been
> removed completely.
>     
> Added (somewhat clumsily named - any suggestions?) properties to spanner, for
> spacing required for transitions: minimum-length-add-bounds and
> minimum-length-add-padding. If true, the extent of bounds protrusion into the
> spacing rod, or padding, is in effect added to minimum-length for spacing.

[davidsg, 2020-03-11 00:32:26]

Removed transition-engraver.cc

[davidsg, 2020-03-11 00:34:32]

On 2020/03/11 00:32:26, davidsg wrote:
> Removed transition-engraver.cc

hyphen-engraver now also listens for transition-events, and makes transitions or
hyphens based on event class.

Also changed a couple of mentions of line to arrow in regtests.

[dan_faithful.be, 2020-03-11 02:02:20]

On Mar 10, 2020, at 04:13, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> I never said that blue is a kind of red.

True.  What you said was this:

> Wouldn't this be much simpler if you'd implement
> the transition as a layout tweak to a hyphen? You'd get something like:
> 
> vowelTransition = \once \override LyricVoice.LyricHyphen.style =
> #'transition
> 
> a \vowelTranstion — b

Which I interpreted as suggesting that a vowel transition is a kind of hyphen.

Regards,
— 
Dan

[Dan Eble, 2020-03-11 02:30:04]

On 2020/03/11 00:34:32, davidsg wrote:
> hyphen-engraver now also listens for transition-events, and makes transitions
or
> hyphens based on event class.

I know some people hate talking about names, but can we talk about this one? 
Think of the kinds of events that a "transition event" might properly refer to:
pretty much any.  The essence of this transition is that it is gradual rather
than abrupt, correct?  How about calling it a gradual-vowel-change-event?  Come
to think of it, is this only for vowels, or would it be appropriate to use it
for, say, sh -> ss?

Second point: Gould writes, "The arrow also shows the length of the transition
between one vowel and the next..." (p. 452).  It sounds (and the examples make
it look) like something with a specific starting moment and duration, more like
a syllable of its own than like a hyphen, as I see it.

[lemzwerg, 2020-03-11 07:21:04]

Some more nits :-)

https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-g...
File Documentation/music-glossary.tely (right):

https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-g...
Documentation/music-glossary.tely:415: * transition arrow::
I think it would be better to replace 'transition arrow' in the glossary with
'vowel transition'.  How a vowel transition gets represented is a technical
detail.

https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-g...
Documentation/music-glossary.tely:7983: D: ?,
A proper German translation of 'vowel transition' is 'Vokalwechsel'.

https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyri...
File input/regression/lyric-transition-padding.ly (right):

https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyri...
input/regression/lyric-transition-padding.ly:4: shorter than minimum-length. 
Instead, space is added if necessary
@code{minimum-length}

https://codereview.appspot.com/565750043/diff/557610044/lily/spanner.cc
File lily/spanner.cc (right):

https://codereview.appspot.com/565750043/diff/557610044/lily/spanner.cc#newco...
lily/spanner.cc:380: SCM add_bounds = me->get_property
("minimum-length-add-bounds");
Are this and the next property internal ones?  If yes, please document them as
such.  Otherwise, please add a regression test to demonstrate how they are used.
 This ensures that your code gets covered as much as possible.

[davidsg, 2020-03-11 09:08:25]

On 2020/03/11 02:30:04, Dan Eble wrote:
> How about calling it a gradual-vowel-change-event?  Come
> to think of it, is this only for vowels, or would it be appropriate to use it
> for, say, sh -> ss?

Gould talks specifically about vowels, but I don't see any reason why it
shouldn't apply to sh->ss, or even from vowel to closed mouth. How about
gradual-syllable-change-event?
 
> It sounds (and the examples make
> it look) like something with a specific starting moment and duration, more
like
> a syllable of its own than like a hyphen, as I see it.
The arrow could start from an empty "" syllable, so not directly at a (visible)
syllable. If this is acceptable I'll add a regression test for this situation.

[lemzwerg, 2020-03-11 10:15:04]

> Gould talks specifically about vowels, but I don't see
> any reason why it shouldn't apply to sh->ss, or even
> from vowel to closed mouth. How about
> gradual-syllable-change-event?

Mhmm, what about simply `vowel-transition-event`?  IMHO it's not necessary to
invent new names.

[davidsg, 2020-03-12 12:06:06]

Rename to vowel-transition-event

[davidsg, 2020-03-12 12:08:03]

On 2020/03/12 12:06:06, davidsg wrote:
> Rename to vowel-transition-event

Trying again with the name 'vowel transition' (sustained consonants are
mentioned explicitly in the docs). Perhaps this is an OK compromise?
    
Updated docs accordingly and added a couple of regtests, including one
documenting properties minimum-length-includes-bounds and
minimum-length-includes-padding.

[lemzwerg, 2020-03-12 17:38:06]

LGTM now, thanks!

https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-type...
File scm/define-music-types.scm (right):

https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-type...
scm/define-music-types.scm:685: . ((description . "A transition between lyric
syllables.")
Maybe s/transition/vowel transition/  ?

[davidsg, 2020-03-13 13:48:10]

Corrected VowelTransitionEvent description

[davidsg, 2020-03-13 13:50:22]

https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-type...
File scm/define-music-types.scm (right):

https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-type...
scm/define-music-types.scm:685: . ((description . "A transition between lyric
syllables.")
On 2020/03/12 17:38:06, lemzwerg wrote:
> Maybe s/transition/vowel transition/  ?

Done.

[davidsg, 2020-03-14 07:44:19]

Added check to prevent issue if some bounds properties were not available.

[davidsg, 2020-03-15 04:50:20]

Replaced property 'minimum-length-includes-padding' with
'minimum-length-correction'.

[hanwenn, 2020-03-15 15:00:50]

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notatio...
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notatio...
Documentation/notation/vocal.itely:894: ah \vowelTransition _ _ _ _
is it vowel transtition or lyric transition?

make the grob name consistent (grob VowelTransition, or identifier
\lyricTransition)

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc
File lily/spanner.cc (right):

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:368: w += -d * r->item_drul_[d]->extent (r->item_drul_[d],
X_AXIS)[-d];
this looks suspect. If you translate either items (relative to the paper-column
it is attached to), then this will leave the rod alone. Shouldn't the extent be
relative to the item' paper column?

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:393: SCM min_length_correction = me->get_property
("minimum-length-correction");
the behavior you add is specific to your new feature, so I think it would be
best to avoid changing spanner.cc (and at the same time, avoiding wholesale
copies of this spanner.cc code)

Could you summarize for me what the behavior should be? Sorry for being a little
dense here.  (And how should they behave across line breaks?)

It is strange to introduce a minimum-length-correction, when you could introduce
a callback for minimum-length that calculates a different value.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:414: r.distance_ -= bounds_protrusion (&r);
this is weird.  You're using r.item_drul_ here, but then in the next line, you
overwrite r.item_drul_. What's going on?

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:421: r.distance_ += bounds_protrusion (&r);
and now you're doing += after doing -=  ?

https://codereview.appspot.com/565750043/diff/553710043/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/565750043/diff/553710043/scm/define-grobs.scm#...
scm/define-grobs.scm:1471: (minimum-length-correction .
,ly:spanner::calc-padding-correction)
The function calc-xxx is usually used for calculating the xxx property, so the
naming is off.

[hanwenn, 2020-03-15 15:10:36]

I tried looking at your patch, but:

branch 'issue565750043_553710043' set up to track remote branch 'master' from
'origin'.
Switched to a new branch 'issue565750043_553710043'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24767  100 24767    0     0  57331      0 --:--:-- --:--:-- --:--:-- 57198
error: patch failed: Documentation/changes.tely:62
error: Documentation/changes.tely: patch does not apply

Can you rebase this on top of master?

[davidsg, 2020-03-15 15:56:49]

Rebased on top of current master.

[davidsg, 2020-03-20 11:37:44]

Revisions following review

[davidsg, 2020-03-20 11:48:02]

Revisions following review

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notatio...
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notatio...
Documentation/notation/vocal.itely:894: ah \vowelTransition _ _ _ _
On 2020/03/15 15:00:49, hanwenn wrote:
> is it vowel transtition or lyric transition?
> 
> make the grob name consistent (grob VowelTransition, or identifier
> \lyricTransition)

Done - all should now be VowelTransition.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc
File lily/spanner.cc (right):

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:368: w += -d * r->item_drul_[d]->extent (r->item_drul_[d],
X_AXIS)[-d];
On 2020/03/15 15:00:49, hanwenn wrote:
> this looks suspect. If you translate either items (relative to the
paper-column
> it is attached to), then this will leave the rod alone. Shouldn't the extent
be
> relative to the item' paper column?

This does seem currently to be working as I expect it to, i.e., for long
syllables we find the value to correct Rod::distance_, so that the _drawn
length_ of the vowel transition _doesn't change_ for wide syllables. Although
perhaps I have overlooked situations where this will be incorrect. I tried the
following:
{
  Paper_column *pc = r->item_drul_[d]->get_column ();
  w += -d * r->item_drul_[d]->extent (pc, X_AXIS)[-d];
}

This doesn't work correctly - see e.g. regtest
vowel-transition-offset-syllable.ly. The drawn length of the transitions change
depending on the width of the syllables. The length _shouldn't_ change, rather
the width of the syllable should be corrected for.

So I've left this unchanged for now, but any pointers are much appreciated if it
still doesn't look right.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:393: SCM min_length_correction = me->get_property
("minimum-length-correction");
On 2020/03/15 15:00:50, hanwenn wrote:
> the behavior you add is specific to your new feature, so I think it would be
> best to avoid changing spanner.cc (and at the same time, avoiding wholesale
> copies of this spanner.cc code)
> 
I've put the vowel transition specific spacing code in back into
vowel-transition.cc, so spanner.cc is untouched. This does duplicate a fair bit
from spanner.cc - I don't know how to avoid this.

> Could you summarize for me what the behavior should be? Sorry for being a
little
> dense here.  (And how should they behave across line breaks?)
> 
Certainly: Vowel transitions should never be omitted due to tight spacing, as
their musical meaning would be lost. Space should instead be added so that the
transition can be drawn (at least) at minimum-length. They extend to the end of
the system if the transition continues on the next system or ends on the first
note on the next system. By default they do not draw on the next system when the
transition ends on the first note on the system.

> It is strange to introduce a minimum-length-correction, when you could
introduce
> a callback for minimum-length that calculates a different value. 
>
The minimum-length-correction property has been removed.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newco...
lily/spanner.cc:414: r.distance_ -= bounds_protrusion (&r);
On 2020/03/15 15:00:50, hanwenn wrote:
> this is weird.  You're using r.item_drul_ here, but then in the next line, you
> overwrite r.item_drul_. What's going on?  

I've rewritten this section - hopefully it looks more sensible.

https://codereview.appspot.com/565750043/diff/553710043/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/565750043/diff/553710043/scm/define-grobs.scm#...
scm/define-grobs.scm:1471: (minimum-length-correction .
,ly:spanner::calc-padding-correction)
On 2020/03/15 15:00:50, hanwenn wrote:
> The function calc-xxx is usually used for calculating the xxx property, so the
> naming is off.

Property has been removed.

[hanwenn, 2020-03-24 21:46:30]

LGTM

Can you verify that the patch description is correct? The current version
doesn't use -> anymore.

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc
File lily/vowel-transition.cc (right):

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
lily/vowel-transition.cc:37: SCM num_length = me->get_property
("minimum-length");
num suggests a number.

minimum_length ?

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
lily/vowel-transition.cc:137: w += -d * r->item_drul_[d]->extent
(r->item_drul_[d], X_AXIS)[-d];
this still looks strange, but if it's problem, it'll be contained within the
vowel-transition code, which is acceptable.

[davidsg, 2020-03-25 08:31:26]

Corrections following review.

[davidsg, 2020-03-25 08:38:23]

Restored accidentally deleted empty line.

[davidsg, 2020-03-25 08:43:25]

Apologies for the noise for the last two patch sets.

[davidsg, 2020-03-25 08:45:31]

Patch description in the issue tracker has been updated.

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc
File lily/vowel-transition.cc (right):

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
lily/vowel-transition.cc:37: SCM num_length = me->get_property
("minimum-length");
On 2020/03/24 21:46:30, hanwenn wrote:
> num suggests a number.
> 
> minimum_length ?

Done.

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
lily/vowel-transition.cc:137: w += -d * r->item_drul_[d]->extent
(r->item_drul_[d], X_AXIS)[-d];
On 2020/03/24 21:46:29, hanwenn wrote:
> this still looks strange, but if it's problem, it'll be contained within the
> vowel-transition code, which is acceptable.

Acknowledged, thanks.

[hanwenn, 2020-03-27 20:22:28]

On 2020/03/25 08:45:31, davidsg wrote:
> Patch description in the issue tracker has been updated.
> 
>
https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc
> File lily/vowel-transition.cc (right):
> 
>
https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
> lily/vowel-transition.cc:37: SCM num_length = me->get_property
> ("minimum-length");
> On 2020/03/24 21:46:30, hanwenn wrote:
> > num suggests a number.
> > 
> > minimum_length ?
> 
> Done.
> 
>
https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition...
> lily/vowel-transition.cc:137: w += -d * r->item_drul_[d]->extent
> (r->item_drul_[d], X_AXIS)[-d];
> On 2020/03/24 21:46:29, hanwenn wrote:
> > this still looks strange, but if it's problem, it'll be contained within the
> > vowel-transition code, which is acceptable.
> 
> Acknowledged, thanks.

this went into master. Can you close the review?