Implement straight flags in scheme

Implement straight flags in scheme

[hanwenn, 2008-12-14 02:44:52]

http://codereview.appspot.com/11046/diff/8/11
File scm/flag-styles.scm (left):

http://codereview.appspot.com/11046/diff/8/11#oldcode17
Line 17: ;; (define-public (add-stroke-straight stencil dir stroke-style)
drop commented stuff.

http://codereview.appspot.com/11046/diff/8/11
File scm/flag-styles.scm (right):

http://codereview.appspot.com/11046/diff/8/11#newcode18
Line 18: (let* ((udmult (if stem-up? 1 -1))
fix naming

http://codereview.appspot.com/11046/diff/8/11#newcode28
Line 28: (let* ((conversion-constant (/ (atan 1 1) 45))
define PI = 3.141.. somewhere and use that.

split this in a polar -> rect function and a degree -> radian function.

http://codereview.appspot.com/11046/diff/8/11#newcode36
Line 36: (define (buildflag flag-stencil remain curr-stencil spacing)
doc string

http://codereview.appspot.com/11046/diff/8/11#newcode57
Line 57: (factor (if (number? fs) (expt 1.12246 fs) 1))
look for magstep to unhardcoded this

http://codereview.appspot.com/11046/diff/8/11#newcode61
Line 61: (up-length (+ (* upflag-length factor) half-stem-thickness))
can you make the up/down distinction generic? Usually up=1, down= -1, so usually
you can just do (* direction thick) to have a direction dependent value.

http://codereview.appspot.com/11046/diff/8/11#newcode80
Line 80: (if (null? stroke-style)
use explicit check 

 (eq? stroke-style 'grace)

(or whatever)

[Carl, 2008-12-14 03:36:43]

Han-Wen,

I've asked about a different possibility for doing the conversion.

I'd like to know what you think about it, just as a general idea of 
programming style.

Thanks,

Carl

http://codereview.appspot.com/11046/diff/8/11
File scm/flag-styles.scm (right):

http://codereview.appspot.com/11046/diff/8/11#newcode28
Line 28: (let* ((conversion-constant (/ (atan 1 1) 45))
On 2008/12/14 02:44:52, hanwenn wrote:
> define PI = 3.141.. somewhere and use that.

How about (define PI-OVER-180  (/ (atan 1 1) 45))
instead of (define PI 3.14159)

Then
(define (degrees->radians angle-degrees)
   (* angle-degrees PI-OVER-180))



> split this in a polar -> rect function and a degree -> radian function.

[Reinhold, 2008-12-14 12:03:35]

http://codereview.appspot.com/11046/diff/8/11
File scm/flag-styles.scm (left):

http://codereview.appspot.com/11046/diff/8/11#oldcode17
Line 17: ;; (define-public (add-stroke-straight stencil dir stroke-style)
On 2008/12/14 02:44:52, hanwenn wrote:
> drop commented stuff.

Exactly. This is what my patch does ;-) 
This was skeleton code that was in there before my patch...

http://codereview.appspot.com/11046/diff/8/11
File scm/flag-styles.scm (right):

http://codereview.appspot.com/11046/diff/8/11#newcode18
Line 18: (let* ((udmult (if stem-up? 1 -1))
On 2008/12/14 02:44:52, hanwenn wrote:
> fix naming

Ah, right. I should simply pass the stem's direction property here. That saves a
local variable and this if check...

http://codereview.appspot.com/11046/diff/8/11#newcode28
Line 28: (let* ((conversion-constant (/ (atan 1 1) 45))
On 2008/12/14 03:36:43, Carl.D.Sorensen wrote:
> On 2008/12/14 02:44:52, hanwenn wrote:
> > define PI = 3.141.. somewhere and use that.
> 
> How about (define PI-OVER-180  (/ (atan 1 1) 45))
> instead of (define PI 3.14159)
> 
> Then
> (define (degrees->radians angle-degrees)
>    (* angle-degrees PI-OVER-180))
> 
> 
> 
> > split this in a polar -> rect function and a degree -> radian function.
> 

Okay, will do.

http://codereview.appspot.com/11046/diff/8/11#newcode36
Line 36: (define (buildflag flag-stencil remain curr-stencil spacing)
On 2008/12/14 02:44:52, hanwenn wrote:
> doc string

Even for internal functions? This is not define-global'ed, but only locally
defined...

Anyway, I can of course add a doc string saying that it's an internal function
to build multiple flags from the single-flag stencil.

http://codereview.appspot.com/11046/diff/8/11#newcode61
Line 61: (up-length (+ (* upflag-length factor) half-stem-thickness))
On 2008/12/14 02:44:52, hanwenn wrote:
> can you make the up/down distinction generic? Usually up=1, down= -1, so
usually
> you can just do (* direction thick) to have a direction dependent value.


Actually, no, for two reasons:
1) The up- and down-flags have different angles and different lengths, so I
can't use a +1/-1 multiplier.
2) The flag is drawn using a rounded polygon, where I need to give the corners
in clockwise order, so for up and down flags the second and fourth corners will
need to be exchanged in any case.

http://codereview.appspot.com/11046/diff/8/11#newcode66
Line 66: (offset (cons 0 (if stem-up? (- thickness) thickness)))
Of course, this can be simplyfied to (cons 0 (* thickness direction))

http://codereview.appspot.com/11046/diff/8/11#newcode80
Line 80: (if (null? stroke-style)
On 2008/12/14 02:44:52, hanwenn wrote:
> use explicit check 
> 
>  (eq? stroke-style 'grace)
> 
> (or whatever)

Okay, the downside is just that for other grace styles, there will be no slash
or so at all. I thought it would be better to have at least a slash (even if
that's not the current stroke-style).

[hanwenn, 2008-12-14 13:09:45]

some small comments, please apply

http://codereview.appspot.com/11046/diff/19/205
File scm/flag-styles.scm (right):

http://codereview.appspot.com/11046/diff/19/205#newcode20
Line 20: Note that here length is the whole length, while flag-x-width is just
the
drop: Note that

http://codereview.appspot.com/11046/diff/19/205#newcode45
Line 45: "Internal function to recursively create a stencil with @code{remain}
flags
Cool. if the function is internal, a single line doc str usually suffices.

http://codereview.appspot.com/11046/diff/19/205#newcode64
Line 64: (stem-up? (eqv? dir UP))
ok - as a nitpick, the foo? is usually reserved for functions that return bool,
rather than the variables.  The lilypond code base is not very consistent with
that though.

http://codereview.appspot.com/11046/diff/19/205#newcode67
Line 67: (fs (ly:grob-property stem-grob 'font-size))
you can pass 0 as default arg, so you can put the call into the magstep arg
directly.

http://codereview.appspot.com/11046/diff/19/205#newcode75
Line 75: (down-off (polar->rectangular down-length downflag-angle))
this still looks repeated between up & down. Use

  (flag-length (if flagup? upflag-length downflag-length))

and fold cases.