Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
LGTM. Carl http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
On 2010/08/13 11:04:00, Carl wrote: LGTM. Carl Thanks Carl. I'll push this patchset shortly (with regression tests). -Patrick http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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 http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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? http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
Op dinsdag 13-07-2010 om 21:49 uur [tijdzone +], schreef pnor...@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 jann...@gnu.org | GNU LilyPond http://lilypond.org Freelance IT http://JoyOfSource.com | Avatar® http://AvatarAcademy.nl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
On 2010/07/05 05:48:16, MikeSol wrote: 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. For purposes of human readability, I think we should keep lineto and curveto, even if they are automatically detectable. But I also think that even if curveto is detectable, lineto isn't because moveto has the same number of arguments as lineto. 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? I suspect that moveto's could be rewritten as calls to ly:stencil-translate, but then you'd need to change all the arguments. And I don't see the benefit of it. Both ps and svg have the moveto functionality available; we might as well use their functionality as the common calls, in my opinion. 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. I, too am wary of loop and for-each constructs in Scheme in general, but I think that in this case it works quite well. Three.5, I think the extents are off for the curves in certain problematic cases, Certainly the extents are off for the curves; the control points bound the curve but don't define the curve. So the extents may be a little bit larger than exact. 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. If path and connected-shape are to be merged, I'd like to see the resulting stencil named path and take the input that's defined in Patrick's patch. I think that path is a very user-friendly way to create complex stencils, and connected-shape somewhat less so. 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. I'm sure that you're good for this offer. Thanks for doing so. Carl http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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, pnor...@gmail.com pnor...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
On 6/26/10 3:27 AM, Mike Solomon mike...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
Op zaterdag 26-06-2010 om 00:06 uur [tijdzone +], schreef pnor...@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 jann...@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 jann...@peder:~/vc/lilypond Jan. -- Jan Nieuwenhuizen jann...@gnu.org | GNU LilyPond http://lilypond.org Freelance IT http://JoyOfSource.com | Avatar® http://AvatarAcademy.nl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
On Sat, Jun 26, 2010 at 11:47 AM, Jan Nieuwenhuizen janneke-l...@xs4all.nl wrote: Op zaterdag 26-06-2010 om 00:06 uur [tijdzone +], schreef pnor...@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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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). http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
Reviewers: carl.d.sorensen_gmail.com, Message: 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. Description: 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 uses all of these optional parameters. * Redo the implementation of \eyeglasses using this new \path command. This allows for using \eyeglasses in the SVG output. TODO/questions: * Are the arguments for \path in a sane order? * Should some of the arguments have default values, not requiring user input unless they want to override them (e.g. line-join and line-cap)? * The documentation of the command can probably be condensed and/or reformatted. * Consider adding support for variable path thickness (to \eyeglasses). * Need a better inline snippet for demonstrating \path Please review this at http://codereview.appspot.com/1730044/show Affected files: M scm/define-markup-commands.scm M scm/output-ps.scm M scm/output-svg.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Add \path markup command, and use it for \eyeglasses. (issue1730044)
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. http://codereview.appspot.com/1730044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
