Implements footnotes in LilyPond

Implements footnotes in LilyPond

[mikesol_ufl.edu, 2011-03-05 11:18:37]

Hey all,

After a bit of back and forth w/ Han Wen, I have drummed up a way to split this
up such that it can be part of LilyPond in two phases.  It follows his
suggestion to push all of the non-balloon-related stuff first, and to push that
second.  It will set the stencil property of FootnoteItem and FootnoteSpanner to
#f in define-grobs.scm.  This means that whatever people write for the
annotation part of the footnote will be swallowed up and not used, whereas the
note part on the bottom of the page will be printed.

If anyone has arguments for pushing this whole thing in its entirety, let me
know.  I am in favor of that for the reasons I stated in my previous e-mail, but
I also realize that big chunks of code pushed all at once can make bug tracking
interminable.

If not, the plan is to push the non-balloon-related stuff today, let it simmer
for a few days, and then push the balloon related stuff used for the in-document
annotations.  I will make a nice long commit message detailing this so that,
hopefully, experimental users don't waste hours figuring out what happened to
their non-appearing annotations.  Alternatively, if anyone feels that I should
split this up but that I am splitting it up the wrong way, please let me know.

Cheers,
MS

[joeneeman, 2011-03-05 12:19:51]

On Sat, Mar 5, 2011 at 10:18 PM, Mike Solomon <mikesol@ufl.edu> wrote:

> Hey all,
>
> After a bit of back and forth w/ Han Wen, I have drummed up a way to split
> this up such that it can be part of LilyPond in two phases.  It follows his
> suggestion to push all of the non-balloon-related stuff first, and to push
> that second.  It will set the stencil property of FootnoteItem and
> FootnoteSpanner to #f in define-grobs.scm.  This means that whatever people
> write for the annotation part of the footnote will be swallowed up and not
> used, whereas the note part on the bottom of the page will be printed.
>
> If anyone has arguments for pushing this whole thing in its entirety, let
> me know.  I am in favor of that for the reasons I stated in my previous
> e-mail, but I also realize that big chunks of code pushed all at once can
> make bug tracking interminable.
>
> If not, the plan is to push the non-balloon-related stuff today, let it
> simmer for a few days, and then push the balloon related stuff used for the
> in-document annotations.  I will make a nice long commit message detailing
> this so that, hopefully, experimental users don't waste hours figuring out
> what happened to their non-appearing annotations.  Alternatively, if anyone
> feels that I should split this up but that I am splitting it up the wrong
> way, please let me know.
>

That sounds fine to me. Do you have a patch showing the part you are going
to push first? I was happy with the spacing-related code up until your
break-visibility changes, but if you're going to include anything since
then, I'd like to have another quick look (I have to catch a plane in 12
hours; if you put up your patch before then, I'll have a look right away).

Cheers,
Joe

[mikesol_ufl.edu, 2011-03-05 12:57:28]

On Mar 5, 2011, at 7:19 AM, Joe Neeman wrote:

> On Sat, Mar 5, 2011 at 10:18 PM, Mike Solomon <mikesol@ufl.edu> wrote:
> Hey all,
> 
> After a bit of back and forth w/ Han Wen, I have drummed up a way to split
this up such that it can be part of LilyPond in two phases.  It follows his
suggestion to push all of the non-balloon-related stuff first, and to push that
second.  It will set the stencil property of FootnoteItem and FootnoteSpanner to
#f in define-grobs.scm.  This means that whatever people write for the
annotation part of the footnote will be swallowed up and not used, whereas the
note part on the bottom of the page will be printed.
> 
> If anyone has arguments for pushing this whole thing in its entirety, let me
know.  I am in favor of that for the reasons I stated in my previous e-mail, but
I also realize that big chunks of code pushed all at once can make bug tracking
interminable.
> 
> If not, the plan is to push the non-balloon-related stuff today, let it simmer
for a few days, and then push the balloon related stuff used for the in-document
annotations.  I will make a nice long commit message detailing this so that,
hopefully, experimental users don't waste hours figuring out what happened to
their non-appearing annotations.  Alternatively, if anyone feels that I should
split this up but that I am splitting it up the wrong way, please let me know.
> 
> That sounds fine to me. Do you have a patch showing the part you are going to
push first? I was happy with the spacing-related code up until your
break-visibility changes, but if you're going to include anything since then,
I'd like to have another quick look (I have to catch a plane in 12 hours; if you
put up your patch before then, I'll have a look right away).
> 
> Cheers,
> Joe
> 

Patch attached.  The stuff that comes from your comments regarding
break-visibility is implemented in Balloon_interface::is_visible.

