|
|
Created:
13 years, 3 months ago by Valentin Villenave Modified:
13 years, 3 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAdd direction-awareness ability to post-event music functions.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update version number #Patch Set 3 : set direction directly at parsing level #Patch Set 4 : Add check. #Patch Set 5 : More compact code #MessagesTotal messages: 19
Greetings everybody, here's a patch that Neil suggested to me (if you remember my earlier postfix-dynamics patch, you may guess where I'm getting at with this). Thoughts? Cheers, Valentin.
Sign in to reply to this message.
This seems like a cool idea! Could it be made more simple by just letting the 0 direction be set as a music property, instead of adding only the 1 and -1 directions? http://codereview.appspot.com/3743043/diff/1/input/regression/music-function-... File input/regression/music-function-post-event.ly (right): http://codereview.appspot.com/3743043/diff/1/input/regression/music-function-... input/regression/music-function-post-event.ly:1: \version "2.13.43" Should be 2.13.44, because .43 is already released. We're working on .44 now. http://codereview.appspot.com/3743043/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/3743043/diff/1/scm/ly-syntax-constructors.scm#n... scm/ly-syntax-constructors.scm:46: (define-ly-syntax-loc (music-function parser loc fun args dir) Why not just let the direction = 0 argument flow through into the music property? Does it cause problems to have a direction = 0 in general?
Sign in to reply to this message.
On 2010/12/20 15:31:43, Carl wrote: > This seems like a cool idea! Thanks! As said before, it really isn't mine :) > Could it be made more simple by just letting the 0 direction be set as a music > property, instead of adding only the 1 and -1 directions? That's because I wanted to preserve the current behavior, that doesn't actually treat the dash as a neutral direction indicator, but simply disregards it. I'm not sure it's entirely safe to do otherwise (although I can't really think of a situation where it wouldn't be).
Sign in to reply to this message.
On 2010/12/20 16:01:46, Valentin Villenave wrote: > On 2010/12/20 15:31:43, Carl wrote: > > This seems like a cool idea! > > Thanks! As said before, it really isn't mine :) > > > Could it be made more simple by just letting the 0 direction be set as a music > > property, instead of adding only the 1 and -1 directions? > > That's because I wanted to preserve the current behavior, that doesn't actually > treat the dash as a neutral direction indicator, but simply disregards it. I'm > not sure it's entirely safe to do otherwise (although I can't really think of a > situation where it wouldn't be). IMO, go ahead and set the neutral direction. If it passes make check, then call it good.
Sign in to reply to this message.
On 2010/12/20 16:22:30, Carl wrote: > IMO, go ahead and set the neutral direction. If it passes make check, then call > it good. New patch set; for now I've just updated the regtest. I really don't feel comfortable passing a direction argument everywhere (since it would also affect *all* music functions, not just the post-event ones). If there is a consensus that it is safe enough, I'll go ahead, but for now since it merely adds one line of code, I feel more comfortable with the current proposal. Cheers, Valentin.
Sign in to reply to this message.
Hi Valentin, It's great to see you tackle this. The code looks pretty good, but I agree with Carl that it's too complicated. I'd go even further though: you should only be setting direction if it's necessary (just like articulations), so there's no need to pass an extra argument to the syntax constructor. Simply set 'direction inside the parser rule itself following the function evaluation. Cheers, Neil
Sign in to reply to this message.
On 2010/12/20 23:18:37, Neil Puttock wrote: > The code looks pretty good, but I agree with Carl that it's too complicated. > I'd go even further though: you should only be setting direction if it's > necessary (just like articulations), so there's no need to pass an extra > argument to the syntax constructor. Simply set 'direction inside the parser > rule itself following the function evaluation. I've been looking for a way of doing this, but it's a bit too advanced for me (I suspect there must be something to unsmob somewhere or a Music object or whatever)... Could you give me a hint? Cheers, Valentin
Sign in to reply to this message.
On 2010/12/20 23:22:22, Valentin Villenave wrote: > I've been looking for a way of doing this, but it's a bit too advanced for me (I > suspect there must be something to unsmob somewhere or a Music object or > whatever)... Could you give me a hint? See the rules for direction_less_event/direction_reqd_event. The music to unsmob is the return value for the rule ($$). Cheers, Neil
Sign in to reply to this message.
On 2010/12/20 23:18:37, Neil Puttock wrote: > Simply set 'direction inside the parser > rule itself following the function evaluation. Hi Neil, something like that? (see new patch set) Cheers, Valentin.
Sign in to reply to this message.
On 2010/12/20 16:22:30, Carl wrote: > IMO, go ahead and set the neutral direction. If it passes make check, then call > it good. Hi Carl, actually, whilst it does pass make check, it breaks... all of my own scores! I use the following function: filterDirections= #(define-music-function (parser location music) (ly:music?) (music-filter (lambda (x) (not (null? (ly:music-property x 'direction)))) music)) that allows me to discriminate dynamics that have an explicit direction (even a neutral one) from those who don't. Of course, I can fix my own code accordingly. But it shows that this change may not be entirely harmless for everybody. Cheers, Valentin
Sign in to reply to this message.
On 2010/12/21 20:45:42, Valentin Villenave wrote: > On 2010/12/20 16:22:30, Carl wrote: > > actually, whilst it does pass make check, it breaks... all of my own scores! > I can't understand why you don't want to push a patch that breaks all your scores... ;-) Does this latest patch break your scores? > I use the following function: > > filterDirections= > #(define-music-function (parser location music) (ly:music?) > (music-filter > (lambda (x) > (not (null? (ly:music-property x 'direction)))) > music)) > > that allows me to discriminate dynamics that have an explicit direction (even a > neutral one) from those who don't. What is the logical distinction you want to make between a neutral direction and the lack of an explicit direction? I'm a bit nervous about using an undocumented lack of a direction setting as a signal to do something. It seems to me to be not very robust. Thanks, Carl
Sign in to reply to this message.
On 2010/12/21 21:24:52, Carl wrote: > I can't understand why you don't want to push a patch that breaks all your > scores... ;-) > > Does this latest patch break your scores? It does, since the 'direction property is never left null (I can work around it, though. But at first I did feel a bit shocked, I must say :-) > What is the logical distinction you want to make between a neutral direction and > the lack of an explicit direction? > > I'm a bit nervous about using an undocumented lack of a direction setting as a > signal to do something. It seems to me to be not very robust. It may not be robust, but it's actually very convenient. Please bear with me for a little while, I'll try to explain. The nicest way to write a piano part with centered dynamics is currently: \new PianoStaff << \new Staff \rightHand \new Dynamics \myDynamics \new Staff \leftHand >> The downside is: writing dynamics separately in a variable filled with skips and rests, is quite cumbersome. I much prefer writing the dynamics on the fly with the right- or left-hand music (or ideally, both). So, what if I could somehow extract the dynamics from these music expressions and gather them in the Dynamics context? \new PianoStaff << \new Staff \removeDynamics \rightHand \new Dynamics \keepOnlyDynamics << \rightHand \\ \leftHand >> \new Staff \removeDynamics \leftHand >> http://repo.or.cz/w/opus_libre.git/blob/HEAD:/lib/80-buildskel.scm#l174 http://repo.or.cz/w/opus_libre.git/blob/HEAD:/lib/80-buildskel.scm#l81 I *could* do this only with contexts and engravers. However, a new issue arises: sooner or later, I'm bound to have to print some dynamics above the upper staff, or below the lower staff (or even: below the upper staff, *but* not vertically centered, but instead closer to the relevant staff). Which I won't be able to do if I've removed the Dynamics_engraver from the Staff context. What to do? There we go: I have to use a music-filter, much like \keepWithTag or \removeWithTag, only it will check for explicit directions. http://repo.or.cz/w/opus_libre.git/blob/HEAD:/lib/libdynamic.scm#l58 This way, I can do: rightHand = \relative c' { c1\f %% This will be sent to the Dynamics context c^\f %% This will be printed above the upper staff c_\f %% This will be printed below the staff, but closer to the upper staff. c\< %% I can even begin a crescendo here... c } leftHand = \relative c { \clef bass c1 c c c c\ff %% ... And finish it here. } It's a hell of a hack (as is my whole framework anyway), and it does add a considerable overhead. But boy, do my scores look good! :-) Cheers, Valentin.
Sign in to reply to this message.
On 2010/12/21 14:19:56, Valentin Villenave wrote: > something like that? (see new patch set) Almost there, but you're still setting 'direction unconditionally instead of checking first, i.e., $$ = run_music_function (PARSER, $2); if ($1) { unsmob $$ and set direction } Cheers, Neil
Sign in to reply to this message.
On 2010/12/21 22:12:46, Valentin Villenave wrote: > It does, since the 'direction property is never left null (I can work around it, > though. But at first I did feel a bit shocked, I must say :-) Use ly:dir? instead of (not (null? ...). Cheers, Neil
Sign in to reply to this message.
On 2010/12/21 22:21:38, Neil Puttock wrote: > Almost there, but you're still setting 'direction unconditionally instead of > checking first, i.e., Done (new patch). However, I can't really understand how this condition could ever be false. What kind of input could trigger the parser rule without actually containing a proper direction? Cheers, Valentin.
Sign in to reply to this message.
On 2010/12/21 22:27:43, Neil Puttock wrote: > Use ly:dir? instead of (not (null? ...). Duh. Nice catch, thanks! Cheers, V.
Sign in to reply to this message.
On 2010/12/21 22:34:23, Valentin Villenave wrote: > Done (new patch). However, I can't really understand how this condition could > ever be false. What kind of input could trigger the parser rule without actually > containing a proper direction? script_dir: '_' { $$ = DOWN; } | '^' { $$ = UP; } | '-' { $$ = CENTER; } ; CENTER = 0, so is always false. Cheers, Neil
Sign in to reply to this message.
On 2010/12/21 22:45:53, Neil Puttock wrote: > CENTER = 0, so is always false. Oh, I didn't know 0 was false, in Guile you can have something like: guile> (define a 0) guile> (if a (display "true\n")) true guile> (yes it's silly, but remember that the only things about programming I know, I learned with LilyPond!) Do you think I can push this patch now? (Not sure where to document it, though. Probably somewhere in the Extending manual.) Cheers, Valentin.
Sign in to reply to this message.
On 2010/12/21 22:56:05, Valentin Villenave wrote: > Oh, I didn't know 0 was false, in Guile you can have something like: > > guile> (define a 0) > guile> (if a (display "true\n")) > true > guile> Yep, Scheme is strict about #f: it's not equal to any other object. > Do you think I can push this patch now? (Not sure where to document it, though. > Probably somewhere in the Extending manual.) I don't think you need to copy the function evaluation result into a new SCM object, but apart from that it LGTM. Cheers, Neil
Sign in to reply to this message.
|