Better staggering of accidental placements.

Better staggering of accidental placements.

Makes sure that, on average, accidentals are closer to the notes
while preserving octaves lining up.

[Keith, 2013-01-13 21:28:14]

Obfuscated, but so was the old code.

I'll leave here the notes I used to decipher it, and maybe come back later to
clean up the code.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:123: Real ape_priority (Accidental_placement_entry
const *a)
Evil name.  The return values are not used as a priority.  Formerly,
"ape_highest_extent()" would have been appropriate, because it was not even then
a priority.  Rather, the highest ape was placed first, then the lowest, then the
second highest...

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:129: bool ape_less (Accidental_placement_entry
*const &a,
Bad name.  The apes were formerly sorted by "priority" (really height) so this
should have been ape_lower().  Now, they are sorted by number of accidentals in
the ape, then by "priority" (really height) to break ties.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:186: vector<Accidental_placement_entry *> asc =
*apes;
accidental sorted-columns (asc)

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:189: // we do the staggering below based on size
'size' must mean number of accidentals in the ape, but you don't stagger based
on 'size', you prioritize based on 'size', then within each groups of apes with
the same 'size', you stagger the based on their height.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:190: // this ensures that if a placement has 4
entries, it will
An accidental-placement-entry (ape) contains the accidentals that get placed
together (same pitch names in different octaves) so it would rarely have 4
entries -- more likely just 2 accidentals.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:191: // always be closer to the NoteColumn than a
placement with 1
Not always closer, merely placed before.  In one of the regression tests, a
single accidental had room to slide closer to a couple previously-placed
octave-pairs of accidentals.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:193: // while still preserving octave alignment
Nothing in here deals with octaves.  The grouping seems to be done in scheme, so
here we can only promise to keep the apes together.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:194: vector<vector<Accidental_placement_entry *> >
ascs;
accidental sorted-column sets (ascs)

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:196: vsize sz = INT_MAX;
Number of accidentals in a group to be placed as a column (sz)

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:197: for (vsize i = 0; i < asc.size (); i++)
The "accidental sorted-columns" (asc) is sorted in an order not directly related
to the order of placement.  We need to break it up in to subsets (ascs) that
contain columns with similar numbers of accidentals in them.  Each sub-set is
'staggered' but the subset containing groups-of-2 accidentals placed before the
subset with single accidentals.

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:210: int parity = 1;
Not really "parity", but which end, highest-or-lowest, of the sub-lists of
accidentals we pick from.

Apparently, when we finish with the accidentals in octaves, we want to pick the
highest of the remaining accidentals to place next.

[benko.pal, 2013-01-13 21:48:20]

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:210: int parity = 1;
could you use bool (or in an improved version more values will be used)?

[Keith, 2013-01-14 20:30:47]

LilyPond is too complicated.

The manual explains engravers and contexts, and you you can
\layout { 
 \context { \Staff \remove "Accidental_engraver" }
 \context { \Voice \consists "Accidental_engraver" }}

but then there are all these properties that mess with that structure, like
 horizontallyAlignOctaveAccidentalsAcrossVoices
 midiChannelMapping

I give up.  I can buy Finale and just drag all the accidentals with the mouse
before I memorize enough to use LilyPond.

[mike7, 2013-01-14 20:40:47]

On 14 janv. 2013, at 21:30, k-ohara5a5a@oco.net wrote:

> LilyPond is too complicated.
> 
> The manual explains engravers and contexts, and you you can
> \layout {
> \context { \Staff \remove "Accidental_engraver" }
> \context { \Voice \consists "Accidental_engraver" }}
> 

But does the manual say that this does not allow you to switch
back and forth whereas a context property does?

I generally suck at documentation, but I wouldn't mind changing
it to reflect the changes in this patch.  Where does this show up?

> but then there are all these properties that mess with that structure,
> like
> horizontallyAlignOctaveAccidentalsAcrossVoices
> midiChannelMapping
> 
> I give up.  I can buy Finale and just drag all the accidentals with the
> mouse before I memorize enough to use LilyPond.
> 
> https://codereview.appspot.com/7101045/

Holy sh*t!!  You can do that in Finale?  Why the f*%& have I been
using LilyPond all this time??!?  I'm outta here!

[lemzwerg, 2013-01-15 01:52:42]

https://codereview.appspot.com/7101045/diff/5001/scm/define-context-propertie...
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/7101045/diff/5001/scm/define-context-propertie...
scm/define-context-properties.scm:310:
(horizontallyStaggerDifferentVoiceOctaveAccidentals ,boolean?
This name is a joke, isn't it?  It has a length of bloody 50 characters!  By all
means, please replace it with some *much* shorter, say,

  \set Staff . accidentalGrouping = #'voice

and

  \set Staff . accidentalGrouping = #'staff