Re: Fixes position of mensural c clef (issue 6503091)
If you do mf '\mode:=proof; input parmesan20' gftodvi parmesan20.2602gf and view the resulting parmesan20.dvi with xdvi, go to your glyph, then press `10 s' to set the shrink factor to 10. This makes the image small enough that you can see even the part of the glyph which is below the lower edge of the paper (indicated by the red box). http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
On 2012/09/25 14:04:03, lemzwerg wrote: LGTM, except one small issue. http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf#newcode816 mf/parmesan-clefs.mf:816: 2.2 reduced_il#); The bbox is still too small. Agreed. Will fix this in the next patch. I tried a variety of ways of displaying the bounding box, but can only seem to be able show the right edge. Is there a way of showing the whole box? http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
LGTM, except one small issue. http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf#newcode816 mf/parmesan-clefs.mf:816: 2.2 reduced_il#); The bbox is still too small. I suggest that you define z2 like this: x2l = 0; top y2 = 2.2 reduced_il; This makes the left vertical element a bit shorter which shouldn't be noticeable, however. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Please review http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
When you say "paper of all available sizes" I don't understand
why changing the paper size should have any effect.
Could you explain?
Sorry, bad wording. I mean the appearance of all available sizes
printed out on paper.
Is there guidance on the the correct way to calculate
the parameters to set_char_box()?
Normally, it should be the tightest bounding box. Just look at other
glyphs.
The benefit of quartercurve is that it
takes the place of 2 points, so is actually shorter.
? What do you mean with `takes the place of 2 points'? What I suggest
is to insert
pickup pencircle scaled 0.5 blot_diameter;
at the beginning of the glyph so that `top', `lft', `rt', and `bot' of a
point are defined, then using e.g.
...
z2r
-- top z3r{down}
.. {left}rt z3r
-- etc.
This is much shorter than your `quartercircle' stuff :-)
http://codereview.appspot.com/6503091/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
- Original Message -
From:
To: ; ; ;
;
Cc: ;
Sent: Monday, September 24, 2012 6:34 PM
Subject: Re: Fixes position of mensural c clef (issue 6503091)
The glyph shape is OK, thanks. Have you tested the appearance on paper
of all available sizes, using a 300 or 600dpi printer?
No. Does this often come out strange? When you say "paper of all available
sizes" I don't understand why changing the paper size should have any
effect. Could you explain?
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (left):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#oldcode902
mf/parmesan-clefs.mf:902:
The dimension of the bounding box is not correct. It's far too large,
as can be seen in the DVI proof.
I was initially aware of this, but then forgot. Is there guidance on the the
correct way to calculate the parameters to set_char_box()?
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (right):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode791
mf/parmesan-clefs.mf:791: ..z2+(0, blot_rad)
This must be `.. {up}z2 + (0, blot_rad)', otherwise the curve doesn't
start with the right direction. This wouldn't be that bad, but it were
different to all other glyph shapes.
Ditto for all other transitions from `--' to `..' (this is, using the
proper `{up}', `{right}', etc.).
Excellent. I was aware of the discountinuity, but not the bext way to fix
it.
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode793
mf/parmesan-clefs.mf:793: --quartercircle scaled blot_diameter rotated
180 shifted (z3r + (blot_rad, blot_rad))
Using `quartercircle' gives good results in the type1 output. I only
ask you to make the line fit into 80 columns :-)
OK. I really should tell you to get a bigger monitor :-)
On the other hand, why not simply using something like
startcurve_point{down} .. {right}endcurve_point
? This might be shorter, and easier to read.
Didn't think of this as an option. The benefit of quartercurve is that it
takes the place of 2 points, so is actually shorter. I'd like to stick with
what I now have, but will remember this for the future.
http://codereview.appspot.com/6503091/
--
Phil Holmes
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
The glyph shape is OK, thanks. Have you tested the appearance on paper
of all available sizes, using a 300 or 600dpi printer?
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (left):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#oldcode902
mf/parmesan-clefs.mf:902:
The dimension of the bounding box is not correct. It's far too large,
as can be seen in the DVI proof.
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (right):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode791
mf/parmesan-clefs.mf:791: ..z2+(0, blot_rad)
This must be `.. {up}z2 + (0, blot_rad)', otherwise the curve doesn't
start with the right direction. This wouldn't be that bad, but it were
different to all other glyph shapes.
Ditto for all other transitions from `--' to `..' (this is, using the
proper `{up}', `{right}', etc.).
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode793
mf/parmesan-clefs.mf:793: --quartercircle scaled blot_diameter rotated
180 shifted (z3r + (blot_rad, blot_rad))
Using `quartercircle' gives good results in the type1 output. I only
ask you to make the line fit into 80 columns :-)
On the other hand, why not simply using something like
startcurve_point{down} .. {right}endcurve_point
? This might be shorter, and easier to read.
http://codereview.appspot.com/6503091/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Please review, including image on the tracker. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
- Original Message - From: To: ; ; ; ; Cc: ; Sent: Monday, September 24, 2012 2:30 PM Subject: Re: Fixes position of mensural c clef (issue 6503091) Well done! It's not there yet due to some bugs I believe, but besides this the code looks OK. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode764 mf/parmesan-clefs.mf:764: save reduced_il, vert_thick, hor_thick, blot_rad; Until the great tab removal operation I ask you to use tabs (with 1tab=8columns) to stay in sync with the remaining code. Willdo. It was just what the editor defaulted to. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode787 mf/parmesan-clefs.mf:787: pat = z1l -- z2l..z2+(0, blot_rad)..z2r The remaining MF files format it like this: pat = z1l -- z2l .. z2 + (0, blot_rad) .. z2r etc. Willdo. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode794 mf/parmesan-clefs.mf:794: unfill pat_mid; The unfill operation leaves no border at the right. Is it really intentional that there is no vertical line at the right of the glyph? At least the clefs in the facsimile scans look differently. It was my intention. However, I've looked at even more (Willi Apel's book) and concluded that there should be a bar at the right - it appears the copyists of some of the Dufay work did not write this with an extra stroke, and so it's not consistently there. I will change this to create the stroke. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode803 mf/parmesan-clefs.mf:803: penlabels (1,2,3,4,5,6); If point X is defined with `penposX', you should use `penlabels(..., X, ...)', otherwise use `labels', but not both at the same time. Thanks http://codereview.appspot.com/6503091/ -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Well done! It's not there yet due to some bugs I believe, but besides this the code looks OK. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode764 mf/parmesan-clefs.mf:764: save reduced_il, vert_thick, hor_thick, blot_rad; Until the great tab removal operation I ask you to use tabs (with 1tab=8columns) to stay in sync with the remaining code. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode787 mf/parmesan-clefs.mf:787: pat = z1l -- z2l..z2+(0, blot_rad)..z2r The remaining MF files format it like this: pat = z1l -- z2l .. z2 + (0, blot_rad) .. z2r etc. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode794 mf/parmesan-clefs.mf:794: unfill pat_mid; The unfill operation leaves no border at the right. Is it really intentional that there is no vertical line at the right of the glyph? At least the clefs in the facsimile scans look differently. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode803 mf/parmesan-clefs.mf:803: penlabels (1,2,3,4,5,6); If point X is defined with `penposX', you should use `penlabels(..., X, ...)', otherwise use `labels', but not both at the same time. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Please review. I can provide an image on the tracker if required. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
> Link here: http://www.philholmes.net/lilypond/mensuralclefs.pdf Thanks. >> Regarding the shape: What are the reasons for changing it? Do you >> think that an asymmetrical clef looks better, coming nearer to >> hand-written originals? Then document it. Or is it an artifact of >> your first MF tries? Then fix it :-) > > OK. OK. Just to be sure. You want me to fix it :-) :-) Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
- Original Message - From: "Werner LEMBERG" To: Cc: ; ; ; ; ; Sent: Sunday, September 09, 2012 7:37 AM Subject: Re: Fixes position of mensural c clef (issue 6503091) [Has your mail containing scanned clef samples reached the list? I haven't received it yet.] It arrivedback directly to me, but I've just noticed it didn't come via the mailing list. Perhaps it's too large (750k). Link here: http://www.philholmes.net/lilypond/mensuralclefs.pdf Your are changing the shape without documenting this fact. And the problem is not 0.1 staff space but loosing the vertical symmetry for no good reasons. My point is that there is no vertical symmetry in hand-drawn 15C clefs, so there is no point it trying to recreate it in 21C machine drawn clefs. While I don't agree with this reasoning, this is not what I'm talking about. Whoever created the original clef made it arbitrarily symmetrical, with no justification, so changing this needs no other justification. Again, you are missing my point. It's not about justification, but about *documentation*. The description of your patch *must* *document* what you are doing, especially if you are changing the shape. Looking at your code changes, this can't be deduced easily. Regarding the shape: What are the reasons for changing it? Do you think that an asymmetrical clef looks better, coming nearer to hand-written originals? Then document it. Or is it an artifact of your first MF tries? Then fix it :-) OK. OK. Just to be sure. You want me to fix it :-) -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
[Has your mail containing scanned clef samples reached the list? I haven't received it yet.] >> Your are changing the shape without documenting this fact. And the >> problem is not 0.1 staff space but loosing the vertical symmetry >> for no good reasons. > > My point is that there is no vertical symmetry in hand-drawn 15C > clefs, so there is no point it trying to recreate it in 21C machine > drawn clefs. While I don't agree with this reasoning, this is not what I'm talking about. > Whoever created the original clef made it arbitrarily symmetrical, > with no justification, so changing this needs no other > justification. Again, you are missing my point. It's not about justification, but about *documentation*. The description of your patch *must* *document* what you are doing, especially if you are changing the shape. Looking at your code changes, this can't be deduced easily. Regarding the shape: What are the reasons for changing it? Do you think that an asymmetrical clef looks better, coming nearer to hand-written originals? Then document it. Or is it an artifact of your first MF tries? Then fix it :-) Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
- Original Message - From: To: ; ; ; Cc: ; Sent: Saturday, September 08, 2012 9:52 PM Subject: Re: Fixes position of mensural c clef (issue 6503091) I was aware of a slight difference, although actually I'm not certain it's that important - these are machine drawn glyphs intending to replicate hand-drawn clefs from the 15th century. How important is 0.1 staff space? This is not the point. Your are changing the shape without documenting this fact. And the problem is not 0.1 staff space but loosing the vertical symmetry for no good reasons. My point is that there is no vertical symmetry in hand-drawn 15C clefs, so there is no point it trying to recreate it in 21C machine drawn clefs. Whoever created the original clef made it arbitrarily symmetrical, with no justification, so changing this needs no other justification. -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
I was aware of a slight difference, although actually I'm not certain it's that important - these are machine drawn glyphs intending to replicate hand-drawn clefs from the 15th century. How important is 0.1 staff space? This is not the point. Your are changing the shape without documenting this fact. And the problem is not 0.1 staff space but loosing the vertical symmetry for no good reasons. I use Gedit and by default for mf files it puts tabs at 4 spaces, and so the indenting looked completely awry. Good goodness! This is a plain wrong default. If in doubt, *always* use 8 spaces for a tab. This is UNIX standard since the very beginning. Everything else is evil. IMHO mixing tabs and spaces is a really bad idea for that reason - different editors indent different ways. Hmm, the rule `1 physical tab = 8 spaces' is standard on UNIX boxes since 30 years? 40 years? As for the overlaps. Ditto I'll look at that. The original code calculated end points in a very odd way - it allowed, for example, of different amounts of shift between the breves, and then ignored that in the calculation of the decorations. I'll see if I can improve this. Thanks. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
>> I fully agree. Since we have no support for (3) yet, I will do a bit >> of (2), and I really hope that Phil can bear with me :-) > > Is it really such a big deal if the code formatting is not perfectly > consistent in every little detail? In particular, I would favor > option > > 5. (relax 2 a bit) [...] My `a bit of (2)' is exactly your (5). Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
On 2012-09-08 08:11, Werner LEMBERG wrote: 1. reject any offers of help from contributors who do not follow the existing formatting. 2. educate each contributor individually, go through multiple rounds of each patch to adjust the formatting, etc. 3. use an automatic formatting tool. 4. combine 2 and 3: use an automatic formatting tool for most of the code style, but still require some additional manual formatting (and go through a few rounds of reviews if necessary). I favor either 3 or 4; we are not in a position to be gratuitously rejecting patches, and having "finicky" manual formatting will discourage some contributors. I fully agree. Since we have no support for (3) yet, I will do a bit of (2), and I really hope that Phil can bear with me :-) Is it really such a big deal if the code formatting is not perfectly consistent in every little detail? In particular, I would favor option 5. (relax 2 a bit) educate each contributor individually when giving feedback about the patch. Don't insist on a new patch for formatting alone, but tell the submitter that (s)he should (i.e. following the RFC terminology strongly recommended, but not absolutely required) fix the indentation before applying. Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://www.kainhofer.com * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * Edition Kainhofer, Music Publisher, http://www.edition-kainhofer.com ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
A quick note to Werner. 1) apologies for being rather brusque, but I'd pretty much killed my self getting this sorted - the original code had some very odd calculation methods in it, and it's the first time I'd ever touched metafont. Your follow up is helpful - please ignore my recent comment on Google - I'm minus a monitor at present and working on a little used laptop and don't have mail at present, so I'm not working linearly in time. I'll look at changing the lengths - I was aware of a slight difference, although actually I'm not certain it's that important - these are machine drawn glyphs intending to replicate hand-drawn clefs from the 15th century. How important is 0.1 staff space? As for the indents, I now understand what you're saying. I use Gedit and by default for mf files it puts tabs at 4 spaces, and so the indenting looked completely awry. IMHO mixing tabs and spaces is a really bad idea for that reason - different editors indent different ways. I just tidied a few of the more obvious ones near my code, but will get rid of those once I have a working monitor. As for the overlaps. Ditto I'll look at that. The original code calculated end points in a very odd way - it allowed, for example, of different amounts of shift between the breves, and then ignored that in the calculation of the decorations. I'll see if I can improve this. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
> I think some of the confusion could be avoided by running all mf > files through expand(1). This is a good idea, and I will do so after Phil has done his work. >> I'm really bewildered that it is apparently so hard for many >> contributors to format and indent new code in the same manner as >> the surrounding code. Is this lack of experience? Is this >> ignorance? Is it arrogance? > > I would rather keep this neutral from any accusation of ignorance or > arrogance -- in *both* directions. I could easily argue it either > way. Certainly. Note that I don't *accuse* anyone, especially not Phil. I'm grateful that he's working on the code, but I'm really astonished about his replies. > 1. reject any offers of help from contributors who do not follow the >existing formatting. > > 2. educate each contributor individually, go through multiple rounds >of each patch to adjust the formatting, etc. > > 3. use an automatic formatting tool. > > 4. combine 2 and 3: use an automatic formatting tool for most of the >code style, but still require some additional manual formatting >(and go through a few rounds of reviews if necessary). > > I favor either 3 or 4; we are not in a position to be gratuitously > rejecting patches, and having "finicky" manual formatting will > discourage some contributors. I fully agree. Since we have no support for (3) yet, I will do a bit of (2), and I really hope that Phil can bear with me :-) Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
On Sat, Sep 08, 2012 at 01:01:15AM +, lemzw...@googlemail.com wrote: > >I mean, in this file there's a weird mixture of tabs and spaces > > Weird? It's always tabs (which is assumed to be represented visually by > eight spaces on screen) followed by less than eight spaces. However, some editors use 4-space tabs. In general, I think it is undesirable to assume that tabs will always be 8 spaces. I think some of the confusion could be avoided by running all mf files through expand(1). > I'm really bewildered that it is apparently so hard for many > contributors to format and indent new code in the same manner as the > surrounding code. Is this lack of experience? Is this ignorance? Is > it arrogance? I would rather keep this neutral from any accusation of ignorance or arrogance -- in *both* directions. I could easily argue it either way. Let's say "lack of experience with 1980s and 1990s unix text formatting conventions". In those old days, whether it was due to limited disk space or character-based teletype machines or something else, there was widespread awareness of tab characters vs. space characters. These days, there is not so much fondness for this distinction. For example, python gives warnings if you use tabs instead of spaces, and in my own personal vim settings I use "soft tabs" (plus tabstop=4) meaning that a tab key indicates 4 spaces, and pressing a delete key if the cursor is in front of 4 spaces will result in deleting 4 spaces. The bottom line is this: as you noted, many contributors find it difficult to format and indent code exactly like the surrounding code including a mixture of tabs and spaces. I see four options: 1. reject any offers of help from contributors who do not follow the existing formatting. 2. educate each contributor individually, go through multiple rounds of each patch to adjust the formatting, etc. 3. use an automatic formatting tool. 4. combine 2 and 3: use an automatic formatting tool for most of the code style, but still require some additional manual formatting (and go through a few rounds of reviews if necessary). I favor either 3 or 4; we are not in a position to be gratuitously rejecting patches, and having "finicky" manual formatting will discourage some contributors. - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
There isn't any indentation standard for mf files, is there? It's implicitly given by the formatting of all MF files. I know that this is far from ideal, but unless someone is stepping forward to write a specification for GNU indent or something similar so that the indentation can be enforced mechanically, I see absolutely no reason to re-format those files. I mean, in this file there's a weird mixture of tabs and spaces Weird? It's always tabs (which is assumed to be represented visually by eight spaces on screen) followed by less than eight spaces. If you are talking about the indentation: I tried hard to be consistent in all files, trying to align vertically where it makes sense, and where it looks nicely. The indentation level itself is a simply a tab. I'm really bewildered that it is apparently so hard for many contributors to format and indent new code in the same manner as the surrounding code. Is this lack of experience? Is this ignorance? Is it arrogance? http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
I'm sorry, but I find this incredibly unhelpful. "You've done it wrong" is insulting to someone who's spent about 40 hours trying to get it right. The original code was wrong, with any number of incorrect assumtions. If you could point me to what is incorrect, I'll try to fix it. I see nothing wrong with the images you've supplied. Oh! Sorry about that. I thought that the differences between the old and new shape are obvious. If you do a blinking comparison it is easy to see that the lower left and right vertical extensions have become shorter and longer, respectively. Since you don't say anywhere that you are going the change the glyph shape itself, I've assumed (and still assume) that this is an unintentional change. On the other hand, the upper left and right vertical extensions now unnecessarily protrude the clef body, which can be dangerous. From mf/README: If outlines intersect, avoid grazing intersections. In case two outlines intersect in an explicitly defined point, include this point in both intersecting paths to avoid problems due to rounding errors. The mix of tabs and spaces made indentation difficult to follow. Please explain why mixing tabs and spaces is good. It's not about mixing tabs and spaces but about consistency, which I consider extremely important. If you do grep -1 -h 'def .* ([^)]*$' parmesan-custodes.mf you can see that the indentation pattern follows this scheme def foo (expr XXX, ..., YYY, ...) in case the `def' doesn't fit into a single line. The same holds for the other MF files (with three exceptions which are formatting mistakes). Another issue is that your indentation changes actually distract from your real code changes. Your patch has become unnecessarily much longer due to the indentation stuff. If you *really* want to change the indentation, it should be done in a separate patch which should be then tagged as `formatting only' or something like that. BTW, this is true in general: Always try to separate formatting and code changes. I found the line break hindered understanding of the syntax, to no benefit. Most editors extend part 80 characters these days. This is exactly the same issue as above. BTW, I don't share your opinion, and except some well-founded exceptions in parmesan-noteheads.mf, *all* source code lines of the MF files are within the 80-char limit. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode854 mf/parmesan-clefs.mf:854: draw_triple_brevis (exact_center + (0, 1.0 staff_space#), On 2012/09/07 18:27:36, lemzwerg wrote: Any reason why you are changing the indentation which is different to all other mf files? There isn't any indentation standard for mf files, is there? I mean, in this file there's a weird mixture of tabs and spaces http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (left): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#oldcode885 mf/parmesan-clefs.mf:885: I'm sorry, but I find this incredibly unhelpful. "You've done it wrong" is insulting to someone who's spent about 40 hours trying to get it right. The original code was wrong, with any number of incorrect assumtions. If you could point me to what is incorrect, I'll try to fix it. I see nothing wrong with the images you've supplied. http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode854 mf/parmesan-clefs.mf:854: draw_triple_brevis (exact_center + (0, 1.0 staff_space#), The mix of tabs and spaces made indentation difficult to follow. Please explain why mixing tabs and spaces is good. http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode866 mf/parmesan-clefs.mf:866: 2.2 half_reduced_il# + staff_space# - 2 ypart exact_center, I found the line break hindered unserstanding of the syntax, to no benefit. Most editors extend part 80 characters these days. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Here are comparison images of the unpatched and patched glyph. The patched version distorts the shape. I've created the snapshots with fontforge; the call to create the PFB was FONTFORGE=foo mf2pt1 --rounding=0.0001 parmesan16.mf Werner <><>___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (left): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#oldcode885 mf/parmesan-clefs.mf:885: Your code is not correct. I'm sending comparison images to the lilypond-devel list. http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode854 mf/parmesan-clefs.mf:854: draw_triple_brevis (exact_center + (0, 1.0 staff_space#), Any reason why you are changing the indentation which is different to all other mf files? http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode866 mf/parmesan-clefs.mf:866: 2.2 half_reduced_il# + staff_space# - 2 ypart exact_center, Please don't make lines longer than 80 lines. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fixes position of mensural c clef (issue 6503091)
Reviewers: , Message: Please review Description: Well, after about a week of study of metafont code and debugging the original, I've finally arrived at an updated version of the mensural C clef. The previous version is undoubtedly misplaced - the centre of the clef should align with a middle C note, but it was half a staff-space low. There were a number of logic errors in the code which meant that the intended use of the shift parameter invariably failed except for the special case that was in the original code. I've also added support for a clef on each staff line while I was at it. Please review this at http://codereview.appspot.com/6503091/ Affected files: M mf/parmesan-clefs.mf M scm/parser-clef.scm Index: mf/parmesan-clefs.mf diff --git a/mf/parmesan-clefs.mf b/mf/parmesan-clefs.mf index 34a09cf20bc40d7b0def688c2af66fc54731362f..934109fe54d5f63c00a006a99ded0370974ef534 100644 --- a/mf/parmesan-clefs.mf +++ b/mf/parmesan-clefs.mf @@ -528,7 +528,7 @@ enddef; % The other parameters are the same as with `draw_brevis'. % def draw_triple_brevis (expr exact_center, bwidth, bheight, -blinethickness, shift) = + blinethickness, shift) = save brevis_width, brevis_height, linethickness; brevis_width# = bwidth; @@ -580,7 +580,7 @@ def draw_triple_brevis (expr exact_center, bwidth, bheight, shifted (2 xoffs + brevis_width, 0); pat_out := pat_out -- reverse pat_out yscaled -1 - shifted (0, -2 yoffs) + shifted (0, 2 yoffs - 2 shift) -- cycle; fill pat_out; @@ -851,9 +851,9 @@ def draw_mensural_c_clef (expr exact_center, reduction) = define_pixels (reduced_il); - draw_triple_brevis (exact_center + (0, 0.5 staff_space#), - 2 reduced_il#, 0.8 staff_space#, - 0.8 linethickness#, staff_space); + draw_triple_brevis (exact_center + (0, 1.0 staff_space#), + 2 reduced_il#, 0.8 staff_space#, + 0.8 linethickness#, staff_space); save half_reduced_il; @@ -862,10 +862,9 @@ def draw_mensural_c_clef (expr exact_center, reduction) = define_pixels (half_reduced_il); set_char_box (0 - xpart exact_center, - 2 reduced_il# + xpart exact_center, - 2.2 half_reduced_il# + staff_space# - - 2 ypart exact_center, - 2.2 half_reduced_il# + 2 ypart exact_center); + 2 reduced_il# + xpart exact_center, + 2.2 half_reduced_il# + staff_space# - 2 ypart exact_center, + 2.2 half_reduced_il# + 2 ypart exact_center); save xoffs, yoffs; @@ -883,38 +882,35 @@ def draw_mensural_c_clef (expr exact_center, reduction) = pickup pencircle transformed T; ellipse := fullcircle transformed T; - lft x11 = lft x13 = xoffs; - top y11 = yoffs + 2.2 half_reduced_il; - bot y13 = yoffs - 2.2 half_reduced_il - staff_space; - rt x15 = rt x17 = xoffs + brevis_width; - y15 = yoffs + 1.4 half_reduced_il; - y17 = yoffs - 1.4 half_reduced_il - staff_space; - - z12 = z14 yscaled -1 shifted (0, -staff_space); - z14 = z9; - z16 = z18 yscaled -1 shifted (0, -staff_space); - rt z18 = lft z14 shifted (brevis_width, 0); + z12 = z9 shifted (0, 2.5 staff_space); + z11 = z12 shifted (0, 1.8 staff_space); + z14 = z9; % z9 is the bottom left of the drawn triple-clef + z13 = z14 shifted (0, -1.1 staff_space); + z16 = z9 shifted (brevis_width - 1.4 linethickness, 0); + z15 = z16 shifted (0, -0.4 staff_space); + z18 = z16 shifted (0, 3.0 staff_space); + z17 = z18 shifted (0, 0.5 staff_space); penpos12 (1.4 linethickness, 0); penpos14 (1.4 linethickness, 0); penpos16 (1.4 linethickness, 0); penpos18 (1.4 linethickness, 0); - fill get_subpath (ellipse, up, down, z11) + fill get_subpath (ellipse, up, down, z11) %top left ornament -- z12l -- z12r -- cycle; - fill get_subpath (ellipse, down, up, z13) + fill get_subpath (ellipse, down, up, z13) %bottom left ornament -- z14r -- z14l -- cycle; - fill get_subpath (ellipse, up, down, z15) --- z16l + fill get_subpath (ellipse, down, up, z15) %bottom right -- z16r +-- z16l -- cycle; - fill get_subpath (ellipse, down, up, z17) --- z18r + fill get_subpath (ellipse, up, down, z17) %top right -- z18l +-- z18r -- cycle; labels (11, 13, 15, 17); Index: scm/parser-clef.scm diff --git a/scm/pars
