Improves some parmesan noteheads.

Improves some parmesan noteheads.

[Bertrand Bordage, 2011-09-12 01:44:33]

Hi everyone,

This patch is finally ready for review!

What has been done for Petrucci/mensural/neomensural:
* Stems centered around the attachment point.
* Attachment point lowered.
* Brevis/longa/maxima pointing downward.
* Use the left-stemmed longa in ligatures.
* A larger hole height for brevis/longa/maxima on staff line.

What has been done for Petrucci only:
* Better notehead design.

Regards,
Bertrand

[lemzwerg, 2011-09-12 13:32:17]

LGTM

    Werner

[benko.pal, 2011-09-12 19:16:26]

there are problems: the patch as is fails the mensural-ligatures regtest.  see
below.

p

http://codereview.appspot.com/4639065/diff/13002/lily/mensural-ligature.cc
File lily/mensural-ligature.cc (right):

http://codereview.appspot.com/4639065/diff/13002/lily/mensural-ligature.cc#ne...
lily/mensural-ligature.cc:147: Direction stem_dir = stem ? get_grob_direction
(stem) : CENTER;
this is unneeded: there are no stemmed notes within ligaturae.

http://codereview.appspot.com/4639065/diff/13002/lily/mensural-ligature.cc#ne...
lily/mensural-ligature.cc:151: -1);
this is bad: shapes within ligaturae depend not only on duration but on context
as well.  in other words: MLP_BREVIS doesn't necessarily correspond to duration
-1, etc.
use note_shape instead of duration_log: turn MLP_BREVIS into "M1", MLP_LONGA
into "M2" and MLP_MAXIMA into "M3".

perhaps we may change the MLP macros in mensural-ligature.hh to be simpler.

http://codereview.appspot.com/4639065/diff/13002/lily/mensural-ligature.cc#ne...
lily/mensural-ligature.cc:160: + (duration_log == -3 ? "lig" : "") + "mensural";
put all computations in lines 145-160 into the block in lines 172-183.

[Bertrand Bordage, 2011-09-12 22:24:24]

Thanks for your reviews!

On 2011/09/12 19:16:26, benko.pal wrote:
>
http://codereview.appspot.com/4639065/diff/13002/lily/mensural-ligature.cc#ne...
> lily/mensural-ligature.cc:147: Direction stem_dir = stem ? get_grob_direction
> (stem) : CENTER;
> this is unneeded: there are no stemmed notes within ligaturae.

I am a total ligature newbie. But I see some stemmed notes in
input/regression/mensural-ligatures.ly .

Of course, I agree that there's a bug. I fixed it in a new patch set.

Bertrand.

[janek, 2011-09-12 22:37:05]

Hi,

i've looked at latest screenshot attached to tracker issue and... wow!  It looks
really great!
I have only small suggestions about some sizes.
You've put a lot of work into this!

thanks,
Janek

http://codereview.appspot.com/4639065/diff/13002/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/4639065/diff/13002/ly/engraver-init.ly#newcode1063
ly/engraver-init.ly:1063: \override Stem #'thickness = #2
I'd make them just a bit thinner, perhaps 1.8.  I think that 2 might get too
thick with smaller font-size (as font-size decreases, the relative thickness
increases).

http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf
File mf/parmesan-noteheads.mf (right):

http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
mf/parmesan-noteheads.mf:272: nm_red_holeheight := 2.5 linethickness;
I'd make this 3 linethickness.

http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
mf/parmesan-noteheads.mf:329: nm_width := staff_space#;
if i'm not mistaken and these are the height and width of half and quarter
noteheads, i'd make them very slightly bigger - something like
nm_height := 1.1 noteheight#;
329 nm_width := 1.05 staff_space#;

[benko.pal, 2011-09-13 07:38:46]

hi Bertrand,

> I am a total ligature newbie. But I see some stemmed notes in
> input/regression/mensural-ligatures.ly .

initial and middle stems are drawn separately (see later if-blocks
of MLP_STEM and add-join), final ones are part of a longa notehead.

