line_count fixes

line_count fixes

1. implementation does not assume staff centred at zero
2. where used for determining whether something falls on a line,
   use Staff_symbol_referencer::on_line or on_staff_line
3. where used for determining whether something is within staff or not,
   use Staff_symbol_referencer::staff_span

[benko.pal, 2012-05-13 19:26:07]

http://codereview.appspot.com/6211047/diff/1/lily/slur-scoring.cc
File lily/slur-scoring.cc (right):

http://codereview.appspot.com/6211047/diff/1/lily/slur-scoring.cc#newcode593
lily/slur-scoring.cc:593: && Staff_symbol_referencer::on_staff_line (on_staff,
(int) rint (pos)))
this condition is stricter than the original, which wasn't bounded from below. 
as a result some regression tests changed:
spacing-horizontal-skyline-grace improved (probably only by chance);
the change in quote-overrides is easy to see, I hope it's an improvement for
all;
slur-symmetry perhaps seems more symmetric to me now, but I'd like someone to
take a look into it;
I can't really see the actual difference in other lower slur changes.

http://codereview.appspot.com/6211047/diff/1/lily/time-signature.cc
File lily/time-signature.cc (right):

http://codereview.appspot.com/6211047/diff/1/lily/time-signature.cc#newcode61
lily/time-signature.cc:61: Real offset = staff_span.is_empty () ? 0.0 :
staff_span.center ();
this line creates a difference for staves not symmetric to 0.
for wildly asymmetric staves (non-centered-bar-lines,
breathing-sign-custom-staff) this is an improvement to me; the changes in
staff-line-positions and rest-on-nonstandard-staff are not that clean.

[Graham Percival, 2012-05-14 18:57:18]

LGTM

[benko.pal, 2012-05-14 20:00:27]

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc#newcode1282
lily/beam.cc:1282: staff_span *= 0.5 / staff_space;
division is silly and works only because staff_space is generally 1.

[Keith, 2012-05-15 07:24:30]

Pál, I'm not on a system today that can compile Lilypond so I cannot look at the
points you asked about, at least not today.  I hope someone else can.

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc#newcode1282
lily/beam.cc:1282: staff_span *= 0.5 / staff_space;
I agree that you do not want the division in this case.
I find that I often need to test by hand with an \override staff-space whenever
I touch code with these conversions.  I've been thinking of adding a couple
regression tests that have this override, but it seems they need to cover quite
a lot.

[Keith, 2012-05-23 05:44:49]

Looks good to me.

The regtest changes look like improvements,
although I wonder if the time-signature and repeat dots should simply center
around the line closest to zero.  If a user sets his line-positions entirely
below zero, he might /want/ the time-signature above his staff lines.  Why else
would he set his line-positions below zero, when he will be using a non-standard
clef placement anyway ?

http://codereview.appspot.com/6211047/diff/6001/lily/breathing-sign.cc
File lily/breathing-sign.cc (right):

http://codereview.appspot.com/6211047/diff/6001/lily/breathing-sign.cc#newcode86
lily/breathing-sign.cc:86: for (Direction i = DOWN;
Somebody recently standardized these loops to use a macro, for(UP_and_DOWN(i)),
issue 2491, so use that macro.

http://codereview.appspot.com/6211047/diff/6001/lily/time-signature.cc
File lily/time-signature.cc (right):

http://codereview.appspot.com/6211047/diff/6001/lily/time-signature.cc#newcode77
lily/time-signature.cc:77: --it;
The STL makes code very easy to read, doesn't it?  Without it, we would have to
write:

Real mid = staff->staff_span().center();
Real closest = linepos[0];
for (int i=0; i<linepos.size(); i++)
  if (fabs(linepos[i] - mid) < fabs(closest - mid))
    closest = linepos[i];

[Keith, 2012-05-23 05:46:23]

http://codereview.appspot.com/6211047/diff/6001/lily/time-signature.cc
File lily/time-signature.cc (right):

http://codereview.appspot.com/6211047/diff/6001/lily/time-signature.cc#newcode77
lily/time-signature.cc:77: --it;
On 2012/05/23 05:44:49, Keith wrote:
> Real mid = staff->staff_span().center();
  Real mid = Staff_symbol::line_span(staff).center();

[benko.pal, 2012-05-25 19:59:29]

hi Keith,

thanks for your review.

2012/5/23  <k-ohara5a5a@oco.net>:
> Looks good to me.
>
> The regtest changes look like improvements,
> although I wonder if the time-signature and repeat dots should simply
> center around the line closest to zero.  If a user sets his
> line-positions entirely below zero, he might /want/ the time-signature
> above his staff lines.  Why else would he set his line-positions below
> zero, when he will be using a non-standard clef placement anyway ?

my aims (developed together with the patch) are currently to
- keep behaviour for staves overriding only line-count (which I like)
- make staves consisting of regularly spaced lines look like those
  overriding only line-count (which I use; in ancient music instead of a
  lower ledger line I override line-positions to '(-6 -4 -2 0 2 4))
- find a solution generic enough to not handle other line spacing specially.

p

[Keith, 2012-07-10 14:59:14]

http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc#newcode151
lily/bar-line.cc:151: Real const gap_to_find = (1.0 + 3 * staffline) /
staff_space;
Pál, see new issue 2648 on the tracker.

The various meanings of staff-space are tricky.
Probably the wisest thing is to reverse this part of the patch for version 2.16,
and try again in the 2.17 versions.

[benko.pal, 2012-07-10 15:40:56]

hi Keith,

2012/7/10  <k-ohara5a5a@oco.net>:
>
> http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc#newcode151
> lily/bar-line.cc:151: Real const gap_to_find = (1.0 + 3 * staffline) /
> staff_space;
> Pál, see new issue 2648 on the tracker.

thanks for making me aware; I'll look at it in the evening.

> The various meanings of staff-space are tricky.
> Probably the wisest thing is to reverse this part of the patch for
> version 2.16, and try again in the 2.17 versions.

I don't mind either way; I hope I can sort this particular problem out quickly.

p

[Keith, 2012-07-14 07:46:59]

http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/6211047/diff/20001/lily/bar-line.cc#newcode151
lily/bar-line.cc:151: Real const gap_to_find = (1.0 + 3 * staffline) /
staff_space;
It seems you want 
Real const gap_to_find = (dot.extent(Y_AXIS).length() 
                          + 3 * staffline
                          ) / staff_space *2 ;
where the *2 is because you compare the gap_to_find with differences of staff
positions, and the distance of a staff space is two positions (as E to G).