Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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.cc#newcode37
> 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.cc#newcode137
> 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?
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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.cc#newcode37
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.cc#newcode137
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.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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.cc#newcode37
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.cc#newcode137
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.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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 ();
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/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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 TimeTime Time Current Dload Upload Total SpentLeft Speed 100 24767 100 247670 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? https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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 _ _ _ _
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#newcode368
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#newcode393
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#newcode414
lily/spanner.cc:414: r.distance_ -= bounds_protrusion ();
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#newcode421
lily/spanner.cc:421: r.distance_ += bounds_protrusion ();
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#newcode1471
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.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm File scm/define-music-types.scm (right): https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm#newcode685 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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
LGTM now, thanks! https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm File scm/define-music-types.scm (right): https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm#newcode685 scm/define-music-types.scm:685: . ((description . "A transition between lyric syllables.") Maybe s/transition/vowel transition/ ? https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
> 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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
El 11/3/20 a las 3:30, nine.fierce.ball...@gmail.com escribió: 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? From an user POV, the most natural name in my opinion is LyricsArrow; it shouldn't be limited by its name to "vowels", "gradual", "syllable" or even "change". Just lyrics, and it looks like an arrow. -- Francisco Vila, Ph.D. - Badajoz (Spain) paconet.org , lilypond.es
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
Some more nits :-)
https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-glossary.tely
File Documentation/music-glossary.tely (right):
https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-glossary.tely#newcode415
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-glossary.tely#newcode7983
Documentation/music-glossary.tely:7983: D: ?,
A proper German translation of 'vowel transition' is 'Vokalwechsel'.
https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyric-transition-padding.ly
File input/regression/lyric-transition-padding.ly (right):
https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyric-transition-padding.ly#newcode4
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#newcode380
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.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
On Mar 10, 2020, at 04:13, Han-Wen Nienhuys 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
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
Han-Wen Nienhuys writes: > On Tue, Mar 10, 2020 at 1:41 AM Dan Eble wrote: >> >> On Mar 9, 2020, at 04:42, Han-Wen Nienhuys wrote: >> > >> > On Mon, Mar 9, 2020 at 12:44 AM Dan Eble 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
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
On Tue, Mar 10, 2020 at 1:41 AM Dan Eble wrote: > > On Mar 9, 2020, at 04:42, Han-Wen Nienhuys wrote: > > > > On Mon, Mar 9, 2020 at 12:44 AM Dan Eble 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 - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
On Mar 9, 2020, at 04:42, Han-Wen Nienhuys wrote: > > On Mon, Mar 9, 2020 at 12:44 AM Dan Eble 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
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
On Mon, Mar 9, 2020 at 12:44 AM Dan Eble 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 - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
On Mar 8, 2020, at 11:21, hanw...@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
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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 https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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.cc#newcode103 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? https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
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/notation/vocal.itely
File Documentation/notation/vocal.itely (right):
https://codereview.appspot.com/565750043/diff/553650043/Documentation/notation/vocal.itely#newcode871
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/lyric-transition-broken.ly
File input/regression/lyric-transition-broken.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly#newcode5
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/lyric-transition-broken.ly#newcode7
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/lyric-transition-broken.ly#newcode17
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/lyric-transition-broken.ly#newcode22
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/lyric-transition-padding.ly
File input/regression/lyric-transition-padding.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-padding.ly#newcode3
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/lyric-transition-padding.ly#newcode5
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/lyric-transition-right-margin.ly
File input/regression/lyric-transition-right-margin.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-right-margin.ly#newcode5
input/regression/lyric-transition-right-margin.ly:5: that the transition
can be drawn at minimum-length."
@code{minimum-length}
https://codereview.appspot.com/565750043/
Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
Reviewers: , Message: 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! Description: Added transition lines for lyrics A transition line (my suggested term) is drawn as an arrow from one syllable to another, and indicates a gradual change of vowel (see Gould pp. 452--453). I propose '->' to be used between lyric syllables (similarly to hyphens are input). Please review this at https://codereview.appspot.com/565750043/ Affected files (+466, -59 lines): M Documentation/changes.tely M Documentation/music-glossary.tely M Documentation/notation/vocal.itely A input/regression/lyric-transition.ly A input/regression/lyric-transition-broken.ly A input/regression/lyric-transition-offset-syllable.ly A input/regression/lyric-transition-padding.ly A input/regression/lyric-transition-right-margin.ly A + lily/include/lyric-transition.hh M lily/lexer.ll A lily/lyric-transition.cc M lily/parser.yy A + lily/transition-engraver.cc M ly/engraver-init.ly M scm/define-event-classes.scm M scm/define-grobs.scm M scm/define-music-display-methods.scm M scm/define-music-types.scm M scm/safe-lily.scm
