Add \path markup command, and use it for \eyeglasses.

Add \path markup command, and use it for \eyeglasses.

* Modified 'path routines to accept optional arguments that
  allow for customizing line-cap styles, line-join styles, and
  whether the path should be filled or not.

* Add \path markup command that takes two arguments: thickness
  and a list of sublists containing the drawing commands and
  their associated arguments.  Line-cap styles, line-join
  styles, and the fill flag are overrideable as properties.

* Redo the implementation of \eyeglasses using this new \path
  command.  This allows for using \eyeglasses in the SVG
  output.

* Automatically calculate extents of the entire path
  (including Bézier curves) by refactoring and using Mike
  Solomon's work.

* Integrate the functionality of the 'connected-shape stencil
  with the 'path stencil.  The demands of 'connected-shape
  were mostly a subset of those provided by 'path, so remove
  'connected-shape too.

[Carl, 2010-06-20 11:07:37]

Thanks for doing this.  I think this looks quite good.

As I mention below, I think it might be best to eliminate the "style" arguments
and make them part of the properties.  We already have 'thickness as a property,
and we could add 'path-details. 

Or we could make 'path-thickness part of 'path-details, and the all the things
needed to control the path would be part of 'path-details.

Thanks,

Carl

http://codereview.appspot.com/1730044/diff/1/2
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/1/2#newcode622
scm/define-markup-commands.scm:622: (number? number? number? boolean? string?)
Can (and/or should) cap, join, and fill become part of a path-details property?

It's convenient when creating a new markup type to put all the arguments needed
into an argument list.  But it's more consistent with LilyPond syntax to have
all the things that affect appearance be properties that can have default values
set (and documented) in the code.

http://codereview.appspot.com/1730044/diff/1/2#newcode668
scm/define-markup-commands.scm:668: \"
I think this inline snippet is fine.  What characteristics of the snippet need
to be "better" in your opinion?

http://codereview.appspot.com/1730044/diff/1/2#newcode684
scm/define-markup-commands.scm:684: (cons 0 0)))
Is it possible to have the path command estimate reasonable extents, rather
than using (0 . 0) and (0 . 0)?  Since we know the thickness of the line, and
we have a list of points, it seems we should be able to keep track of the
maximum and minimum X and Y coordinates during the path creation.

[Patrick McCarty, 2010-06-21 22:39:53]

Thanks Carl.

I'll be uploading a new patch shortly.

-Patrick

http://codereview.appspot.com/1730044/diff/1/2
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/1/2#newcode622
scm/define-markup-commands.scm:622: (number? number? number? boolean? string?)
On 2010/06/20 11:07:37, Carl wrote:
> Can (and/or should) cap, join, and fill become part of a path-details
property?
> 
> It's convenient when creating a new markup type to put all the arguments
needed
> into an argument list.  But it's more consistent with LilyPond syntax to have
> all the things that affect appearance be properties that can have default
values
> set (and documented) in the code.

Thank you for the suggestion.  I have decided to change these into separate
properties.  Let me know what you think.

http://codereview.appspot.com/1730044/diff/1/2#newcode668
scm/define-markup-commands.scm:668: \"
On 2010/06/20 11:07:37, Carl wrote:
> I think this inline snippet is fine.  What characteristics of the snippet need
> to be "better" in your opinion?

I thought it would be nice to have a snippet that is more "musical", but I
suppose it doesn't matter at this point.

http://codereview.appspot.com/1730044/diff/1/2#newcode684
scm/define-markup-commands.scm:684: (cons 0 0)))
On 2010/06/20 11:07:37, Carl wrote:
> Is it possible to have the path command estimate reasonable extents, rather
> than using (0 . 0) and (0 . 0)?  Since we know the thickness of the line, and
> we have a list of points, it seems we should be able to keep track of the
> maximum and minimum X and Y coordinates during the path creation.

This should be possible, but I'm not sure how to implement it, especially when
relative coordinates are involved.

[Carl, 2010-06-22 03:50:24]

I've added my draft of an algorithm to estimate the extents.

Thanks,

Carl

