Uses horizontal skylines in accidental placement

Uses horizontal skylines in accidental placement

Currently flats in 3rds and 4ths seem too snug, but everything
else seems OK.  It gets rid of a lot of m4g1c numbers in
accidental-placement.cc.

[Keith, 2012-09-06 06:23:42]

I like it, but we need to figure out what went wrong with 'accidental-tie.ly'. 
There was some trickery involving holding space open for the accidental that
might be needed on the second note of a tied pair, iff the tie is broken across
lines.

Ted Ross' textbook puts the upper flat about 0.5 staff-space right of the lower
flat in fourths and fifths.

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

http://codereview.appspot.com/6489086/diff/1/lily/accidental-placement.cc#new...
lily/accidental-placement.cc:125: // right is up because we're horizontal
Okay, so why do we care about 'up'?  Oh, the "right"-most edge of the
horizontal-skyline is the top of the accidental glyph, which location we use for
priority.

[janek, 2012-09-06 08:06:09]

Patchset 2 screws everything up, LOL!
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705606/accidental-spacing-...

In previous output (see tracker issue
http://code.google.com/p/lilypond/issues/detail?id=2811#c4) there were several
cases of too close accidentals.  I think adding horizontal padding may solve
them fine.

cheers,
Janek

[mike7, 2012-09-06 09:56:18]

On 6 sept. 2012, at 10:06, janek.lilypond@gmail.com wrote:

> Patchset 2 screws everything up, LOL!

It was one of my more colossal breakings of LilyPond - I'll add it to the top
10.

>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705606/accidental-spacing-...
> 
> In previous output (see tracker issue
> http://code.google.com/p/lilypond/issues/detail?id=2811#c4) there were
> several cases of too close accidentals.  I think adding horizontal
> padding may solve them fine.

It may be more subtle than that.
Compile stuff with the most recent patch set and then send me a diagram of how
far stuff should be spaced and I'll make it happen. I'm already taking Keith's
suggestion from Ross into account in some new work but I've only have 5 minutes
to work on it and won't have time again for a bit.

Cheers,
MS

> 
> cheers,
> Janek
> 
> http://codereview.appspot.com/6489086/
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

[janek, 2012-09-06 22:12:07]

Bah, Rietveld ate my message.

On Thu, Sep 6, 2012 at 11:55 AM, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
>
>> I think adding horizontal padding may solve them fine.
>
> It may be more subtle than that.

Quite possible.

> Compile stuff with the most recent patch set and then send me a diagram of how
far stuff should be spaced and I'll make it happen.

What about pngs with "suggested" in their names, attached to
http://code.google.com/p/lilypond/issues/detail?id=2141 ?
BTW, patchset 4 doesn't apply to current master
(3cd24002639b7b48a9f0c7245101bdb721ce21c0) - probably because of
2c00d625e88fb68342ac2fbaaffc6e8f9a182483.

cheers,
Janek

[Keith, 2012-09-07 07:11:48]

The output looks very nice, even with half-sharps, reverse flats, etc.
The patch makes compilation take 6% longer (depending on accidental density).

The reverse flats don't get any extra space around their stems, but I haven't
find a case where they need it,  because they are placed in the same order as
regular flats so they do not nest.

Maybe it would be cleaner to have a blanket rule that the skylines always
enclose the 'spine' of the glyph, assuming the spine has x-coordinate zero, by
merging a box of dimensions
  0.4* X-extent()  by  Y-extent ()
into the skylines.

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc
File lily/accidental.cc (right):

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode86
lily/accidental.cc:86: * suicide, so it is not pure.
Is this the book of Leviticus ?   The completion of the concept is "..., but we
use this function /before/ line-breaking"

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode101
lily/accidental.cc:101: if (glyph_name == "accidentals.flat"
Since this is such a special case, it would be nicer to encode the extra space
around a flat's stem as a non-printing the glyph outline --- if we can interest
anyone with sufficient MetaFont competence.

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode110
lily/accidental.cc:110: Real left = (*sky)[LEFT].max_height ();
Doesn't matter much for the current flat symbol, but in general you don't want
to merge in a box that goes this far left.

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode111
lily/accidental.cc:111: Real right = (*sky)[RIGHT].average_height () * 0.75;
A fraction of max_height() is easier to visualize than average_height.

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode113
lily/accidental.cc:113: Real up = (*sky)[RIGHT].right ();
Maybe just use extent(Y_AXIS)?

http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode116
lily/accidental.cc:116: Skyline_pair merge_with_me (boxes, Y_AXIS);
Is it feasible to just merge with the right-hand skyline of the pair?

[mike7, 2012-09-07 20:56:00]

On 7 sept. 2012, at 10:11, k-ohara5a5a@oco.net wrote:

> The output looks very nice, even with half-sharps, reverse flats, etc.
> The patch makes compilation take 6% longer (depending on accidental
> density).
> 
> The reverse flats don't get any extra space around their stems, but I
> haven't find a case where they need it,  because they are placed in the
> same order as regular flats so they do not nest.
> 
> Maybe it would be cleaner to have a blanket rule that the skylines
> always enclose the 'spine' of the glyph, assuming the spine has
> x-coordinate zero, by merging a box of dimensions
> 0.4* X-extent()  by  Y-extent ()
> into the skylines.

Good call - that'll speed things up.

> 
> 
> 
> 
> http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc
> File lily/accidental.cc (right):
> 
> http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode86
> lily/accidental.cc:86: * suicide, so it is not pure.
> Is this the book of Leviticus ?   The completion of the concept is "...,
> but we use this function /before/ line-breaking"
> 

I will see if I can make it more New-Testament sounding...I hope my old hebrew
school teachers aren't reading this thread.

>
http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode101
> lily/accidental.cc:101: if (glyph_name == "accidentals.flat"
> Since this is such a special case, it would be nicer to encode the extra
> space around a flat's stem as a non-printing the glyph outline --- if we
> can interest anyone with sufficient MetaFont competence.

Your 0.4 suggestion above makes a lot of sense - I'll just do that for now.

> 
>
http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode110
> lily/accidental.cc:110: Real left = (*sky)[LEFT].max_height ();
> Doesn't matter much for the current flat symbol, but in general you
> don't want to merge in a box that goes this far left.

Ok.

> 
>
http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode111
> lily/accidental.cc:111: Real right = (*sky)[RIGHT].average_height () *
> 0.75;
> A fraction of max_height() is easier to visualize than average_height.

Ok.

> 
>
http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode113
> lily/accidental.cc:113: Real up = (*sky)[RIGHT].right ();
> Maybe just use extent(Y_AXIS)?
> 
>
http://codereview.appspot.com/6489086/diff/12001/lily/accidental.cc#newcode116
> lily/accidental.cc:116: Skyline_pair merge_with_me (boxes, Y_AXIS);
> Is it feasible to just merge with the right-hand skyline of the pair?

Yes - will do.

> 
> http://codereview.appspot.com/6489086/

[Keith, 2012-09-08 05:28:02]

Still looks great (except when Phil's computer cancels double-flats, etc.) and
now I measure it 2% /faster/ than master.

http://codereview.appspot.com/6489086/diff/8003/lily/accidental.cc
File lily/accidental.cc (right):

http://codereview.appspot.com/6489086/diff/8003/lily/accidental.cc#newcode81
lily/accidental.cc:81: if (!to_boolean (me->get_property ("restore-first"))
Do you have a plan for an "else" section?  It looks like accidentals with a
restoring natural get an empty skyline.  They still print correctly for me,
somehow, but Phil's regtest result fails with 'accidental-single-double.ly' and
similar.

[Keith, 2012-09-08 05:46:33]

On 2012/09/08 05:28:02, Keith wrote:
> now I measure it 2% /faster/ than master.

Of course that makes no sense.  I got confused of which executable I had when
switching between patches.  This patch is still about 6% slower than master, buy
maybe worth it.

Also, 'accidental-single-double.ly' is broke for me as well with starting with
patch set 2, so nothing mysterious there.  

(It works if I just bypass the test for "restore-first".)

[mike7, 2012-09-08 15:43:57]

On 8 sept. 2012, at 08:46, k-ohara5a5a@oco.net wrote:

> On 2012/09/08 05:28:02, Keith wrote:
>> now I measure it 2% /faster/ than master.
> 
> Of course that makes no sense.  I got confused of which executable I had
> when switching between patches.  This patch is still about 6% slower
> than master, buy maybe worth it.
> 
> Also, 'accidental-single-double.ly' is broke for me as well with
> starting with patch set 2, so nothing mysterious there.
> 
> (It works if I just bypass the test for "restore-first".)
> 
> http://codereview.appspot.com/6489086/

OK - I'll be able to fix all the broken stuff on Monday or Tuesday.

~Mike

[janek, 2012-09-27 11:57:13]

On Thu, Sep 27, 2012 at 1:20 PM, Janek Warchoł <janek.lilypond@gmail.com>
wrote:
> On Sat, Sep 8, 2012 at 5:43 PM, mike@mikesolomon.org
> <mike@mikesolomon.org> wrote:
>> OK - I'll be able to fix all the broken stuff on Monday or Tuesday.
>
> Whoah, this was pushed and i didn't notice anything... and regtests
> that James posted are no longer available to download.  Pity.
> I suppose that the final version of the patch changes some accidental
> spacing, so i'll check it now.

Um, Mike, i've just checked and current master has many cases of bad
accidental placement - many accidentals are too close to each other.
For example, <es' a'!> .  It looks pretty much like the output of
first patchset that i checked...  Have you changed anything there at
all?

cheers,
Janek

[mike7, 2012-09-27 19:01:42]

On 27 sept. 2012, at 13:56, Janek Warchoł <janek.lilypond@gmail.com> wrote:

> On Thu, Sep 27, 2012 at 1:20 PM, Janek Warchoł <janek.lilypond@gmail.com>
wrote:
>> On Sat, Sep 8, 2012 at 5:43 PM, mike@mikesolomon.org
>> <mike@mikesolomon.org> wrote:
>>> OK - I'll be able to fix all the broken stuff on Monday or Tuesday.
>> 
>> Whoah, this was pushed and i didn't notice anything... and regtests
>> that James posted are no longer available to download.  Pity.
>> I suppose that the final version of the patch changes some accidental
>> spacing, so i'll check it now.
> 
> Um, Mike, i've just checked and current master has many cases of bad
> accidental placement - many accidentals are too close to each other.
> For example, <es' a'!> .  It looks pretty much like the output of
> first patchset that i checked...  Have you changed anything there at
> all?
> 

Hey Janek,

There were three patchsets (maybe more).  If it looks catastrophic, I trust you
- just revert it in staging and send me a short example w/ the regression and
I'll work on it.

Cheers,
MS

[janek, 2012-09-28 05:16:14]

On Thursday, September 27, 2012, mike@mikesolomon.org wrote:

> On 27 sept. 2012, at 13:56, Janek Warchoł <janek.lilypond@gmail.com>
> wrote:
> > Um, Mike, i've just checked and current master has many cases of bad
> > accidental placement - many accidentals are too close to each other.
> > For example, <es' a'!> .  It looks pretty much like the output of
> > first patchset that i checked...  Have you changed anything there at
> > all?
>
> Hey Janek,
>
> There were three patchsets (maybe more).  If it looks catastrophic, I
> trust you - just revert it in staging and send me a short example w/ the
> regression and I'll work on it.
>

It looks to me that this patch is way past staging, so i'm not sure if it
makes sense to revert it.  After all, accidental spacing before your patch
was bad in some cases, and after your patch is bad in some cases - the
difference is just which cases are wrong ;)
Anyway, i'll make a comparison and post the results in a comment to issue
2141.

cheers,
Janek

[Keith, 2012-09-28 06:56:36]

http://codereview.appspot.com/6489086/diff/11/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

http://codereview.appspot.com/6489086/diff/11/lily/accidental-placement.cc#ne...
lily/accidental-placement.cc:377: Real offset =
-ape->horizontal_skylines_[RIGHT].distance (left_skyline);
Both Mike and Janek thought the accidentals needed a bit more padding for close
intervals, so I think Janek should try horizon_padding here with 
distance(left_skyline, 0.1)

If you think it worth making a tunable property of AccidentalPlacement, fine. 
But the old parameters were not tunable, and better to have a non-tunable
correct spacing than non-tunable incorrect spacing.

[janek, 2012-09-28 07:30:05]

accidental spacing comparison posted as comment 4 in issue 2141
http://code.google.com/p/lilypond/issues/detail?id=2141#c4

On Fri, Sep 28, 2012 at 8:56 AM,  <k-ohara5a5a@oco.net> wrote:
> Both Mike and Janek thought the accidentals needed a bit more padding
> for close intervals, so I think Janek should try horizon_padding here
> with  distance(left_skyline, 0.1)
>
> If you think it worth making a tunable property of AccidentalPlacement,
> fine.  But the old parameters were not tunable, and better to have a
> non-tunable correct spacing than non-tunable incorrect spacing.

I'll try experimenting today.

cheers,
Janek

[janek, 2012-10-04 22:31:35]

On Fri, Sep 28, 2012 at 8:56 AM,  <k-ohara5a5a@oco.net> wrote:
>
http://codereview.appspot.com/6489086/diff/11/lily/accidental-placement.cc#ne...
> lily/accidental-placement.cc:377: Real offset =
> -ape->horizontal_skylines_[RIGHT].distance (left_skyline);
> Both Mike and Janek thought the accidentals needed a bit more padding
> for close intervals, so I think Janek should try horizon_padding here
> with  distance(left_skyline, 0.1)

>
> http://codereview.appspot.com/6489086/diff/11/lily/accidental-placement.cc
> File lily/accidental-placement.cc (right):
>
I apologize for not replying earlier.  I've tried horizon_padding and
i like the results; see more information in tracker issue
http://code.google.com/p/lilypond/issues/detail?id=2141#c16

thanks,
Janek

[Keith, 2013-07-17 06:18:18]

https://codereview.appspot.com/6489086/diff/11/lily/accidental.cc
File lily/accidental.cc (right):

https://codereview.appspot.com/6489086/diff/11/lily/accidental.cc#newcode89
lily/accidental.cc:89: (my_stencil->smobbed_copy (), 0.0, Y_AXIS));
We need to check that my_stencil exists here, in case someone defines new
alterations and forgets to assign a glyph for one of them.

newPitchNames = #`(
   (c . ,(ly:make-pitch -1 0 NATURAL))
   (ceseh . ,(ly:make-pitch -1 0 1/9)))
#(ly:parser-set-note-names parser newPitchNames)
{c''2 ceseh''}

I'll add a test and push after a `make check`