Re: Improve implementation of dashed slurs

[Carl D. Sorensen]

On 4/17/09 1:34 PM, "address@hidden" <address@hidden> wrote:

Thanks for the review, Neil.

> 
> 
> 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?


> 
> http://codereview.appspot.com/41099/diff/1021/54
> File input/new/making-slurs-with-complex-dash-structure.ly (right):
> 
> http://codereview.appspot.com/41099/diff/1021/54#newcode3
> Line 3: \header{
> space

Fixed, thanks.

> 
> 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.

> 
> http://codereview.appspot.com/41099/diff/1021/58
> File lily/arpeggio.cc (right):
> 
> http://codereview.appspot.com/41099/diff/1021/58#newcode168
> Line 168: Stencil mol (Lookup::slur (curve, lt, lt, SCM_UNDEFINED));
> add dash ability too?

I thought about that.  If we add it, I think it's the only arpeggio
indicator that can be dashed.  I'm willing to add it if it's deemed
necessary, but it seemed to me that it would be best to leave it alone right
now.

> 
> 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.

> 
> 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?

> http://codereview.appspot.com/41099/diff/1021/59#newcode305
> Line 305: {
> no {}
> 
> http://codereview.appspot.com/41099/diff/1021/59#newcode309
> Line 309: {
> no {}

Fixed, along with line 299.
> 
> http://codereview.appspot.com/41099/diff/1021/62
> File lily/lookup.cc (right):
> 
> 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?

> 
> 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
  */

> 
> http://codereview.appspot.com/41099/diff/1021/62#newcode363
> Line 363: if ((dash_details == SCM_UNDEFINED) || (dash_details ==
> SCM_EOL))
> !scm_is_pair ()

much better, thanks!
> 
> http://codereview.appspot.com/41099/diff/1021/62#newcode364
> Line 364: { /* solid slur  */
> new line

Fixed

> 
> http://codereview.appspot.com/41099/diff/1021/62#newcode368
> Line 368: { /* dashed or combination slur */
> new line

Fixed.

> 
> 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.

I do think there is use for non-adjacent segments.  So I decided to just let
the user be in charge of it.  I don't think it will break the code, although
it may lead to some weird results.


> http://codereview.appspot.com/41099/diff/1021/62#newcode382
> Line 382: {
> no {}

Fixed.

> 
> http://codereview.appspot.com/41099/diff/1021/62#newcode408
> Line 408: }/* end for num_segments */
> don't need these comments

Fixed.

> 
> 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.

My initial thought was that they didn't belong in property-init.ly; my
second thought was that this was exactly the place for them.  That way, all
of the identifiers that change (phrasing) slur/tie behavior are in one
place, rather than split across two different files.

I'd welcome clarification on how one makes such a decision.

> 
> 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?
> 
> 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.

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

Carl