Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1193)

Issue 14268043: Issue 3560: Completion_heads_engraver with \scaleDurations

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by dak
Modified:
10 years, 5 months ago
Reviewers:
benko.pal, Keith, lemzwerg
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 3560: Completion_heads_engraver with \scaleDurations Both the Completion_heads_engraver and Completion_rest_engraver have to make the decision whether to interpret a scale factor on a note or as actually scaling the note duration itself (similar to \times/\tuplet) or merely as an artifical means of shifting the onset of the next note (or, actually, combination of both). The completion engravers need to make an actual decision here. The previously employed heuristic was that integer scale factors were interpreted as a delay of the next onset while other rational scale factors implied an actual scaling of the note value. In the first case, any durations produced by the completion engravers received a scale factor of 1, in the second, they inherited the original scale factor. The visual note duration needed for filling the measure depended on the chosen scale factor. This heuristic lead to surprises like issue 1772: the behavior of issue 1772 was triggered by scale factors like *3/2 but not by *3/1. Now the heuristic has been dropped, so either scale factor will lead to the observed behavior. The solution is to completely refrain from using scaled durations in the presence of completion engravers for the visual arrangement of music, instead using the more cumbersome form of using parallel music with skips which also has the advantage of creating more accurate Midi. Since the completion engravers can't otherwise make a consistent decision for choosing the scale factor of the resulting music, it is best to retain the factor of the original note/rest duration.

Patch Set 1 #

Patch Set 2 : Rewrite regtests #

Patch Set 3 : Combining with other regtest proposals is left as an exercise to the reader... #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -49 lines) Patch
M input/regression/completion-heads-factor.ly View 1 1 chunk +62 lines, -15 lines 0 comments Download
M input/regression/completion-rest.ly View 1 2 chunks +11 lines, -8 lines 0 comments Download
M lily/completion-note-heads-engraver.cc View 1 2 2 chunks +9 lines, -10 lines 3 comments Download
M lily/completion-rest-engraver.cc View 1 2 2 chunks +9 lines, -10 lines 0 comments Download
M ly/engraver-init.ly View 1 2 1 chunk +2 lines, -0 lines 1 comment Download
M scm/c++.scm View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M scm/define-context-properties.scm View 1 2 2 chunks +17 lines, -6 lines 1 comment Download
M scm/lily.scm View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10
lemzwerg
LGTM.
10 years, 6 months ago (2013-10-02 09:07:56 UTC) #1
dak
Rewrite regtests
10 years, 6 months ago (2013-10-03 13:54:01 UTC) #2
benko.pal
LGTM
10 years, 6 months ago (2013-10-03 21:07:47 UTC) #3
dak
Combining with other regtest proposals is left as an exercise to the reader...
10 years, 6 months ago (2013-10-25 11:36:27 UTC) #4
dak
https://codereview.appspot.com/14268043/diff/9001/scm/define-context-properties.scm File scm/define-context-properties.scm (right): https://codereview.appspot.com/14268043/diff/9001/scm/define-context-properties.scm#newcode218 scm/define-context-properties.scm:218: (completionFactor ,rational-or-procedure? With Guile-2, one would probably just make ...
10 years, 6 months ago (2013-10-25 11:49:00 UTC) #5
benko.pal
basically LGTM https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc#newcode200 lily/completion-note-heads-engraver.cc:200: factor_ = robust_scm2rational (factor, 0); 1 would ...
10 years, 6 months ago (2013-10-25 12:52:55 UTC) #6
dak
https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc#newcode200 lily/completion-note-heads-engraver.cc:200: factor_ = robust_scm2rational (factor, 0); On 2013/10/25 12:52:55, benko.pal ...
10 years, 6 months ago (2013-10-25 13:10:53 UTC) #7
Keith
The simplicity is just lovely. https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc#newcode205 lily/completion-note-heads-engraver.cc:205: if (nb.main_part_ && nb ...
10 years, 6 months ago (2013-10-26 06:37:13 UTC) #8
dak
On 2013/10/26 06:37:13, Keith wrote: > The simplicity is just lovely. Always hard to tell ...
10 years, 5 months ago (2013-11-01 20:20:35 UTC) #9
Keith
10 years, 5 months ago (2013-11-02 02:35:55 UTC) #10
On 2013/11/01 20:20:35, dak wrote:
> On 2013/10/26 06:37:13, Keith wrote:
> > The simplicity is just lovely.
> 
> Always hard to tell sarcasm from enthusiasm.
  
This patch does simplify things -- especially in comparison to the version that
changed behavior based on scale-factors being integral multiples of things. 
Only history makes things complicated.

>
https://codereview.appspot.com/14268043/diff/9001/ly/engraver-init.ly#newcode637
> > ly/engraver-init.ly:637: completionFactor = #ly:duration-scale
> > We do need to keep issue 1532 fixed, since several people depend on and
expect
> > that behavior.
> > 
> > %% Historically, completion_heads_engraver has split integer-scaled
> > %% notes into notes with no scaling   c1*3  ==>  {c1~ c1~ c1~}
> >   completionFactor = #
> >   (lambda (dur)
> >     (let ((scale (ly:duration-scale dur)))
> >          (if (integer? scale)
> >               1
> >               scale)))

> Now that does not change that having to crank out some anonymous
> lambda function for some common behavior is a bad idea.  At least the
> previous default behavior should be available as a stock function, and
> one can conceivably teach convert-ly to continue providing the old
> inconsistent behavior via an appropriate assignment whenever one of
> the completion engravers is being used.  Please remember that this old
> behavior is _also_ not what "people" expected, or we would not be
> having this issue.

The function above provides the old behavior.

You could make the C-code fall-back to the consistent behavior if someone
un-defines 'completionFactor'
 factor_ = robust_scm2rational("completionFactor", note_dur.factor());
Then I'll update the docs patch at http://codereview.appspot.com/14633043
so the known-issue just says "Historically, blah blah...  \unset
completionFactor removes this inconsistency."
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b