http://codereview.appspot.com/1730044/diff/1/2
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/1/2#newcode684
scm/define-markup-commands.scm:684: (cons 0 0)))
On 2010/06/21 22:39:53, Patrick McCarty wrote:
> On 2010/06/20 11:07:37, Carl wrote:
> > Is it possible to have the path command estimate reasonable extents, rather
> > than using (0 . 0) and (0 . 0)?  Since we know the thickness of the line,
and
> > we have a list of points, it seems we should be able to keep track of the
> > maximum and minimum X and Y coordinates during the path creation.
> 
> This should be possible, but I'm not sure how to implement it, especially when
> relative coordinates are involved.

My first thought would be to start with currentx=currenty= xmax=xmin=ymax=ymin =
0.

For an absolute move, set currentx = move x, currenty = move y.

For a relative move, set currentx += movex, currenty += movey.

If currentx > xmax, xmax=currentx.  If currenty > ymax, ymax=currenty.
If currentx < xmin, xmin = currentx.  If currenty < ymin, ymin = currenty.

For curves, go one point at a time.  The control points bound the curve, so you
can use the control points as if they were curve points.

When you're done with all the points, add half the thickness to xmax and ymax,
and subtract half the thickness from xmin and ymin.

I haven't tried it, but it seems to me it should work.

[hanwenn, 2010-06-22 04:27:22]

cool idea for a patch

http://codereview.appspot.com/1730044/diff/1/2
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/1/2#newcode667
scm/define-markup-commands.scm:667: closepath
just a random comment: while the syntax/interface makes sense for someone typing
a .ly, you should probably also have a variant of this command that takes a
scheme list.  If you want to estimate the extents of the path, you'll have to
understand the path anyway.

[janneke, 2010-06-22 13:59:41]

On 2010/06/22 04:27:22, hanwenn wrote:
> cool idea for a patch

Interesting -- why is path given as a string instead of
a list of commands, ie

  "moveto 0 0
   rcurveto 1 2"

  '((moveto 0 0)
    (curveto 1 2))

moveto and curveto can be defined in the .scm backends
appropriately -- possibly we already have some primitives
that we could reuse (or want to rename).

[Patrick McCarty, 2010-06-26 00:06:54]

On 2010/06/22 03:50:24, Carl wrote:
> On 2010/06/21 22:39:53, Patrick McCarty wrote:
> > On 2010/06/20 11:07:37, Carl wrote:
> > > Is it possible to have the path command estimate reasonable
> > > extents, rather than using (0 . 0) and (0 . 0)?  Since we
> > > know the thickness of the line, and we have a list of
> > > points, it seems we should be able to keep track of the
> > > maximum and minimum X and Y coordinates during the path
> > > creation.
> > 
> > This should be possible, but I'm not sure how to implement it,
> > especially when relative coordinates are involved.
> 
> My first thought would be to start with currentx=currenty=
> xmax=xmin=ymax=ymin = 0.
> 
> For an absolute move, set currentx = move x, currenty = move y.
> 
> For a relative move, set currentx += movex, currenty += movey.
> 
> If currentx > xmax, xmax=currentx.  If currenty > ymax,
> ymax=currenty.  If currentx < xmin, xmin = currentx.  If
> currenty < ymin, ymin = currenty.
> 
> For curves, go one point at a time.  The control points bound
> the curve, so you can use the control points as if they were
> curve points.
> 
> When you're done with all the points, add half the thickness to
> xmax and ymax, and subtract half the thickness from xmin and
> ymin.
> 
> I haven't tried it, but it seems to me it should work.

Hi Carl,

Thanks for your help.

I've uploaded a new patch set that (more or less) follows your
algorithm above, and also changes the interface again (according
to Han-Wen's and Jan's comments).

Let me know what you think.

Thanks,
Patrick

[mikesol_ufl.edu, 2010-06-26 09:21:59]

Sorry - I should have replied to this earlier, but I was away from my
computer.
In woodwinds, I believe that the "make-connected-shape-stencil" does exactly
this - I use it to draw paths and automate finding extents.  Rather than
adding two things w/ similar functionalities to Lilypond, I think it would
be intelligent to roll both of them into one.  Patrick: can you look at the
patch and let me know what you think?  I have no objection to changing my
work if you feel that there is a better way to implement it along the lines
of the patch that you've uploaded.

