Fixing issue 37 with extra position callback

Fixing issue 37 with extra position callback
Adds beam collision engraver

Goodbye whitespace

[Carl, 2011-01-08 18:38:55]

Looks great, Mike!

You have some code indentation issues that don't match our style.  Other than
that, I think it's good to go.

Of course, we do need a regression test as well.

Thanks,

Carl

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen Nienhuys
<hanwen@xs4all.nl>
Copyright should be 2011 Mike Solomon.

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
lily/beam-collision-engraver.cc:48: {
brace not needed

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
lily/beam-collision-engraver.cc:55: {
remove {

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
lily/beam-collision-engraver.cc:57: {
remove {

http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
lily/beam-collision-engraver.cc:83: {
remove {

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

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode944
lily/beam.cc:944: {
indent { 2 spaces

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode962
lily/beam.cc:962: {
indent 2 spaces

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode967
lily/beam.cc:967: magic = -2.0;
{ on separate line, indented 2 spaces

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode972
lily/beam.cc:972: {
indent

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode992
lily/beam.cc:992: {
indentation

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode997
lily/beam.cc:997: } else {
indentation

http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode1008
lily/beam.cc:1008: } else
else on its own line, same indentation as if

http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly#newcode68
ly/engraver-init.ly:68: \consists "Beam_collision_engraver"
Also add to TabStaff

[mike_apollinemike.com, 2011-01-08 19:15:06]

Carl,

Thanks for the comments!
I've integrated all but one of them - when you say "Also add to TabStaff," what
do you mean?  I was under the impression that TabStaff would inherit this
engraver because it inherits from Staff.

Cheers,
MS

On Jan 8, 2011, at 1:38 PM, Carl.D.Sorensen@gmail.com wrote:

> Reviewers: MikeSol,
> 
> Message:
> Looks great, Mike!
> 
> You have some code indentation issues that don't match our style.  Other
> than that, I think it's good to go.
> 
> Of course, we do need a regression test as well.
> 
> Thanks,
> 
> Carl
> 
> 
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc
> File lily/beam-collision-engraver.cc (right):
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen
> Nienhuys <hanwen@xs4all.nl>
> Copyright should be 2011 Mike Solomon.
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:48: {
> brace not needed
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:55: {
> remove {
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:57: {
> remove {
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:83: {
> remove {
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode944
> lily/beam.cc:944: {
> indent { 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode962
> lily/beam.cc:962: {
> indent 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode967
> lily/beam.cc:967: magic = -2.0;
> { on separate line, indented 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode972
> lily/beam.cc:972: {
> indent
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode992
> lily/beam.cc:992: {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode997
> lily/beam.cc:997: } else {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode1008
> lily/beam.cc:1008: } else
> else on its own line, same indentation as if
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly#newcode68
> ly/engraver-init.ly:68: \consists "Beam_collision_engraver"
> Also add to TabStaff
> 
> Description:
> Fixing issue 37 with extra position callback
> Adds beam collision engraver
> 
> Goodbye whitespace
> 
> Please review this at http://codereview.appspot.com/3928041/
> 
> Affected files:
> A lily/beam-collision-engraver.cc
> M lily/beam.cc
> M lily/include/beam.hh
> M ly/engraver-init.ly
> M scm/define-grobs.scm
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel

[mikesol_ufl.edu, 2011-01-08 19:15:09]

Carl,

Thanks for the comments!
I've integrated all but one of them - when you say "Also add to TabStaff," what
do you mean?  I was under the impression that TabStaff would inherit this
engraver because it inherits from Staff.

Cheers,
MS

On Jan 8, 2011, at 1:38 PM, Carl.D.Sorensen@gmail.com wrote:

> Reviewers: MikeSol,
> 
> Message:
> Looks great, Mike!
> 
> You have some code indentation issues that don't match our style.  Other
> than that, I think it's good to go.
> 
> Of course, we do need a regression test as well.
> 
> Thanks,
> 
> Carl
> 
> 
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc
> File lily/beam-collision-engraver.cc (right):
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen
> Nienhuys <hanwen@xs4all.nl>
> Copyright should be 2011 Mike Solomon.
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:48: {
> brace not needed
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:55: {
> remove {
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:57: {
> remove {
> 
>
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#...
> lily/beam-collision-engraver.cc:83: {
> remove {
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode944
> lily/beam.cc:944: {
> indent { 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode962
> lily/beam.cc:962: {
> indent 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode967
> lily/beam.cc:967: magic = -2.0;
> { on separate line, indented 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode972
> lily/beam.cc:972: {
> indent
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode992
> lily/beam.cc:992: {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode997
> lily/beam.cc:997: } else {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode1008
> lily/beam.cc:1008: } else
> else on its own line, same indentation as if
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly#newcode68
> ly/engraver-init.ly:68: \consists "Beam_collision_engraver"
> Also add to TabStaff
> 
> Description:
> Fixing issue 37 with extra position callback
> Adds beam collision engraver
> 
> Goodbye whitespace
> 
> Please review this at http://codereview.appspot.com/3928041/
> 
> Affected files:
> A lily/beam-collision-engraver.cc
> M lily/beam.cc
> M lily/include/beam.hh
> M ly/engraver-init.ly
> M scm/define-grobs.scm
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel

[mikesol_ufl.edu, 2011-01-08 19:48:11]

All is fixed.  Attached is the original patch plus one with Carl's suggested
formatting changes.

I ran all of the regtests and nothing breaks.

I'd like one other person to test these patches out w/ the attached .ly document
to confirm.  Then, if there are no other other comments, please push the patches
and close the Google issue.

Cheers,
MS

[Neil Puttock, 2011-01-08 19:52:19]

On 8 January 2011 19:48, Mike Solomon <mikesol@ufl.edu> wrote:

> I ran all of the regtests and nothing breaks.

This is not good enough.

You *must* do a proper regression test comparison.

Withough doing make check, I can already see it will fail since you
haven't documented covering-note-heads in beam.cc and
define-grob-properties.scm.

Cheers,
Neil

[c_sorensen, 2011-01-08 19:55:26]

On 1/8/11 12:48 PM, "Mike Solomon" <mikesol@ufl.edu> wrote:

> All is fixed.  Attached is the original patch plus one with Carl's suggested
> formatting changes.
> 

Latest patch is posted on Rietveld, as

 http://codereview.appspot.com/3928041/

Thanks,

Carl

[c_sorensen, 2011-01-08 19:59:43]

On 1/8/11 12:52 PM, "Neil Puttock" <n.puttock@gmail.com> wrote:

> On 8 January 2011 19:48, Mike Solomon <mikesol@ufl.edu> wrote:
> 
>> I ran all of the regtests and nothing breaks.
> 
> This is not good enough.
> 
> You *must* do a proper regression test comparison.

As defined in the CG, this means:

Run 

make test-baseline 

before applying patches.

Run 

make check 

after applying patches.

Then check the results found in

out/test-results/index.html

See C.G. section 9.4.

Thanks,

Carl

[c_sorensen, 2011-01-08 20:05:13]

On 1/8/11 12:48 PM, "Mike Solomon" <mikesol@ufl.edu> wrote:

> All is fixed.  Attached is the original patch plus one with Carl's suggested
> formatting changes.
> 
> I ran all of the regtests and nothing breaks.
> 
> I'd like one other person to test these patches out w/ the attached .ly
> document to confirm.  Then, if there are no other other comments, please push
> the patches and close the Google issue.

It's still missing a regtest.  We need a file something like

input/regression/beam-note-head-collision.ly

created from the bug report file.  It should be quite a bit shorter than
your full test file.

Thanks,

Carl

[Carl, 2011-01-09 02:35:22]

Latest patch set uploaded with full side-by-side-diffs.

Thanks,

Carl

[Carl, 2011-01-09 02:40:12]

LGTM.

Don't forget to fix your copyright on beam-collision-engraver.

One set of braces to be removed.

Thanks,

Carl

http://codereview.appspot.com/3928041/diff/17001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):

http://codereview.appspot.com/3928041/diff/17001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen Nienhuys
<hanwen@xs4all.nl>
Copyright 2011 Mike Solomon

http://codereview.appspot.com/3928041/diff/17001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/17001/lily/beam.cc#newcode969
lily/beam.cc:969: {
No brackets here

[mikesol_ufl.edu, 2011-01-09 03:13:46]

Done - attached is a fresh patch set.  The first three are what's already on
Rietveld, and the 4th is Carl's formatting & copyright changes.

Cheers,
MS

[Carl, 2011-01-09 05:25:43]

New patches pushed.

Thanks,

Carl

[Keith, 2011-01-09 07:13:23]

I only re-checked that one score, after the patch to overcome the autobeam
confusion.  Only unnecessary lengthening now is some stems on cue notes, but it
really sticks out.  I could only reduce it to four required ingredients: manual
beams, 32nd notes, dots, and CueVoice.
\music = \relative c'' {
  \repeat unfold 4 {
    g16.[ b32] g16. b32
    g8.[ b16]  g8. b16
} }
\new Voice {
  \new CueVoice {\music }
}

I'm out for a while, but give somebody time to try piano music, where the fix
might actually help.

[Carl, 2011-01-12 04:39:13]

New patch set uploaded.

[Graham Percival (old account), 2011-01-22 21:28:08]

Regtest comparison looks fine, builds the docs from scratch.  Let's push it in
24 hours if there's no complaints.

[Carl, 2011-01-23 05:20:04]

LGTM.

Carl

[Neil Puttock, 2011-01-23 17:46:58]

Looks good, but there's still some excessive beam translation in
slur-scoring.ly.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
File input/regression/beam-collision.ly (right):

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:2: \header{
\header {

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:3: texidoc="@cindex Beam Collisions
indent

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:12: \time 4/4
indent

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:13: << { s128 e [ s cis, ] } \\ { b'' [ s b ]
} >> r8..
e[

etc.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:24: #include "item.hh"
resort alphabetically

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:45: void process_acknowledged ();
remove

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:52:
Beam_collision_engraver::stop_translation_timestep()
timestep ()

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:54: for (vsize i=0; i <
covered_interior_grobs_.size (); i++)
i = 0 (etc.)

Why use a separate vector?  There's no difference in processing, so it would
make more sense to add all the grobs to covered_grobs_.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:70: in auto beaming, end beams are signaled with
their beams at a later timestep.
acknowledge manual beams only instead, then there's no need for this hack

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:96: active_beams_.erase (active_beams_.begin() +
j);
begin ()

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:103:
Beam_collision_engraver::Beam_collision_engraver() {}
engraver ()

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:106:
Beam_collision_engraver::process_acknowledged() {}
remove

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:115:
Beam_collision_engraver::acknowledge_accidental (Grob_info i)
Since you're acknowledging noteheads, they already cache accidentals, so you
could extract them later.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode969
lily/beam.cc:969: Stencil *stencil = unsmob_stencil (covered_grob->get_property
("stencil"));
use

grob->is_live ()

then

grob->extent (common, axis)

instead of extracting stencil

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode974
lily/beam.cc:974: width = stencil->extent(X_AXIS)[RIGHT] -
stencil->extent(X_AXIS)[LEFT];
= extent (X_AXIS).length ()

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode979
lily/beam.cc:979: height = stencil->extent(Y_AXIS)[UP] -
stencil->extent(Y_AXIS)[DOWN];
= extent (Y_AXIS).length ()

http://codereview.appspot.com/3928041/diff/30001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/3928041/diff/30001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:972: (covered-grobs ,ly:grob-array? "Note heads
that could potentially
Grobs

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

http://codereview.appspot.com/3928041/diff/30001/scm/define-grobs.scm#newcode380
scm/define-grobs.scm:380: ly:beam::move-to-avoid-collisions
fix indent (tabs)

[Graham Percival (old account), 2011-01-23 17:53:45]

I noticed another nitpick.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
File input/regression/beam-collision.ly (right):

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:1: \version "2.13.46"
2.13.47

[hanwenn, 2011-01-23 18:05:39]

Hi Carl, 

thanks for diving into this!

I love the idea of handling beam collisions, but I think the design of the
backend code is flawed.  See my comments.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode972
lily/beam.cc:972: sw[LEFT] = covered_grob->relative_coordinate (commonx, X_AXIS)
+ stencil->extent(X_AXIS)[LEFT] - x0;
why not use covered_grob->extent(common_x, X)[LEFT] instead?

the same for all other stencil->extent manipulations.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode973
lily/beam.cc:973: sw[RIGHT] = covered_grob->relative_coordinate (commony,
Y_AXIS) + stencil->extent(Y_AXIS)[DOWN] - pos[LEFT];
* what is sw ?  can you rename to something more explanatory?

* you are mixing X and Y in an interval.  Shouldnt sw be an Offset ?

* Why is this code not symmetric in UP and DOWN, using flip() ?  This looks
error prone wrt collisions that work for upstems, but not for down stems and
vice-versa

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
it looks like this only handles the 1st collision found, and exits after
circumventing the first one.  The risks are 

* in case of multiple grobs, the resolution is dependent on the order of
constructing the covered grobs list.  Unpredictable and arbitrary.

* that you will park the beam on top of something else, like the next grob you
are skipping with this return.

* in the last case, you may even create a beam with enormous stems (ugly) that
still has a collision.

Why not calculate all the locations of all covered grobs, and work collisions
into the scores for beam positions?

See beam-quanting.cc; you'd have to add another scoring pass to
Beam::quanting().

[hanwenn, 2011-01-23 18:06:21]

Sorry, I got the author incorrect.  

Thanks for looking into this, Mike!

[hanwenn, 2011-01-23 18:11:08]

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1014
lily/beam.cc:1014: pos[RIGHT] += offset;
also, these offsets disregard carefully computed beam quantizations. Even though
these are floats, the positions are not continuously variable.

[hanwenn, 2011-01-23 18:12:57]

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
> it looks like this only handles the 1st collision found, and exits after
> circumventing the first one.  The risks are 
> 
> * in case of multiple grobs, the resolution is dependent on the order of
> constructing the covered grobs list.  Unpredictable and arbitrary.
> 
> * that you will park the beam on top of something else, like the next grob you
> are skipping with this return.
> 
> * in the last case, you may even create a beam with enormous stems (ugly) that
> still has a collision.
> 
> Why not calculate all the locations of all covered grobs, and work collisions
> into the scores for beam positions?
> 
> See beam-quanting.cc; you'd have to add another scoring pass to
> Beam::quanting().

doing it with beam quant scoring instead will free you of getting the symmetry
wrt up/down correct (which has been tricky in my experience).

[MikeSol, 2011-01-24 02:58:41]

I made a lot of changes to get things looking more lilypond-y.

I also now have it better handling scenarios where the subdivisions are wacky. 
Check out:

\relative c' { << { b32 [ c,16 c'8 d,64 e'8. c,32 e'' ] } \\ { f8 [ c32 d'32
bes,,64 c'64 c64 c'128 ] } >> }

The low D is snug as a bug in a rug, as is the high C.

Cheers,
MS

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
File input/regression/beam-collision.ly (right):

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:1: \version "2.13.46"
On 2011/01/23 17:53:45, Graham Percival wrote:
> 2.13.47

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:2: \header{
On 2011/01/23 17:46:58, Neil Puttock wrote:
> \header {

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:3: texidoc="@cindex Beam Collisions
On 2011/01/23 17:46:58, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:12: \time 4/4
On 2011/01/23 17:46:58, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/3928041/diff/30001/input/regression/beam-collis...
input/regression/beam-collision.ly:13: << { s128 e [ s cis, ] } \\ { b'' [ s b ]
} >> r8..
On 2011/01/23 17:46:58, Neil Puttock wrote:
> e[
> 
> etc.

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:24: #include "item.hh"
On 2011/01/23 17:46:58, Neil Puttock wrote:
> resort alphabetically

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:45: void process_acknowledged ();
On 2011/01/23 17:46:58, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:52:
Beam_collision_engraver::stop_translation_timestep()
On 2011/01/23 17:46:58, Neil Puttock wrote:
> timestep ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:54: for (vsize i=0; i <
covered_interior_grobs_.size (); i++)
On 2011/01/23 17:46:58, Neil Puttock wrote:
> i = 0 (etc.)
> 
> Why use a separate vector?  There's no difference in processing, so it would
> make more sense to add all the grobs to covered_grobs_.

Key signatures, time signatures, and clefs that come in at the same timestep as
the beam's beginning will not come under the beam.  Thus, they need to be
treated differently than noteheads, which could intersect with a beam that
begins during their timestep.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:70: in auto beaming, end beams are signaled with
their beams at a later timestep.
On 2011/01/23 17:46:58, Neil Puttock wrote:
> acknowledge manual beams only instead, then there's no need for this hack

OK - but as far as I know, there is no way to have DECLARE_ACKNOWLEDGER
(manual_beam) .  What would be the appropriate way to do this?

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:96: active_beams_.erase (active_beams_.begin() +
j);
On 2011/01/23 17:46:58, Neil Puttock wrote:
> begin ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:103:
Beam_collision_engraver::Beam_collision_engraver() {}
On 2011/01/23 17:46:58, Neil Puttock wrote:
> engraver ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:106:
Beam_collision_engraver::process_acknowledged() {}
On 2011/01/23 17:46:58, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam-collision-engraver...
lily/beam-collision-engraver.cc:115:
Beam_collision_engraver::acknowledge_accidental (Grob_info i)
On 2011/01/23 17:46:58, Neil Puttock wrote:
> Since you're acknowledging noteheads, they already cache accidentals, so you
> could extract them later.

True - I was imagining the scenario where an accidental somehow shows up w/o a
notehead.  If this is inconceivable, I can change the code.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode972
lily/beam.cc:972: sw[LEFT] = covered_grob->relative_coordinate (commonx, X_AXIS)
+ stencil->extent(X_AXIS)[LEFT] - x0;
On 2011/01/23 18:05:39, hanwenn wrote:
> why not use covered_grob->extent(common_x, X)[LEFT] instead?
> 
> the same for all other stencil->extent manipulations.

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode973
lily/beam.cc:973: sw[RIGHT] = covered_grob->relative_coordinate (commony,
Y_AXIS) + stencil->extent(Y_AXIS)[DOWN] - pos[LEFT];
On 2011/01/23 18:05:39, hanwenn wrote:
> * what is sw ?  can you rename to something more explanatory?
> 
> * you are mixing X and Y in an interval.  Shouldnt sw be an Offset ?
> 
> * Why is this code not symmetric in UP and DOWN, using flip() ?  This looks
> error prone wrt collisions that work for upstems, but not for down stems and
> vice-versa

Fixed namings to better reflect what's actually going on.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode974
lily/beam.cc:974: width = stencil->extent(X_AXIS)[RIGHT] -
stencil->extent(X_AXIS)[LEFT];
On 2011/01/23 17:46:58, Neil Puttock wrote:
> = extent (X_AXIS).length ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode979
lily/beam.cc:979: height = stencil->extent(Y_AXIS)[UP] -
stencil->extent(Y_AXIS)[DOWN];
On 2011/01/23 17:46:58, Neil Puttock wrote:
> = extent (Y_AXIS).length ()

Done.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1014
lily/beam.cc:1014: pos[RIGHT] += offset;
On 2011/01/23 18:11:08, hanwenn wrote:
> also, these offsets disregard carefully computed beam quantizations. Even
though
> these are floats, the positions are not continuously variable. 

I don't quite get what you mean - the offsets tack on extra space iff there is a
collision.  It keeps all of the quanting data.

I hope that my clearer code will assuage your fears that this affects anything
other than certain limited scenarios - in the regtests, it eliminates collisions
in certain regtests that have them and leaves everything else alone.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
> it looks like this only handles the 1st collision found, and exits after
> circumventing the first one.  The risks are 
> 
> * in case of multiple grobs, the resolution is dependent on the order of
> constructing the covered grobs list.  Unpredictable and arbitrary.
> 
> * that you will park the beam on top of something else, like the next grob you
> are skipping with this return.
> 
> * in the last case, you may even create a beam with enormous stems (ugly) that
> still has a collision.
> 
> Why not calculate all the locations of all covered grobs, and work collisions
> into the scores for beam positions?
> 
> See beam-quanting.cc; you'd have to add another scoring pass to
> Beam::quanting().

It does not disregard other collisions (check out the regtest).

I don't know beam quanting well enough - someone who knows how to do this better
than I could integrate it in there.  For now, I believe that this method works
well.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:05:39, hanwenn wrote:
> it looks like this only handles the 1st collision found, and exits after
> circumventing the first one.  The risks are 
> 
> * in case of multiple grobs, the resolution is dependent on the order of
> constructing the covered grobs list.  Unpredictable and arbitrary.
> 
> * that you will park the beam on top of something else, like the next grob you
> are skipping with this return.
> 
> * in the last case, you may even create a beam with enormous stems (ugly) that
> still has a collision.
> 
> Why not calculate all the locations of all covered grobs, and work collisions
> into the scores for beam positions?
> 
> See beam-quanting.cc; you'd have to add another scoring pass to
> Beam::quanting().

This is not the case.

Try:

\relative c' {
  \time 2/4
  << { \times 4/5 { s32 e[ s g s g, s e'' s b'] } } \\
     { \times 4/5 { c,,32[ s c'' s g, s b s d'' s] } } >>
}

and you'll see.

http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
lily/beam.cc:1015: return ly_interval2scm (pos);
On 2011/01/23 18:12:57, hanwenn wrote:
> On 2011/01/23 18:05:39, hanwenn wrote:
> > it looks like this only handles the 1st collision found, and exits after
> > circumventing the first one.  The risks are 
> > 
> > * in case of multiple grobs, the resolution is dependent on the order of
> > constructing the covered grobs list.  Unpredictable and arbitrary.
> > 
> > * that you will park the beam on top of something else, like the next grob
you
> > are skipping with this return.
> > 
> > * in the last case, you may even create a beam with enormous stems (ugly)
that
> > still has a collision.
> > 
> > Why not calculate all the locations of all covered grobs, and work
collisions
> > into the scores for beam positions?
> > 
> > See beam-quanting.cc; you'd have to add another scoring pass to
> > Beam::quanting().
> 
> doing it with beam quant scoring instead will free you of getting the symmetry
> wrt up/down correct (which has been tricky in my experience).

I do not believe the symmetry to be an issue.

\relative c' {
  \time 2/4
  << { \times 4/5 { s32 e[ s g s g, s e, s e'''] } } \\
     { \times 4/5 { c''32[ s c'' s g, s b s d'' s] } } >>
}

(that is crazy...yes...but I think it yields a great result w/ the proposed
patch!)

http://codereview.appspot.com/3928041/diff/30001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/3928041/diff/30001/scm/define-grob-properties.s...
scm/define-grob-properties.scm:972: (covered-grobs ,ly:grob-array? "Note heads
that could potentially
On 2011/01/23 17:46:58, Neil Puttock wrote:
> Grobs

Done.

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

http://codereview.appspot.com/3928041/diff/30001/scm/define-grobs.scm#newcode380
scm/define-grobs.scm:380: ly:beam::move-to-avoid-collisions
On 2011/01/23 17:46:58, Neil Puttock wrote:
> fix indent (tabs)

Done.

[Carl, 2011-01-24 03:47:45]

New patch set uploaded.

[hanwenn, 2011-01-24 04:46:15]

On Mon, Jan 24, 2011 at 12:58 AM,  <mtsolo@gmail.com> wrote:

>> acknowledge manual beams only instead, then there's no need for this
>
> hack
>
> OK - but as far as I know, there is no way to have DECLARE_ACKNOWLEDGER
> (manual_beam) .  What would be the appropriate way to do this?

Look at the cause of the beam grob. If it is a BeamEvent, it is manual.



> http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1014
> lily/beam.cc:1014: pos[RIGHT] += offset;
> On 2011/01/23 18:11:08, hanwenn wrote:
>>
>> also, these offsets disregard carefully computed beam quantizations.
>
> Even though
>>
>> these are floats, the positions are not continuously variable.
>
> I don't quite get what you mean - the offsets tack on extra space iff
> there is a collision.  It keeps all of the quanting data.

I am talking about the case that there is a collision.  Look at Beam::quanting,

  Real straddle = 0.0;
  Real sit = (beam_thickness - slt) / 2;
  Real inter = 0.5;
  Real hang = 1.0 - (beam_thickness - slt) / 2;
  Real quants [] = {straddle, sit, inter, hang };

as you can see, for each end of the beam, there are 4 allowed
positions per staff space (with the inter being forbidden if the end
of the beam is inside the staff, iirc).

In the case of a collision, your code adds an amount derived from

  covered_grob->extent(Y)

which is an arbitrary amount.  Such offsets could cause horizontal
beams to appear like

  ========
  ========  beam
  ========
                   tiny sliver of whitespace that should not be there.
  ======== staffline

for sloped beams, unquanted positions are not desirable either.

> I hope that my clearer code will assuage your fears that this affects
> anything other than certain limited scenarios - in the regtests, it

that is not so much my fear.  I am concerned with putting in code that
we know is flawed in advance, when we have a much better mechanism to
deal with this (see below).


> eliminates collisions in certain regtests that have them and leaves
> everything else alone.
>
> http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
> lily/beam.cc:1015: return ly_interval2scm (pos);
> On 2011/01/23 18:05:39, hanwenn wrote:
>>
>> it looks like this only handles the 1st collision found, and exits
>
> after
>>
>> circumventing the first one.  The risks are
>
>> * in case of multiple grobs, the resolution is dependent on the order
>
> of
>>
>> constructing the covered grobs list.  Unpredictable and arbitrary.
>
>> * that you will park the beam on top of something else, like the next
>
> grob you
>>
>> are skipping with this return.
>
>> * in the last case, you may even create a beam with enormous stems
>
> (ugly) that
>>
>> still has a collision.
>
>> Why not calculate all the locations of all covered grobs, and work
>
> collisions
>>
>> into the scores for beam positions?
>
>> See beam-quanting.cc; you'd have to add another scoring pass to
>> Beam::quanting().
>
> It does not disregard other collisions (check out the regtest).

Let me try to cook up some examples.

> I don't know beam quanting well enough - someone who knows how to do
> this better than I could integrate it in there.  For now, I believe that
> this method works well.

The quanting code is not that difficult to understand; it is actually
a lot easier to follow than most of the collision code, as it is
declarative: the code gives scores to configurations, and then there
is a separate step that picks the one with the minimum badness score.

You already have written most of the logic to implement the scoring version.

It requires a function that takes a (left-y, right-y) pair and
determines if the beam produced collides with a grobs. There is no
need to calculate how much to shift anything, and the scoring will
have the possibility to tilt the beam to avoid collisions, or to
shorten stems relative to the default; something that you are
foregoing by just shifting in the beam-direction.

> http://codereview.appspot.com/3928041/diff/30001/lily/beam.cc#newcode1015
> lily/beam.cc:1015: return ly_interval2scm (pos);
> On 2011/01/23 18:05:39, hanwenn wrote:
>>
>> it looks like this only handles the 1st collision found, and exits
>
> after
>>
>> circumventing the first one.  The risks are
>


I am sorry, I misread this code here.  You are completely right that
it handles multiple collisions.


> symmetry
>>
>> wrt up/down correct (which has been tricky in my experience).
>
> I do not believe the symmetry to be an issue.

Can you try make use of symmetry anyway, to keep our code base clean and tight?

+          if (dir == UP)
+            offset = max (offset, temp_offset);
+          else
+            offset = min (offset, temp_offset);

you could write

  minmax(dir, offset, temp_offset)

similarly,

+              if (dir == UP)
+                {
+                  if (y_val > beam_y1)
..
+              else
+                {
+                  if (y_val < beam_y1)


could be

  if (dir * (y_val - beam_y1) > 0)


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[hanwenn, 2011-01-24 05:05:45]

On Mon, Jan 24, 2011 at 2:46 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:

>> I hope that my clearer code will assuage your fears that this affects
>> anything other than certain limited scenarios - in the regtests, it
>
> that is not so much my fear.  I am concerned with putting in code that
> we know is flawed in advance, when we have a much better mechanism to
> deal with this (see below).

Actually, there are some serious show-stoppers in this approach. See
the attached file. It appears your code kicks into action even if
there are no collisions.  We should probably double check if the
regression test detects beam changes accurately.  If not, we have a
serious hole in our coverage.

(I was going to look for more subtle failure cases, but this error
prevents me from demonstrating them.)



\new Staff \relative c' {
<<
     \new Voice
     { \stemUp c8[ c c c] c8[ c c c]
       c8[ c c c]
     }
  \new Voice {
    \voiceThree s2
    \stemUp e'8[ e e e]
    \set fontSize = #-4 c8[ c c c]

  }
>>
}

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[Keith, 2011-01-24 07:03:54]

Mike, I tried Chopin preludes Opus 28 No. 1 (where I thought there might be
trouble because beams need to cross stems there) and No. 2 (the classic example
that benefits from this fix).
These, and the large score + parts I use for informal checking, look good.
Hope you can keep weaving it in to the fabric of Lilypond, to cooperate with the
existing code, with help as needed.

[mikesol_ufl.edu, 2011-01-24 11:42:17]

Fixed, although I have no idea what the "desired output" is in this case (I have
a feeling it's "Hey...you asked for it...").

Cheers,
MS

[hanwenn, 2011-01-24 12:46:55]

On Mon, Jan 24, 2011 at 9:42 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> Fixed, although I have no idea what the "desired output" is in this case (I
have a feeling it's "Hey...you asked for it...").

IMO, in the last case, the stems should be shortened so there is no
collision; why is there a collision now?

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mikesol_ufl.edu, 2011-01-24 12:49:52]

I had just reverted it to the old behavior (I think...).

The question is: when we have a collision like the third example, how much do we
shorten the stems before it becomes ridiculous?  We could shorten the stems to
avoid the collision there, but if the note were a perfect 5th lower, it'd lead
to a very squashed result.

I'm just not sure how low is too low before you have to give up and print a
collision.

Cheers,
MS

On Jan 24, 2011, at 7:46 AM, Han-Wen Nienhuys wrote:

> On Mon, Jan 24, 2011 at 9:42 AM, Mike Solomon <mikesol@ufl.edu> wrote:
>> Fixed, although I have no idea what the "desired output" is in this case (I
have a feeling it's "Hey...you asked for it...").
> 
> IMO, in the last case, the stems should be shortened so there is no
> collision; why is there a collision now?
> 
> -- 
> Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[hanwenn, 2011-01-24 13:22:56]

On Mon, Jan 24, 2011 at 10:49 AM, Mike Solomon <mikesol@ufl.edu> wrote:
> I had just reverted it to the old behavior (I think...).
>
> The question is: when we have a collision like the third example, how much do
we shorten the stems before it becomes ridiculous?  We could shorten the stems
to avoid the collision there, but if the note were a perfect 5th lower, it'd
lead to a very squashed result.

If the note were a 5th lower, there would not be a collision to start with.

> I'm just not sure how low is too low before you have to give up and print a
collision.

That is the advantage of scoring: you can give points for how bad the
shortened stem is vs. how bad the colllision is, and let the scoring
figure it out.

The larger context of my objections to your patch is that we used to
have (pre 1.6.0) beam formatting routines like yours: a sequence of
routines that each tried to modify #'positions on its own to fix one
thing (slopes, quantizations, concavity, etc.).   The result was a
mess as all these routines interacted in unpredictable ways, and
although it would work for a large number of cases, there were always
new errors that required more complicated conditional fixups.  We
never got it working 100%, so we threw everything out and moved to
score based formatting.  If you dig in the archive, you can see it
happening around 1.5.42.  In particular, you can see old code being
thrown out in commit 46c5beb169abbc5a03bd57562cda1669640e4c72

Let me whip up a prototype for you to see how it works.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[hanwenn, 2011-01-24 14:28:06]

On Mon, Jan 24, 2011 at 11:22 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> Let me whip up a prototype for you to see how it works.

See attached (it applies on top of issue3928041_55001.diff)

It is imperfect in several ways,

- see TODOs in the code.
- 4th beat 1st line: should go below
- 5th beat 1st line: should be closer to collision
- 4th beat 2nd line: any position will better

it needs some tuning in how all the penalties work together (try the
#'debug-scoring property, together with #'inspect-quants),
but I hope you will agree the code is cleaner and handles more cases.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[Neil Puttock, 2011-01-24 23:32:18]

On 24 January 2011 02:58,  <mtsolo@gmail.com> wrote:

> Key signatures, time signatures, and clefs that come in at the same
> timestep as the beam's beginning will not come under the beam.  Thus,
> they need to be treated differently than noteheads, which could
> intersect with a beam that begins during their timestep.

Hmm, I see what you mean.  In that case, it's probably a good idea to
follow the approach used by the Slur_engraver: instead of ack'ing all
the relevant break-aligned grobs, wait until stop_translation_timestep
() and add the currentCommandColumn if it exists.  Then you can
collect the extra covered items later from the NonMusicalPaperColumn's
'elements array (like Slur::replace_breakable_encompass_objects ()).

Cheers,
Neil

[Neil Puttock, 2011-01-24 23:40:05]

On 24 January 2011 14:28, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:

> See attached (it applies on top of issue3928041_55001.diff)

+      common[a] = common_refpoint_of_array (covered_grobs, me, Axis (a));

I suspect this line needs changing, otherwise it junks the calculated
refpoint for the stems (which means we no longer get the correct
refpoint for cross-staff beams).

+      common[a] = common_refpoint_of_array (covered_grobs, common[a],
Axis (a));

Cheers,
Neil

[hanwenn, 2011-01-25 00:59:39]

On Mon, Jan 24, 2011 at 9:40 PM, Neil Puttock <n.puttock@gmail.com> wrote:
> +      common[a] = common_refpoint_of_array (covered_grobs, me, Axis (a));
>
> I suspect this line needs changing, otherwise it junks the calculated
> refpoint for the stems (which means we no longer get the correct
> refpoint for cross-staff beams).
>
> +      common[a] = common_refpoint_of_array (covered_grobs, common[a],
> Axis (a));

Yes, you are correct.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen