Re: shortened flags: choosing appropriate flag (issue4410049)
On 10/06/11 09:30, lemniskata.bernoull...@gmail.com wrote: > Ian, > > thanks for review! > 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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
On 2011/06/09 08:12:53, MikeSol wrote: Hey Janek, All the metafont stuff looks good! Last time we touched base, I recall that we had talked about looking into embedding a lot of this info into the font - did that prove to be not doable? Other than that, I have one comment below about the C++ stuff. I'm working on it with Carl's help, see "parameters in metafont files" thread. http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode612 lily/stem.cc:612: I remember we worked on a Scheme version of this a while back - I would suggest that you replace this bit of code with that. It'll be easier to maintain and debug, and it also hardcodes less values. There is also a problem with the already computed bit - you are assuming that people will not change fonts midway through a work. I have your patch. I remember that i had send you my comments describing your code, so that you could check whether i understood it correctly. Did you read it? Its in "And still cleaner..." thread, message sent on April 25th. cheers, Janek http://codereview.appspot.com/4410049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
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 Hulin (gmail) wrote:
Spelling nitpick:
chosing => choosing
Done.
http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly#newcode11
input/regression/shortened-flags-cues.ly:11: \new CueVoice {
On 2011/06/09 04:55:10, Ian Hulin (gmail) wrote:
Use four-space indents in this file rather then eight-space ones.
Done.
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#newcode757
lily/stem.cc:757: when a flagged stem has manually overriden length, the
flag should be
On 2011/06/09 04:55:10, Ian Hulin (gmail) wrote:
Nitpick:
overriden => overridden
Done.
http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode790
lily/stem.cc:790: else
On 2011/06/09 04:55:10, Ian Hulin (gmail) wrote:
// (flag_style is not blank)
It's been a long time since the if() and there was lots of involved
stuff in the
then {} clause.
Done.
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...
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/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
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 worked on a Scheme version of this a while back Code for flags in Scheme is in scm/flag-styles.scm, regtests (i.e. sample files) are input/regression/flags-*.ly http://codereview.appspot.com/4410049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
Hey Janek, All the metafont stuff looks good! Last time we touched base, I recall that we had talked about looking into embedding a lot of this info into the font - did that prove to be not doable? Other than that, I have one comment below about the C++ stuff. Cheers, Mike 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: I remember we worked on a Scheme version of this a while back - I would suggest that you replace this bit of code with that. It'll be easier to maintain and debug, and it also hardcodes less values. There is also a problem with the already computed bit - you are assuming that people will not change fonts midway through a work. http://codereview.appspot.com/4410049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
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: }
Spelling nitpick:
chosing => choosing
http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly#newcode11
input/regression/shortened-flags-cues.ly:11: \new CueVoice {
Use four-space indents in this file rather then eight-space ones.
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#newcode757
lily/stem.cc:757: when a flagged stem has manually overriden length, the
flag should be
Nitpick:
overriden => overridden
http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode790
lily/stem.cc:790: else
// (flag_style is not blank)
It's been a long time since the if() and there was lots of involved
stuff in the then {} clause.
http://codereview.appspot.com/4410049/diff/16001/lily/stem.cc#newcode792
lily/stem.cc:792:
/*
Look up the font character allowing for the variant stem length
*/
You've done a lot of heavy lifting to get here, this is the fix; shout
about it for the benefit of maintainers.
http://codereview.appspot.com/4410049/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
Hi,
2011/4/17 :
> Please add a stemful regtest with several overrides just to make sure
> that this works completely.
Added.
2011/4/18 Han-Wen Nienhuys :
> You are initializing this list just once, while in principle it can
> have different values for every loaded font. Even if it may work, it
> is wrong.
>
> As a general rule we try to avoid global variables, since they always
> are or become a pain in the ass.
ok...
I thought i had to cut down computations.
>>> this seems awfully kludgy. Can't we just export another list of dimension
>>> variables directly in the font? See gen-emmentaler-script.py and
>>> mf/out/*table*
>>
>> From what i understand gen-emmentaler-scripts.py it extracts some
>> variables, like glyph size, from mf files. That's quite not what we need
>> to do - the information that we need must be specified by font-designer
>> manually and separately from actual flag variables. (these numbers do
>> not appear in flag code mf variables nor can be calculated from them.)
>
> have a look at feta-generic.mf - the dimensions are abitrary and
> designer specified.
Do you mean line 36
black_notehead_width# := 1.0 staff_space#;
?
I don't see it how it is used in table files later.
May i ask for a small example?
2011/4/18 Han-Wen Nienhuys
>
> On Mon, Apr 18, 2011 at 11:16 AM, Han-Wen Nienhuys wrote:
> >
> > You are initializing this list just once, while in principle it can
> > have different values for every loaded font. Even if it may work, it
> > is wrong.
> >
> > As a general rule we try to avoid global variables, since they always
> > are or become a pain in the ass.
>
> Overall, I dont see why this needs precomputation.
So you say that we should calculate it each time a flag is being
attached to a stem?
> If you give the flags sequential suffixes, eg.
>
> u3_0
> u3_1
> u3_2
>
> then you can do
>
>
> get_flag(Real stemlength) {
> flagname = .. // "u3"
> int suffix = 0
>
> int best = -1;
> Real best_dist = INFINITY;
> while (true) {
> Box b =fm->get_char (flagname + string(suffix))
> if b.empty() {
> break;
> }
> Real dy = b[Y].length() - stemlength
> if abs(dy) < best_dits {
> best_dist = abs(dy)
> best = suffix
> }
> suffix ++;
> }
Looks very straightforward indeed.
If i knew how to add to font a "property" about desired stem length
and how to read it, i think it would be easy to fix this.
thanks,
Janek
PS what about scheme stuff? will it be necessary to rewrite this into
scheme later, so that everything that is done with flags using c++
could be done with scheme? (like in flags-default.ly)
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
On Mon, Apr 18, 2011 at 11:16 AM, Han-Wen Nienhuys wrote:
>> }
>> uses (i think) two different design sizes (one for cues, one for normal
>> notes) and yet everything is fine - change from regular flags to
>> shortened flags take place on note d both in cues and normal notes.
>> Also when i compile this and a copy of this file with
>> #(set-global-staff-size 15) the results are correct.
>> Perhaps i'm not understanding something?
>
> You are initializing this list just once, while in principle it can
> have different values for every loaded font. Even if it may work, it
> is wrong.
>
> As a general rule we try to avoid global variables, since they always
> are or become a pain in the ass.
Overall, I dont see why this needs precomputation.
If you give the flags sequential suffixes, eg.
u3_0
u3_1
u3_2
then you can do
get_flag(Real stemlength) {
flagname = .. // "u3"
int suffix = 0
int best = -1;
Real best_dist = INFINITY;
while (true) {
Box b =fm->get_char (flagname + string(suffix))
if b.empty() {
break;
}
Real dy = b[Y].length() - stemlength
if abs(dy) < best_dits {
best_dist = abs(dy)
best = suffix
}
suffix ++;
}
Mike promised he would have a look at an alternative implementation of
the C++ by the end of this week.
--
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: shortened flags: choosing appropriate flag (issue4410049)
On Mon, Apr 18, 2011 at 5:02 AM, wrote:
> \relative c'' {
> \new CueVoice {
> \voiceOne \autoBeamOff
> a8 b c d e f g a
> a,16 b c d e f g a
> }
>
> \new Voice {
> \voiceOne \autoBeamOff
> a,16 b c d e f g a
> a,8 b c d e f g a
> }
> }
> uses (i think) two different design sizes (one for cues, one for normal
> notes) and yet everything is fine - change from regular flags to
> shortened flags take place on note d both in cues and normal notes.
> Also when i compile this and a copy of this file with
> #(set-global-staff-size 15) the results are correct.
> Perhaps i'm not understanding something?
You are initializing this list just once, while in principle it can
have different values for every loaded font. Even if it may work, it
is wrong.
As a general rule we try to avoid global variables, since they always
are or become a pain in the ass.
> http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode650
> lily/stem.cc:650: /* examine available standard flags glyphs.
> On 2011/04/18 03:42:53, hanwenn wrote:
>>
>> this seems awfully kludgy. Can't we just export another list of
>
> dimension
>>
>> variables directly in the font? See gen-emmentaler-script.py and
>> mf/out/*table*
>
> From what i understand gen-emmentaler-scripts.py it extracts some
> variables, like glyph size, from mf files. That's quite not what we need
> to do - the information that we need must be specified by font-designer
> manually and separately from actual flag variables. (these numbers do
> not appear in flag code mf variables nor can be calculated from them.)
have a look at feta-generic.mf - the dimensions are abitrary and
designer specified.
--
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: shortened flags: choosing appropriate flag (issue4410049)
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 available_flag_lengths[2][5];
On 2011/04/18 03:42:53, hanwenn wrote:
You cannot do this.
This will screw up if two .ly files use fonts of different design
sizes: the
settings of the first file will leak into the 2nd.
Hmm, i'm not sure what you mean. This:
\relative c'' {
\new CueVoice {
\voiceOne \autoBeamOff
a8 b c d e f g a
a,16 b c d e f g a
}
\new Voice {
\voiceOne \autoBeamOff
a,16 b c d e f g a
a,8 b c d e f g a
}
}
uses (i think) two different design sizes (one for cues, one for normal
notes) and yet everything is fine - change from regular flags to
shortened flags take place on note d both in cues and normal notes.
Also when i compile this and a copy of this file with
#(set-global-staff-size 15) the results are correct.
Perhaps i'm not understanding something?
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode620
lily/stem.cc:620:
On 2011/04/17 10:23:59, MikeSol wrote:
It seems like this new stuff should be declared in stem.hh instead of
here.
Judging by pitch.cc and pitch.hh example I thought that deleting this
line in stem.cc and adding
private:
static vector available_flag_lengths[2][5];
before closing brace in stem.hh would do this, but make says
/home/janek/lilypond-git/lily/stem.cc:674: error:
'available_flag_lengths' was not declared in this scope
changing 'available_flag_lengths' into 'Stem::available_flag_lengths'
didn't make it work...
How this should be done?
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode637
lily/stem.cc:637: already_computed = 1;
On 2011/04/17 10:23:59, MikeSol wrote:
In general, this seems to be hardcoding a lot of aspects of the names
of fonts.
This is not problematic so long as these names don't change, but
perhaps it'd be
wise to add a comment in the metafont file warning people to revamp
this code if
they ever change the name of fonts.
Ok, i'll add a comment.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode650
lily/stem.cc:650: /* examine available standard flags glyphs.
On 2011/04/18 03:42:53, hanwenn wrote:
this seems awfully kludgy. Can't we just export another list of
dimension
variables directly in the font? See gen-emmentaler-script.py and
mf/out/*table*
From what i understand gen-emmentaler-scripts.py it extracts some
variables, like glyph size, from mf files. That's quite not what we need
to do - the information that we need must be specified by font-designer
manually and separately from actual flag variables. (these numbers do
not appear in flag code mf variables nor can be calculated from them.)
Should i explain this in more detail?
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode660
lily/stem.cc:660: glyph_name[8] !=NULL )
On 2011/04/17 10:23:59, MikeSol wrote:
isdigit (glyph_name[7])
You are referring to whitespace?
I was desperately trying to fit in 80 characters line width :)
Fixed now.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode669
lily/stem.cc:669:
On 2011/04/17 10:23:59, MikeSol wrote:
substr (7,1)
Done.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode673
lily/stem.cc:673: = ::atof(glyph_name.substr(9,string::npos).c_str());
On 2011/04/17 10:23:59, MikeSol wrote:
::atof (glyph_name.substr (0, string::npos).c_str ())
Done.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode757
lily/stem.cc:757: */
On 2011/04/17 10:23:59, MikeSol wrote:
How won't the current patch apply to user-overriden lengths?
I meant that compiling { \autoBeamOff f'8 \override Stem #'length = #5
f'8 } should result in the second flag being a shorter variant to better
fit shorter stem.
I suppose implementing this would mean changing the order in which
things are currently done in Lily?
Sounds like a nightmare...
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode768
lily/stem.cc:768: = pow (2, (robust_scm2double
(me->get_property("font-size"), 0.0)) / 6);
On 2011/04/17 10:23:59, MikeSol wrote:
get_property ("font-size")
Done.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
On 2011/04/17 10:23:59, MikeSol wrote:
while (abs (available_flag_lengths [dir == 'd' ? 1 : 0][log -
3][flag_variant_number]
Done.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
On 2011/04/17 10:23:59, MikeSol wrote:
stem_length / 2
Done.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
On 2011/04/17 10:23:59, MikeSol wrote:
Hardcoding the use of duration log in arrays and vectors happens a
couple times
Re: shortened flags: choosing appropriate flag (issue4410049)
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 available_flag_lengths[2][5]; You cannot do this. This will screw up if two .ly files use fonts of different design sizes: the settings of the first file will leak into the 2nd. http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode650 lily/stem.cc:650: /* examine available standard flags glyphs. this seems awfully kludgy. Can't we just export another list of dimension variables directly in the font? See gen-emmentaler-script.py and mf/out/*table* http://codereview.appspot.com/4410049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: shortened flags: choosing appropriate flag (issue4410049)
Please add a stemful regtest with several overrides just to make sure
that this works completely.
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#newcode620
lily/stem.cc:620:
It seems like this new stuff should be declared in stem.hh instead of
here.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode637
lily/stem.cc:637: already_computed = 1;
In general, this seems to be hardcoding a lot of aspects of the names of
fonts. This is not problematic so long as these names don't change, but
perhaps it'd be wise to add a comment in the metafont file warning
people to revamp this code if they ever change the name of fonts.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode660
lily/stem.cc:660: glyph_name[8] !=NULL )
isdigit (glyph_name[7])
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode669
lily/stem.cc:669:
substr (7,1)
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode673
lily/stem.cc:673: = ::atof(glyph_name.substr(9,string::npos).c_str());
::atof (glyph_name.substr (0, string::npos).c_str ())
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode757
lily/stem.cc:757: */
How won't the current patch apply to user-overriden lengths?
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode768
lily/stem.cc:768: = pow (2, (robust_scm2double
(me->get_property("font-size"), 0.0)) / 6);
get_property ("font-size")
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
while (abs (available_flag_lengths [dir == 'd' ? 1 : 0][log -
3][flag_variant_number]
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
stem_length / 2
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode777
lily/stem.cc:777: - stem_length/2)
Hardcoding the use of duration log in arrays and vectors happens a
couple times in the source - I'd say keep it here, but this type of
coding should be revisited post 2.14 in case, for whatever reason,
duration log changes one day.
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode779
lily/stem.cc:779: abs (available_flag_lengths
[(dir=='d')?1:0][log-3][flag_variant_number+1]
same as above
http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode781
lily/stem.cc:781: flag_variant_number++;
same as above
http://codereview.appspot.com/4410049/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
shortened flags: choosing appropriate flag (issue4410049)
Reviewers: , Message: Passes regtests (i see a regression in partcombine-midi.ly, however from Graham's message about midi regtests http://lists.gnu.org/archive/html/lilypond-devel/2011-04/msg00283.html i understand that it's not my fault). If i understand correctly, the default way of calculating flags is using c++ code which i modified, but there's also another possibility - using scheme. Looks like it's done in flag-styles.scm. However, i don't know scheme; writing c++ code took me weeks (and a lot of help was required), so i think implementing this algorithm in scheme is definately beyond my skill. 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. Please review this at http://codereview.appspot.com/4410049/ Affected files: M lily/stem.cc M mf/feta-flags.mf ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
