Sketchy sketch for a modification to the typesetting backend.

Sketchy sketch for a modification to the typesetting backend.

[MikeSol, 2011-04-25 03:01:44]

This patch fixes issue 1632.

Cheers,
MS

[hanwenn, 2011-04-25 03:20:51]

LGTM

http://codereview.appspot.com/4446060/diff/3/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4446060/diff/3/lily/beam.cc#newcode1208
lily/beam.cc:1208: if ((Beam::is_cross_staff (me)
comment why we are doing this.

http://codereview.appspot.com/4446060/diff/3/lily/beam.cc#newcode1214
lily/beam.cc:1214: if (Beam::is_cross_staff (covered[i]))
use

 if (is beam && is cross staff)

[MikeSol, 2011-04-25 09:56:35]

http://codereview.appspot.com/4446060/diff/3/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4446060/diff/3/lily/beam.cc#newcode1208
lily/beam.cc:1208: if ((Beam::is_cross_staff (me)
On 2011/04/25 03:20:51, hanwenn wrote:
> comment why we are doing this.

If we don't do this, this continues to be a problem:


<<
  \new Staff = up \relative c'' {
\time 2/4
\autoBeamOff
    b8 b b b
  }
  \new Staff = down \relative c'' {
    b b b b
    \change Staff = up
  }
>>

as does this


<<
  \new Staff = up \relative c'' {
\time 2/4
\autoBeamOff
    b8 b b b
  }
  \new Staff = down \relative c'' {
    b b b b
    \change Staff = up
  }
>>

as does this


<<
  \new Staff = up \relative c'' {
\time 2/4
    b8 b b b
  }
  \new Staff = down \relative c'' {
    b b b b
    \change Staff = up
  }
>>

Granted, I could keep this in the engraver as it previously was, but I was under
the impression you wanted this logic in beam.cc.

[Graham Percival (old account), 2011-04-25 11:30:56]

regtests look fine.

[hanwenn, 2011-04-25 12:05:02]

On Mon, Apr 25, 2011 at 6:56 AM,  <mtsolo@gmail.com> wrote:
>
> http://codereview.appspot.com/4446060/diff/3/lily/beam.cc
> File lily/beam.cc (right):
>
> http://codereview.appspot.com/4446060/diff/3/lily/beam.cc#newcode1208
> lily/beam.cc:1208: if ((Beam::is_cross_staff (me)
> On 2011/04/25 03:20:51, hanwenn wrote:
>>
>> comment why we are doing this.
>
> If we don't do this, this continues to be a problem:

I mean: add a short comment to the code.


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

[MikeSol, 2011-04-25 12:45:26]

I realized that it was way too restrictive to have cross-staff beams avoid stems
and beams - Reinhold had sent out a Beethoven example a while back that would
need cross staff beams to take other beams into consideration.

The real problem is that certain cross staff beams are not in fact cross staff
beams.  This patch weeds them out of beam collision work via a new function
Beam::is_fake_cross_staff.  Ultimately, this sorta logic can disappear once the
beam collision engraver moves up to the score level, which seems like something
that'll cause several bugs in the unstable version (if history is any
indication) and thus is something I'm holding off on until this current crop of
bugs dies down.

[hanwenn, 2011-04-25 19:57:47]

On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
> I realized that it was way too restrictive to have cross-staff beams
> avoid stems and beams - Reinhold had sent out a Beethoven example a
> while back that would need cross staff beams to take other beams into
> consideration.

You're not really into the lilypond spirit of things yet. We don't
strive to be perfect; the objective is just to be better than the last
release. It's ok to not handle some cases if it's too hairy.

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

[mike_apollinemike.com, 2011-04-25 20:13:07]

On Apr 25, 2011, at 3:57 PM, Han-Wen Nienhuys wrote:

> On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
>> I realized that it was way too restrictive to have cross-staff beams
>> avoid stems and beams - Reinhold had sent out a Beethoven example a
>> while back that would need cross staff beams to take other beams into
>> consideration.
> 
> You're not really into the lilypond spirit of things yet. We don't
> strive to be perfect; the objective is just to be better than the last
> release. It's ok to not handle some cases if it's too hairy.
> 

Is that a LGTM in disguise? :)

Cheers,
MS

[hanwenn, 2011-04-25 21:37:17]

On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
> I realized that it was way too restrictive to have cross-staff beams
> avoid stems and beams - Reinhold had sent out a Beethoven example a
> while back that would need cross staff beams to take other beams into
> consideration.
>
> The real problem is that certain cross staff beams are not in fact cross
> staff beams.  This patch weeds them out of beam collision work via a new
> function Beam::is_fake_cross_staff.  Ultimately, this sorta logic can
> disappear once the beam collision engraver moves up to the score level,
> which seems like something that'll cause several bugs in the unstable
> version (if history is any indication) and thus is something I'm holding
> off on until this current crop of bugs dies down.

Can you try with looking for the common Y refpoint of stems and beam,
and seeing if that is an Y-aligment instead?  This code looks a bit
kludgey.

alternatively, put all staff_syms into a set<Grob*> and check if the length > 1.

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

[mike_apollinemike.com, 2011-04-25 21:43:20]

On Apr 25, 2011, at 5:37 PM, Han-Wen Nienhuys wrote:

> On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
>> I realized that it was way too restrictive to have cross-staff beams
>> avoid stems and beams - Reinhold had sent out a Beethoven example a
>> while back that would need cross staff beams to take other beams into
>> consideration.
>> 
>> The real problem is that certain cross staff beams are not in fact cross
>> staff beams.  This patch weeds them out of beam collision work via a new
>> function Beam::is_fake_cross_staff.  Ultimately, this sorta logic can
>> disappear once the beam collision engraver moves up to the score level,
>> which seems like something that'll cause several bugs in the unstable
>> version (if history is any indication) and thus is something I'm holding
>> off on until this current crop of bugs dies down.
> 
> Can you try with looking for the common Y refpoint of stems and beam,
> and seeing if that is an Y-aligment instead?  This code looks a bit
> kludgey.
> 

What is a Y-alignment?  I tried

git grep y-alignment
git grep Y-alignment
git grep Yalignment
git grep yalignment
git grep y_alignment

but got meager results.

Cheers,
MS

> alternatively, put all staff_syms into a set<Grob*> and check if the length >
1.
> 
> -- 
> Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[hanwenn, 2011-04-25 22:10:51]

On Mon, Apr 25, 2011 at 6:43 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>
> On Apr 25, 2011, at 5:37 PM, Han-Wen Nienhuys wrote:
>
>> On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
>>> I realized that it was way too restrictive to have cross-staff beams
>>> avoid stems and beams - Reinhold had sent out a Beethoven example a
>>> while back that would need cross staff beams to take other beams into
>>> consideration.
>>>
>>> The real problem is that certain cross staff beams are not in fact cross
>>> staff beams.  This patch weeds them out of beam collision work via a new
>>> function Beam::is_fake_cross_staff.  Ultimately, this sorta logic can
>>> disappear once the beam collision engraver moves up to the score level,
>>> which seems like something that'll cause several bugs in the unstable
>>> version (if history is any indication) and thus is something I'm holding
>>> off on until this current crop of bugs dies down.
>>
>> Can you try with looking for the common Y refpoint of stems and beam,
>> and seeing if that is an Y-aligment instead?  This code looks a bit
>> kludgey.
>>
>
> What is a Y-alignment?  I tried
>
> git grep y-alignment
> git grep Y-alignment
> git grep Yalignment
> git grep yalignment
> git grep y_alignment
>
> but got meager results.


Look for Align_interface. IIRC, the type has an get_axis() method (or similar).

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

[mike_apollinemike.com, 2011-04-25 22:37:54]

On Apr 25, 2011, at 5:37 PM, Han-Wen Nienhuys wrote:

> On Mon, Apr 25, 2011 at 9:45 AM,  <mtsolo@gmail.com> wrote:
>> I realized that it was way too restrictive to have cross-staff beams
>> avoid stems and beams - Reinhold had sent out a Beethoven example a
>> while back that would need cross staff beams to take other beams into
>> consideration.
>> 
>> The real problem is that certain cross staff beams are not in fact cross
>> staff beams.  This patch weeds them out of beam collision work via a new
>> function Beam::is_fake_cross_staff.  Ultimately, this sorta logic can
>> disappear once the beam collision engraver moves up to the score level,
>> which seems like something that'll cause several bugs in the unstable
>> version (if history is any indication) and thus is something I'm holding
>> off on until this current crop of bugs dies down.
> 
> Can you try with looking for the common Y refpoint of stems and beam,
> and seeing if that is an Y-aligment instead?  This code looks a bit
> kludgey.
> 
> alternatively, put all staff_syms into a set<Grob*> and check if the length >
1.

I did the former.  New patch set up.

Cheers,
MS

[hanwenn, 2011-04-26 02:09:59]

LGTM

http://codereview.appspot.com/4446060/diff/11001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/4446060/diff/11001/lily/beam.cc#newcode1209
lily/beam.cc:1209: if (is_cross_staff (me) && Align_interface::has_interface
(common_refpoint_of_array (stems, me, Y_AXIS)))
can you add a TODO here to use the Align trick for is_cross_staff ?

[mike_apollinemike.com, 2011-04-26 11:15:50]

On Apr 25, 2011, at 10:09 PM, hanwenn@gmail.com wrote:

> LGTM
> 
> 
> http://codereview.appspot.com/4446060/diff/11001/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/4446060/diff/11001/lily/beam.cc#newcode1209
> lily/beam.cc:1209: if (is_cross_staff (me) &&
> Align_interface::has_interface (common_refpoint_of_array (stems, me,
> Y_AXIS)))
> can you add a TODO here to use the Align trick for is_cross_staff ?
> 

Done, pushed as f9116bac398f94d95e237aa3b1fc222a59ad8e3c

Cheers,
MS