Re: Added transition lines for lyrics (issue 565750043 by address@hidden)

[davidgrant . no]

Revisions following review


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

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notation/vocal.itely#newcode894
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#newcode368
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#newcode393
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#newcode414
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#newcode1471
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.

https://codereview.appspot.com/565750043/