~Mike


On 6/26/10 2:06 AM, "pnorcks@gmail.com" <pnorcks@gmail.com> wrote:

> On 2010/06/22 03:50:24, Carl wrote:
>> On 2010/06/21 22:39:53, Patrick McCarty wrote:
>>> On 2010/06/20 11:07:37, Carl wrote:
>>>> Is it possible to have the path command estimate reasonable
>>>> extents, rather than using (0 . 0) and (0 . 0)?  Since we
>>>> know the thickness of the line, and we have a list of
>>>> points, it seems we should be able to keep track of the
>>>> maximum and minimum X and Y coordinates during the path
>>>> creation.
>>> 
>>> This should be possible, but I'm not sure how to implement it,
>>> especially when relative coordinates are involved.
> 
>> My first thought would be to start with currentx=currenty=
>> xmax=xmin=ymax=ymin = 0.
> 
>> For an absolute move, set currentx = move x, currenty = move y.
> 
>> For a relative move, set currentx += movex, currenty += movey.
> 
>> If currentx > xmax, xmax=currentx.  If currenty > ymax,
>> ymax=currenty.  If currentx < xmin, xmin = currentx.  If
>> currenty < ymin, ymin = currenty.
> 
>> For curves, go one point at a time.  The control points bound
>> the curve, so you can use the control points as if they were
>> curve points.
> 
>> When you're done with all the points, add half the thickness to
>> xmax and ymax, and subtract half the thickness from xmin and
>> ymin.
> 
>> I haven't tried it, but it seems to me it should work.
> 
> Hi Carl,
> 
> Thanks for your help.
> 
> I've uploaded a new patch set that (more or less) follows your
> algorithm above, and also changes the interface again (according
> to Han-Wen's and Jan's comments).
> 
> Let me know what you think.
> 
> Thanks,
> Patrick
> 
> 
> http://codereview.appspot.com/1730044/show
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel
> 

[Carl, 2010-06-26 13:35:59]

LGTM, with a couple of minor spacing issues.

Carl