The patch currently represents about 85% of the original, omitting the 15% that
Han Wan had previously identified as hold-off-on-able for a first push (the
actual annotations).  These are relatively painless to add back in.

I realize that this is not a "small" chunk, but if I shaved anything else off,
the footnotes wouldn't work.

I'm running regtests now & will report back if anything breaks.

Cheers,
MS

P.S. The original patch resides at http://codereview.appspot.com/4245062

[Neil Puttock, 2011-03-05 13:50:00]

On 5 March 2011 12:57, Mike Solomon <mikesol@ufl.edu> wrote:

> Patch attached.  The stuff that comes from your comments regarding
> break-visibility is implemented in Balloon_interface::is_visible.
> The patch currently represents about 85% of the original, omitting the 15%
> that Han Wan had previously identified as hold-off-on-able for a first push
> (the actual annotations).  These are relatively painless to add back in.
> I realize that this is not a "small" chunk, but if I shaved anything else
> off, the footnotes wouldn't work.
> I'm running regtests now & will report back if anything breaks.
> Cheers,
> MS
> P.S. The original patch resides at http://codereview.appspot.com/4245062

I think there are several parts to this patch which should be removed:

1) the break-visibility code in balloon.cc

You've implemented something which looks suspiciously like a callback,
but you've also added a function to output-lib.scm which you call
instead via scm_call_1 () (you shouldn't need to do this for a grob).
All this code should be part of a callback for FootnoteItem
#'break-visibility.

This code doesn't work very well in some cases:

\new Staff \with {
  \consists Footnote_engraver
}
\relative c' {
  \footnoteGrob #'Clef #'(0 . 2) foo bar
  c1
}

-> no footnote

\new Staff \with {
  \consists Footnote_engraver
}
\relative c' {
  c1
  \footnoteGrob #'Clef #'(0 . 2) foo bar
  c1
}

-> no footnote (good!), but suicided clefs are still accounted for
(all three) in account_for_footnotes ()

\new Staff \with {
  \consists Footnote_engraver
}
\relative c' {
  c1 \break
  \footnoteGrob #'Clef #'(0 . 2) foo bar
  c1
}

-> two footnotes, account_for_foonotes () still thinks there are three

For all these cases setting FootnoteItem #'break-visibility to
`inherit-x-parent-visibility' produces better results.

2) all the code related to `spanner-placement'

This is a really bad interface for selecting the correct spanner.  I'd
much prefer something which would index the broken_intos_ (though I
guess this would run into the same problem you're having with the
footnote height calculations).

@@ -1237,7 +1277,7 @@
Page_breaking::space_systems_with_fixed_number_per_page (vsize
configuration,
       while (system_count_on_this_page < systems_per_page_
 	     && line < cached_line_details_.size ())
 	{
-	  Line_details const &cur_line = cached_line_details_[line];
+	  Line_details &cur_line = cached_line_details_[line];

looks like it's left over from a previous patch

Cheers,
Neil

[joeneeman, 2011-03-05 14:26:34]

On Sat, Mar 5, 2011 at 11:57 PM, Mike Solomon <mikesol@ufl.edu> wrote:

> On Mar 5, 2011, at 7:19 AM, Joe Neeman wrote:
>
> On Sat, Mar 5, 2011 at 10:18 PM, Mike Solomon <mikesol@ufl.edu> wrote:
>
>> Hey all,
>>
>> After a bit of back and forth w/ Han Wen, I have drummed up a way to split
>> this up such that it can be part of LilyPond in two phases.  It follows his
>> suggestion to push all of the non-balloon-related stuff first, and to push
>> that second.  It will set the stencil property of FootnoteItem and
>> FootnoteSpanner to #f in define-grobs.scm.  This means that whatever people
>> write for the annotation part of the footnote will be swallowed up and not
>> used, whereas the note part on the bottom of the page will be printed.
>>
>> If anyone has arguments for pushing this whole thing in its entirety, let
>> me know.  I am in favor of that for the reasons I stated in my previous
>> e-mail, but I also realize that big chunks of code pushed all at once can
>> make bug tracking interminable.
>>
>> If not, the plan is to push the non-balloon-related stuff today, let it
>> simmer for a few days, and then push the balloon related stuff used for the
>> in-document annotations.  I will make a nice long commit message detailing
>> this so that, hopefully, experimental users don't waste hours figuring out
>> what happened to their non-appearing annotations.  Alternatively, if anyone
>> feels that I should split this up but that I am splitting it up the wrong
>> way, please let me know.
>>
>
> That sounds fine to me. Do you have a patch showing the part you are going
> to push first? I was happy with the spacing-related code up until your
> break-visibility changes, but if you're going to include anything since
> then, I'd like to have another quick look (I have to catch a plane in 12
> hours; if you put up your patch before then, I'll have a look right away).
>
> Cheers,
> Joe
>
>
> Patch attached.  The stuff that comes from your comments regarding
> break-visibility is implemented in Balloon_interface::is_visible.
>

I find the return value of Balloon_interface::is_visible strange (first of
all, the name suggests that it should just be bool). For example, it returns
BEGINNING_OF_LINE if the footnote is visible and in the middle of the line
(but only if the parent has a break-visibility property). I might be missing
something, but it looks to me like you can get rid of this function, and
just set FootnoteItem 'break-visibility to
(lambda (grob)
 (let ((parent (ly:grob-parent grob X))
        (par-vis (ly:grob-property parent 'break-visibility))
        (parent-visibility (if (vector? par-vis) par-vis #(#t #t #t))))
  (vector
   (vector-ref parent-visibility 0)
   (vector-ref parent-visibility 1)
   (and
    (vector-ref parent-visibility 2)
    (not (vector-ref parent-visibility 0))))))
(There's still a corner case here, because you need the footnote to be
visible if it is in the first column of the score...)

I don't like the interpolation stuff for spanner-placement. Why not just
restrict the valid values to -1 and 1? It seems that any other value doesn't
really have user-predictable behaviour anyway...

I think you have a bug in system.cc:281, because there is no guaranteed
order between the grobs whose rank is the ending column. In particular, the
one that appears at the beginning of the next line might appear first, and
then you will break prematurely. Maybe something like:

bool end_of_line_visible = true;
if (Spanner *s ...)
 ...
if (Item *item ...)
  {
    end_of_line_visible = (LEFT == item->break_status_dir ());
    ...
  }
if (pos < (int)start)
  continue;
if (pos > (int)end)
  break;
if (pos == (int)end && !end_of_line_visible)
  continue;
...

[mikesol_ufl.edu, 2011-03-05 14:38:18]

On Mar 5, 2011, at 8:49 AM, Neil Puttock wrote:

> On 5 March 2011 12:57, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> Patch attached.  The stuff that comes from your comments regarding
>> break-visibility is implemented in Balloon_interface::is_visible.
>> The patch currently represents about 85% of the original, omitting the 15%
>> that Han Wan had previously identified as hold-off-on-able for a first push
>> (the actual annotations).  These are relatively painless to add back in.
>> I realize that this is not a "small" chunk, but if I shaved anything else
>> off, the footnotes wouldn't work.
>> I'm running regtests now & will report back if anything breaks.
>> Cheers,
>> MS
>> P.S. The original patch resides at http://codereview.appspot.com/4245062
> 
> I think there are several parts to this patch which should be removed:
> 
> 1) the break-visibility code in balloon.cc
> 
> You've implemented something which looks suspiciously like a callback,
> but you've also added a function to output-lib.scm which you call
> instead via scm_call_1 () (you shouldn't need to do this for a grob).
> All this code should be part of a callback for FootnoteItem
> #'break-visibility.
> 
> This code doesn't work very well in some cases:
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> no footnote
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  c1
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> no footnote (good!), but suicided clefs are still accounted for
> (all three) in account_for_footnotes ()
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  c1 \break
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> two footnotes, account_for_foonotes () still thinks there are three
> 
> For all these cases setting FootnoteItem #'break-visibility to
> `inherit-x-parent-visibility' produces better results.
> 

Done - thanks for bearing with me as I learn about break-visibility.  It is a
corner of the code that I never had to deal with directly, so I'm still getting
my sea legs.

> 2) all the code related to `spanner-placement'
> 
> This is a really bad interface for selecting the correct spanner.  I'd
> much prefer something which would index the broken_intos_ (though I
> guess this would run into the same problem you're having with the
> footnote height calculations).
> 

You hit the nail on the head.  Currently, the page-spacer is figuring out the
heights of footnotes before the spanners are broken.  So, because broken_intos_
doesn't exist yet, this is the only way I could come up with to make this work.

If you feel this is too hackish, I could make this direction-only (LEFT, CENTER,
RIGHT) with CENTER defaulting to LEFT and have the footnote only apply to the
first and last spanner.  But, for long spanners, this seems less than ideal.  As
always, your suggestions are welcome!

> @@ -1237,7 +1277,7 @@
> Page_breaking::space_systems_with_fixed_number_per_page (vsize
> configuration,
>       while (system_count_on_this_page < systems_per_page_
> 	     && line < cached_line_details_.size ())
> 	{
> -	  Line_details const &cur_line = cached_line_details_[line];
> +	  Line_details &cur_line = cached_line_details_[line];
> 
> looks like it's left over from a previous patch
> 

Fixed.

Thank you very much, Neil, for helping this thing evolve so much!  I feel that
I'm learning a lot about LilyPond by integrating all these comments, and I
really appreciate it.

If all looks good, please let me know - otherwise, please let me know what still
needs to change.

Cheers,
MS

[mikesol_ufl.edu, 2011-03-05 14:54:31]

On Mar 5, 2011, at 9:26 AM, Joe Neeman wrote:

> On Sat, Mar 5, 2011 at 11:57 PM, Mike Solomon <mikesol@ufl.edu> wrote:
> On Mar 5, 2011, at 7:19 AM, Joe Neeman wrote:
> 
>> On Sat, Mar 5, 2011 at 10:18 PM, Mike Solomon <mikesol@ufl.edu> wrote:
>> Hey all,
>> 
>> After a bit of back and forth w/ Han Wen, I have drummed up a way to split
this up such that it can be part of LilyPond in two phases.  It follows his
suggestion to push all of the non-balloon-related stuff first, and to push that
second.  It will set the stencil property of FootnoteItem and FootnoteSpanner to
#f in define-grobs.scm.  This means that whatever people write for the
annotation part of the footnote will be swallowed up and not used, whereas the
note part on the bottom of the page will be printed.
>> 
>> If anyone has arguments for pushing this whole thing in its entirety, let me
know.  I am in favor of that for the reasons I stated in my previous e-mail, but
I also realize that big chunks of code pushed all at once can make bug tracking
interminable.
>> 
>> If not, the plan is to push the non-balloon-related stuff today, let it
simmer for a few days, and then push the balloon related stuff used for the
in-document annotations.  I will make a nice long commit message detailing this
so that, hopefully, experimental users don't waste hours figuring out what
happened to their non-appearing annotations.  Alternatively, if anyone feels
that I should split this up but that I am splitting it up the wrong way, please
let me know.
>> 
>> That sounds fine to me. Do you have a patch showing the part you are going to
push first? I was happy with the spacing-related code up until your
break-visibility changes, but if you're going to include anything since then,
I'd like to have another quick look (I have to catch a plane in 12 hours; if you
put up your patch before then, I'll have a look right away).
>> 
>> Cheers,
>> Joe
>> 
> 
> Patch attached.  The stuff that comes from your comments regarding
break-visibility is implemented in Balloon_interface::is_visible.
> 
> I find the return value of Balloon_interface::is_visible strange (first of
all, the name suggests that it should just be bool). For example, it returns
BEGINNING_OF_LINE if the footnote is visible and in the middle of the line (but
only if the parent has a break-visibility property). I might be missing
something, but it looks to me like you can get rid of this function, and just
set FootnoteItem 'break-visibility to
> (lambda (grob)
>  (let ((parent (ly:grob-parent grob X))
>         (par-vis (ly:grob-property parent 'break-visibility))
>         (parent-visibility (if (vector? par-vis) par-vis #(#t #t #t))))
>   (vector
>    (vector-ref parent-visibility 0)
>    (vector-ref parent-visibility 1)
>    (and
>     (vector-ref parent-visibility 2)
>     (not (vector-ref parent-visibility 0))))))
> (There's still a corner case here, because you need the footnote to be visible
if it is in the first column of the score...)
> 

I ultimately incorporated a suggestion from Neil.  It seems more streamlined &
LilyPondish.  That said, it does lead to double printing for stuff like time
signatures.  Perhaps what you're proposing could be part of the docs?  I
originally thought that what you're proposing should be default behavior, but
more and more, I think that it's minimally annoying just to ask the user to
override break-visible for FootnoteItem, as I'm not sure what the best default
behavior is given the needs of users.

> I don't like the interpolation stuff for spanner-placement. Why not just
restrict the valid values to -1 and 1? It seems that any other value doesn't
really have user-predictable behaviour anyway...
> 

It doesn't, but like I said in my previous e-mail, I am reticent to disafford
the user the ability to place the footnote on a long spanner (i.e. on a
crescendo that spans 8 lines, it may be important to indicate that on the 5th
line, the crescendo could potentially cut off...I know that sounds kinda
bizarre, but I want to propose the most robust solution possible, and I think
that it'd be better to warn the user in the docs to set this property to -1 or 1
for predictable behavior rather than to limit them to only this.

> I think you have a bug in system.cc:281, because there is no guaranteed order
between the grobs whose rank is the ending column. In particular, the one that
appears at the beginning of the next line might appear first, and then you will
break prematurely. Maybe something like:
> 
> bool end_of_line_visible = true;
> if (Spanner *s ...)
>  ...
> if (Item *item ...)
>   {
>     end_of_line_visible = (LEFT == item->break_status_dir ());
>     ...
>   }
> if (pos < (int)start)
>   continue;
> if (pos > (int)end)
>   break;
> if (pos == (int)end && !end_of_line_visible)
>   continue;
> ...
> 

Fixed.

Thank you for all of your help!  A new patch set is attached to this e-mail:
please disregard the previous one.

Please let me know what you think - I'm flying in 6ish hours, so I can make any
necessary changes and/or push before then.  After that my internet access will
be more spotty.

Cheers,
MS

[Neil Puttock, 2011-03-05 15:43:44]

On 5 March 2011 14:38, Mike Solomon <mikesol@ufl.edu> wrote:

> Done - thanks for bearing with me as I learn about break-visibility.  It is a
corner of the code that I never had to deal with directly, so I'm still getting
my sea legs.

I suggest you remove the fallback value from
inherit-x-parent-visibility (or if you prefer, add another callback
for y-parent visibility without a default) otherwise grobs which don't
care about break-visibility (such as noteheads) will lose their
footnotes.

> If you feel this is too hackish, I could make this direction-only (LEFT,
CENTER, RIGHT) with CENTER defaulting to LEFT and have the footnote only apply
to the first and last spanner.  But, for long spanners, this seems less than
ideal.  As always, your suggestions are welcome!

I'm afraid I'm at a loss to suggest anything better, so I'll have to
put up with it (unless anybody else can think of a better way.

Cheers,
Neil

[mikesol_ufl.edu, 2011-03-05 15:59:33]

On Mar 5, 2011, at 10:43 AM, Neil Puttock wrote:

> On 5 March 2011 14:38, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> Done - thanks for bearing with me as I learn about break-visibility.  It is a
corner of the code that I never had to deal with directly, so I'm still getting
my sea legs.
> 
> I suggest you remove the fallback value from
> inherit-x-parent-visibility (or if you prefer, add another callback
> for y-parent visibility without a default) otherwise grobs which don't
> care about break-visibility (such as noteheads) will lose their
> footnotes.
> 

Yikes...sorry, didn't realize it had this default.
I went with the y-parent visibility option, as I don't want to start writing
convert-ly rules for inherit-x-parent-visibility.  This may be a good idea,
though, for someone willing to take it on.

>> If you feel this is too hackish, I could make this direction-only (LEFT,
CENTER, RIGHT) with CENTER defaulting to LEFT and have the footnote only apply
to the first and last spanner.  But, for long spanners, this seems less than
ideal.  As always, your suggestions are welcome!
> 
> I'm afraid I'm at a loss to suggest anything better, so I'll have to
> put up with it (unless anybody else can think of a better way.
> 

I agree that this is less than ideal.  Honestly, the only way I could think of
would be to trigger system breaking before every page configuration evaluation
(which would give us access to broken spanners).  But this would likely involve
all sorts of rewrites that are the subject of another thread.

Patch attached - I just rebased the second one, as it is a minor change.

Lemme know what you think!

Cheers,
MS

[mikesol_ufl.edu, 2011-03-06 15:58:52]

On Mar 5, 2011, at 10:43 AM, Neil Puttock wrote:

> On 5 March 2011 14:38, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> Done - thanks for bearing with me as I learn about break-visibility.  It is a
corner of the code that I never had to deal with directly, so I'm still getting
my sea legs.
> 
> I suggest you remove the fallback value from
> inherit-x-parent-visibility (or if you prefer, add another callback
> for y-parent visibility without a default) otherwise grobs which don't
> care about break-visibility (such as noteheads) will lose their
> footnotes.
> 
>> If you feel this is too hackish, I could make this direction-only (LEFT,
CENTER, RIGHT) with CENTER defaulting to LEFT and have the footnote only apply
to the first and last spanner.  But, for long spanners, this seems less than
ideal.  As always, your suggestions are welcome!
> 
> I'm afraid I'm at a loss to suggest anything better, so I'll have to
> put up with it (unless anybody else can think of a better way.

The first half has been pushed with everything changed save this one caveat that
you bring up.  If after a week people have better suggestions after having
played around with these footnotes, I'll incorporate that into push 2/2.

Cheers,
MS