Issue 4442082: Allows glissandi between chords (Closed)
|
Created:
13 years ago by MikeSol Modified:
4 years, 3 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAllows glissandi between chords
Patch Set 1 #Patch Set 2 : Adds regtest and glissando-index property. #
Total comments: 7
MessagesTotal messages: 19
It gets the job done on this tiny example. I'm guessing this breaks something somewhere (haven't run the regtests yet), but I figured I'd get it out there in case any of you have time to test it, run regtests, try to break it, make suggestions, etc..
Sign in to reply to this message.
The test file I've been using is:
\relative c' {
c1 \glissando g'
c,1 \glissando s1 g'
<c, e>1 \glissando <g' b>
<c, e>1 \glissando s1 <g' b>
\set glissandoMap = #'((0 . 1) (1 . 0))
<c, g'>1 \glissando s1 <d a'>
\set glissandoMap = #'((0 . 0) (0 . 1) (0 . 2))
c1 \glissando s1 <d f a>
\set glissandoMap = #'((2 . 0) (1 . 0) (0 . 0))
<d f a>1 \glissando s1 c
}
Sign in to reply to this message.
Regtest uploaded and glissando-index property added to allow for the tweaking of individual Glissando grobs.
Sign in to reply to this message.
Excellent work! A couple of comments below. http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... File scm/define-context-properties.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... scm/define-context-properties.scm:260: (glissandoMap ,list? "A map in the form of '((source1 . target1) I think we ought to enable the current behavior (one glissando line per chord, instead of one per note) to be kept (so we won't break existing scores). Perhaps a value of #f should be used to indicate keeping the old behavior. http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.scm File scm/define-grob-interfaces.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.sc... scm/define-grob-interfaces.scm:103: '(glissando-index)) As I mention elsewhere, I don't think this is a user property, so needn't be included in the interface. http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.sc... scm/define-grob-properties.scm:412: (glissando-index ,integer? "The index of a glissando in its note It seems to me like this should be an internal grob property, rather than a user grob property, and that it need not be part of an interface. I can't imagine how it ought to be tweaked by the user.
Sign in to reply to this message.
On Apr 26, 2011, at 7:30 AM, Carl.D.Sorensen@gmail.com wrote: > Excellent work! Thanks! > A couple of comments below. > > http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... > File scm/define-context-properties.scm (right): > > http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... > scm/define-context-properties.scm:260: (glissandoMap ,list? "A map in > the form of '((source1 . target1) > I think we ought to enable the current behavior (one glissando line per > chord, instead of one per note) to be kept (so we won't break existing > scores). Perhaps a value of #f should be used to indicate keeping the > old behavior. > The old behavior can be achieved via: \set glissandoMap = #'((0 . 0)) What would be the appropriate .ly file in ly/ to put this in? > http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.scm > File scm/define-grob-interfaces.scm (right): > > http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.sc... > scm/define-grob-interfaces.scm:103: '(glissando-index)) > As I mention elsewhere, I don't think this is a user property, so > needn't be included in the interface. > See below. > http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.scm > File scm/define-grob-properties.scm (right): > > http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.sc... > scm/define-grob-properties.scm:412: (glissando-index ,integer? "The > index of a glissando in its note > It seems to me like this should be an internal grob property, rather > than a user grob property, and that it need not be part of an interface. > I can't imagine how it ought to be tweaked by the user. Even if a property is moved to an internal property in define-grob-properties.scm, doesn't it still need to be declared in an interface so that internals.texi may be written? Cheers, MS
Sign in to reply to this message.
On 4/26/11 5:55 AM, "mike@apollinemike.com" <mike@apollinemike.com> wrote: > On Apr 26, 2011, at 7:30 AM, Carl.D.Sorensen@gmail.com wrote: > >> File scm/define-grob-properties.scm (right): >> >> http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.sc >> m#newcode412 >> scm/define-grob-properties.scm:412: (glissando-index ,integer? "The >> index of a glissando in its note >> It seems to me like this should be an internal grob property, rather >> than a user grob property, and that it need not be part of an interface. >> I can't imagine how it ought to be tweaked by the user. > > Even if a property is moved to an internal property in > define-grob-properties.scm, doesn't it still need to be declared in an > interface so that internals.texi may be written? As far as I can see, most of the internal properties don't have interfaces. And they don't show up in internals.texi. I think I'm OK with this as a policy -- internal properties aren't to be changed by the user. Thanks, Carl
Sign in to reply to this message.
On Apr 26, 2011, at 9:52 AM, Carl Sorensen wrote: > On 4/26/11 5:55 AM, "mike@apollinemike.com" <mike@apollinemike.com> wrote: > >> On Apr 26, 2011, at 7:30 AM, Carl.D.Sorensen@gmail.com wrote: >> > >>> File scm/define-grob-properties.scm (right): >>> >>> http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.sc >>> m#newcode412 >>> scm/define-grob-properties.scm:412: (glissando-index ,integer? "The >>> index of a glissando in its note >>> It seems to me like this should be an internal grob property, rather >>> than a user grob property, and that it need not be part of an interface. >>> I can't imagine how it ought to be tweaked by the user. >> >> Even if a property is moved to an internal property in >> define-grob-properties.scm, doesn't it still need to be declared in an >> interface so that internals.texi may be written? > > As far as I can see, most of the internal properties don't have interfaces. > And they don't show up in internals.texi. > > I think I'm OK with this as a policy -- internal properties aren't to be > changed by the user. Hey Carl, Maybe we're not talking about the same thing...I just want to make sure that I'm picking up what you're putting down. I did a quick git grep on various internal properties and found interfaces for all of them. To wit: keep-alive-with is in the hara-kiri-group-spanner-interface spacing-wishes is in the spaceable-grob-interface bounded-by-me is in the paper-column-interface I get the sense that all of the internal properties in define-grob-properties.scm belong to some interface. When you say "most of the internal properties don't have interfaces," are you referring to something else? Cheers, MS
Sign in to reply to this message.
On Apr 26, 2011, at 7:55 AM, mike@apollinemike.com wrote: > On Apr 26, 2011, at 7:30 AM, Carl.D.Sorensen@gmail.com wrote: > >> Excellent work! > > Thanks! > >> A couple of comments below. >> >> http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... >> File scm/define-context-properties.scm (right): >> >> http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties... >> scm/define-context-properties.scm:260: (glissandoMap ,list? "A map in >> the form of '((source1 . target1) >> I think we ought to enable the current behavior (one glissando line per >> chord, instead of one per note) to be kept (so we won't break existing >> scores). Perhaps a value of #f should be used to indicate keeping the >> old behavior. >> > > The old behavior can be achieved via: > > \set glissandoMap = #'((0 . 0)) > > What would be the appropriate .ly file in ly/ to put this in? Alternatively, we could write a convert-ly rule that changes all \glissando to \set glissandoMap = #'((0 . 0)) \glissando \unset glissandoMap. I think that the majority of users who write glissandi between chords will want full-chord glissandi. So, it seems that #'() should be the default value for glissandoMap (as it is in the proposed patch). Thoughts? Cheers, MS
Sign in to reply to this message.
On 4/26/11 11:00 AM, "mike@apollinemike.com" <mike@apollinemike.com> wrote: > On Apr 26, 2011, at 7:55 AM, mike@apollinemike.com wrote: > >> On Apr 26, 2011, at 7:30 AM, Carl.D.Sorensen@gmail.com wrote: >> >> The old behavior can be achieved via: >> >> \set glissandoMap = #'((0 . 0)) >> >> What would be the appropriate .ly file in ly/ to put this in? > > Alternatively, we could write a convert-ly rule that changes all \glissando to > \set glissandoMap = #'((0 . 0)) \glissando \unset glissandoMap. > > I think that the majority of users who write glissandi between chords will > want full-chord glissandi. So, it seems that #'() should be the default value > for glissandoMap (as it is in the proposed patch). > If all notes should be part of the glissando, then I think we should *not* write a convert-ly rule. Rather, we should make sure that the new setting is noted in changes, and we should make sure the new behavior is documented. Thanks, Carl
Sign in to reply to this message.
On 2011/04/26 17:11:02, c_sorensen_byu.edu wrote: > On 4/26/11 11:00 AM, "mike@apollinemike.com" <mailto:mike@apollinemike.com> wrote: > > > On Apr 26, 2011, at 7:55 AM, mailto:mike@apollinemike.com wrote: > > > >> On Apr 26, 2011, at 7:30 AM, mailto:Carl.D.Sorensen@gmail.com wrote: > >> > >> The old behavior can be achieved via: > >> > >> \set glissandoMap = #'((0 . 0)) > >> > >> What would be the appropriate .ly file in ly/ to put this in? > > > > Alternatively, we could write a convert-ly rule that changes all \glissando to > > \set glissandoMap = #'((0 . 0)) \glissando \unset glissandoMap. > > > > I think that the majority of users who write glissandi between chords will > > want full-chord glissandi. So, it seems that #'() should be the default value > > for glissandoMap (as it is in the proposed patch). > > > If all notes should be part of the glissando, then I think we should *not* > write a convert-ly rule. Rather, we should make sure that the new setting > is noted in changes, and we should make sure the new behavior is documented. > > Thanks, > > Carl > > Hey Carl et al, Before I push this patch, are there any further questions/concerns regarding it? Specifically: (a) Are people all right with glissando'ed chords being default behavior (given how ties work in lilypond, this seems to make sense)? (b) With respect to my previous question about internal-properties, is it ok to have glissando-index in define-grob-properties.scm and to put it in the glissando-interface? If not, where should I put it? Thanks! ~Mike
Sign in to reply to this message.
Looks mostly good, but I second Carl's suggestions. http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... File input/regression/glissando-chord.ly (right): http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... input/regression/glissando-chord.ly:1: \version "2.13.61" 2.15.0. I don't think that this commit is going to be in the 2.13 branch.
Sign in to reply to this message.
On 4/27/11 5:00 PM, "mtsolo@gmail.com" <mtsolo@gmail.com> wrote: > On 2011/04/26 17:11:02, c_sorensen_byu.edu wrote: >> On 4/26/11 11:00 AM, "mike@apollinemike.com" > <mailto:mike@apollinemike.com> wrote: > >>> On Apr 26, 2011, at 7:55 AM, mailto:mike@apollinemike.com wrote: >>> >>>> On Apr 26, 2011, at 7:30 AM, mailto:Carl.D.Sorensen@gmail.com > wrote: >>>> >>>> The old behavior can be achieved via: >>>> >>>> \set glissandoMap = #'((0 . 0)) >>>> >>>> What would be the appropriate .ly file in ly/ to put this in? >>> >>> Alternatively, we could write a convert-ly rule that changes all > \glissando to >>> \set glissandoMap = #'((0 . 0)) \glissando \unset glissandoMap. >>> >>> I think that the majority of users who write glissandi between > chords will >>> want full-chord glissandi. So, it seems that #'() should be the > default value >>> for glissandoMap (as it is in the proposed patch). >>> >> If all notes should be part of the glissando, then I think we should > *not* >> write a convert-ly rule. Rather, we should make sure that the new > setting >> is noted in changes, and we should make sure the new behavior is > documented. > >> Thanks, > >> Carl > > > > Hey Carl et al, > > Before I push this patch, are there any further questions/concerns > regarding it? > Specifically: > > (a) Are people all right with glissando'ed chords being default > behavior (given how ties work in lilypond, this seems to make sense)? I'm fine with it, but I'm not a good reference -- my experience with music is too limited. > > (b) With respect to my previous question about internal-properties, is > it ok to have glissando-index in define-grob-properties.scm and to put > it in the glissando-interface? If not, where should I put it? Personally, I think it should be in define-grob-properties.scm as an internal property, and not in an interface. There are lots of examples of properties like that, as far as I can see. For example, least-squares-dy, maybe-loose, positioning-done, important-column-ranks, pure-Y-extent. In my mind, we don't have interfaces to things that the user couldn't set properly, and glissando-index is one of those. We can read without an interface, we just can't set without an interface. But I could easily be wrong on this. Thanks, Carl
Sign in to reply to this message.
On 4/28/11 6:35 AM, "Carl Sorensen" <c_sorensen@byu.edu> wrote: > On 4/27/11 5:00 PM, "mtsolo@gmail.com" <mtsolo@gmail.com> wrote: > >> On 2011/04/26 17:11:02, c_sorensen_byu.edu wrote: >>> On 4/26/11 11:00 AM, "mike@apollinemike.com" >> <mailto:mike@apollinemike.com> wrote: >> >>>> On Apr 26, 2011, at 7:55 AM, mailto:mike@apollinemike.com wrote: >>>> >>>>> On Apr 26, 2011, at 7:30 AM, mailto:Carl.D.Sorensen@gmail.com >> wrote: >>>>> >>>>> The old behavior can be achieved via: >>>>> >>>>> \set glissandoMap = #'((0 . 0)) >>>>> >>>>> What would be the appropriate .ly file in ly/ to put this in? >>>> >>>> Alternatively, we could write a convert-ly rule that changes all >> \glissando to >>>> \set glissandoMap = #'((0 . 0)) \glissando \unset glissandoMap. >>>> >>>> I think that the majority of users who write glissandi between >> chords will >>>> want full-chord glissandi. So, it seems that #'() should be the >> default value >>>> for glissandoMap (as it is in the proposed patch). >>>> >>> If all notes should be part of the glissando, then I think we should >> *not* >>> write a convert-ly rule. Rather, we should make sure that the new >> setting >>> is noted in changes, and we should make sure the new behavior is >> documented. >> >>> Thanks, >> >>> Carl >> >> >> >> Hey Carl et al, >> >> Before I push this patch, are there any further questions/concerns >> regarding it? >> Specifically: >> >> (a) Are people all right with glissando'ed chords being default >> behavior (given how ties work in lilypond, this seems to make sense)? > > I'm fine with it, but I'm not a good reference -- my experience with music > is too limited. > >> >> (b) With respect to my previous question about internal-properties, is >> it ok to have glissando-index in define-grob-properties.scm and to put >> it in the glissando-interface? If not, where should I put it? > > Personally, I think it should be in define-grob-properties.scm as an > internal property, and not in an interface. There are lots of examples of > properties like that, as far as I can see. For example, least-squares-dy, > maybe-loose, positioning-done, important-column-ranks, pure-Y-extent. In my > mind, we don't have interfaces to things that the user couldn't set > properly, and glissando-index is one of those. We can read without an > interface, we just can't set without an interface. > > But I could easily be wrong on this. In fact, I was wrong on this. Please ignore all my comments on the interface. Mike's approach is correct. Thanks, Carl
Sign in to reply to this message.
On Apr 28, 2011, at 8:22 AM, percival.music.ca@gmail.com wrote: > Looks mostly good, but I second Carl's suggestions. > > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > File input/regression/glissando-chord.ly (right): > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > input/regression/glissando-chord.ly:1: \version "2.13.61" > 2.15.0. I don't think that this commit is going to be in the 2.13 > branch. > > http://codereview.appspot.com/4442082/ Thanks. Pushed as 09f09c054a7c985424925605c237c78b9adb4047. We have our first 2.15.0 regtest. w00t! Cheers, MS
Sign in to reply to this message.
On Apr 28, 2011, at 8:22 AM, percival.music.ca@gmail.com wrote: > Looks mostly good, but I second Carl's suggestions. > > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > File input/regression/glissando-chord.ly (right): > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > input/regression/glissando-chord.ly:1: \version "2.13.61" > 2.15.0. I don't think that this commit is going to be in the 2.13 > branch. > > http://codereview.appspot.com/4442082/ Thanks. Pushed as 09f09c054a7c985424925605c237c78b9adb4047. We have our first 2.15.0 regtest. w00t! Cheers, MS
Sign in to reply to this message.
On 28 April 2011 14:13, mike@apollinemike.com <mike@apollinemike.com> wrote: > Thanks. Pushed as 09f09c054a7c985424925605c237c78b9adb4047. > > We have our first 2.15.0 regtest. w00t! Mike, you can't do this. There's no 2.15 branch yet, which means the version string will break compilation (since it's in advance of master). Cheers, Neil
Sign in to reply to this message.
http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... File input/regression/glissando-chord.ly (right): http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... input/regression/glissando-chord.ly:4: texidoc = "LilyPond typesets glissandi between chords." This should be two separate tests: one for the basic functionality and another testing 'glissando-index. http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc File lily/glissando-engraver.cc (right): http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc#ne... lily/glissando-engraver.cc:51: SCM map; Unless you need to preserve the value of glissandoMap between timesteps, this is redundant. http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc#ne... lily/glissando-engraver.cc:85: map = get_property ("glissandoMap"); SCM map = get_property ("glissandoMap"); (see note above)
Sign in to reply to this message.
On Apr 28, 2011, at 9:47 AM, n.puttock@gmail.com wrote: > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > File input/regression/glissando-chord.ly (right): > > http://codereview.appspot.com/4442082/diff/3001/input/regression/glissando-ch... > input/regression/glissando-chord.ly:4: texidoc = "LilyPond typesets > glissandi between chords." > This should be two separate tests: one for the basic functionality and > another testing 'glissando-index. > > http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc > File lily/glissando-engraver.cc (right): > > http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc#ne... > lily/glissando-engraver.cc:51: SCM map; > Unless you need to preserve the value of glissandoMap between timesteps, > this is redundant. > > http://codereview.appspot.com/4442082/diff/3001/lily/glissando-engraver.cc#ne... > lily/glissando-engraver.cc:85: map = get_property ("glissandoMap"); > SCM map = get_property ("glissandoMap"); > > (see note above) > > http://codereview.appspot.com/4442082/ All fixed in 5eba06f78bb08b84415544ed9e3cf0171a830aa6. Cheers, MS
Sign in to reply to this message.
On Mon, Apr 25, 2011 at 4:10 PM, <mtsolo@gmail.com> wrote: > The test file I've been using is: > > > \relative c' { > c1 \glissando g' > c,1 \glissando s1 g' > <c, e>1 \glissando <g' b> > <c, e>1 \glissando s1 <g' b> > \set glissandoMap = #'((0 . 1) (1 . 0)) this interface is somewhat kludgy. Fingerings can be put on notes with much less hassle. One idea: < \tweak #'glissando-connect #1 c f > < d e > would generate 1 glissando from C to D. -- Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
