Fix Issue 1290

Fix Issue 1290

Create an optional horizontal-padding parameter to Skyline::distance
When horizontal-padding is not zero, padding is added to both systems
when calculating the distance.

Get the System's skyline-horizontal-padding property, and pass
it to distance () in page-layout-problem.cc

Add regression test for this feature.

[Carl, 2011-01-02 05:02:11]

Here is a patch to fix issue 1290.

It works, but it may need to be cleaned up.  I'm not sure the code is as elegant
as it could be.  I'm not really comfortable with all of the C++ syntax used in
lilypond.

Please review it carefully, and let me know how it can be improved.

Thanks,

Carl

[joeneeman, 2011-01-02 05:19:57]

I don't know if it's important, but it may be worth mentioning somewhere that
skyline-horizontal-padding works differently for VerticalAxisGroup and System.
(Because in VerticalAxisGroup, skyline-horizontal-padding takes effect while the
outside-staff-grobs are being placed).

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:274: minimum_distance = (*sky)[UP].distance
(bottom_skyline_, scm_to_double (prob->get_property
("skyline-horizontal-padding")));
robust_scm2double is probably better

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
Why restrict to y_intercept_ < 0?

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i->y_intercept_ < 0 ? i->y_intercept_ : 0 ,
i->y_intercept_)));
...and if you do restrict to y_intercept_ < 0, then this is redundant.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding, a,
(Direction) 1);
pad_sky instead of padSky
UP instead of (Direction) 1
and perhaps a comment about why you need to use UP instead of sky_ (which would
seem intuitive)

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = &other;
I'm not sure, but I think "Skyline const *" is more consistent with the rest of
the code.

[Trevor Daniels, 2011-01-02 09:01:11]

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 { <c'''-1
e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
Can not a more precise and shorter test using \break be devised?  With this code
interleaving occurs only once right at the bottom even without the override.

[Keith, 2011-01-02 23:09:02]

Yippee for the patch.
Doesn't quite work yet, probably because the function that uses your new code,
append_prob, is not used in the situation where we need the extra padding. 
append_system is called. (Confirmed by printf-debugging using your and my .ly
test files)

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 { <c'''-1
e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
On 2011/01/02 09:01:11, Trevor Daniels wrote:
> Can not a more precise and shorter test
I had a test file for 1290, and related, that shows what does and does not get
displaced by the various skylines:
#(ly:set-option 'debug-skylines #t)
\score {
  {
    \repeat unfold 2 {
      \mark "mark"
      a,2_"fa" gisis'''!^"gg" | 
      \mark "m"
      b,4 a'_"tx" c'2 \break
    }
  } \layout { 
    ragged-right = ##t
    indent = #0 
    \context {
      \Score
      \override System #'skyline-horizontal-padding = #0.0
      \override TimeSignature #'stencil = ##f
    }
  }
}

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:204: Real minimum_distance = up_skyline.distance
(bottom_skyline_) + padding;
We need to call your distance here, no?

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:266: Page_layout_problem::append_prob (Prob *prob,
Spring const& spring, Real padding)
This function is not called for either of our test cases.

[Keith, 2011-01-02 23:54:14]

It is interesting to see what gets horizontally padded and what does not --
accidentals do not, for example. 
Whether by accident or design, the choices on what to pad seem reasonable.

It seems that the same padded skyline is used both for page layout, and for
drawing the outer skylines with debug-skylines.  However, I could not follow the
code to see if the same function is called.  I would worry about confusion in
the future if System's skyline is padded two different places in the code.

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:204: Real minimum_distance = up_skyline.distance
(bottom_skyline_) + padding;
On 2011/01/02 23:09:02, Keith wrote:
If I call using Carl's new function here,

  Real minimum_distance = up_skyline.distance (bottom_skyline_, 
    robust_scm2double (sys->get_property ("skyline-horizontal-padding"), 0)) +
padding;

I like the behavior much than the way 2.12.3 worked.  Old 2.12's horizontal
padding would push rehearsal marks and bar numbers away from notes on their home
staff.

[Carl, 2011-01-03 03:48:09]

Thanks for all the comments.

Keith has identified the correct place to put include the system horizontal
padding, and it now works properly.  

New patch set coming shortly.

Carl

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 { <c'''-1
e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
On 2011/01/02 09:01:11, Trevor Daniels wrote:
> Can not a more precise and shorter test using \break be devised?  With this
code
> interleaving occurs only once right at the bottom even without the override.

Yes, I'll fix it in the next patch.

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-hori...
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 { <c'''-1
e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
On 2011/01/02 23:09:02, Keith wrote:
> On 2011/01/02 09:01:11, Trevor Daniels wrote:
> > Can not a more precise and shorter test
> I had a test file for 1290, and related, that shows what does and does not get
> displaced by the various skylines:
> #(ly:set-option 'debug-skylines #t)
> \score {
>   {
>     \repeat unfold 2 {
>       \mark "mark"
>       a,2_"fa" gisis'''!^"gg" | 
>       \mark "m"
>       b,4 a'_"tx" c'2 \break
>     }
>   } \layout { 
>     ragged-right = ##t
>     indent = #0 
>     \context {
>       \Score
>       \override System #'skyline-horizontal-padding = #0.0
>       \override TimeSignature #'stencil = ##f
>     }
>   }
> }

Thanks, Keith.  I'll look into this as a regression test.

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:204: Real minimum_distance = up_skyline.distance
(bottom_skyline_) + padding;
On 2011/01/02 23:09:02, Keith wrote:
> We need to call your distance here, no?

Yes, thanks for finding this!  You saved me the time of doing it!

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
On 2011/01/02 05:19:58, joeneeman wrote:
> Why restrict to y_intercept_ < 0?
 Because if I have a y intercept that is less than zero, and create a box with
that y intercept, the padded skyline comes out with all zero heights.  I don't
know why it works that way, but it does.

I tried several things to get it to behave differently, but was unsuccessful. 
And I don't think that a negative skyline will ever be limiting in inter-system
spacing, so I didn't feel that it was a problem.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i->y_intercept_ < 0 ? i->y_intercept_ : 0 ,
i->y_intercept_)));
On 2011/01/02 05:19:58, joeneeman wrote:
> ...and if you do restrict to y_intercept_ < 0, then this is redundant.
Yes, this is redundant, left over from one of my tries to properly deal with
negative skylines.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding, a,
(Direction) 1);
On 2011/01/02 05:19:58, joeneeman wrote:
> pad_sky instead of padSky
> UP instead of (Direction) 1
> and perhaps a comment about why you need to use UP instead of sky_ (which
would
> seem intuitive)