http://codereview.appspot.com/1730044/diff/17001/12002
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/17001/12002#newcode2782
scm/define-markup-commands.scm:2782: (make-override-markup '(line-cap-style .
butt)
indentation -- (make-override-markup is indented too far.

http://codereview.appspot.com/1730044/diff/17001/12003
File scm/output-ps.scm (right):

http://codereview.appspot.com/1730044/diff/17001/12003#newcode279
scm/output-ps.scm:279: (ly:warning (_ "unknown line-cap-style: ~S")
Apparently there's a whitespace error on this line, as it wraps with nothing
visible on the wrapped part.

[c_sorensen, 2010-06-26 13:58:15]

On 6/26/10 3:27 AM, "Mike Solomon" <mikesol@ufl.edu> wrote:

> Sorry - I should have replied to this earlier, but I was away from my
> computer.
> In woodwinds, I believe that the "make-connected-shape-stencil" does exactly
> this - I use it to draw paths and automate finding extents.  Rather than
> adding two things w/ similar functionalities to Lilypond, I think it would
> be intelligent to roll both of them into one.  Patrick: can you look at the
> patch and let me know what you think?  I have no objection to changing my
> work if you feel that there is a better way to implement it along the lines
> of the patch that you've uploaded.


I think the path stencil has a better format that Mike's
make-connected-shape stencil, because its input is tied closely to
postscript and SVG definitions.

However, if the path stencil won't work for the woodwind diagrams, because
it doesn't calculate the extents carefully enough, I'm not opposed to having
both stencils available.

I admit I'm somewhat ignorant of the need for all of the checking in
make-connected-shape, but it seems like overkill for the path stencil.

Thanks,

Carl

[janneke-list_xs4all.nl, 2010-06-26 14:47:14]

Op zaterdag 26-06-2010 om 00:06 uur [tijdzone +0000], schreef
pnorcks@gmail.com:

> Let me know what you think.

It's getting there...

This is not exactly what I had in mind

+    (if (or (eq? (caar commands) 'moveto)
+	    (eq? (caar commands) 'rmoveto))
+	(let ((command (car commands)))
+	  (begin


I was thinking to let GUILE do that work using eval,
look at this below

scm/ps-path.scm:

    ;; ps-path module: postscript PATH routines
    ;; move/add these to scm/output-ps.scm? 

    (define-module (scm ps-path)
      :export (moveto))

    (define-public (moveto x y)
      (format #t "~a ~a moveto (postscript)\n" x y))
    ;; end ps-path.scm

and eval-path.scm:

    #! /usr/bin/guile -s
    !#

    (define (main args)
      (display "main\n")
      (format #t "scm ps-path: ~A\n" (resolve-module '(scm ps-path)))
      (format #t "scm-ps-path moveto: ~A\n" (module-ref (resolve-module '(scm
ps-path)) 'moveto))
      (eval '(moveto 1 2) (resolve-module '(scm ps-path)))
      (map (lambda (x) (eval-string x (resolve-module '(scm ps-path))))
	   (cdr args)))

    (main (command-line))
    ;; end eval-path.scm

    16:44:45 janneke@peder:~/vc/lilypond
    $ GUILE_LOAD_PATH=. ./eval-ps-path.scm 
    main
    scm ps-path: #<directory (scm ps-path) 7f7cccaf3fa0>
    scm-ps-path moveto: #<procedure moveto (x y)>
    1 2 moveto (postscript)
    16:44:54 janneke@peder:~/vc/lilypond

Jan.

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[hanwenn, 2010-06-26 15:10:41]

On Sat, Jun 26, 2010 at 11:47 AM, Jan Nieuwenhuizen
<janneke-list@xs4all.nl> wrote:
> Op zaterdag 26-06-2010 om 00:06 uur [tijdzone +0000], schreef
> pnorcks@gmail.com:
>
>> Let me know what you think.
>
> It's getting there...
>
> This is not exactly what I had in mind
>
> +    (if (or (eq? (caar commands) 'moveto)
> +           (eq? (caar commands) 'rmoveto))
> +       (let ((command (car commands)))
> +         (begin

> I was thinking to let GUILE do that work using eval,
> look at this below

For small languages (like the stencil definitions or paths), explicit
checking might work well enough.  See for example
write-system-signature in stencil.scm.

Also, if you use a module, where would you keep interpretation state
(eg. current-point) ?

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[MikeSol, 2010-07-05 05:48:16]

Great work!

http://codereview.appspot.com/1730044/diff/17001/12002
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/17001/12002#newcode743
scm/define-markup-commands.scm:743: 
I think this is EXCELLENT work and has enough shared features with the
recently-committed connected-shape stencil so that the best elements of the two
can be combined into one unified stencil.  What I think is very strong about
this approach (and what connected-shape lacks) is the relative versus absolute
distinction with coordinates: the fact that here one can choose between the two
on a line-by-line basis is very slick.

I see 3.5 places where the patch may need improvement.  1 is that lineto and
curveto seem unnecessary, as they can be automatically detected by the number of
function arguments.  Two is that I try to use predefined lilypond commands as
much as possible when they exist - could the moveto's be calls to
ly:stencil-translate?  Third is that I am wary of any loop and/or for-each
construct in scheme: I think there is a way to do this with tail-regression that
dispenses with the loop and is more Schemy.  Three.5, I think the extents are
off for the curves in certain problematic cases, but that'll work hopefully work
itself out via this proposition, to wit:

I think the best way to move forward on this patch would be to work on merging
its functionality and nomenclature into the connected-shape stencil.  I'd be
more than happy to iron that out with you on the sidelines - just shoot me an
email and we'll get that up and running.

[Patrick McCarty, 2010-07-13 21:49:56]

I am currently working on integrating my work with Mike's new stencil routines,
so I'll post a new patch when that's ready.

Jan: I like your idea about using a separate module to evaluate the various
commands, though IIUC, this would be duplicating the work done by the backend
"path" procedures in output-ps.scm and output-svg.scm, since they already take a
list of commands and convert them to either PostScript or SVG paths.

Thanks,
Patrick

http://codereview.appspot.com/1730044/diff/17001/12002
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/1730044/diff/17001/12002#newcode2782
scm/define-markup-commands.scm:2782: (make-override-markup '(line-cap-style .
butt)
On 2010/06/26 13:35:59, Carl wrote:
> indentation -- (make-override-markup is indented too far.

Fixed, thanks.

http://codereview.appspot.com/1730044/diff/17001/12003
File scm/output-ps.scm (right):

http://codereview.appspot.com/1730044/diff/17001/12003#newcode279
scm/output-ps.scm:279: (ly:warning (_ "unknown line-cap-style: ~S")
On 2010/06/26 13:35:59, Carl wrote:
> Apparently there's a whitespace error on this line, as it wraps with nothing
> visible on the wrapped part.

This is an artifact of the way Rietveld chose to display the diff.

If you look at the line numbers, you'll see that everything lines up fine.

[janneke-list_xs4all.nl, 2010-07-14 07:02:59]

Op dinsdag 13-07-2010 om 21:49 uur [tijdzone +0000], schreef
pnorcks@gmail.com:

> Jan: I like your idea about using a separate module to evaluate the
> various commands, though IIUC, this would be duplicating the work done
> by the backend "path" procedures in output-ps.scm and output-svg.scm,
> since they already take a list of commands and convert them to either
> PostScript or SVG paths.

Yes, I did not realise that / imagine we had such routines already.
Would [have] be[en] nice to fold this functionality together.


-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[Patrick McCarty, 2010-07-28 18:51:57]

Hi all,

This patchset integrates some of Mike's work in order to fix the stencil extents
for paths and to combine some duplicate functionality.

Thanks,
Patrick

[Carl, 2010-07-28 19:08:16]

Looks very nice!  Just a few comments.

Thanks,

Carl

http://codereview.appspot.com/1730044/diff/35001/36003
File scm/define-stencil-commands.scm (left):

http://codereview.appspot.com/1730044/diff/35001/36003#oldcode30
scm/define-stencil-commands.scm:30: connected-shape
Shouldn't path be in this list to replace connected-shape?

http://codereview.appspot.com/1730044/diff/35001/36007
File scm/stencil.scm (right):

http://codereview.appspot.com/1730044/diff/35001/36007#newcode294
scm/stencil.scm:294: (define-public (path-min-max origin pointlist)
Does this need define-public, instead of define?

Does it need to be called from a .ly file?

http://codereview.appspot.com/1730044/diff/35001/36007#newcode359
scm/stencil.scm:359: (define-public (make-connected-path-stencil pointlist
thickness
If it's define-public, it needs a doc string.  Does it need to be define-public?

[Patrick McCarty, 2010-07-28 20:29:10]

Thanks Carl.  I'll upload my changes shortly.

-Patrick

http://codereview.appspot.com/1730044/diff/35001/36003
File scm/define-stencil-commands.scm (left):

http://codereview.appspot.com/1730044/diff/35001/36003#oldcode30
scm/define-stencil-commands.scm:30: connected-shape
On 2010/07/28 19:08:16, Carl wrote:
> Shouldn't path be in this list to replace connected-shape?

path is already in the list; see line 43.

http://codereview.appspot.com/1730044/diff/35001/36007
File scm/stencil.scm (right):

http://codereview.appspot.com/1730044/diff/35001/36007#newcode294
scm/stencil.scm:294: (define-public (path-min-max origin pointlist)
On 2010/07/28 19:08:16, Carl wrote:
> Does this need define-public, instead of define?
> 
> Does it need to be called from a .ly file?

No, this one is for internal use.  I'll remove the "-public".

http://codereview.appspot.com/1730044/diff/35001/36007#newcode359
scm/stencil.scm:359: (define-public (make-connected-path-stencil pointlist
thickness
On 2010/07/28 19:08:16, Carl wrote:
> If it's define-public, it needs a doc string.  Does it need to be
define-public?

I think this one should be exported, since it is just as useful as the other
stencil routines in this file, even though it's a bit limited.

I'll add a doc string.

[Carl, 2010-08-13 11:04:00]

LGTM. 

Carl

[Patrick McCarty, 2010-08-13 22:11:13]

On 2010/08/13 11:04:00, Carl wrote:
> LGTM. 
> 
> Carl

Thanks Carl.

I'll push this patchset shortly (with regression tests).

-Patrick