> Of course, I agree that there's a bug. I fixed it in a new patch set.

looks good and clever, but I can't actually test it now;
perhaps tonight I'll have the time.
I hope you've checked the regtests.

thanks,
p

[Bertrand Bordage, 2011-09-13 13:50:04]

On 2011/09/12 22:37:05, janek wrote: 
> i've looked at latest screenshot attached to tracker issue and... wow!  It
looks
> really great!
Thanks a lot :)

>
http://codereview.appspot.com/4639065/diff/13002/ly/engraver-init.ly#newcode1063
> ly/engraver-init.ly:1063: \override Stem #'thickness = #2
> I'd make them just a bit thinner, perhaps 1.8.  I think that 2 might get too
> thick with smaller font-size (as font-size decreases, the relative thickness
> increases).

You're right, I'll do that. Do you want to make a patch that sets some other
parameters for the PetrucciStaff?

>
http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
> mf/parmesan-noteheads.mf:272: nm_red_holeheight := 2.5 linethickness;
> I'd make this 3 linethickness.

That's what I tried at first, but this strangely looks more readable with 2.5.
But I haven't yet compared printed results of different sizes, so you're maybe
right.

>
http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
> mf/parmesan-noteheads.mf:329: nm_width := staff_space#;
> if i'm not mistaken and these are the height and width of half and quarter
> noteheads, i'd make them very slightly bigger - something like
> nm_height := 1.1 noteheight#;
> 329 nm_width := 1.05 staff_space#;

Do you know what neomensural and mensural styles are inspired of? I always
thought these notes were tiny, but it's maybe intended. As I am not an expert, I
kept the shapes as is, but a rebuild would be great since we could get rid of
draw_diamond_head (and finally have good ancient fonts).

Anyway, the next steps will be (in order) to redesign the ancient rests,
accidentals, clefs, time signatures and flags. Do I count you in?

Bertrand

[pkx166h, 2011-09-13 14:56:14]

Thought I'd commented on this one but see

http://code.google.com/p/lilypond/issues/detail?id=1839#c13

There are the reg test differences.

[janek, 2011-09-13 16:25:07]

2011/9/13  <bordage.bertrand@gmail.com>:
>
http://codereview.appspot.com/4639065/diff/13002/ly/engraver-init.ly#newcode1063
>>
>> ly/engraver-init.ly:1063: \override Stem #'thickness = #2
>> I'd make them just a bit thinner, perhaps 1.8.  I think that 2 might
>> get too thick with smaller font-size (as font-size decreases,
>> the relative thickness increases).
>
> You're right, I'll do that. Do you want to make a patch that sets some
> other parameters for the PetrucciStaff?

No, i have little experience with ancient music and therefore i don't
have any more ideas what could use some fine-tuning.

...actually, one things comes to my mind: staffline thickness.  It may
be worth considering to make them a bit thicker.
In that case, the tail of longa note should be thickened a bit, too
(from 1.3 to 1.4 linethickness?).  In fact, they may be thickened even
if we don't change PetrucciStaff's linethickness.

>
http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
>>
>> mf/parmesan-noteheads.mf:272: nm_red_holeheight := 2.5 linethickness;
>> I'd make this 3 linethickness.
>
> That's what I tried at first, but this strangely looks more readable
> with 2.5. But I haven't yet compared printed results of different sizes,
> so you're maybe right.

That's surprising...  I may try to print a test file myself,
but i can't promise anything since i don't have good enough
printer and therefore would need to go find a print shop first.

>
http://codereview.appspot.com/4639065/diff/13002/mf/parmesan-noteheads.mf#new...
>>
>> mf/parmesan-noteheads.mf:329: nm_width := staff_space#;
>> if these are the height and width of half and quarter
>> noteheads, i'd make them very slightly bigger - something like
>> nm_height := 1.1 noteheight#;
>> nm_width := 1.05 staff_space#;
>
> Do you know what neomensural and mensural styles are inspired of? I
> always thought these notes were tiny, but it's maybe intended. As I am
> not an expert, I kept the shapes as is, but a rebuild would be great
> since we could get rid of draw_diamond_head (and finally have good
> ancient fonts).

