Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1405)

Issue 4410049: shortened flags: choosing appropriate flag

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 11 months ago by Janek Warchol
Modified:
12 years, 9 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

choosing appropriate flag for shortened stems Some stems are shorter than usual and require special shortened flags. This code searches through available flag variants and chooses the best one. extracting info about available flag glyphs We need to know what length variants of flags are available in the font used - this might change with time and/or font used, so it cannot be hardcoded. Adding some shorter flags for testing These flags are very rough and used only to see if c++ code is indeed working. Flag functions instead of defining glyphs directly We will need many length variants of every flag. Therefore instead of writing flag code directly in glyph definition, it should be written as a function and called back later as appropriate. The argument shortening is the amount the flag should be shorter than default. As for now it is used in a very primitive way, only to demonstrate the shortening effect and to test c++ code on something.

Patch Set 1 #

Total comments: 28

Patch Set 2 : mostly fixing whitespace #

Patch Set 3 : add a warning in mf file about glyph naming #

Patch Set 4 : adding regtests #

Total comments: 12

Patch Set 5 : whitespace fix #

Patch Set 6 : style nitpicks #

Unified diffs Side-by-side diffs Delta from patch set Stats (+446 lines, -52 lines) Patch
A input/regression/shortened-flags.ly View 1 2 3 1 chunk +30 lines, -0 lines 0 comments Download
A input/regression/shortened-flags-cues.ly View 1 2 3 4 5 1 chunk +28 lines, -0 lines 0 comments Download
A input/regression/shortened-flags-length-overrides.ly View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
M lily/stem.cc View 1 2 3 4 5 21 chunks +165 lines, -23 lines 0 comments Download
M mf/feta-flags.mf View 1 2 3 4 12 chunks +173 lines, -29 lines 0 comments Download

Messages

Total messages: 13
Janek Warchol
Passes regtests (i see a regression in partcombine-midi.ly, however from Graham's message about midi regtests ...
12 years, 11 months ago (2011-04-14 23:07:51 UTC) #1
MikeSol
Please add a stemful regtest with several overrides just to make sure that this works ...
12 years, 11 months ago (2011-04-17 10:23:59 UTC) #2
hanwenn
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode619 lily/stem.cc:619: static vector<Real> available_flag_lengths[2][5]; You cannot do this. This will ...
12 years, 11 months ago (2011-04-18 03:42:53 UTC) #3
Janek Warchol
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode619 lily/stem.cc:619: static vector<Real> available_flag_lengths[2][5]; On 2011/04/18 03:42:53, hanwenn wrote: > ...
12 years, 11 months ago (2011-04-18 08:02:38 UTC) #4
hanwenn
On Mon, Apr 18, 2011 at 5:02 AM, <lemniskata.bernoullego@gmail.com> wrote: > \relative c'' { > ...
12 years, 11 months ago (2011-04-18 14:16:26 UTC) #5
hanwenn
On Mon, Apr 18, 2011 at 11:16 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote: >> } >> ...
12 years, 11 months ago (2011-04-18 14:23:22 UTC) #6
Janek Warchol
Hi, 2011/4/17 <mtsolo@gmail.com>: > Please add a stemful regtest with several overrides just to make ...
12 years, 11 months ago (2011-04-20 11:18:38 UTC) #7
Ian Hulin (gmail)
Spelling commenting and formatting changes only. Cheers, Ian http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly File input/regression/shortened-flags-cues.ly (right): http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly#newcode6 input/regression/shortened-flags-cues.ly:6: } ...
12 years, 9 months ago (2011-06-09 04:55:10 UTC) #8
MikeSol
Hey Janek, All the metafont stuff looks good! Last time we touched base, I recall ...
12 years, 9 months ago (2011-06-09 08:12:53 UTC) #9
Reinhold
http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode612 lily/stem.cc:612: On 2011/06/09 08:12:54, MikeSol wrote: > I remember we ...
12 years, 9 months ago (2011-06-09 08:33:33 UTC) #10
Janek Warchol
Ian, thanks for review! http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly File input/regression/shortened-flags-cues.ly (right): http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly#newcode6 input/regression/shortened-flags-cues.ly:6: } On 2011/06/09 04:55:10, Ian ...
12 years, 9 months ago (2011-06-10 08:30:42 UTC) #11
Janek Warchol
On 2011/06/09 08:12:53, MikeSol wrote: > Hey Janek, > > All the metafont stuff looks ...
12 years, 9 months ago (2011-06-10 08:41:15 UTC) #12
Ian Hulin (gmail)
12 years, 9 months ago (2011-06-10 09:04:17 UTC) #13
On 10/06/11 09:30, lemniskata.bernoullego@gmail.com wrote:
> Ian,
>
> thanks for review!
<snip>
> http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode792
> lily/stem.cc:792:
> On 2011/06/09 04:55:10, Ian Hulin (gmail) wrote:
>> /*
>>     Look up the font character allowing for the variant stem length
>> */
>
> I don't get it...
>
That block comment is what I understood the next statement to be doing ,
and it's the fix in that routine. 

If I've understood it wrong, change the comment words, and it shows the
need for a comment!

Cheers,

Ian
>> You've done a lot of heavy lifting to get here, this is the  fix;
>
> Thanks :)
>
>> shout about it for the benefit of maintainers.
>
> I'm shouting, but maybe my voice is too soft :)
>
> http://codereview.appspot.com/4410049/

Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b