Re: Improve implementation of dashed slurs

[Neil Puttock]

2009/4/18 Carl D. Sorensen <address@hidden>:
>
>
>
> On 4/17/09 1:34 PM, "address@hidden" <address@hidden> wrote:

>> http://codereview.appspot.com/41099/diff/1021/52
>> File Documentation/user/expressive.itely (right):
>>
>> http://codereview.appspot.com/41099/diff/1021/52#newcode634
>> Line 634: @lilypondfile[verbatim,lilyquote,texidoc,doctitle]
>> @ignore this unless you're going to run makelsr.py (or create input/lsr
>> file by hand)
>
> In order to build my docs, I copied the file from input/new to input/lsr.
>
> I thought that the doc build process would get files from input/new if they
> didn't exist in input/lsr.
>
> Can you summarize the process for me?  If I want to add a new snippet to
> the docs, how should I do it?

I'll reply in the other thread once I've digested it properly.

>>
>> http://codereview.appspot.com/41099/diff/1021/54#newcode4
>> Line 4: texidoc = "
>> needs snippet directory tag otherwise makelsr.py will fail
>>
>> lsrtags = "rhythms, expressive"
>
> Fixed, thanks.  I wasn't sure what tags to use.

I think the only place where they're all listed is in makelsr.py.

>>
>> http://codereview.appspot.com/41099/diff/1021/59
>> File lily/bezier.cc (right):
>>
>> http://codereview.appspot.com/41099/diff/1021/59#newcode277
>> Line 277: Offset p[CONTROL_COUNT][CONTROL_COUNT];
>> Offset is described in offset.hh as 2d vector, so should use a vector of
>> Offsets
>
> I read offset.hh as a one-d vector (in computer science terms), with two
> elements; element [X_AXIS] and element[Y_AXIS], so it represents a point in
> two-dimensional space (hence the 2-d vector).
>
> I want a matrix of offsets here.  The first index is the control point
> number.  The second index is the equation order (3 for the bezier, 2 for the
> first interpolation, 1 for the second interpolation, and 0 for the third
> interpolation).  This usage is correct.

If I'd looked in bezier.hh I might have realized before talking nonsense. :)

>>
>> http://codereview.appspot.com/41099/diff/1021/59#newcode296
>> Line 296: Bezier::extract (Real t_min, Real t_max)
>> ensure t_min and t_max stay within bounds?
>>
>> negative values and >1 produce outlandish curves
>>
>
> OK, so I think I need some help here.  I'm perfectly willing to check the
> bounds and give a warning message if the normal bounds aren't met.  But I'm
> not sure how to do that.  Can you give me a pointer to how to do it, a place
> in the code to look for an example, and/or an expression to grep?

I don't think a warning's necessary, but you can probably get away
with using min () and max ().  See line-interface.cc for a few
examples.

>> http://codereview.appspot.com/41099/diff/1021/62#newcode346
>> Line 346: Lookup::slur (Bezier curve, Real curvethick, Real linethick,
>> is passing SCM allowed in lookup.cc?
>
> Why would it not be?  lookup.hh has a prototype for slur:
>
>  static Stencil slur (Bezier controls, Real cthick, Real thick,
>                       SCM dash_definition);
>
> Is there a problem with this as a prototype?
>
> The code works.  Is it a time bomb waiting to explode on me?

Of course not. :)  It just struck me as out of place, since none of
the methods in lookup.cc uses SCMs.

>>
>> http://codereview.appspot.com/41099/diff/1021/62#newcode351
>> Line 351: /* calculate the offset for the two beziers that make the
>> sandwich
>> tidy comments
>
> Is this right?
>
>  /*
>      calculate the offset for the two beziers that make the sandwich
>      for the slur
>  */

lgtm

>>
>> http://codereview.appspot.com/41099/diff/1021/62#newcode370
>> Line 370: for (int i=0; i<num_segments; i++)
>> i < num_segments
>>
>> should print warning if segments overlap, or just silently ignore?
>>
>
> I thought about it, and decided to allow segments to overlap.  I'm not
> sure if that was just because I'm lazy, or if I really thought there might
> be some use for overlapping segments.

Fair enough.  It doesn't seem to do any harm anyway.

>>
>> http://codereview.appspot.com/41099/diff/1021/66
>> File ly/property-init.ly (right):
>>
>> http://codereview.appspot.com/41099/diff/1021/66#newcode13
>> Line 13: #(define (make-simple-dash-definition dash-fraction
>> dash-period)
>> this doesn't belong here
>>
>> http://codereview.appspot.com/41099/diff/1021/66#newcode19
>> Line 19: slurDashPattern =
>> these should go in music-functions-init.ly
>
> Why should these go in music-functions-init.ly instead of property-init.ly?
> I'm not disagreeing with you, I'm asking for the criterion for choosing.

All the entries in property-init.ly (apart from pointAndClickOn/Off
which should also be moved) are simple identifiers which mainly
involve constant property settings.

>>
>> http://codereview.appspot.com/41099/diff/1021/67
>> File python/convertrules.py (right):
>>
>> http://codereview.appspot.com/41099/diff/1021/67#newcode2903
>> Line 2903: if (re.search(r'\'dash-fraction', str) or
>> re.search(r'\'dash-period', str):
>> limit search to Slur/Tie/PhrasingSlur otherwise users will get this
>> message for other grobs
>
> does re.search("(Slur|Tie)\s+\'dash-fraction", str)  look right?

You need a hash before the escaped quote, otherwise it looks OK.

One thing I forgot to mention: the NOT_SMART macro expects a sentence
continuation rather than a new sentence.

>>
>> http://codereview.appspot.com/41099/diff/1021/68
>> File scm/define-grob-properties.scm (right):
>>
>> http://codereview.appspot.com/41099/diff/1021/68#newcode174
>> Line 174: (dash-definition ,pair? "List of @code{dash-elements} defining
>> the
>> list?
>
> Han-Wen suggested to me in the past that list? is a very expensive operator
> because it must check every element in the list to assure that it's properly
> formed.  He suggested that unless such a syntax check is needed, it's better
> to use pair?, so that's what I've done here.

Isn't this a special case though, since it's the type check for a
backend property?

As far as I can see, all the user properties which take lists use
list?, otherwise the IR docs would display the wrong type.

> Thanks for the careful review, and for what it's taught me about the
> LilyPond standards.

You're welcome.

Regards,
Neil