Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-23 Thread m...@apollinemike.com 
On Mar 22, 2011, at 9:51 PM, Han-Wen Nienhuys wrote:

 On Tue, Mar 22, 2011 at 7:54 PM, Neil Puttock n.putt...@gmail.com wrote:
 On 22 March 2011 22:48, m...@apollinemike.com m...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-22 Thread Neil Puttock 
On 21 March 2011 01:59, Han-Wen Nienhuys hanw...@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
attachment: pitched-trill-master.pngattachment: pitched-trill-2.13.54.png___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-22 Thread m...@apollinemike.com 
On Mar 22, 2011, at 6:46 PM, Neil Puttock wrote:

 On 21 March 2011 01:59, Han-Wen Nienhuys hanw...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-22 Thread Neil Puttock 
On 22 March 2011 22:48, m...@apollinemike.com m...@apollinemike.com wrote:

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

Sounds good to me.

Cheers,
Neil

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-22 Thread Han-Wen Nienhuys 
On Tue, Mar 22, 2011 at 7:54 PM, Neil Puttock n.putt...@gmail.com wrote:
 On 22 March 2011 22:48, m...@apollinemike.com m...@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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-21 Thread mtsolo 

Patch updated with Han Wen's suggestions.

http://codereview.appspot.com/4271054/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-21 Thread hanwenn 

LGTM

http://codereview.appspot.com/4271054/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-20 Thread Han-Wen Nienhuys 
On Sun, Mar 20, 2011 at 8:41 AM,  mts...@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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-20 Thread Colin Campbell 

On 11-03-20 05:41 AM, mts...@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)

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes accidental suggestions in the beam collision engraver (issue4271054)

2011-03-20 Thread Han-Wen Nienhuys 
On Sun, Mar 20, 2011 at 10:59 PM, Han-Wen Nienhuys hanw...@gmail.com wrote:
 On Sun, Mar 20, 2011 at 8:41 AM,  mts...@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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen

___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel