Axis group interface ignores column rank for pure-from-neighbor-interface

Axis group interface ignores column rank for pure-from-neighbor-interface

[Keith, 2012-03-19 00:00:05]

\paper { #(set-paper-size "a3") }
\new StaffGroup <<
  \new Staff { \repeat unfold 8 {R1 e'1} }
  \addlyrics { Worked twice... 
      but then I ploughed... straight... through. }
  \new Staff { R1*16 }  >>

[Keith, 2012-03-20 02:36:06]

http://codereview.appspot.com/5843063/diff/5001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5843063/diff/5001/lily/axis-group-interface.cc#...
lily/axis-group-interface.cc:330: || Pure_from_neighbor_interface::has_interface
(me))
Now that the engraver is working correctly, do you still need this change ?
I think the test against 'start' and 'end' was there to better estimate the
height of just what is on the currently-considered line.  If you bypass this
test, then sometimes the page breaking will be fooled :

\paper {#(set-paper-size "a6" 'landscape) }
\new PianoStaff <<
  \new Staff \relative f'' {
    c4 c c g a a g2 | e'4 e d2 | c2 r2
    c4 c c g a a g2 | e'4 e d2 | c2 r2
  }
  \addlyrics {
   Old Mac Don -- ald had a farm, in O -- hi -- o. \set stanza = "1."
   On that farm he had some cows, in O -- hi -- o. }
  \addlyrics {
   \repeat unfold 11 \skip1 \set stanza = "2."
   On that farm he had some pigs, in O -- hi -- o. }
  \addlyrics {
   \repeat unfold 11 \skip1 \set stanza = "3."
   On that farm he had some dogs, in O -- hi -- o. }
  \addlyrics {
   \repeat unfold 11 \skip1 \set stanza = "4."
   On that farm he had some ducks, in O -- hi -- o. }
  \addlyrics {
   \repeat unfold 11 \skip1 \set stanza = "5."
   On that farm he had some sheep, in O -- hi -- o. }
  \new Staff {
    \clef bass
    c1 f2 c2 c2 g2 c1
    c1 f2 c2 c2 g2 c1
  }
  >>

We should stop lyrics from breaking through span bars at the stage where
horizontal spacing constraints are determined, when we should have start=0 and
end=INT_MAX because the whole score is conceptually one long line at that point.

[mike_apollinemike.com, 2012-03-20 06:51:08]

On Mar 20, 2012, at 3:36 AM, k-ohara5a5a@oco.net wrote:

>
http://codereview.appspot.com/5843063/diff/5001/lily/axis-group-interface.cc#...
> lily/axis-group-interface.cc:330: ||
> Pure_from_neighbor_interface::has_interface (me))
> Now that the engraver is working correctly, do you still need this
> change ?

It looks like I do, otherwise the problem creeps back in.
The solution may be to have pure-from-neighbor-interface stuff only ever factor
into extra-spacing-height and never height.  So SpanBarStub would need some
surgery.  But I'm not sure...I'll have a look tomorrow or later today.

Cheers,
MS

[Keith, 2012-03-20 17:33:41]

On Mon, 19 Mar 2012 23:51:04 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:

> On Mar 20, 2012, at 3:36 AM, k-ohara5a5a@oco.net wrote:
>
>> do you still need this change ?
>
> It looks like I do,

I had expected that separation-item.cc calls pure_height with start=0 and
end=INT_MAX for horizontal spacing; then the original test would do just what we
want without your change.  Now I vaguely remember that breakable columns get
their skylines differently.

Somebody asks for pure_height of a SpanBarStub with start and end only one
measure apart, to which the honest answer from a SpanBarStub would be its height
if that measure were alone on a line, but the answer seems to be used for
horizontal spacing of longer lines.  That is not good.

> have pure-from-neighbor-interface [affect] only extra-spacing-height
That would make more sense, but you need to pick a base Y-extent from which the
extra-spacing-height extends.

[mike_apollinemike.com, 2012-03-20 18:54:29]

On Mar 20, 2012, at 6:33 PM, Keith OHara wrote:

> On Mon, 19 Mar 2012 23:51:04 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> 
>> On Mar 20, 2012, at 3:36 AM, k-ohara5a5a@oco.net wrote:
>> 
>>> do you still need this change ?
>> 
>> It looks like I do,
> 
> I had expected that separation-item.cc calls pure_height with start=0 and
end=INT_MAX for horizontal spacing; then the original test would do just what we
want without your change.  Now I vaguely remember that breakable columns get
their skylines differently.
> 

This was theoretically buggy, as cached skylines for items should always go from
0 to INT_MAX but they were sometimes being passed other starts and ends.  I've
rectified this in item.cc in the newest patchset.

> Somebody asks for pure_height of a SpanBarStub with start and end only one
measure apart, to which the honest answer from a SpanBarStub would be its height
if that measure were alone on a line, but the answer seems to be used for
horizontal spacing of longer lines.  That is not good.
> 

The extra-spacing-height solution fixes this.

>> have pure-from-neighbor-interface [affect] only extra-spacing-height
> That would make more sense, but you need to pick a base Y-extent from which
the extra-spacing-height extends.
> 

My new patchset uses an empty extent, which gets ignored unless
extra-spacing-height is tacked on.

[Keith, 2012-03-21 05:58:42]

Yippee; no more grobs getting their pure_height from their neighbors!

Don't forget to update the commit message.  You can also "edit this issue" to
update the title and summary here on Rietveld.

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
File input/regression/pure-from-neighbor-interface-pure-height.ly (right):

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
input/regression/pure-from-neighbor-interface-pure-height.ly:1: \version
"2.15.34"
.35
The title makes less sense now.  "lyrics-spanbarstub.ly" ?

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
input/regression/pure-from-neighbor-interface-pure-height.ly:4: texidoc =
"@code{axis-group-interface::pure-height} does not take spanned
The description no longer fits.  Maybe
"empty measures do not confuse SpanBarStub.  These lyrics should remain clear of
the span bars."

http://codereview.appspot.com/5843063/diff/7/lily/item.cc
File lily/item.cc (right):

http://codereview.appspot.com/5843063/diff/7/lily/item.cc#newcode245
lily/item.cc:245: cache_pure_height (Grob::pure_height (this, 0, INT_MAX));
This commit :
http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=9fe7859a859...
depended on having 'start' and 'end' passed through, so we should put this
back.

Caching in this way assumes that the pure_height of an Item is independent of
where the line-breaks are.

Anything that gets pure_height from its neighbors would break that assumption,
but now it seems there are no such Items, as they all use
pure-from-neighbors-interface to set their extra-spacing-height.

Tied accidentals break that assumption as well, but that's not our fault.  I'll
write a cautionary comment in a separate commit.

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm#newcode1883
scm/define-grobs.scm:1883: (Y-extent . (1 . -1))
I suggest (Y-extent . #f)
because (1 . -1) looks so much like (-1 . 1), and it also makes suspicious
people like me think it is an uncommented magic number.

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm#newcode437
scm/output-lib.scm:437: (let* ((height (ly:grob-pure-height grob grob 0
10000000))
The C code uses INT_MAX to represent the end of the whole score, so it would be
nice to use the same here, if possible.

[MikeSol, 2012-03-21 07:05:50]

Running regtests.  Will post new patch once it gets a clean bill of health.

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
File input/regression/pure-from-neighbor-interface-pure-height.ly (right):

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
input/regression/pure-from-neighbor-interface-pure-height.ly:1: \version
"2.15.34"
On 2012/03/21 05:58:42, Keith wrote:
> .35
> The title makes less sense now.  "lyrics-spanbarstub.ly" ?

Done.

http://codereview.appspot.com/5843063/diff/7/input/regression/pure-from-neigh...
input/regression/pure-from-neighbor-interface-pure-height.ly:4: texidoc =
"@code{axis-group-interface::pure-height} does not take spanned
On 2012/03/21 05:58:42, Keith wrote:
> The description no longer fits.  Maybe
> "empty measures do not confuse SpanBarStub.  These lyrics should remain clear
of
> the span bars."

Done.

http://codereview.appspot.com/5843063/diff/7/lily/item.cc
File lily/item.cc (right):

http://codereview.appspot.com/5843063/diff/7/lily/item.cc#newcode245
lily/item.cc:245: cache_pure_height (Grob::pure_height (this, 0, INT_MAX));
On 2012/03/21 05:58:42, Keith wrote:
> This commit :
>
http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=9fe7859a859...
> depended on having 'start' and 'end' passed through, so we should put this
back.
> 
> Caching in this way assumes that the pure_height of an Item is independent of
> where the line-breaks are.
> 
> Anything that gets pure_height from its neighbors would break that
assumption,
> but now it seems there are no such Items, as they all use
> pure-from-neighbors-interface to set their extra-spacing-height.
> 
> Tied accidentals break that assumption as well, but that's not our fault. 
I'll
> write a cautionary comment in a separate commit.

Done.

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/define-grobs.scm#newcode1883
scm/define-grobs.scm:1883: (Y-extent . (1 . -1))
On 2012/03/21 05:58:42, Keith wrote:
> I suggest (Y-extent . #f)
> because (1 . -1) looks so much like (-1 . 1), and it also makes suspicious
> people like me think it is an uncommented magic number.

Done.

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm#newcode437
scm/output-lib.scm:437: (let* ((height (ly:grob-pure-height grob grob 0
10000000))
On 2012/03/21 05:58:42, Keith wrote:
> The C code uses INT_MAX to represent the end of the whole score, so it would
be
> nice to use the same here, if possible.

There'd have to be a constant defined for INT_MAX in guile.  I believe that
INT_MAX can change between computers (not sure if it's the compiler or system
that does this).  Probably worth doing in a separate commit where INT-MAX is
defined in the same place as all of the PI constants and then plugged anywhere
where there's a large integer.

[dak, 2012-03-21 08:22:25]

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/5843063/diff/7/scm/output-lib.scm#newcode437
scm/output-lib.scm:437: (let* ((height (ly:grob-pure-height grob grob 0
10000000))
On 2012/03/21 07:05:50, MikeSol wrote:
> On 2012/03/21 05:58:42, Keith wrote:
> > The C code uses INT_MAX to represent the end of the whole score, so it would
> be
> > nice to use the same here, if possible.
> 
> There'd have to be a constant defined for INT_MAX in guile.  I believe that
> INT_MAX can change between computers (not sure if it's the compiler or system
> that does this).  Probably worth doing in a separate commit where INT-MAX is
> defined in the same place as all of the PI constants and then plugged anywhere
> where there's a large integer.

INT_MAX would be a stupid constant to use in Scheme since it rather certainly
does not fit in a single cell and thus does not have an efficient
representation.

If we need such a thing, we should pick an arbitrary large constant that is
large enough to do the job and small enough not to trigger multi-cell
representations in any Guile implementation.