Well, my suggestion of increasing their size a bit was based on the
scanned example you provided.  In that example quarter and half
noteheads look roughly as big as breve and longa noteheads, while the
current version of your patch makes them smaller.

> Anyway, the next steps will be (in order) to redesign the ancient rests,
> accidentals, clefs, time signatures and flags. Do I count you in?

Yes!  doing font stuff can be fun :)

James: all regtest differencies seem desired to me.

cheers,
Janek

[benko.pal, 2011-09-14 05:56:42]

hi all,

the patch is ok from my point of view.  a minor question: the only change
in mensural-ligatures.ly is the version bump - is that needed?

>> Do you know what neomensural and mensural styles are inspired of?

I believe it uses the shapes of noteheads found in manuscript,
but scales them to fit between staff lines like modern noteheads
so that they don't collide if used in chords (extremely rare; I've seen
them only in cadences with breves or longae, never with the diamond
shape).

have you seen the facsimiles in the message
http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00398.html
?

p

[pacovila, 2011-09-14 17:28:10]

2011/9/14 Benkő Pál <benko.pal@gmail.com>:
> hi all,
>
> the patch is ok from my point of view.  a minor question: the only change
> in mensural-ligatures.ly is the version bump - is that needed?
>
>>> Do you know what neomensural and mensural styles are inspired of?
>
> I believe it uses the shapes of noteheads found in manuscript,
> but scales them to fit between staff lines like modern noteheads
> so that they don't collide if used in chords (extremely rare; I've seen
> them only in cadences with breves or longae, never with the diamond
> shape).
>
> have you seen the facsimiles in the message
> http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00398.html
> ?

The attached file in that message must be a joke!


-- 
Francisco Vila. Badajoz (Spain)
www.paconet.org , www.csmbadajoz.com

[benko.pal, 2011-09-14 19:02:48]

2011/9/14 Francisco Vila <paconet.org@gmail.com>:

>> have you seen the facsimiles in the message
>> http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00398.html
>> ?
>
> The attached file in that message must be a joke!

aargh, my bad.  as I look at it now,
it's actually a gzip-compressed tar archive,
you can unpack it by tar xzf, or renaming it to
extension .tar.gz or .tgz.

sorry,
p

[benko.pal, 2011-09-14 21:05:28]

hi Bertrand,

> What has been done for Petrucci/mensural/neomensural:
> * Stems centered around the attachment point.
> * Attachment point lowered.
> * Brevis/longa/maxima pointing downward.
> * Use the left-stemmed longa in ligatures.

exactly what is this last one?

p

[Bertrand Bordage, 2011-09-15 17:08:13]

New patch set (inspired by Janek's ideas).

On 2011/09/14 21:05:28, benko.pal wrote:
> > * Use the left-stemmed longa in ligatures.
> 
> exactly what is this last one?

When note_shape is MLP_LONGA and the direction of the stem is UP, then the
left-stemmed longa is used.
I know, this should be Mensural_ligature_engraver's job.
The mensural ligature engraver needs to be rewritten. All the postscript stuff
should somehow be replaced with good old MetaFont.

Thanks,
Bertrand.

[benko.pal, 2011-09-15 18:46:42]

>> > * Use the left-stemmed longa in ligatures.
>
>> exactly what is this last one?
>
> When note_shape is MLP_LONGA and the direction of the stem is UP, then
> the left-stemmed longa is used.

oh, indeed.  I must have messed up my build, now I see it.
unfortunately this is bad.
look at the first row of the mensural-ligatures.ly regtest:
the second LB and the second SS are indistinguishable.

> I know, this should be Mensural_ligature_engraver's job.
> The mensural ligature engraver needs to be rewritten. All the postscript
> stuff should somehow be replaced with good old MetaFont.

I don't mind that (I didn't eve know that the Stencil - Interval
manipulations were PostScript) and I'm willing to take part.

p

[benko.pal, 2011-09-15 19:30:48]

hi Bertrand,

