Code review - Issue 4714043: Add hihat halfopen glyph to fonthttps://codereview.appspot.com/2011-08-22T20:10:24+00:00rietveld
Message from unknown
2011-07-14T01:41:41+00:00Carlurn:md5:f945d1607a6c162cbd9a4aab75590711
Message from Carl.D.Sorensen@gmail.com
2011-07-14T01:43:56+00:00Carlurn:md5:7342646f0ca58950c821f5ccb98873d9
Here is the patch for the new hihat halfopen glyph as requested in Issue 1688.
If you are not building in a clean directory, you will need to do
rm mf/out/* && make
in order to generate the font with the new glyph
Message from unknown
2011-07-14T02:01:07+00:00Carlurn:md5:0ebb114c00d535e0dd1413683226643a
Message from lemniskata.bernoullego@gmail.com
2011-07-14T19:50:37+00:00Janek Warcholurn:md5:0af399a7154b3f2e787105fa26140def
The slash looks too long to me, especially when compared to halfopen (unstopped). I'd change factor to
factor := 11/10 * sqrt(1 + 25/16);
or maybe even
factor := 4/4 * sqrt(1 + 25/16);
otherwise LGTM.
thanks,
Janek
Message from Carl.D.Sorensen@gmail.com
2011-07-14T20:07:53+00:00Carlurn:md5:f3b056e607a199fb6872b833416a5117
On 2011/07/14 19:50:37, Janek Warchol wrote:
> The slash looks too long to me, especially when compared to halfopen
> (unstopped).
I wondered about that myself. The line is exactly the same length as the line in halfopen. Maybe I should make the glyph the same height as halfopen, instead of having the line be the same length.
I haven't been able to find any good examples of the notation on the web to be able to reference for the appropriate size.
Thanks,
Carl
Message from lemniskata.bernoullego@gmail.com
2011-07-14T20:23:46+00:00Janek Warcholurn:md5:873d17b54c88d23fecc92233163aa1cc
2011/7/14 <Carl.D.Sorensen@gmail.com>:
> On 2011/07/14 19:50:37, Janek Warchol wrote:
>>
>> The slash looks too long to me, especially when compared to halfopen
>> (unstopped).
>
> I wondered about that myself. The line is exactly the same length as
> the line in halfopen.
Either dvi proofing (run
mf feta20; gftodvi $feta20.2602gf
and open resulting dvi file, hihat is 72nd glyph) fools me or you've
miscalculated it.
I've measured it on my screen and the slash in hihat halfopen is about
1.25 longer than slash in halfopen unstopped.
> Maybe I should make the glyph the same height as
> halfopen, instead of having the line be the same length.
>
> I haven't been able to find any good examples of the notation on the web
> to be able to reference for the appropriate size.
I don't have anything to compare too, but i think it'll be fine if we
just make it consistent with halfopen unstopped.
cheers,
Janek
Message from pkx166h@gmail.com
2011-07-14T20:35:21+00:00pkx166hurn:md5:1b07be69752d10542384b60634ca732a
Make and reg tests ok.
Message from n.puttock@gmail.com
2011-07-14T20:37:03+00:00Neil Puttockurn:md5:b3f7e87d93f528aff998b8b1b3ed0776
Hi Carl,
I can't comment on the code itself, but the name of the new glyph seems wrong to me, since it implies that the existing glyph isn't for hihat. IIRC, it was added specifically for this purpose, and looking at Elaine Gould's example (p. 298 of Behind Bars) I'd say we already have the correct default glyph for half-open hihat.
Cheers,
Neil
Message from unknown
2011-08-17T04:41:04+00:00Carlurn:md5:7c11753ad30968682bab4310e2b2db4e
Message from Carl.D.Sorensen@gmail.com
2011-08-17T04:44:09+00:00Carlurn:md5:b7e766c2d4449fda37b34c13c7a721f0
I've got a new patch set up.
I changed the name to halfopenvertical.
I think that the halfopen symbol currently in the font is used for french horn, but Gould does use it for hihat. So I decided to just describe the symbol, rather than specify its use.
Also, I shortened up the vertical stroke. Janek, you were right about the line being 25% too long. I had calculated the length in terms of the width of the zero, but drew it in terms of the height of the zero. At any rate, I shortened it by even more than the 25%, because I think it looks better.
Thanks for your earlier reviews!
Message from lemzwerg@googlemail.com
2011-08-17T05:25:36+00:00lemzwergurn:md5:b58a44f1f2378db46a6e08592d5acfc1
LGTM.
http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf
File mf/feta-scripts.mf (right):
http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf#newcode633
mf/feta-scripts.mf:633: height# / 2, height# / 2);
Vertical align mismatch.
Message from unknown
2011-08-17T05:35:17+00:00Carlurn:md5:fcfb57f23a5203d66975671926db1016
Message from Carl.D.Sorensen@gmail.com
2011-08-17T05:35:34+00:00Carlurn:md5:51926566b57a2e79c0d96c829a5315d0
Thanks Werner. Fixed.
http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf
File mf/feta-scripts.mf (right):
http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf#newcode633
mf/feta-scripts.mf:633: height# / 2, height# / 2);
On 2011/08/17 05:25:36, lemzwerg wrote:
> Vertical align mismatch.
Done.
Message from janek.lilypond@gmail.com
2011-08-22T20:10:24+00:00janekurn:md5:8e516745211c8326517c52e8d4f86986
LGTM!
(and sorry for delay)
thanks,
Janek