Fixes accidental suggestions in the beam collision engraver

Fixes accidental suggestions in the beam collision engraver

[MikeSol, 2011-03-20 11:41:53]

Hey all,

A bug just hit the French list.  It seems like a critical regression.

\score {             %avec "surcharge" des ligatures double croches allongées
   \new Staff {
 \time 2/2
 \set suggestAccidentals = ##t
g'4 fis'8 [  g'8 ]  a'8 [  g'8 a'16 g'16  fis'!16 e'16 ]   |
d'1
    }

}
\score {         %sans "surcharge" des ligatures : bon
       \new Staff {
 \time 2/2
 \set suggestAccidentals = ##t
g'4 fis'8   g'8   a'8   g'8 a'16 g'16  fis'!16 e'16    |
d'1
    }

}

This patch proposes a fix.

Cheers,
Mike

[hanwenn, 2011-03-21 01:59:22]

On Sun, Mar 20, 2011 at 8:41 AM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> Hey all,
>
> A bug just hit the French list.  It seems like a critical regression.
>

Hi,

There is not much else we can do: the suggestion accidental uses the
stem extent to determine Y positions, but that requires formatting the
beam, which looks at the Y position of the accidental again.

The intention of the fix is incorrect, but can you make the logic
explicit? That is, add an interface symbol for "normal" accidentals
(normal-accidental-interface, inline-accidental-interface, ... ?) and
acknowledge that explicitly?  If not, we will screw in the same way
for any other accidental-like grob, eg. TrillPitchAccidental).

Also, add this snippet to the regtest:

{
  \set suggestAccidentals = ##t
   a'8[ fis'16 g'16]
}

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

[cpkc_shaw.ca, 2011-03-21 04:15:24]

On 11-03-20 05:41 AM, mtsolo@gmail.com wrote:
> Reviewers: ,
>
> Message:
> Hey all,
>
> A bug just hit the French list.  It seems like a critical regression.
>
> \score {             %avec "surcharge" des ligatures double croches
> allongées
>    \new Staff {
>  \time 2/2
>  \set suggestAccidentals = ##t
> g'4 fis'8 [  g'8 ]  a'8 [  g'8 a'16 g'16  fis'!16 e'16 ]   |
> d'1
>     }
>
> }
> \score {         %sans "surcharge" des ligatures : bon
>        \new Staff {
>  \time 2/2
>  \set suggestAccidentals = ##t
> g'4 fis'8   g'8   a'8   g'8 a'16 g'16  fis'!16 e'16    |
> d'1
>     }
>
> }
>
> This patch proposes a fix.
>
> Cheers,
> Mike
>
> Description:
> Fixes accidental suggestions in the beam collision engraver
>
> Please review this at http://codereview.appspot.com/4271054/
>
> Affected files:
>   M lily/beam-collision-engraver.cc
>
>
> Index: lily/beam-collision-engraver.cc
> diff --git a/lily/beam-collision-engraver.cc 
> b/lily/beam-collision-engraver.cc
> index 
>
39e614c2a15dea10f3b72f88110959c35dc389a1..e0dade7b8ea3bc4d2f9eea3d4437b94102f85c80

> 100644
> --- a/lily/beam-collision-engraver.cc
> +++ b/lily/beam-collision-engraver.cc
> @@ -160,7 +160,8 @@ Beam_collision_engraver::acknowledge_note_head 
> (Grob_info i)
>  void
>  Beam_collision_engraver::acknowledge_accidental (Grob_info i)
>  {
> -  covered_grobs_.push_back (i.grob ());
> +  if (!i.grob ()->internal_has_interface (ly_symbol2scm 
> ("accidental-suggestion-interface")))
> +    covered_grobs_.push_back (i.grob ());
>  }
>
>  void
>
>
>

Added as issue 1570, Mike.

Colin Campbell
Bug Squad

-- 
The test of our progress is not whether we add more to the abundance
of those who have much, it is whether we provide enough for those who
have too little.
-Franklin D. Roosevelt, 32nd US President (1882-1945)

[hanwenn, 2011-03-21 04:30:53]

On Sun, Mar 20, 2011 at 10:59 PM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> On Sun, Mar 20, 2011 at 8:41 AM,  <mtsolo@gmail.com> wrote:
>> Reviewers: ,
>>
>> Message:
>> Hey all,
>>
>> A bug just hit the French list.  It seems like a critical regression.
>>
>
> Hi,
>
> There is not much else we can do: the suggestion accidental uses the
> stem extent to determine Y positions, but that requires formatting the
> beam, which looks at the Y position of the accidental again.
>
> The intention of the fix is incorrect, but can you make the logic

I mean: it's correct.
(d'oh)

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

[MikeSol, 2011-03-21 10:37:39]

Patch updated with Han Wen's suggestions.

[hanwenn, 2011-03-21 12:50:17]

LGTM

[Neil Puttock, 2011-03-22 22:46:01]

On 21 March 2011 01:59, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:

> The intention of the fix is incorrect, but can you make the logic
> explicit? That is, add an interface symbol for "normal" accidentals
> (normal-accidental-interface, inline-accidental-interface, ... ?) and
> acknowledge that explicitly?  If not, we will screw in the same way
> for any other accidental-like grob, eg. TrillPitchAccidental).

I think this is a mistake: the Beam_collision_engraver already
acknowledges TrillPitchGroup (via note-head-interface), and pitched
trills are usually placed inside a stave, possibly between beamed
notes.  The only other object which would potentially be acknowledged
is an AmbitusAccidental, and they obviously never appear near a beam.

Ignoring TrillPitchAccidental produces poorer output for wide pitched trills:

\relative c'' {
  \pitchedTrill
  g8[\startTrillSpan bes e]\stopTrillSpan
}

Cheers,
Neil

[mike_apollinemike.com, 2011-03-22 22:48:35]

On Mar 22, 2011, at 6:46 PM, Neil Puttock wrote:

> On 21 March 2011 01:59, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> 
>> The intention of the fix is incorrect, but can you make the logic
>> explicit? That is, add an interface symbol for "normal" accidentals
>> (normal-accidental-interface, inline-accidental-interface, ... ?) and
>> acknowledge that explicitly?  If not, we will screw in the same way
>> for any other accidental-like grob, eg. TrillPitchAccidental).
> 
> I think this is a mistake: the Beam_collision_engraver already
> acknowledges TrillPitchGroup (via note-head-interface), and pitched
> trills are usually placed inside a stave, possibly between beamed
> notes.  The only other object which would potentially be acknowledged
> is an AmbitusAccidental, and they obviously never appear near a beam.
> 
> Ignoring TrillPitchAccidental produces poorer output for wide pitched trills:
> 
> \relative c'' {
>  \pitchedTrill
>  g8[\startTrillSpan bes e]\stopTrillSpan
> }
> 

Would an acceptable alternative be giving the TrillPitchAccidental the
inline-accidental-interface?

Cheers,
MS

[Neil Puttock, 2011-03-22 22:54:34]

On 22 March 2011 22:48, mike@apollinemike.com <mike@apollinemike.com> wrote:

> Would an acceptable alternative be giving the TrillPitchAccidental the
inline-accidental-interface?

Sounds good to me.

Cheers,
Neil

[hanwenn, 2011-03-23 01:51:38]

On Tue, Mar 22, 2011 at 7:54 PM, Neil Puttock <n.puttock@gmail.com> wrote:
> On 22 March 2011 22:48, mike@apollinemike.com <mike@apollinemike.com> wrote:
>
>> Would an acceptable alternative be giving the TrillPitchAccidental the
inline-accidental-interface?
>
> Sounds good to me.

Sorry - I was confused.  I was thinking that trill-pitch-interface was
applied to accidentals directly above or below the 'tr' symbol.

Nevertheless, I think it is good to be explicit about the difference
between accidental-interface (the function of an accidental) and
inline-accidental-interface (the accidental symbols that get printed
on the staff.)

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

[mike_apollinemike.com, 2011-03-23 10:52:13]

On Mar 22, 2011, at 9:51 PM, Han-Wen Nienhuys wrote:

> On Tue, Mar 22, 2011 at 7:54 PM, Neil Puttock <n.puttock@gmail.com> wrote:
>> On 22 March 2011 22:48, mike@apollinemike.com <mike@apollinemike.com> wrote:
>> 
>>> Would an acceptable alternative be giving the TrillPitchAccidental the
inline-accidental-interface?
>> 
>> Sounds good to me.
> 
> Sorry - I was confused.  I was thinking that trill-pitch-interface was
> applied to accidentals directly above or below the 'tr' symbol.
> 
> Nevertheless, I think it is good to be explicit about the difference
> between accidental-interface (the function of an accidental) and
> inline-accidental-interface (the accidental symbols that get printed
> on the staff.)

Pushed.
fe21cb68b77e99a6d0cf89dbf9313400456d1163

Cheers,
MS