Yes, thank you.  These are good suggestions.  I will fix them.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = &other;
On 2011/01/02 05:19:58, joeneeman wrote:
> I'm not sure, but I think "Skyline const *" is more consistent with the rest
of
> the code.

Thanks, I'll try to see if that works.

[Carl, 2011-01-03 15:20:31]

I've made the changes, and now the patch actually works.

Thanks all for your comments!

Carl

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:266: Page_layout_problem::append_prob (Prob *prob,
Spring const& spring, Real padding)
On 2011/01/02 23:09:02, Keith wrote:
> This function is not called for either of our test cases.

Yes, I guess this is called for adding a prob that is not a system.  So I'm
removing the code here.

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#n...
lily/page-layout-problem.cc:274: minimum_distance = (*sky)[UP].distance
(bottom_skyline_, scm_to_double (prob->get_property
("skyline-horizontal-padding")));
On 2011/01/02 05:19:58, joeneeman wrote:
> robust_scm2double is probably better

removed (see above)

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
OK, I got it to work properly now.  I have a better algorithm for handling the
bottom of the extracted box.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i->y_intercept_ < 0 ? i->y_intercept_ : 0 ,
i->y_intercept_)));
Now I handle the y_intercept_ < 0 problem properly here.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding, a,
(Direction) 1);
Using sky_ now.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = &other;
On 2011/01/02 05:19:58, joeneeman wrote:
> I'm not sure, but I think "Skyline const *" is more consistent with the rest
of
> the code.

Done.

[Keith, 2011-01-04 01:46:45]

The new patch fixes the issue, but we can't yet use it in larger scores, if we
want to do that.

Whenever there is more than one staff in a system, and any non-zero
skyline-horizontal-padding, minimum_distance is computed reasonably for the
first and second systems, but not the third and later.  For a 2-staff system
(with non-protruding bass clefs to make the math easier) the patch computes
minimum_distance
3.05, 7.33, 29.41, 29.38
as it adds four systems to a page.

[Keith, 2011-01-04 02:02:45]

Oope, I forgot to remove bar numbers.  
If I remove the bar numbers from the /beginning/ of the line, the patch is
working in the few pieces I've tested.  Bar numbers at the middle or end of the
line, tempo marks, and rehearsal numbers, all cause no problem.

[Carl, 2011-01-04 03:42:26]

On 2011/01/04 01:46:45, Keith wrote:
> For a 2-staff system
> (with non-protruding bass clefs to make the math easier) the patch computes
> minimum_distance
> 3.05, 7.33, 29.41, 29.38
> as it adds four systems to a page.

Can you send me a test file so I can check it out?

Thanks,

Carl

[Carl, 2011-01-04 18:34:42]

Thanks, Keith for identifying the problem with bar numbers at the beginning of
the line.

I've fixed that problem, and I'm more confident in the logic of the box
extraction and padded skyline creation.

I've modified the regression test so it has 3 systems, and thus would catch the
problem that Keith identified with the bar numbers.

Please review.

Thanks,

Carl

[Keith, 2011-01-05 01:45:56]

I tried to break it; it did not break. I say push it.

On 2011/01/02 05:19:57, joeneeman wrote:
> I don't know if it's important, but it may be worth mentioning somewhere that
> skyline-horizontal-padding works differently for VerticalAxisGroup and System.

The existing docs are good enough, in that they let me find how things were
/supposed/ to work while I was attacking 1290 by tweaking knobs.  Optional text,
if Carl finds it more accurate:

  ./scm/define-grob-properties.scm : 722
-     (skyline-horizontal-padding ,number? "For determining the
-vertical distance between two staves, it is possible to have a
-configuration which would result in a tight interleaving of grobs from
-the top staff and the bottom staff.  The larger this parameter is, the
-farther apart the staves are placed in such a configuration.")
+     (skyline-horizontal-padding ,number? "Padding to prevent tight
+interleaving of grobs.  This property, in a VerticalAxisGroup,
+defines a minimum horizontal distance that will remain clear
+during the placement of outside-staff objects,
+and while setting vertical spacing between staves.
+In a System, this property defines the horizontal clearance
+to be maintained during vertical spacing of systems.")

http://codereview.appspot.com/3832046/diff/21001/input/regression/skyline-hor...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/21001/input/regression/skyline-hor...
input/regression/skyline-horizontal-padding.ly:15: \repeat unfold 3 { <c'''-1
e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' \break}
No need for 3 systems. It does not, in fact, reveal the bug in previous patch (I
tried it). I could find no way to show that bug with just one staff per system.

[c_sorensen, 2011-01-05 04:07:06]

On 1/4/11 6:45 PM, "k-ohara5a5a@oco.net" <k-ohara5a5a@oco.net> wrote:

> I tried to break it; it did not break. I say push it.

Thanks for trying!  And thanks for your help in identifying the right place
to put in the call to the new function.

> 
> On 2011/01/02 05:19:57, joeneeman wrote:
>> I don't know if it's important, but it may be worth mentioning
> somewhere that
>> skyline-horizontal-padding works differently for VerticalAxisGroup and
> System.
> 
> The existing docs are good enough, in that they let me find how things
> were /supposed/ to work while I was attacking 1290 by tweaking knobs.
> Optional text, if Carl finds it more accurate:

I think that the issue Joe is pointing at is that the skylines are added to
the grobs that make up the vertical axis group.  But the skylines aren't
added to the System; they're just used in calculating the distances.

I'm not sure that the distinction is really important, as a practical
matter.

Thanks,

Carl

[Graham Percival (old account), 2011-01-05 05:11:04]

LGTM, and it builds everything fine from scratch.

[Carl, 2011-01-07 14:30:17]

I've added comments about the need for the horizontal-padding in the distance
call to be used with System grobs, and I've moved the skyline-horizontal-padding
from an override to a default value for the System grob.

I think this should make everything work right out of the box now, without
requiring overrides.

Any further comments?

Thanks,

Carl

[joeneeman, 2011-01-07 16:47:27]

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
On 2011/01/03 03:48:09, Carl wrote:
> On 2011/01/02 05:19:58, joeneeman wrote:
> > Why restrict to y_intercept_ < 0?
>  Because if I have a y intercept that is less than zero, and create a box with
> that y intercept, the padded skyline comes out with all zero heights.  I don't
> know why it works that way, but it does.

That's very strange. Do you have an example file that triggers this behaviour?

> I tried several things to get it to behave differently, but was unsuccessful. 
> And I don't think that a negative skyline will ever be limiting in
inter-system
> spacing, so I didn't feel that it was a problem.

Probably not, but I still feel that it's bad to have methods with strange corner
cases. At the very least, there should be a comment explaining that the distance
function will throw away any constraints with negative height.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
lily/skyline.cc:391: {
I still don't understand why this function is so complicated. Does this not
work?

Real start = -infinity_f;
vector<Box> boxes;
list<Building>::const_iterator end = src.buildings_.end ();
for (list<Building>::const_iterator i = src.buildings_.begin (); i != end; sta
     rt=i->end_, i++)
  if ((i->slope_ == 0) && !isinf (i->y_intercept_))
    boxes.push_back (Box (Interval (start, i->end_),
                          Interval (-infinity_f, i->y_intercept_)));

buildings_ = internal_build_skyline (&boxes, horizon_padding, X_AXIS, UP);
sky_ = src.sky_;

By the way, the above code (and your version also) will break if we every switch
to using more accurate outlines for grobs (ie. more accurate than bounding
boxes) because it could be throwing away interesting things when it discards
non-horizontal segments.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
lily/skyline.cc:419: Axis vert_axis = other_axis (horizon_axis);
This should have X_AXIS instead of "a", because you put the horizon axis into
the X_AXIS of your boxes.

[c_sorensen, 2011-01-07 17:08:09]

On 1/7/11 9:47 AM, "joeneeman@gmail.com" <joeneeman@gmail.com> wrote:

> 
> 
> http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
> File lily/skyline.cc (right):
> 
> http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
> lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
> i->y_intercept_ >= 0)
> On 2011/01/03 03:48:09, Carl wrote:
>> On 2011/01/02 05:19:58, joeneeman wrote:
>>> Why restrict to y_intercept_ < 0?
>>   Because if I have a y intercept that is less than zero, and create a
> box with
>> that y intercept, the padded skyline comes out with all zero heights.
> I don't
>> know why it works that way, but it does.
> 
> That's very strange. Do you have an example file that triggers this
> behaviour?

The regression test file in the patch triggers this behavior.

> 
>> I tried several things to get it to behave differently, but was
> unsuccessful.
>> And I don't think that a negative skyline will ever be limiting in
> inter-system
>> spacing, so I didn't feel that it was a problem.
> 
> Probably not, but I still feel that it's bad to have methods with
> strange corner cases. At the very least, there should be a comment
> explaining that the distance function will throw away any constraints
> with negative height.
> 
> http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
> lily/skyline.cc:391: {
> I still don't understand why this function is so complicated. Does this
> not work?
> 
> Real start = -infinity_f;
> vector<Box> boxes;
> list<Building>::const_iterator end = src.buildings_.end ();
> for (list<Building>::const_iterator i = src.buildings_.begin (); i !=
> end; sta
>       rt=i->end_, i++)
>    if ((i->slope_ == 0) && !isinf (i->y_intercept_))
>      boxes.push_back (Box (Interval (start, i->end_),
>                            Interval (-infinity_f, i->y_intercept_)));
> 
> buildings_ = internal_build_skyline (&boxes, horizon_padding, X_AXIS,
> UP);
> sky_ = src.sky_;

I will try this.  I have not yet tried it. I'll get back to you on the
results.

> 
> By the way, the above code (and your version also) will break if we
> every switch to using more accurate outlines for grobs (ie. more
> accurate than bounding boxes) because it could be throwing away
> interesting things when it discards non-horizontal segments.

That is true, but at that point we will not be using Boxes to define
skylines.

> 
> http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
> lily/skyline.cc:419: Axis vert_axis = other_axis (horizon_axis);
> This should have X_AXIS instead of "a", because you put the horizon axis
> into the X_AXIS of your boxes.

I'll fix this.  Thanks for the comments.

Thanks,

Carl

[Neil Puttock, 2011-01-07 21:41:51]

Hi Carl,

Do we have to set a default for skyline-horizontal-padding?  It has a
detrimental effect on some of the regtests (particularly
stem-length-estimation.ly).

Cheers,
Neil

[Neil Puttock, 2011-01-07 21:59:41]

http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-hor...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-hor...
input/regression/skyline-horizontal-padding.ly:12: \score {
needs a \book { } block to prevent lilypond-book hijacking the system-system
spacing

[c_sorensen, 2011-01-07 23:28:19]

On 1/7/11 2:41 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> Hi Carl,
> 
> Do we have to set a default for skyline-horizontal-padding?  It has a
> detrimental effect on some of the regtests (particularly
> stem-length-estimation.ly).

Well, no, we don't.  However, the original bug says that we're crowding
stuff too closely together.  If we don't change the default, we aren't
really solving the original request unless we also do the Documentation.

I was hoping that by setting the default, we'd get good spacing and we
wouldn't need the override.

I see three possibilities:

1) Change the default value for System skyline-horizontal-padding to see if
we can make both regtests succeed.

2) Eliminate the default value for System skyline-horizontal-padding, and
use an override in skyline-horizontal-padding.ly (and document it more
clearly).

3) Leave the default value as it is, and \override it to zero in
stem-length-estimation.ly.

I think I prefer option 1, followed by 3, followed by 2.

Thanks,

Carl

[Keith, 2011-01-07 23:53:34]

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

http://codereview.appspot.com/3832046/diff/27001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1940: (skyline-horizontal-padding . 2)
too big for a default.  I suggest 0.5.

A horizontal space of _4-times_ this value is maintained between potential
interleavers, because the diagonal starts one padding a way from the grob, the
vertical is two paddings away, and grobs from each system are padded.

I realize that a value of 1.5 or greater is required to affect your regression
test, but an \override in the score would make a more useful regression test
anyway.  

The only musical example I have seen where I would want the padding is the
regtest les-nereides.ly, which greatly improved with horizontal-padding 0.5.

[Carl, 2011-01-08 00:34:35]

Thanks for the feedback.  I've responded to each of the suggestions you've given
me.

Carl

http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-hor...
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-hor...
input/regression/skyline-horizontal-padding.ly:12: \score {
On 2011/01/07 21:59:41, Neil Puttock wrote:
> needs a \book { } block to prevent lilypond-book hijacking the system-system
> spacing

Done.

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
lily/skyline.cc:391: {
It worked, once I changed boxes from a vector to a list.

Thanks for the insight!

http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
lily/skyline.cc:419: Skyline padded_skyline = Skyline (boxes, horizon_padding,
a, UP);
On 2011/01/07 16:47:27, joeneeman wrote:
> This should have X_AXIS instead of "a", because you put the horizon axis into
> the X_AXIS of your boxes.

Done.

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

http://codereview.appspot.com/3832046/diff/27001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1940: (skyline-horizontal-padding . 2)
On 2011/01/07 23:53:34, Keith wrote:
> too big for a default.  I suggest 0.5.
> 
> A horizontal space of _4-times_ this value is maintained between potential
> interleavers, because the diagonal starts one padding a way from the grob, the
> vertical is two paddings away, and grobs from each system are padded.
> 
> I realize that a value of 1.5 or greater is required to affect your regression
> test, but an \override in the score would make a more useful regression test
> anyway.  
> 
> The only musical example I have seen where I would want the padding is the
> regtest les-nereides.ly, which greatly improved with horizontal-padding 0.5.

Done.  0.5 also works fine with stem-length-estimation.ly.

[Carl, 2011-01-08 00:35:46]

On 2011/01/07 21:41:51, Neil Puttock wrote:
> Hi Carl,
> 
> Do we have to set a default for skyline-horizontal-padding?  It has a
> detrimental effect on some of the regtests (particularly
> stem-length-estimation.ly).

I've set the default to 0.5, in accordance with Keith's suggestions.  It leaves
stem-length-estimation.ly alone now.

[c_sorensen, 2011-01-08 03:48:31]

On 1/7/11 4:28 PM, "Carl Sorensen" <c_sorensen@byu.edu> wrote:

> On 1/7/11 2:41 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:
> 
>> Hi Carl,
>> 
>> Do we have to set a default for skyline-horizontal-padding?  It has a
>> detrimental effect on some of the regtests (particularly
>> stem-length-estimation.ly).
> 
> I was hoping that by setting the default, we'd get good spacing and we
> wouldn't need the override.
> 
> I see three possibilities:
> 
> 1) Change the default value for System skyline-horizontal-padding to see if
> we can make both regtests succeed.
> 
> 2) Eliminate the default value for System skyline-horizontal-padding, and
> use an override in skyline-horizontal-padding.ly (and document it more
> clearly).
> 
> 3) Leave the default value as it is, and \override it to zero in
> stem-length-estimation.ly.

Further checking shows that option 1 can't work.

A default value of skyline-horizontal-padding of 1.2 gets
skyline-horizontal-padding.ly to work well.

An override to System 'skyline-horizontal-padding = #0 in
stem-length-estimation.ly gets it working well.  Since we already have
overrides to make this regtest work, I don't see it as a big problem to add
another.

Thanks,

Carl