Re: shortened flags: choosing appropriate flag (issue4410049)

[lemniskata . bernoullego]

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:
> 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<Real> 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
> 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.

ok

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]
On 2011/04/17 10:23:59, MikeSol wrote:
> same as above

Done.

http://codereview.appspot.com/4410049/diff/1/lily/stem.cc#newcode781
lily/stem.cc:781: flag_variant_number++;
On 2011/04/17 10:23:59, MikeSol wrote:
> same as above

Done.

http://codereview.appspot.com/4410049/