Re: fix representation switching from line-position to staff-space (issue 6778050)

[benko . pal]

Reviewers: Keith, dak,


http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc
File lily/breathing-sign.cc (left):

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#oldcode88
lily/breathing-sign.cc:88: int const int_dim = (int) ydim[i];
On 2012/10/27 19:36:21, Keith wrote:
> If you want to avoid ending the divisi at a fractional position, maybe
just
> ydim[i] = int_dim;

this juggling was done only because Symbol_referencer::on_staff_line
takes an int, whereas the staff positions are Real's.

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

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#newcode99
lily/breathing-sign.cc:99: standard algorithms are suitable to find the
upper line of
On 2012/10/27 19:51:49, dak wrote:
> Do we really need a complicated variable line arrangement algorithm
here?

I want staves with line-positions like (-2 0 2 4) work.  I could arrange
for this a bit simpler, but the moral I drew from the line-count saga is
that from time to time comes a user with tablature or line-positions (-4
4) or (-4 0 4)  (I know, they are not using \divisioMaior, only repeat
signs, bar lines, slurs, time signatures, etc.), and if I can cater for
them, then I should do it.

> Nobody complained before this was "fixed" 2.15-ish.

I apologise, my original "fix" is plain silly.

> It really looks like an overly complex solution to a self-made
problem.

of course I wouldn't mind anybody coming up with a more elegant
solution, but the original code relying on the line-count property is
unacceptable for me (not here, but at enough other places to make me
hunt these down).

> I think that for most of those problems, the very old code looking
only at the
> line-count property (even when it was overruled via line-positions for
special
> effects) was producing exactly what people expected and wanted.

I started this series about a year ago because I couldn't make the old
code produce what I wanted.  see also your comment in
http://code.google.com/p/lilypond/issues/detail?id=2783#c35
(BTW let me note that I'm working on rests as hinted at in comment 34.)

> All that magic and second-guessing does not appear to have resulted in
an
> improvement for actual scores.  Not in results, and most certainly not
in
> predictability.

if "common" staves are not predictable, it's a bug and I'm willing to
work on it.

having said all this, I don't mind if this particular patch is rejected.

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#newcode112
lily/breathing-sign.cc:112: assert (line_pos.begin () < it);
On 2012/10/27 19:36:21, Keith wrote:
> Can you assert this here, and similarly a few lines down?   If someone
uses a
> collapsed staff with all line-positions equal, I would have thought
that
> lower_bound() would return a pointer to the first line-position, and
similarly
> upper_bound() a pointer to one beyond the last line-position.

you are right; what's more, assertions are futile in such cases, this
condition must be checked (and perhaps I'll just do nothing).

Description:
fix representation switching from line-position to staff-space

Please review this at http://codereview.appspot.com/6778050/

Affected files:
  M input/regression/breathing-sign-ancient.ly
  M lily/breathing-sign.cc