|
|
Created:
9 years, 11 months ago by PhilEHolmes Modified:
9 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionThis moves voiced rests closer together, more closely matching the recommendations of Gould, Read, etc. There are a lot of regression test changes, but all associated with repositioned rests.
Patch Set 1 #
Total comments: 1
Patch Set 2 : Updated with David N's suggestion #
Total comments: 4
MessagesTotal messages: 11
Please review
Sign in to reply to this message.
On 2014/05/27 09:38:11, PhilEHolmes wrote: > Please review According to the comparison images in https://code.google.com/p/lilypond/issues/detail?id=3902#c5 this is definitely an improvement. (I had looked for that issue myself, but I have to admit that I have to stop when entering the C++ area ...). But what would you think about going one step further and place voiced rests by default at the same position as \oneVoice rests? This would make the regularly asked question about merging rests obsolete. The rest would appear identical to a \oneVoice rest if there isn't any other object in the way. The voice number assignment would then "only" determine the direction the rests would move to. Another approach which would be an even more significant improvement (but definitely much more involved) would be to try to determine pitches of surrounding notes in the same voice, which can avoid ambiguities sometimes. (Maybe it's a good idea to reread http://blog.steinberg.net/2014/02/development-diary-part-the-fifth/#more-523) which was my first encounter of the problem). Please note that these are only suggestions, no objections against the patch.
Sign in to reply to this message.
LGTM. What about making this configurable? I can imagine that for some special situations a higher default position of rests is desirable (e.g., fugues and the like).
Sign in to reply to this message.
On 2014/05/27 10:00:53, lemzwerg wrote: > LGTM. > > What about making this configurable? I can imagine that for some special > situations a higher default position of rests is desirable (e.g., fugues and the > like). +1. Maybe with my suggested \oneVoice behaviour as the default?
Sign in to reply to this message.
----- Original Message ----- From: <lilyliska@googlemail.com> To: <PhilEHolmes@googlemail.com> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Tuesday, May 27, 2014 10:59 AM Subject: Re: Reposition voiced rests (Issue 3902) (issue 101720045) > On 2014/05/27 09:38:11, PhilEHolmes wrote: >> Please review > > According to the comparison images in > https://code.google.com/p/lilypond/issues/detail?id=3902#c5 this is > definitely an improvement. (I had looked for that issue myself, but I > have to admit that I have to stop when entering the C++ area ...). > > But what would you think about going one step further and place voiced > rests by default at the same position as \oneVoice rests? This would > make the regularly asked question about merging rests obsolete. The rest > would appear identical to a \oneVoice rest if there isn't any other > object in the way. The voice number assignment would then "only" > determine the direction the rests would move to. I would definitely argue against that. I frequently want separated rests for separate voices, and to remove that would not match the recomended typesetting procedure. If I wanted "merged" rests, I'd just spacer-rest one of them and use \voiceOne. > Another approach which would be an even more significant improvement > (but definitely much more involved) would be to try to determine pitches > of surrounding notes in the same voice, which can avoid ambiguities > sometimes. > (Maybe it's a good idea to reread > http://blog.steinberg.net/2014/02/development-diary-part-the-fifth/#more-523) > which was my first encounter of the problem). > > Please note that these are only suggestions, no objections against the > patch. > > https://codereview.appspot.com/101720045/ I'm starting with something very simple here, so that's too complex at my stage of understanding. If you look at all the comments on the issue, you'll see my disquiet over the misplaced ledger lines. That will likely be a new issue that I may work on later. -- Phil Holmes
Sign in to reply to this message.
----- Original Message ----- From: <lemzwerg@googlemail.com> To: <PhilEHolmes@googlemail.com> Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org> Sent: Tuesday, May 27, 2014 11:00 AM Subject: Re: Reposition voiced rests (Issue 3902) (issue 101720045) > LGTM. > > What about making this configurable? I can imagine that for some > special situations a higher default position of rests is desirable > (e.g., fugues and the like). > > https://codereview.appspot.com/101720045/ In a sense it is already configurable, either with pitched rests or \override Rest.staff-position -- Phil Holmes
Sign in to reply to this message.
https://codereview.appspot.com/101720045/diff/1/lily/rest.cc File lily/rest.cc (right): https://codereview.appspot.com/101720045/diff/1/lily/rest.cc#newcode138 lily/rest.cc:138: To be consistent with your change above, shouldn't this be: return neutral + 2 * dir;
Sign in to reply to this message.
----- Original Message ----- From: <david.nalesnik@gmail.com> To: <PhilEHolmes@googlemail.com>; <lemzwerg@googlemail.com>; <lilyliska@googlemail.com>; <email@philholmes.net>; <mail@philholmes.net> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Tuesday, May 27, 2014 3:12 PM Subject: Re: Reposition voiced rests (Issue 3902) (issue 101720045) > > https://codereview.appspot.com/101720045/diff/1/lily/rest.cc > File lily/rest.cc (right): > > https://codereview.appspot.com/101720045/diff/1/lily/rest.cc#newcode138 > lily/rest.cc:138: > To be consistent with your change above, shouldn't this be: > > return neutral + 2 * dir; > > https://codereview.appspot.com/101720045/ Probably. I can't think of a use case where this code would actually be exercised, so would welcome a suggestion. -- Phil Holmes
Sign in to reply to this message.
On 2014/05/27 14:27:07, email_philholmes.net wrote: > ----- Original Message ----- > From: <mailto:david.nalesnik@gmail.com> > To: <mailto:PhilEHolmes@googlemail.com>; <mailto:lemzwerg@googlemail.com>; > <mailto:lilyliska@googlemail.com>; <mailto:email@philholmes.net>; <mailto:mail@philholmes.net> > Cc: <mailto:lilypond-devel@gnu.org>; <mailto:reply@codereview-hr.appspotmail.com> > Sent: Tuesday, May 27, 2014 3:12 PM > Subject: Re: Reposition voiced rests (Issue 3902) (issue 101720045) > > > > > > https://codereview.appspot.com/101720045/diff/1/lily/rest.cc > > File lily/rest.cc (right): > > > > https://codereview.appspot.com/101720045/diff/1/lily/rest.cc#newcode138 > > lily/rest.cc:138: > > To be consistent with your change above, shouldn't this be: > > > > return neutral + 2 * dir; > > > > https://codereview.appspot.com/101720045/ > > > Probably. I can't think of a use case where this code would actually be > exercised, so would welcome a suggestion. > > -- > Phil Holmes > It appears that this is a check that the automatic calculations of the positions of voiced half and whole rests are sensible. The check can fail when you mess with the number of staff lines, in which case the fallback is assigned. Hmm. It doesn't appear that I can attach anything here, so I'll have to give my examples (before and after with the revised line) on the page for Issue 3902.
Sign in to reply to this message.
Updated with David N's suggestion
Sign in to reply to this message.
Looks like a new property is the only way to get an overall improvement. LilyPond has good infrastructure for using properties, so there is not much code to add. https://codereview.appspot.com/101720045/diff/2/lily/rest.cc File lily/rest.cc (right): https://codereview.appspot.com/101720045/diff/2/lily/rest.cc#newcode75 lily/rest.cc:75: pos = 2 * dir; pos = dir * robust_scm2int(me->get_property("voiced-rest-position"), 2); https://codereview.appspot.com/101720045/diff/2/lily/rest.cc#newcode139 lily/rest.cc:139: return neutral + 2 * dir; This one should remain 2, because the voiced rest position has already been applied. /* one staff-space, 2 staff positions, from the neutral position */ The logic above seems that it would prevent half- and longer rests from crossing the centerline, even if voiced-rest-position is 0, which I think you want. https://codereview.appspot.com/101720045/diff/2/lily/rest.cc#newcode319 lily/rest.cc:319: "minimum-distance " As you say, Rest.minimum-distance is ignored, so we can remove it here. https://codereview.appspot.com/101720045/diff/2/lily/rest.cc#newcode320 lily/rest.cc:320: "style " "voiced-rest-position " would go here, along with an entry in scm/define-grob-properties.scm
Sign in to reply to this message.
|