Woodwind diagrams

Woodwind diagrams

Implements woodwind diagrams in lilypond.

[Carl, 2010-05-31 14:06:48]

The new patch worked.

I've put some comments in line.

Great job, Mike!

http://codereview.appspot.com/1425041/diff/1/8
File scm/lily-library.scm (right):

http://codereview.appspot.com/1425041/diff/1/8#newcode492
scm/lily-library.scm:492: (cons
In general, I prefer to have the first argument on the line with (cons.  There
may be a case where wrapping works better if it's split, but that should be the
exception, not the rule.

Is this really intended as an interval function [(x-min . x-max )] or is it
intended as a coordinate function[(x . y)]?  If these functions are applied to
(x . y) pairs, let's change the name to (coord-* instead of (interval-*.

http://codereview.appspot.com/1425041/diff/1/8#newcode514
scm/lily-library.scm:514: (define-public (interval-rotate interval
degrees-in-radians)
This looks like you're using an interval as an x-y pair.  But intervals are
(x-min . x-max) or (y-min . y-max) pairs.

Used coord (see bezier.scm, and maybe move all the coord stuff to
lily-library.scm) instead of intervals.

http://codereview.appspot.com/1425041/diff/1/8#newcode520
scm/lily-library.scm:520: (radius
(radius should line up with (interval, not ((interval

http://codereview.appspot.com/1425041/diff/1/8#newcode588
scm/lily-library.scm:588: (if
It's more standard practice to put the test on the same line as the if.

http://codereview.appspot.com/1425041/diff/1/8#newcode610
scm/lily-library.scm:610: (/
I think you should not have the operators by themselves on a line.

http://codereview.appspot.com/1425041/diff/1/10
File scm/output-ps.scm (right):

http://codereview.appspot.com/1425041/diff/1/10#newcode105
scm/output-ps.scm:105: (define (connected-shape pointlist thick x-scale y-scale
connect fill)
I find it much easier to read this function in this indentation format:

(define (connected-shape pointlist thick x-scale y-scale connect fill)
   (ly:format "~a~4f ~4f ~4f ~4f ~a ~a draw_connected_shape"
     (string-concatenate
       (map (lambda (x) 
              (apply (if (eq? (length x) 6)
               (lambda (x1 x2 x3 x4 x5 x6) 
                 (ly:format "~4f ~4f ~4f ~4f ~4f ~4f 6 " x1 x2 x3 x4 x5 x6))
               (lambda (x1 x2) 
                 (ly:format "~4f ~4f 2 " x1 x2)))
             x)) 
         (reverse pointlist)))       (length pointlist)
       x-scale
       y-scale
       thick       (if connect "true" "false")
       (if fill "true" "false")))

http://codereview.appspot.com/1425041/diff/1/10#newcode112
scm/output-ps.scm:112: (eqv? (length x) 6)
Integer comparisons can always be eq?

http://codereview.appspot.com/1425041/diff/1/11
File scm/output-svg.scm (right):

http://codereview.appspot.com/1425041/diff/1/11#newcode359
scm/output-svg.scm:359: (new-start-angle
line up with second  (new-start-angle, not ((new-start-angle

http://codereview.appspot.com/1425041/diff/1/11#newcode363
scm/output-svg.scm:363: (sqrt
line up with (*

http://codereview.appspot.com/1425041/diff/1/11#newcode365
scm/output-svg.scm:365: (*
No space is saved by breaking the lines after these operators, because the
indent is the same place as you'd be without the break.  So you should keep them
on the same line:

(+ (* (* y-radius y-radius))
      (* (cos new-start-angle) (cos new-start-angle))

etc.

http://codereview.appspot.com/1425041/diff/1/11#newcode446
scm/output-svg.scm:446: "M0 0~a ~a"
can we avoid the ~a ~a construct?  That is a potential place for a user to
insert arbitrary PS code.  If we can't, we should either check the data or
identify (in a comment) the potential security risk.

http://codereview.appspot.com/1425041/diff/1/12
File scm/stencil.scm (right):

http://codereview.appspot.com/1425041/diff/1/12#newcode538
scm/stencil.scm:538: "Returns a function drawing an arrow from here to
@var{destination}."
Should be more like "Returns a function drawing a line from current point to
@var{destination}, with optional arrows of @var{max-size} on start and end
controlled by @var{start?} and @var{end?}."

[Neil Puttock, 2010-05-31 14:48:13]

Hi Mike,

This is super work, you're obviously a schemer extraordinaire. ;)

I've copied my comments from the original set, and added a few more (you'll see
some reiterate Carl's points).

I think woodwind-diagrams.scm is a bit unwieldy in its present form; it would be
easier to digest if the instrument alists were moved to a separate file.

Cheers,
Neil

http://codereview.appspot.com/1425041/diff/1/2
File input/regression/woodwind-diagrams-empty.ly (right):

http://codereview.appspot.com/1425041/diff/1/2#newcode1
input/regression/woodwind-diagrams-empty.ly:1: \version "2.12.0"
"2.13.23"

(or latest development version when ready to apply)

http://codereview.appspot.com/1425041/diff/1/2#newcode12
input/regression/woodwind-diagrams-empty.ly:12: #'(1.0 0.1 #t ()) }}
      #'(1.0 0.1 #t ())
  }
}

etc.

http://codereview.appspot.com/1425041/diff/1/3
File input/regression/woodwind-diagrams-key-lists.ly (right):

http://codereview.appspot.com/1425041/diff/1/3#newcode1
input/regression/woodwind-diagrams-key-lists.ly:1: \version "2.12.0"
"2.13.23"

http://codereview.appspot.com/1425041/diff/1/8
File scm/lily-library.scm (right):

http://codereview.appspot.com/1425041/diff/1/8#newcode283
scm/lily-library.scm:283: (define-public (map-alist-keys function keys alist)
this needs renaming: there's already a function called `map-alist-keys' in this
file

http://codereview.appspot.com/1425041/diff/1/8#newcode287
scm/lily-library.scm:287: @code{((a . -1) (b . 2) (c . 3) (d . 4))}"
the order's not preserved; I get the following output:

((b . 2) (a . -1) (c . 3) (d . 4))

http://codereview.appspot.com/1425041/diff/1/8#newcode289
scm/lily-library.scm:289: alist
(if (null? keys)
    alist

(and all other `if' forms)

http://codereview.appspot.com/1425041/diff/1/8#newcode296
scm/lily-library.scm:296: (assoc-remove (car keys) alist)))
assoc-remove should be defined in this file, not in woodwind-diagrams.scm

http://codereview.appspot.com/1425041/diff/1/8#newcode494
scm/lily-library.scm:494: (operator (interval-end operand) (interval-end
interval)))
(cons (operator (interval-start operand) (interval-start interval))
      (operator (interval-end operand) (interval-end interval)))

(and all other cons pairs)

http://codereview.appspot.com/1425041/diff/1/8#newcode573
scm/lily-library.scm:573: (define-public (return-interval iv) iv)
= Guile's `identity' procedure

http://codereview.appspot.com/1425041/diff/1/8#newcode624
scm/lily-library.scm:624: (let* ((complex (make-polar
let

http://codereview.appspot.com/1425041/diff/1/10
File scm/output-ps.scm (right):

http://codereview.appspot.com/1425041/diff/1/10#newcode114
scm/output-ps.scm:114: (x1 x2 x3 x4 x5 x6)
(lambda (x1 x2 x3 x4 x5 x6)

(and all other lambda forms)

http://codereview.appspot.com/1425041/diff/1/11
File scm/output-svg.scm (right):

http://codereview.appspot.com/1425041/diff/1/11#newcode360
scm/output-svg.scm:360: (- new-start-angle (* TWO-PI (floor (/ new-start-angle
TWO-PI)))))
move to a separate function rather than rebinding

(same for new-end-angle)

http://codereview.appspot.com/1425041/diff/1/11#newcode378
scm/output-svg.scm:378: (/
move to separate function to save duplication

http://codereview.appspot.com/1425041/diff/1/12
File scm/stencil.scm (right):

http://codereview.appspot.com/1425041/diff/1/12#newcode223
scm/stencil.scm:223: `(0.0 ,TWO-PI))))
insert space after this (and following other local defines)

http://codereview.appspot.com/1425041/diff/1/12#newcode249
scm/stencil.scm:249: (define (ordering-function-1 a b) (< (car a) (car b)))
car< in lily-library.scm

http://codereview.appspot.com/1425041/diff/1/12#newcode250
scm/stencil.scm:250: (define (ordering-function-2 a b) (<= (car a) (car b)))
rename car<= (could also be added to lily-library.scm)

http://codereview.appspot.com/1425041/diff/1/12#newcode331
scm/stencil.scm:331: ; Zeros of the bezier curve
;;

http://codereview.appspot.com/1425041/diff/1/12#newcode342
scm/stencil.scm:342: ; Apply L'hopital's rule to get the zeros if 0/0
;;

http://codereview.appspot.com/1425041/diff/1/12#newcode538
scm/stencil.scm:538: "Returns a function drawing an arrow from here to
@var{destination}."
doc start?/end?

http://codereview.appspot.com/1425041/diff/1/13
File scm/woodwind-diagrams.scm (right):

http://codereview.appspot.com/1425041/diff/1/13#newcode88
scm/woodwind-diagrams.scm:88: (if (member y input-list) #t #f)))
(pair? (memv y input-list))

http://codereview.appspot.com/1425041/diff/1/13#newcode117
scm/woodwind-diagrams.scm:117: (define (return-x x) x)
= identity

http://codereview.appspot.com/1425041/diff/1/13#newcode177
scm/woodwind-diagrams.scm:177: ;Makes the alist used to generate
woodwind-data-alist.
move inside function

http://codereview.appspot.com/1425041/diff/1/13#newcode234
scm/woodwind-diagrams.scm:234: (ly:stencil-in-color stencil 0.5 0.5 0.5))
(ly:stencil-in-color stencil grey))

http://codereview.appspot.com/1425041/diff/1/13#newcode420
scm/woodwind-diagrams.scm:420: (ly:text-interface::interpret-markup
interpret-markup

http://codereview.appspot.com/1425041/diff/1/13#newcode434
scm/woodwind-diagrams.scm:434: (ly:text-interface::interpret-markup
interpret-markup

http://codereview.appspot.com/1425041/diff/1/13#newcode799
scm/woodwind-diagrams.scm:799: (define (generate-flute-family-entry flute-name)
I think it might be better to move these alists to a separate file.

http://codereview.appspot.com/1425041/diff/1/13#newcode805
scm/woodwind-diagrams.scm:805: `(,flute-name .
`(,flute-name
  . (

etc.

(see scm/define-grobs.scm for an example of alist formatting)

http://codereview.appspot.com/1425041/diff/1/13#newcode2597
scm/woodwind-diagrams.scm:2597: (define (update-possb-list input-key
possibility-list cannonic-list)
canonic-list

http://codereview.appspot.com/1425041/diff/1/13#newcode2599
scm/woodwind-diagrams.scm:2599: (throw
use ly:error or ly:warning

http://codereview.appspot.com/1425041/diff/1/13#newcode2833
scm/woodwind-diagrams.scm:2833: The following instruments are supported:
you need to catch invalid instrument names

http://codereview.appspot.com/1425041/diff/1/13#newcode2928
scm/woodwind-diagrams.scm:2928: (assemble-stencils
markup commands return stencils, so (assemble-stencils ...) should suffice

[Carl, 2010-05-31 17:47:28]

Mike,

I'm sorry, but I had some comments earlier that I somehow erased with
inadvertent keystrokes, so I added them back in.

Thanks,

Carl

http://codereview.appspot.com/1425041/diff/1/8
File scm/lily-library.scm (right):

http://codereview.appspot.com/1425041/diff/1/8#newcode522
scm/lily-library.scm:522: (+
You might want to define some functions (perhaps in lily-lib.scm)

(define (square x) (* x x))

(define (radius coordinate)
  (sqrt (+ (square (car coordinate) (cdr coordinate)))))

and perhaps 

(define (x-value coordinate) (car coordinate))
(define (y-value coordinate) (cdr coordinate))

http://codereview.appspot.com/1425041/diff/1/8#newcode527
scm/lily-library.scm:527: (eqv? (interval-start interval) 0.0)
You can avoid all these tests by just using the two-argument function of atan

(angle (atan (interval-end interval) (interval-start interval)))

or even better

(angle (atan (y-value coordinate) (x-value coordinate)))

recognizing that these cons cells aren't really intervals.

http://codereview.appspot.com/1425041/diff/1/10
File scm/output-ps.scm (right):

http://codereview.appspot.com/1425041/diff/1/10#newcode105
scm/output-ps.scm:105: (define (connected-shape pointlist thick x-scale y-scale
connect fill)
On 2010/05/31 14:06:49, Carl wrote:
> I find it much easier to read this function in this indentation format:
> 
> (define (connected-shape pointlist thick x-scale y-scale connect fill)
>    (ly:format "~a~4f ~4f ~4f ~4f ~a ~a draw_connected_shape"
>      (string-concatenate
>        (map (lambda (x) 
>               (apply (if (eq? (length x) 6)
>                (lambda (x1 x2 x3 x4 x5 x6) 
>                  (ly:format "~4f ~4f ~4f ~4f ~4f ~4f 6 " x1 x2 x3 x4 x5 x6))
>                (lambda (x1 x2) 
>                  (ly:format "~4f ~4f 2 " x1 x2)))
>              x)) 
>          (reverse pointlist)))       (length pointlist)
>        x-scale
>        y-scale
>        thick       (if connect "true" "false")
>        (if fill "true" "false")))
> 

Done.

http://codereview.appspot.com/1425041/diff/1/11
File scm/output-svg.scm (right):

http://codereview.appspot.com/1425041/diff/1/11#newcode354
scm/output-svg.scm:354: ((new-start-angle
You can take advantage of the functions you've defined elsewhere:

(new-start-angle (* PI-OVER-180 (angle-0-360 start-angle)))

[Patrick McCarty, 2010-06-18 22:42:00]

Hi Mike,

This is very impressive.  Thanks for your work on this.  I just have a few
comments for you about the SVG-related code.

Thanks,
Patrick

http://codereview.appspot.com/1425041/diff/12001/13007
File scm/lily-library.scm (right):

http://codereview.appspot.com/1425041/diff/12001/13007#newcode583
scm/lily-library.scm:583: 
If you want to use these procedures in output-svg.scm, they all need to be
`define-public'-ified.  Like so:

(define-public PI (* 4 (atan 1)))

etc.

http://codereview.appspot.com/1425041/diff/12001/13010
File scm/output-svg.scm (right):

http://codereview.appspot.com/1425041/diff/12001/13010#newcode352
scm/output-svg.scm:352: (define (partial-ellipse x-radius y-radius start-angle
end-angle thick connect fill)
Move the definition of `new-start-angle' to here so that this code will compile
(`new-start-angle' is used in `make-ellipse-radius')

http://codereview.appspot.com/1425041/diff/12001/13010#newcode362
scm/output-svg.scm:362: (new-end-angle (* PI-OVER-180 (angle-0-360 end-angle)))
PI-OVER-180 and other procedures will work once you make the changes in
lily-library.scm.

http://codereview.appspot.com/1425041/diff/12001/13010#newcode399
scm/output-svg.scm:399: (if
Please condense this block like so:

(if connect
    (ly:format "L~4f,~4f"
	       (* start-radius (cos new-start-angle))
	       (- (* start-radius (sin new-start-angle))))
    "")))))))

http://codereview.appspot.com/1425041/diff/12001/13010#newcode418
scm/output-svg.scm:418: ;; Hopefully will be mitigated by ~4f below.
Uh, how is this a security risk?  PS code is not interpreted by SVG agents, and
here we are just dealing with simple numbers and strings that turn into SVG
markup.

http://codereview.appspot.com/1425041/diff/12001/13010#newcode421
scm/output-svg.scm:421: (string-concatenate
I would condense this block too, to make it more readable:

(string-concatenate
  (map (lambda (x)
	 (apply
	   (if (eq? (length x) 6)
	       (lambda (x1 x2 x3 x4 x5 x6)
		 (ly:format "C~4f ~4f ~4f ~4f ~4f ~4f"
			    (* x1 x-scale)
			    (- (* x2 y-scale))
			    (* x3 x-scale)
			    (- (* x4 y-scale))
			    (* x5 x-scale)
			    (- (* x6 y-scale))))
	       (lambda (x1 x2)
		 (ly:format "L~4f ~4f"
			    (* x-scale x1)
			    (- (* y-scale x2)))))
	   x))
       pointlist))

[dak, 2013-04-15 12:38:26]

https://codereview.appspot.com/1425041/diff/12001/ps/music-drawing-routines.ps
File ps/music-drawing-routines.ps (right):

https://codereview.appspot.com/1425041/diff/12001/ps/music-drawing-routines.p...
ps/music-drawing-routines.ps:196: +% Note that filled is not boolean to permit
for different graylevels (ie for trill keys)
What's the reason for this falsehood?  "filled" is quite obviously boolean and
used as such.

[mike7, 2013-04-15 12:48:38]

On 15 avr. 2013, at 15:38, dak@gnu.org wrote:

> 
>
https://codereview.appspot.com/1425041/diff/12001/ps/music-drawing-routines.ps
> File ps/music-drawing-routines.ps (right):
> 
>
https://codereview.appspot.com/1425041/diff/12001/ps/music-drawing-routines.p...
> ps/music-drawing-routines.ps:196: +% Note that filled is not boolean to
> permit for different graylevels (ie for trill keys)
> What's the reason for this falsehood?  "filled" is quite obviously
> boolean and used as such.

You're right, this comment is wrong.  Seems like filled is only being used for
boolean.

Cheers,
MS