Re: note-collison.cc: Scale shifts by width of note at left; issue 1713 (issue 6189048)

[janek . lilypond]

LGTM
I apologize that you had to wait.  Thank you for your work!!


http://codereview.appspot.com/6189048/diff/9003/input/regression/collision-whole.ly
File input/regression/collision-whole.ly (right):

http://codereview.appspot.com/6189048/diff/9003/input/regression/collision-whole.ly#newcode16
input/regression/collision-whole.ly:16: }
I suggest to use this as regression test:

\paper { indent = 0 }

\markup "collisions - quarter note in upper voice"

\new Staff <<
  { \repeat unfold 12 { b'4*4 } }
  \\
  {
    \override Staff.NoteHead #'style = #'altdefault
    c''4*4        b' a'
    c''2*2        b' a'
    c''1          b' a'
    c''\breve*1/2 b' a'
  }


\markup "collisions - half note in upper voice"

\new Staff <<
  { \repeat unfold 12 { b'2*2 } }
  \\
  {
    \override Staff.NoteHead #'style = #'altdefault
    c''4*4        b' a'
    c''2*2        b' a'
    c''1          b' a'
    c''\breve*1/2 b' a'
  }


\markup "collisions - whole note in upper voice"

\new Staff <<
  { \repeat unfold 12 { b'1 } }
  \\
  {
    \override Staff.NoteHead #'style = #'altdefault
    c''4*4        b' a'
    c''2*2        b' a'
    c''1          b' a'
    c''\breve*1/2 b' a'
  }


\markup "collisions - breve note in upper voice"

\new Staff <<
  {
    \override Staff.NoteHead #'style = #'altdefault
    \repeat unfold 12 { b'\breve*1/2 }
  }
  \\
  {
    \override Staff.NoteHead #'style = #'altdefault
    c''4*4        b' a'
    c''2*2        b' a'
    c''1          b' a'
    c''\breve*1/2 b' a'
  }


http://codereview.appspot.com/6189048/diff/9003/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/6189048/diff/9003/lily/note-collision.cc#newcode181
lily/note-collision.cc:181: && (up_ball_type <= 0 || down_ball_type <=
0));
Excellent solution!

I was wondering "how the hell Lily can know which noteheads are
rectangular (non-oval)" - your solution is great!  In the future we may
want to use some shape recognition algorithms (or simply adapt Mike's
skylines for use in horizontal spacing), but for now this is perfect.

But i think the comment should read "for breves and longer"?
Actually, i would rephrase that comment to
"breves and longer notes are rectangular in shape, so if they are
present we shift columns as in full collision".

http://codereview.appspot.com/6189048/diff/9003/lily/note-collision.cc#newcode301
lily/note-collision.cc:301: in calc_positioning_done(), by the width of
the downstem note.
Why don't we get gid of that multiplication?  It seems to be
complicating things and happens in an unexpected (by me at least) place
- is there a reason for it?

http://codereview.appspot.com/6189048/