sorry, I messed up my build last time, and the mensural-ligatures.ly regtest is
still bad.  look at the first row: the last LB and last SS are
indistinguishable.  2.14 is right.

all: what would be the good place for the ligature description in the second
attachment of

http://lists.gnu.org/archive/html/lilypond-devel/2005-02/msg00198.html

?

but let me repeat (and enhance) it here:

A ligature consists of breves, longae and maximae, joined by a vertical line.
There are some exceptions:
1. at the beginning
   a. a ligature may begin with two semibreves: this is denoted by
      a left upward stem.  The note shapes are like breves.
   b. if the second note is lower than the first (descending start),
      then encoding of the duration of the first note is changed as
      - a longa is denoted by a simple brevis head;
      - a brevis is denoted by a left downward stem (and brevis head).
2. at the end
   if the last note is lower than the penultimate one (descending end),
    and it is
    a. a longa, then it is represented by a brevis head;
    b. a breve, then the last two notes are drawn as parallelogram
       (ligatura obliqua, flexa).
       This is possible only if the penultimate note would otherwise have
       brevis shape, i.e. it must be a brevis, or the ligature must be
       either LB or SSB

Notes:
1. reading a ligature is unabiguous; writing one is not:
   a. any two brevis heads can be replaced by a flexa
      (except 2.a. above; can be requested by \obliqua _between_
      the two notes)
   b. stems of maximae are often omitted
      (this implementation omits them always).
2. theorists claim that a pair of semibreves are admitted anywhere,
   but I have never seen this usage.
3. any note can be dotted or colored independently of others
   (even either note of a flexa).
4. stems of longae (and maximae, if used) are always drawn down
   and on the right side of the note.

http://codereview.appspot.com/4639065/diff/32001/lily/mensural-ligature.cc
File lily/mensural-ligature.cc (right):

http://codereview.appspot.com/4639065/diff/32001/lily/mensural-ligature.cc#ne...
lily/mensural-ligature.cc:150: Direction stem_dir = stem ? get_grob_direction
(stem) : CENTER;
throw these two lines out

http://codereview.appspot.com/4639065/diff/32001/lily/mensural-ligature.cc#ne...
lily/mensural-ligature.cc:175: index = prefix + ((stem_dir == UP) ? "sl" : "d");
prefix + "d"

[Bertrand Bordage, 2011-09-15 19:55:09]

Ok Pal, I removed the left-stemmed longa.
I don't know where you could put these rules.
Maybe we should start a new doc part that references the established engraving
rules?

Bertrand

[benko.pal, 2011-09-15 21:09:08]

hi Bertrand,

> Ok Pal, I removed the left-stemmed longa.

thanks, mensural-ligatures.ly is now ok.

p

[pkx166h, 2011-09-15 21:12:29]

Passes make and new reg test differences (look ok) attached here

http://code.google.com/p/lilypond/issues/detail?id=1839#c17

[benko.pal, 2011-09-16 08:20:02]

> Passes make and new reg test differences (look ok) attached here

in all four .png's I can see empty rectangles instead of longae.

p

[Bertrand Bordage, 2011-09-17 16:40:11]

Pushed as:
0dcc93c0a5a97d048db2f7631966f41ae0059ab5
and
0e31cd90e44673eca7ac59705ce4bed33dd8e80e

Thank you all for this review!
Bertrand

[Keith, 2011-10-02 23:00:49]

Bertrand, It looks like the additional font lookups are significantly slowing
down the Windows build,
http://code.google.com/p/lilypond/issues/detail?id=1926

Mabye it is the multiple calls to find_by_name().  It seems the logic that
chooses the glyph could be simpler, and more efficient.
... or be put within
  if (style != ly_symbol2scm ("default"))

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc
File lily/note-head.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode57
lily/note-head.cc:57: Stencil out = fm->find_by_name (idx_either + suffix);
Look first for a symmetric glyph noteheads.s2

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode58
lily/note-head.cc:58: if (out.is_empty ())
If there is none, figure out our direction; suppose up.

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode69
lily/note-head.cc:69: out = fm->find_by_name (idx_either + "r" + suffix);
If there was no noteheads.s2, look for noteheads.ur2
If there was a noteheads.s2, forget it and look for noteheads.sr2

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode77
lily/note-head.cc:77: out = fm->find_by_name (idx_either);
If there was a noteheads.sr2 or .ur2,
  and we are on the centerline, 
  then look for noteheads.sr2 or .ur2 again,
otherwise look for noteheads.s2 or .u2

[Keith, 2011-10-03 01:15:48]

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc
File lily/note-head.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode42
lily/note-head.cc:42: 
/* Cache the previously-used stencils */
static Stencil recent_symmetric_heads[4/*2 - durlog*/];
static Stencil recent_symmetric_heads_inspace[4];

out =  on_staff_line ? recent_symmetric_heads[2-durlog]
  : recent_symmetric_heads_inspace[2-durlog];
if (!out.is_empty()) return out;

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode49
lily/note-head.cc:49: }
if cached return cached

if !on_staff_line
  if sr
    cache sr
    return sr
  else if up
    if ur return ur
    else if u return u
  else 
    if dr return dr
    else if d return d
if s
  cache s // in _inspace[] if we are in a space
  return s
else if up
  if u return u
else
  if d return d
else fail

[Keith, 2011-10-03 04:57:59]

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc
File lily/note-head.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/note-head.cc#newcode49
lily/note-head.cc:49: }
Yuck.  The decision tree is complex enough that two tests of font capability
seem to be required for the most common case; that will be slow on Windows if
the tests require font-lookups.

Caching would help, but if it complicates the code then future changes will be
harder.

Best would be to use the same method as shape-notes, where the engraver
determines the part of glyph name that depends on pitch.

Alternatively, use a property of NoteHead, along-side of 'style', to indicate if
a reduced-hole version is expected in the font, and look for the glyph only when
property says to expect it.

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode853
lily/stem.cc:853: extract_grob_set (me, "note-heads", heads);
'f' already points to the first head; why not just use that?

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode855
lily/stem.cc:855: if (attach && !scm_is_eq (style, ly_symbol2scm ("mensural"))
To adjust the attach points, shouldn't you adjust charwx in the font?

[Bertrand Bordage, 2011-10-03 08:52:27]

Thanks Keith, I'll quickly fix that in a new issue.

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode853
lily/stem.cc:853: extract_grob_set (me, "note-heads", heads);
... OK.

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode855
lily/stem.cc:855: if (attach && !scm_is_eq (style, ly_symbol2scm ("mensural"))
Of course, but we only choose the position of the attachment. By default, the
stem is right-aligned...

[Keith, 2011-10-03 09:30:31]

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode855
lily/stem.cc:855: if (attach && !scm_is_eq (style, ly_symbol2scm ("mensural"))
On 2011/10/03 08:52:27, Bertrand Bordage wrote:
> By default, the stem is right-aligned...
Right-aligned for up-stems, left-aligned for down-stems, so that most fonts can
conveniently use the outer edges of the stencil as the attachment point, using
the character width.
If your font attaches stems differently, charwx is provided to specify the
difference.
Here you seem to change the rules (costing users time for four additional
property-lookups per stem) without updating mf/README with your change to the
rules.

[Bertrand Bordage, 2011-10-07 09:47:23]

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode855
lily/stem.cc:855: if (attach && !scm_is_eq (style, ly_symbol2scm ("mensural"))
I meant: the stem is aligned to the right of the attachment point (and to its
left for down-stems).  For centered stems like we have in ancient styles, the
stems have to be centered around the attachment point, which is positioned at
the center of the notehead.

[Keith, 2011-10-07 17:56:08]

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4639065/diff/42001/lily/stem.cc#newcode855
lily/stem.cc:855: if (attach && !scm_is_eq (style, ly_symbol2scm ("mensural"))
On 2011/10/07 09:47:24, Bertrand Bordage wrote:
> stems have to be centered around the attachment point, which is positioned at
> the center of the notehead.

Okay; I thought the centered case was excluded by if(attach)