Gets vertical skylines from grob stencils

Gets vertical skylines from grob stencils

[MikeSol, 2012-02-04 21:28:16]

Sorry in advance for the whitespace errors.

This patch provides a generic framework for building vertical skylines out of
boxes by traversing through a stencil.  It currently only implements skylines
for three stencil types (draw-line, named-glyph, and glyph-string) and can do
more for other glyphs.  It is modular, so as skyline estimation improve for a
given stencil, that function can be worked on alone w/o touching the other parts
of stencil-integral.scm.

The implementation for glyph-string is ugly and slightly incorrect for y heights
of these stencils...it would be infinitely easier if the stencil could contain
information about y extents, but unfortunately, PangoGlyphGeometry does not
carry height information.  This error does not have a bearing on the visual
results in this patch, but it'd be good to find a way to make the dimensions
exact.

I'm also not sure that line thickness is tacked on correctly for the draw-line
boxes, but it seems to yield correct results.

Cheers,
MS

[MikeSol, 2012-02-06 16:42:47]

Hey all,

Could people please try this patch out on a couple real world scores and do some
benchmarking compared to current master?  It adds a lot of calculations to
lilypond and a lot of them happen in Scheme, so I wanna make sure lilypond
doesn't take a large processing hit.

Cheers,
MS

[MikeSol, 2012-02-06 16:59:35]

Should have added before: the most recent patch set is not bug free.
I'm fixing all of the regtest issues, but what I need most from other people who
have a few minutes are benchmarks.

Cheers,
MS

[mail_philholmes.net, 2012-02-06 17:06:16]

----- Original Message ----- 
From: <mtsolo@gmail.com>
To: <mtsolo@gmail.com>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Monday, February 06, 2012 4:59 PM
Subject: Re: Gets vertical skylines from grob stencils (issue 5626052)


> Should have added before: the most recent patch set is not bug free.
> I'm fixing all of the regtest issues, but what I need most from other
> people who have a few minutes are benchmarks.
>
> Cheers,
> MS
>
> http://codereview.appspot.com/5626052/


Does this have to be complex input?   If not, make doc exercises lilypond 
quite a lot.

--
Phil Holmes

[Neil Puttock, 2012-02-06 17:36:06]

On 6 February 2012 16:59,  <mtsolo@gmail.com> wrote:
> Should have added before: the most recent patch set is not bug free.

Cyclic dependencies for TextScript Y-offset.

But you've just fixed that by the look of it. ;)

> I'm fixing all of the regtest issues, but what I need most from other
> people who have a few minutes are benchmarks.

L'Isle joyeuse:

master: 0m30.432s
patched: 0m46.997s

Psalm 94 (Reubke):

master: 1m31.692s
patched: 2m0.817s

Cheers,
Neil

[mike_apollinemike.com, 2012-02-06 17:50:59]

On Feb 6, 2012, at 6:36 PM, Neil Puttock wrote:

> On 6 February 2012 16:59,  <mtsolo@gmail.com> wrote:
>> Should have added before: the most recent patch set is not bug free.
> 
> Cyclic dependencies for TextScript Y-offset.
> 
> But you've just fixed that by the look of it. ;)
> 

"fixed" is a generous word for what I've done...
I've removed the Y-offset check, but the issue now is that anything with
outside-staff-priority and vertical-skylines will have its Y-offset ignored when
it is being moved.
This means that it could come in lower than what it would be if the original
offset were there.

if you add the line:
elements[i]->relative_offset (y_common, Y_AXIS);
anywhere in the loop from which I removed the Y-offset and then run
volta-multi-staff.ly, you'll see a strange shift in the volta bracket.
i have absolutely no idea what is causing this - it'll take me some time to sort
it out.
in the final patch, though, the relative_offset would need to be factored in so
that grobs with outside staff priority could not dip below their
relative_coordinate.

>> I'm fixing all of the regtest issues, but what I need most from other
>> people who have a few minutes are benchmarks.
> 
> L'Isle joyeuse:
> 
> master: 0m30.432s
> patched: 0m46.997s
> 
> Psalm 94 (Reubke):
> 
> master: 1m31.692s
> patched: 2m0.817s

Thanks!
In my opinion, these times are too high to justify the gain that one gets from
using fine-tuned vertical skylines.  In the final version of the patch, I'll
limit them to slurs, ties, tuplet-brackets, volta repeats, and treble clefs. 
Other grobs' vertical-skylines can be turned on via overrides for anyone who
wants tighter spacing.  We can even create a tight-spacing.ly file that people
can include to get the overrides in one fell swoop.

Cheers,
MS

[wl_gnu.org, 2012-02-06 18:45:56]

>> L'Isle joyeuse:
>> 
>> master: 0m30.432s
>> patched: 0m46.997s
>> 
>> Psalm 94 (Reubke):
>> 
>> master: 1m31.692s
>> patched: 2m0.817s
> 
> In my opinion, these times are too high to justify the gain that one
> gets from using fine-tuned vertical skylines.

Yes, unfortunately.


    Werner

[Graham Percival, 2012-02-06 18:52:38]

On Mon, Feb 06, 2012 at 07:45:47PM +0100, Werner LEMBERG wrote:
> > In my opinion, these times are too high to justify the gain that one
> > gets from using fine-tuned vertical skylines.
> 
> Yes, unfortunately.

+1

is it at all possible to make this a user-configurable option?
I know this would add complexity to the code base, but how bad
would it be?

- Graham

[joeneeman, 2012-02-06 19:00:55]

Could you publicize work-in-progress patches like this as a git branch
instead of a Rietveld issue? Git branches are easy for reviewers to check
out, fast-forward, revert, etc. That way, people can offer broad comments.
When the patch is "finished" (ie. you can't think of anything you need to
change), then upload it to rietveld, where it's easy to make specific,
in-line comments.

Avoiding rietveld until the patch is done will also prevent the situation
where someone makes a bunch of detailed comments to lines that you're in
the middle of changing anyway.

Cheers,
Joe

On Sat, Feb 4, 2012 at 1:28 PM, <mtsolo@gmail.com> wrote:

> Reviewers: ,
>
> Message:
> Sorry in advance for the whitespace errors.
>
> This patch provides a generic framework for building vertical skylines
> out of boxes by traversing through a stencil.  It currently only
> implements skylines for three stencil types (draw-line, named-glyph, and
> glyph-string) and can do more for other glyphs.  It is modular, so as
> skyline estimation improve for a given stencil, that function can be
> worked on alone w/o touching the other parts of stencil-integral.scm.
>
> The implementation for glyph-string is ugly and slightly incorrect for y
> heights of these stencils...it would be infinitely easier if the stencil
> could contain information about y extents, but unfortunately,
> PangoGlyphGeometry does not carry height information.  This error does
> not have a bearing on the visual results in this patch, but it'd be good
> to find a way to make the dimensions exact.
>
> I'm also not sure that line thickness is tacked on correctly for the
> draw-line boxes, but it seems to yield correct results.
>
> Cheers,
> MS
>
> Description:
> Gets vertical skylines from grob stencils
>
> Please review this at
http://codereview.appspot.com/**5626052/<http://codereview.appspot.com/5626052/>
>
> Affected files:
>  A
input/regression/tuplet-**bracket-vertical-skylines.ly<http://tuplet-bracket-vertical-skylines.ly>
>  A
input/regression/volta-**bracket-vertical-skylines.ly<http://volta-bracket-vertical-skylines.ly>
>  M lily/axis-group-interface.cc
>  A lily/box-scheme.cc
>  M lily/include/box.hh
>  M lily/include/skyline.hh
>  M lily/skyline-pair.cc
>  A lily/skyline-scheme.cc
>  M lily/skyline.cc
>  M lily/stencil-scheme.cc
>  M lily/tuplet-bracket.cc
>  M lily/volta-bracket.cc
>  M scm/define-grobs.scm
>  M scm/lily.scm
>  A scm/stencil-integral.scm
>
>
>
> ______________________________**_________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
>
https://lists.gnu.org/mailman/**listinfo/lilypond-devel<https://lists.gnu.org...
>

[mike_apollinemike.com, 2012-02-06 19:25:34]

On Feb 6, 2012, at 8:00 PM, Joe Neeman wrote:

> Could you publicize work-in-progress patches like this as a git branch instead
of a Rietveld issue?

Ok.  For the git-handicapped (me), how does one do this?

Cheers,
MS

[janek, 2012-02-06 22:23:08]

On 2012/02/06 19:25:34, mike_apollinemike.com wrote:
> On Feb 6, 2012, at 8:00 PM, Joe Neeman wrote:
> 
> > Could you publicize work-in-progress patches like this as a git branch
instead
> of a Rietveld issue?
> 
> Ok.  For the git-handicapped (me), how does one do this?

Push a private branch to our official repository?

git push origin local-branch-to-be-pushed:target-branch

if 'target-branch' does not exist, it will be created.

HTH,
Janek

[MikeSol, 2012-02-14 09:26:55]

Sorry for whitespace problems.

After receiving several good general comments regarding the structure of this
patch, I have incorporated them all into my work and am now putting it on
Reitveld for more technical review.

Please run:

make clean
./autogen.sh --disable-optimising
make all

To make sure that it works.

[janek, 2012-02-14 16:11:44]

Mike,

On Tue, Feb 14, 2012 at 10:26 AM,  <mtsolo@gmail.com> wrote:
> After receiving several good general comments regarding the structure of
> this patch, I have incorporated them all into my work and am now putting
> it on Reitveld for more technical review.

thank you once again for this awesome work!
I didn't take part in the discussion, but the time came for extensive
testing and that's what i like doing.
Unfortunately i have some bad news: my files fail to compile.  I
attach source code (for tota-pulchra, run the only .ly file present -
choir 15.ly).  Here are the logs i got:

GNU LilyPond 2.15.30
Processing `..//trillspan-slanted-accidentals.ly'
Parsing...
Interpreting music...
Preprocessing graphical objects...
Finding the ideal number of pages...
Fitting music on 1 page...
Drawing systems...ERROR: In procedure ly:grob::vertical-skylines-from-stencil:
ERROR: Unbound variable: box-hash

GNU LilyPond 2.15.30
Processing `../tota-pulchra/choir 15.ly'
Parsing...
Interpreting music...
Interpreting music...
Interpreting music...
Interpreting music...
Interpreting music...
[8][16][24][32][40][48][56][64][72][80][88][96][104][112][120]
Preprocessing graphical objects...lilypond: grob-property.cc:252:
scm_unused_struct* Grob::try_callback_on_alist(scm_unused_struct**,
scm_unused_struct*, scm_unused_struct*): Assertion `value == ((SCM)
((((4)) << 8) + scm_tc8_flag)) || value == marker' failed.
./skytest: line 8: 14965 Aborted
~/skylines/out/bin/lilypond -ddebug-skylines=#t $dir/*.ly

both of these compile fine with current master
(3d8f4559228bd8a4a30bb024163b64d425b76f18).
Just to make sure, here's how i set-up my testing build:

<a fresh clone of our savannah repository>
janek@lilydev2 ~/skylines (master)$ git checkout -b skylines
origin/dev/skylines-cached
Branch skylines set up to track remote branch dev/skylines-cached from origin.
Switched to a new branch 'skylines'
janek@lilydev2 ~/skylines (skylines)$ ./autogen.sh --disable-optimising
stepmake/aclocal.m4 is newer.  Copying file.
processing .
Running autoconf ...
Running ./configure --disable-optimising ...
(...)
janek@lilydev2 ~/skylines (skylines)$ make all
(...)

thanks,
Janek

[mike_apollinemike.com, 2012-02-14 18:17:33]

On Feb 14, 2012, at 4:11 PM, Janek Warchoł wrote:

> Mike,
> 
> On Tue, Feb 14, 2012 at 10:26 AM,  <mtsolo@gmail.com> wrote:
>> After receiving several good general comments regarding the structure of
>> this patch, I have incorporated them all into my work and am now putting
>> it on Reitveld for more technical review.
> 
> thank you once again for this awesome work!
> I didn't take part in the discussion, but the time came for extensive
> testing and that's what i like doing.
> Unfortunately i have some bad news: my files fail to compile.  I
> attach source code (for tota-pulchra, run the only .ly file present -
> choir 15.ly).

Thanks for taking the time to check this!

During your font build, does it create a file called font-cache.scm?  You can
do:

find . -name "font-cache.scm"

in your LilyPond directory.  It should, in theory, be present in mf/out and then
get copied to a new folder font/scm somewhere in the build directory.

Cheers,
MS

[janek, 2012-02-14 18:46:41]

On Tue, Feb 14, 2012 at 7:17 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Thanks for taking the time to check this!
>
> During your font build, does it create a file called font-cache.scm?  You can
do:
>
> find . -name "font-cache.scm"

Calling this inside top source directory returns:

./mf/out/font-cache.scm

So it exists, but isn't copied i guess.

cheers,
Janek

[mike_apollinemike.com, 2012-02-15 01:06:06]

On Feb 14, 2012, at 6:46 PM, Janek Warchoł wrote:

>> find . -name "font-cache.scm"
> 
> Calling this inside top source directory returns:
> 
> ./mf/out/font-cache.scm
> 
> So it exists, but isn't copied i guess.

Is there something to the effect of:

build/share/lilypond/current/fonts/scm/font-cache.scm

either the scm folder or the file?  Julien - in GNUmakefile.in, does this seem
like it's doing the correct thing?  What's odd is that it worked at some point
on my computer, so I'm thinking that either I'm not providing correct
instructions to ya'll about how to clean out the build directory and/or that the
way I wrote the rules in GNUmakefile.in are currently incorrect.

Cheers,
MS

[janek, 2012-02-15 06:00:51]

On Wed, Feb 15, 2012 at 2:05 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Is there something to the effect of:
>
> build/share/lilypond/current/fonts/scm/font-cache.scm
>
> either the scm folder or the file?

Umm, i don't have anything like that.  First of all, i didn't use
build/ directory - everything was compiled in top source directory (I
know that's a bad practice, but wanted to stick with steps you posted
on Feb 9 as closely as possible).
Taking build/ aside, i have 'share/' subdirectory inside out/.  When i
go to TOP_SRC/out/share/lilypond/current/fonts/scm, there's only one
file named '*.scm' (yes, an asterisk).  When i click it Ubuntu says
that it's broken:

The Link "*.scm" is Broken. Move it to Trash?
This link cannot be used, because its target
"../../../../../../mf/out/*.scm" doesn't exist.

Hope this helps.

> Julien - in GNUmakefile.in, does this
> seem like it's doing the correct thing?  What's odd is that it worked at
> some point on my computer, so I'm thinking that either I'm not providing
> correct instructions to ya'll about how to clean out the build directory
> and/or that the way I wrote the rules in GNUmakefile.in are currently
> incorrect.

Well, Graham always says "don't trust make clean, press the red button
and nuke everything"...

cheers,
Janek

[mike_apollinemike.com, 2012-02-15 07:07:56]

On Feb 15, 2012, at 6:00 AM, Janek Warchoł wrote:

> On Wed, Feb 15, 2012 at 2:05 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> Is there something to the effect of:
>> 
>> build/share/lilypond/current/fonts/scm/font-cache.scm
>> 
>> either the scm folder or the file?
> 
> Umm, i don't have anything like that.  First of all, i didn't use
> build/ directory - everything was compiled in top source directory (I
> know that's a bad practice, but wanted to stick with steps you posted
> on Feb 9 as closely as possible).
> Taking build/ aside, i have 'share/' subdirectory inside out/.  When i
> go to TOP_SRC/out/share/lilypond/current/fonts/scm, there's only one
> file named '*.scm' (yes, an asterisk).  When i click it Ubuntu says
> that it's broken:
> 
> The Link "*.scm" is Broken. Move it to Trash?
> This link cannot be used, because its target
> "../../../../../../mf/out/*.scm" doesn't exist.
> 

Interesting.
I'm assuming this comes from line 248 and 249 of GNUmakefile.in, but what's odd
is that it creates a file with an asterisk.  It is literally a copy and paste of
everything around it.  Of course, this is not necessarily a recipe for success,
but I don't see why it'd be that far off the mark.

Does your mf/out/font-cache.scm have a bunch of definitions in it (for something
like box-hash)?  If it does, that means that the file is building correctly,
which is half the battle.

As for nuking everything and starting over, that may work (i.e. removing the
entire out directory) - I'm not sure, but if it doesn't tie up your computer and
prevent you from doing other things, it's worth a shot!  Thanks for your
patience - what's difficult is that it works well on my machine (even after
`make clean', so it's tough for me to troubleshoot it.

Cheers,
MS

[janek, 2012-02-15 07:25:23]

On Wed, Feb 15, 2012 at 8:07 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Interesting.
> I'm assuming this comes from line 248 and 249 of GNUmakefile.in,
> but what's odd is that it creates a file with an asterisk.  It is literally
> a copy and paste of everything around it.  Of course, this is not
> necessarily a recipe for success, but I don't see why it'd be that far
> off the mark.
>
> Does your mf/out/font-cache.scm have a bunch of definitions in it
> (for something like box-hash)?  If it does, that means that the file
> is building correctly, which is half the battle.

It's 5000 lines of things like this:
(hashq-set! boxes-for-emmentaler-11 'rests.1 '((paths (357 0 18 0) (18
0 8 0 0 8 0 18) (0 18 0 138) (0 138 0 148 8 156 18 156) (18 156 357
156) (357 156 367 156 375 148 375 138) (375 138 375 18) (375 18 375 8
367 0 357 0)) (mmx 0.0 . 375.0) (mmy 0.0 . 156.0)))

shall i try to copy the file manually to
TOP_SRC/out/share/lilypond/current/fonts/scm and see if it will not
crash?

> As for nuking everything and starting over, that may work (i.e. removing
> the entire out directory) - I'm not sure, but if it doesn't tie up your
computer
> and prevent you from doing other things, it's worth a shot!

Umm, i already did it.  I even cloned a separate repository from
savannah to make sure that no old cruft of mine is hanging around, and
made everything from scratchy-scratch.
I meant that you should try it ;-)

> Thanks for your patience -

No problem!  Your change is so awesome that this is the least i can do.

HTH,
Janek

PS i'm idling in our IRC channel now.

[mike_apollinemike.com, 2012-02-15 07:37:15]

On Feb 15, 2012, at 7:24 AM, Janek Warchoł wrote:

> It's 5000 lines of things like this:
> (hashq-set! boxes-for-emmentaler-11 'rests.1 '((paths (357 0 18 0) (18
> 0 8 0 0 8 0 18) (0 18 0 138) (0 138 0 148 8 156 18 156) (18 156 357
> 156) (357 156 367 156 375 148 375 138) (375 138 375 18) (375 18 375 8
> 367 0 357 0)) (mmx 0.0 . 375.0) (mmy 0.0 . 156.0)))
> 
> shall i try to copy the file manually to
> TOP_SRC/out/share/lilypond/current/fonts/scm and see if it will not
> crash?

Please do - this'll allow you to test the patch.  I just nuked my build
directory and it fails for me in the same way that it does for you, so I can now
reproduce the problem and will fix it soon.

Cheers,
MS

[janek, 2012-02-15 07:49:13]

On Wed, Feb 15, 2012 at 8:37 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Feb 15, 2012, at 7:24 AM, Janek Warchoł wrote:
>
>> It's 5000 lines of things like this:
>> (hashq-set! boxes-for-emmentaler-11 'rests.1 '((paths (357 0 18 0) (18
>> 0 8 0 0 8 0 18) (0 18 0 138) (0 138 0 148 8 156 18 156) (18 156 357
>> 156) (357 156 367 156 375 148 375 138) (375 138 375 18) (375 18 375 8
>> 367 0 357 0)) (mmx 0.0 . 375.0) (mmy 0.0 . 156.0)))
>>
>> shall i try to copy the file manually to
>> TOP_SRC/out/share/lilypond/current/fonts/scm and see if it will not
>> crash?
>
> Please do - this'll allow you to test the patch.

Of the two pieces i've sent you (attached again), trillspan-slanted
compiles, but tota-pulchra fails with a very similar error:

GNU LilyPond 2.15.30
Processing `../tota-pulchra/choir 15.ly'
Parsing...
Interpreting music...
Interpreting music...
Interpreting music...
Interpreting music...
Interpreting music...
[8][16][24][32][40][48][56][64][72][80][88][96][104][112][120]
Preprocessing graphical objects...lilypond: grob-property.cc:252:
scm_unused_struct* Grob::try_callback_on_alist(scm_unused_struct**,
scm_unused_struct*, scm_unused_struct*): Assertion `value == ((SCM)
((((4)) << 8) + scm_tc8_flag)) || value == marker' failed.
./skytest: line 8: 25839 Aborted
~/skylines/out/bin/lilypond -ddebug-skylines=#t $dir/*.ly

HTH,
Janek

[mike_apollinemike.com, 2012-02-15 07:59:14]

On Feb 15, 2012, at 7:48 AM, Janek Warchoł wrote:

> On Wed, Feb 15, 2012 at 8:37 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> On Feb 15, 2012, at 7:24 AM, Janek Warchoł wrote:
>> 
>>> It's 5000 lines of things like this:
>>> (hashq-set! boxes-for-emmentaler-11 'rests.1 '((paths (357 0 18 0) (18
>>> 0 8 0 0 8 0 18) (0 18 0 138) (0 138 0 148 8 156 18 156) (18 156 357
>>> 156) (357 156 367 156 375 148 375 138) (375 138 375 18) (375 18 375 8
>>> 367 0 357 0)) (mmx 0.0 . 375.0) (mmy 0.0 . 156.0)))
>>> 
>>> shall i try to copy the file manually to
>>> TOP_SRC/out/share/lilypond/current/fonts/scm and see if it will not
>>> crash?
>> 
>> Please do - this'll allow you to test the patch.
> 
> Of the two pieces i've sent you (attached again), trillspan-slanted
> compiles, but tota-pulchra fails with a very similar error:
> 
> GNU LilyPond 2.15.30
> Processing `../tota-pulchra/choir 15.ly'
> Parsing...
> Interpreting music...
> Interpreting music...
> Interpreting music...
> Interpreting music...
> Interpreting music...
> [8][16][24][32][40][48][56][64][72][80][88][96][104][112][120]
> Preprocessing graphical objects...lilypond: grob-property.cc:252:
> scm_unused_struct* Grob::try_callback_on_alist(scm_unused_struct**,
> scm_unused_struct*, scm_unused_struct*): Assertion `value == ((SCM)
> ((((4)) << 8) + scm_tc8_flag)) || value == marker' failed.
> ./skytest: line 8: 25839 Aborted
> ~/skylines/out/bin/lilypond -ddebug-skylines=#t $dir/*.ly
> 

Thanks for the feedback!  I'll do some testing using this.

Cheers,
MS

[janek, 2012-02-15 08:11:07]

On Wed, Feb 15, 2012 at 8:59 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Thanks for the feedback!  I'll do some testing using this.

I found it!  It was a BarNumber tweak that was causing this:

\override BarNumber #'self-alignment-X =
                    #(lambda (grob)
                       (let ((break-dir (ly:item-break-dir grob)))
                         (set! (ly:grob-property grob 'self-alignment-X)
                               (if (= break-dir RIGHT)
                                   1
                                   0))))

See lines 102-108 of attached ../tota-pulchra.ly.  If you detele them,
it compiles.

HTH,
Janek

[mike_apollinemike.com, 2012-02-15 08:17:26]

On Feb 15, 2012, at 8:10 AM, Janek Warchoł wrote:

> On Wed, Feb 15, 2012 at 8:59 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> Thanks for the feedback!  I'll do some testing using this.
> 
> I found it!  It was a BarNumber tweak that was causing this:
> 
> \override BarNumber #'self-alignment-X =
>                    #(lambda (grob)
>                       (let ((break-dir (ly:item-break-dir grob)))
>                         (set! (ly:grob-property grob 'self-alignment-X)
>                               (if (= break-dir RIGHT)
>                                   1
>                                   0))))

Many a time have I been bit by the -1/1 instead of 0/1 bug...glad you found it.
A new patch is up that fixes the makefile problem.  Lemme know what you think of
the example files you compile whenever you get a chance & thanks for your work!

Cheers,
MS

[janek, 2012-02-15 09:29:46]

On Wed, Feb 15, 2012 at 9:17 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Many a time have I been bit by the -1/1 instead of 0/1 bug...glad you found
it.

Glad i helped :)

> A new patch is up that fixes the makefile problem.

Works for me.  I did an out-of-tree build this time and the shortcut
named font-cache.scm inside
TOP_SRC/build/out/share/lilypond/current/fonts seems to be ok.
Also, if i remove that BarNumber override, all my files compile.
I'll examine them and give you my thoughts on the output - are pdfs
with problematic places highlighted and commented (inside pdf) ok for
you?

cheers,
Janek

[mike_apollinemike.com, 2012-02-15 10:13:55]

On Feb 15, 2012, at 9:29 AM, Janek Warchoł wrote:

> On Wed, Feb 15, 2012 at 9:17 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> Many a time have I been bit by the -1/1 instead of 0/1 bug...glad you found
it.
> 
> Glad i helped :)
> 
>> A new patch is up that fixes the makefile problem.
> 
> Works for me.  I did an out-of-tree build this time and the shortcut
> named font-cache.scm inside
> TOP_SRC/build/out/share/lilypond/current/fonts seems to be ok.
> Also, if i remove that BarNumber override, all my files compile.
> I'll examine them and give you my thoughts on the output - are pdfs
> with problematic places highlighted and commented (inside pdf) ok for
> you?
> 


That's fine.
The BarNumber override problem is interesting, as I have never seen this error
before and I'm not sure how the present patch plays into it.  I'll do some
investigating tonight or over the weekend.

CHeers,
MS

[Graham Percival, 2012-02-15 10:26:17]

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile
File mf/GNUmakefile (right):

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile#newcode231
mf/GNUmakefile:231: $(LILYPOND_BINARY) --verbose
$(top-src-dir)/ly/generate-font-integrals > $@
This looks good, but not immediately relevant to the rest of the vertical
skylines stuff.  Could you make this a separate patch so that it can be pushed
sooner?

[mike_apollinemike.com, 2012-02-15 10:31:56]

On Feb 15, 2012, at 10:26 AM, graham@percival-music.ca wrote:

> 
> http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile
> File mf/GNUmakefile (right):
> 
> http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile#newcode231
> mf/GNUmakefile:231: $(LILYPOND_BINARY) --verbose
> $(top-src-dir)/ly/generate-font-integrals > $@
> This looks good, but not immediately relevant to the rest of the
> vertical skylines stuff.  Could you make this a separate patch so that
> it can be pushed sooner?
> 

The only thing is that it'd take pushing a few other files (all of the new stuff
in the .scm directory) to work.  Once these have been commented on and LGTM'd,
I'll make this the first push.

[reinhold_kainhofer.com, 2012-02-15 14:25:57]

On 06/02/2012 18:50, mike@apollinemike.com wrote:
>>> I'm fixing all of the regtest issues, but what I need most from other
>>> people who have a few minutes are benchmarks.
>> L'Isle joyeuse:
>> [...]
> Thanks!
> In my opinion, these times are too high to justify the gain that one gets from
using fine-tuned vertical skylines.  

Agreed.


> In the final version of the patch, I'll limit them to slurs, ties,
tuplet-brackets, volta repeats, and treble clefs.  

What about hairpins? They are one of the main problem areas in my scores
(and they are even worse in a piano staff).

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[joeneeman, 2012-02-16 08:09:10]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:659: vector<Grob *> *riders)
I don't understand why riders are necessary. Is it because of this cyclic
dependence stuff?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode467
lily/skyline.cc:467: Strips all old sloped material, adds new.
You're assuming that all sloped material came from skyline padding. That's true
right now, but there's no reason that it will continue to be true. It's probably
better to avoid adding padding at creation time altogether, and instead to add
it when calling Skyline::distance.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
This is linear in the number of buildings, but it should be constant.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
Ditto

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc#new...
lily/stencil-integral.cc:543: SCM glyph_info = scm_hashq_ref (font_list,
scm_string_to_symbol (glyph), SCM_EOL);
There isn't much error-checking here. What if the user substitutes an unofficial
font which isn't in the list?

By the way, lilypond's fonts embed extra data in font tables (look for LILC and
LILY in open-type-font.cc). That may be a better way to embed this information
than by putting it in a scm file. For example, it would allow unofficial fonts
to support integrals by embedding an extra table.

[MikeSol, 2012-02-16 10:08:39]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:659: vector<Grob *> *riders)
On 2012/02/16 08:09:10, joeneeman wrote:
> I don't understand why riders are necessary. Is it because of this cyclic
> dependence stuff?

Not cyclical, but imagine that a grob (say tuplet bracket, for example) has its
outside staff priority set whereas one of its Y_AXIS children (say tuplet
number, for example), doesn't.  If the tuplet number's skyline is added to the
skyline_pair using add boxes before its parent is shifted, it will be placed too
low in the skyline.  Thus, it must be added to the skyline only after its
parent's outside-staff-priority has been set to SCM_BOOL_F.  This is why riders
are used.

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode467
lily/skyline.cc:467: Strips all old sloped material, adds new.
On 2012/02/16 08:09:10, joeneeman wrote:
> You're assuming that all sloped material came from skyline padding. That's
true
> right now, but there's no reason that it will continue to be true. It's
probably
> better to avoid adding padding at creation time altogether, and instead to
add
> it when calling Skyline::distance.

Right you are...I agree that this is entirely dependent on the implementation of
skylines at the moment.  I know that I always respond to you that it's something
that can be done in a future patch, but I'd put this in that category as well,
as it seems like a separate problem that doesn't need to be tacked on to this
patch to be solved.  That said, if I forget, nag me!

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
On 2012/02/16 08:09:10, joeneeman wrote:
> This is linear in the number of buildings, but it should be constant.

How would one do this?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
On 2012/02/16 08:09:10, joeneeman wrote:
> Ditto

Ditto

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/stencil-integral.cc#new...
lily/stencil-integral.cc:543: SCM glyph_info = scm_hashq_ref (font_list,
scm_string_to_symbol (glyph), SCM_EOL);
On 2012/02/16 08:09:10, joeneeman wrote:
> There isn't much error-checking here. What if the user substitutes an
unofficial
> font which isn't in the list?
> 
> By the way, lilypond's fonts embed extra data in font tables (look for LILC
and
> LILY in open-type-font.cc). That may be a better way to embed this
information
> than by putting it in a scm file. For example, it would allow unofficial
fonts
> to support integrals by embedding an extra table.

I think this is a good idea...I'll have a look at it.  Question, though - aren't
font tables done as alists?  I think it's important to use a hash table here, as
there is a lot of data getting thrown around and hash table look ups are in
constant time.  I'll investigate.

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile
File mf/GNUmakefile (right):

http://codereview.appspot.com/5626052/diff/23001/mf/GNUmakefile#newcode231
mf/GNUmakefile:231: $(LILYPOND_BINARY) --verbose
$(top-src-dir)/ly/generate-font-integrals > $@
On 2012/02/15 10:26:17, Graham Percival wrote:
> This looks good, but not immediately relevant to the rest of the vertical
> skylines stuff.  Could you make this a separate patch so that it can be
pushed
> sooner?

Sorry for not responding to this comment inlined...the response is that it'd be
hard as it requires new files and/or changes to old ones to go along w/ it.

[joeneeman, 2012-02-16 10:40:19]

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:659: vector<Grob *> *riders)
On 2012/02/16 10:08:39, MikeSol wrote:
> On 2012/02/16 08:09:10, joeneeman wrote:
> > I don't understand why riders are necessary. Is it because of this cyclic
> > dependence stuff?
> 
> Not cyclical, but imagine that a grob (say tuplet bracket, for example) has
its
> outside staff priority set whereas one of its Y_AXIS children (say tuplet
> number, for example), doesn't.  If the tuplet number's skyline is added to the
> skyline_pair using add boxes before its parent is shifted, it will be placed
too
> low in the skyline.  Thus, it must be added to the skyline only after its
> parent's outside-staff-priority has been set to SCM_BOOL_F.  This is why
riders
> are used.

Could you document this reasoning, please?

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode668
lily/skyline.cc:668: Skyline::left () const
On 2012/02/16 10:08:39, MikeSol wrote:
> On 2012/02/16 08:09:10, joeneeman wrote:
> > This is linear in the number of buildings, but it should be constant.
> 
> How would one do this?

The buildings are in increasing order, so this should work:

Real last_end = -infinity_f;
for (...)
  {
    if (i->y_intercept_ > -infinity_f)
      return last_end;
    last_end = i->end_;
  }
return infinity_f;

http://codereview.appspot.com/5626052/diff/23001/lily/skyline.cc#newcode680
lily/skyline.cc:680: Skyline::right () const
On 2012/02/16 10:08:39, MikeSol wrote:
> On 2012/02/16 08:09:10, joeneeman wrote:
> > Ditto
> 
> Ditto

...and here it's a doubly-linked list so you can iterate backwards.

[mike_apollinemike.com, 2012-02-16 11:14:44]

On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:

> 
> The buildings are in increasing order, so this should work:
> 
> Real last_end = -infinity_f;
> for (...)
>  {
>    if (i->y_intercept_ > -infinity_f)
>      return last_end;
>    last_end = i->end_;
>  }
> return infinity_f;
> 

Hey,

I know this is a lame question, but how would one write the reverse iterator.  I
tried using reverse_iterator and g++ barfs, and I can't use the same syntax one
would use in a for loop w/ a vector.

Thanks!

Cheers,
MS

[hanwenn, 2012-02-16 12:16:16]

2012/2/15 Janek Warchoł <janek.lilypond@gmail.com>:

>> Does your mf/out/font-cache.scm have a bunch of definitions in it
>> (for something like box-hash)?  If it does, that means that the file
>> is building correctly, which is half the battle.
>
> It's 5000 lines of things like this:
> (hashq-set! boxes-for-emmentaler-11 'rests.1 '((paths (357 0 18 0) (18
> 0 8 0 0 8 0 18) (0 18 0 138) (0 138 0 148 8 156 18 156) (18 156 357
> 156) (357 156 367 156 375 148 375 138) (375 138 375 18) (375 18 375 8
> 367 0 357 0)) (mmx 0.0 . 375.0) (mmy 0.0 . 156.0)))

This sounds wrong.  Where does this data come from ultimately?

Without knowing more, I suspect this data should be computed at font
generation time, and it should be embedded in the OTF file.

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

[hanwenn, 2012-02-16 12:21:45]

On Thu, Feb 16, 2012 at 8:08 AM,  <mtsolo@gmail.com> wrote:
>> By the way, lilypond's fonts embed extra data in font tables (look for
>
> LILC and
>>
>> LILY in open-type-font.cc). That may be a better way to embed this
>
> information
>>
>> than by putting it in a scm file. For example, it would allow
>
> unofficial fonts
>>
>> to support integrals by embedding an extra table.
>
>
> I think this is a good idea...I'll have a look at it.  Question, though
> - aren't font tables done as alists?  I think it's important to use a
> hash table here, as there is a lot of data getting thrown around and
> hash table look ups are in constant time.  I'll investigate.

OTF tables are arbitrary binary data.  IIRC, the fonts are compressed
anyway, so the on-disk space is not that relevant.

When we read the fonts, we interpret the data, see open-type-font.cc,
and alist is just a very convenient form for (de)serializing data. You
could do whatever format works best for you; since the font is only
read once, you don't have to worry about performance when loading it.

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

[janek, 2012-02-16 12:42:17]

On Wed, Feb 15, 2012 at 3:25 PM, Reinhold Kainhofer
<reinhold@kainhofer.com> wrote:
> On 06/02/2012 18:50, mike@apollinemike.com wrote:
>> In my opinion, these times are too high to justify the gain
>> that one gets from using fine-tuned vertical skylines.
>
> Agreed.

I strongly disagree.  The gain is enormous - how much time would it
take to correct all these issues by hand?  (not mentioning that manual
corrections will break every time layout changes!)

>> In the final version of the patch, I'll limit them to
>> slurs, ties, tuplet-brackets, volta repeats, and treble clefs.
>
> What about hairpins? They are one of the main problem areas in my scores
> (and they are even worse in a piano staff).

what about beams?
{ g''16[ d'' a' e'] }
\addlyrics { la }

And while we're at it, what about rotated trillspans? ;)

LilyPond's greatest strength is beautiful /automated engraving/, even
for the people who don't have specialistic knowledge about engraving.
If we turn this feature off for some objects, it will mean that many
users will have sub-optimal scores.

I have one thought concerning computation time.
From what i see, currently every stencil's skyline is created from 10
boxes.  That's a good amount for medium-sized slur, but it's not
necessary for a short (~5 ss long) tie, or a clef.  And it's
definitely overkill for an accidental; on the other hand, full-line
phrasing slur should have much more boxes.
Maybe it would be feasible to have varying amount of boxes for
different objects?  In it's simplest form, the number of boxes could
depend on object's width.

cheers,
Janek

[joeneeman, 2012-02-16 20:31:09]

On Thu, Feb 16, 2012 at 3:14 AM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

>
> On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:
>
> >
> > The buildings are in increasing order, so this should work:
> >
> > Real last_end = -infinity_f;
> > for (...)
> >  {
> >    if (i->y_intercept_ > -infinity_f)
> >      return last_end;
> >    last_end = i->end_;
> >  }
> > return infinity_f;
> >
>
> Hey,
>
> I know this is a lame question, but how would one write the reverse
> iterator.  I tried using reverse_iterator and g++ barfs, and I can't use
> the same syntax one would use in a for loop w/ a vector.
>

That's a bit hard to say without knowing what you tried... did you use
rbegin() and rend()?

Cheers,
Joe

[mike_apollinemike.com, 2012-02-17 07:42:54]

On Feb 16, 2012, at 9:31 PM, Joe Neeman wrote:

> 
> On Thu, Feb 16, 2012 at 3:14 AM, mike@apollinemike.com <mike@apollinemike.com>
wrote:
> 
> On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:
> 
> >
> > The buildings are in increasing order, so this should work:
> >
> > Real last_end = -infinity_f;
> > for (...)
> >  {
> >    if (i->y_intercept_ > -infinity_f)
> >      return last_end;
> >    last_end = i->end_;
> >  }
> > return infinity_f;
> >
> 
> Hey,
> 
> I know this is a lame question, but how would one write the reverse iterator. 
I tried using reverse_iterator and g++ barfs, and I can't use the same syntax
one would use in a for loop w/ a vector.
> 
> That's a bit hard to say without knowing what you tried... did you use
rbegin() and rend()? 
> 

Yup - I more or less copied the example from cplusplus.com.  As an experiment,
try writing a reverse iterator in that file to the tune of:

  for (list<Building>::reverse_iterator i = buildings_.rbegin (); i !=
buildings_.rend (); ++i)

Anywhere in a function.  You should see a compiler error.

I fixed it, though, by reversing a copy of the list and iterating forward
through it.

Cheers,
MS

[mike_apollinemike.com, 2012-02-17 07:47:23]

On Feb 16, 2012, at 1:41 PM, Janek Warchoł wrote:

> On Wed, Feb 15, 2012 at 3:25 PM, Reinhold Kainhofer
> <reinhold@kainhofer.com> wrote:
>> On 06/02/2012 18:50, mike@apollinemike.com wrote:
>>> In my opinion, these times are too high to justify the gain
>>> that one gets from using fine-tuned vertical skylines.
>> 
>> Agreed.
> 
> I strongly disagree.  The gain is enormous - how much time would it
> take to correct all these issues by hand?  (not mentioning that manual
> corrections will break every time layout changes!)
> 

Note that this whole "slow down" thing is much less relevant now that it was
before.  My current benchmarks show a 0.2-2 second per minute slowdown depending
on the file.

> I have one thought concerning computation time.
> From what i see, currently every stencil's skyline is created from 10
> boxes.  That's a good amount for medium-sized slur, but it's not
> necessary for a short (~5 ss long) tie, or a clef.  And it's
> definitely overkill for an accidental; on the other hand, full-line
> phrasing slur should have much more boxes.
> Maybe it would be feasible to have varying amount of boxes for
> different objects?  In it's simplest form, the number of boxes could
> depend on object's width.

This is not impossible, but I'd encourage everyone to do some benchmarking on
their favorite scores in order to see the performance hit taken.  Janek's right
that certain grobs need more boxes than others and that this is a brute force
approach, but even with the brute-forcitude of the approach, I still think that
the performance hit is minimal.  A later patch can suggest gradations in the
number of boxes that are user settable (for the moment there are only two, and
they're easy to spot in stencil-integral.cc - CURVE_QUANTIZATION and
ELLIPSE_QUANTIZATION).  This'd take rewriting stencil-integral.cc to look more
like beam-quanting.cc (creating a class instead of a series of functions) which,
again, is doable, but the change is already so huge that I want to just get it
in the code more-or-less bug free before re-tinkering with it.

Cheers,
MS

[mike_apollinemike.com, 2012-02-17 08:00:53]

On Feb 16, 2012, at 1:21 PM, Han-Wen Nienhuys wrote:

> On Thu, Feb 16, 2012 at 8:08 AM,  <mtsolo@gmail.com> wrote:
>>> By the way, lilypond's fonts embed extra data in font tables (look for
>> 
>> LILC and
>>> 
>>> LILY in open-type-font.cc). That may be a better way to embed this
>> 
>> information
>>> 
>>> than by putting it in a scm file. For example, it would allow
>> 
>> unofficial fonts
>>> 
>>> to support integrals by embedding an extra table.
>> 
>> 
>> I think this is a good idea...I'll have a look at it.  Question, though
>> - aren't font tables done as alists?  I think it's important to use a
>> hash table here, as there is a lot of data getting thrown around and
>> hash table look ups are in constant time.  I'll investigate.
> 
> This sounds wrong.  Where does this data come from ultimately?
> 

The svg files.  I read them into lilypond and generate this informatin.

> Without knowing more, I suspect this data should be computed at font
> generation time, and it should be embedded in the OTF file.

It is created at font generation time (git grep font-cache.scm in
mf/GNUmakefile) but it is not part of the otf-table.  As I said, I'd like to do
this, but I'm not comfortable enough with build to know how its done.  Where and
when are these files generated?  Are they generated by metafont or python or
lilypond?  What I'd likely have to do is read the otf-tables into lilypond, run
the code used to make font-chace.scm, and instead of writing this data to
font-cache.scm overwrite the otf-table files w/ new otf-tables that contain this
data.  This doesn't seem impossible, but I'd want to make sure it's the right
way to go.  The advantage of font-cache.scm is that it defines box-hash in
LilyPond at runtime, and as hash-table-lookups are in constant time, this is
lightning fast compared to an alist, which is in linear time.

> 

> OTF tables are arbitrary binary data.  IIRC, the fonts are compressed
> anyway, so the on-disk space is not that relevant.
> 
> When we read the fonts, we interpret the data, see open-type-font.cc,
> and alist is just a very convenient form for (de)serializing data. You
> could do whatever format works best for you; since the font is only
> read once, you don't have to worry about performance when loading it.
> 


A separate issue should be opened in the tracker to use hash-tables instead of
alists for otf tables.  I'm trying to think of this project in separable,
manageable chunks.  I'd like the first chunk to be the pushing of the skyline
code you see on Rietveld so that there is a direct effect straightaway in scores
& so that we can get user feedback before we make infrastructural changes (which
may depend on what people wind up asking for after the change).

Cheers,
MS

[joeneeman, 2012-02-17 08:14:33]

On Thu, Feb 16, 2012 at 11:42 PM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

> On Feb 16, 2012, at 9:31 PM, Joe Neeman wrote:
>
>
> On Thu, Feb 16, 2012 at 3:14 AM, mike@apollinemike.com <
> mike@apollinemike.com> wrote:
>
>>
>> On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:
>>
>> >
>> > The buildings are in increasing order, so this should work:
>> >
>> > Real last_end = -infinity_f;
>> > for (...)
>> >  {
>> >    if (i->y_intercept_ > -infinity_f)
>> >      return last_end;
>> >    last_end = i->end_;
>> >  }
>> > return infinity_f;
>> >
>>
>> Hey,
>>
>> I know this is a lame question, but how would one write the reverse
>> iterator.  I tried using reverse_iterator and g++ barfs, and I can't use
>> the same syntax one would use in a for loop w/ a vector.
>>
>
> That's a bit hard to say without knowing what you tried... did you use
> rbegin() and rend()?
>
>
> Yup - I more or less copied the example from cplusplus.com.  As an
> experiment, try writing a reverse iterator in that file to the tune of:
>
>   for (list<Building>::reverse_iterator i = buildings_.rbegin (); i !=
> buildings_.rend (); ++i)
>
> Anywhere in a function.  You should see a compiler error.
>

How about -- instead of ++?

I fixed it, though, by reversing a copy of the list and iterating forward
> through it.
>

That's still linear time, though...

[joeneeman, 2012-02-17 08:22:49]

On Thu, Feb 16, 2012 at 11:42 PM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

> On Feb 16, 2012, at 9:31 PM, Joe Neeman wrote:
>
>
> On Thu, Feb 16, 2012 at 3:14 AM, mike@apollinemike.com <
> mike@apollinemike.com> wrote:
>
>>
>> On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:
>>
>> >
>> > The buildings are in increasing order, so this should work:
>> >
>> > Real last_end = -infinity_f;
>> > for (...)
>> >  {
>> >    if (i->y_intercept_ > -infinity_f)
>> >      return last_end;
>> >    last_end = i->end_;
>> >  }
>> > return infinity_f;
>> >
>>
>> Hey,
>>
>> I know this is a lame question, but how would one write the reverse
>> iterator.  I tried using reverse_iterator and g++ barfs, and I can't use
>> the same syntax one would use in a for loop w/ a vector.
>>
>
> That's a bit hard to say without knowing what you tried... did you use
> rbegin() and rend()?
>
>
> Yup - I more or less copied the example from cplusplus.com.  As an
> experiment, try writing a reverse iterator in that file to the tune of:
>
>   for (list<Building>::reverse_iterator i = buildings_.rbegin (); i !=
> buildings_.rend (); ++i)
>
> Anywhere in a function.  You should see a compiler error.
>

Yup, which is fixed by changing reverse_iterator to const_reverse_iterator.

The hint was in the error message:
 no known conversion for ... 'std::_List_const_iterator<Building>' to
'const std::_List_iterator<Building>&'

[mike_apollinemike.com, 2012-02-17 08:39:01]

On Feb 17, 2012, at 9:22 AM, Joe Neeman wrote:

> On Thu, Feb 16, 2012 at 11:42 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Feb 16, 2012, at 9:31 PM, Joe Neeman wrote:
> 
>> 
>> On Thu, Feb 16, 2012 at 3:14 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>> 
>> On Feb 16, 2012, at 11:40 AM, joeneeman@gmail.com wrote:
>> 
>> >
>> > The buildings are in increasing order, so this should work:
>> >
>> > Real last_end = -infinity_f;
>> > for (...)
>> >  {
>> >    if (i->y_intercept_ > -infinity_f)
>> >      return last_end;
>> >    last_end = i->end_;
>> >  }
>> > return infinity_f;
>> >
>> 
>> Hey,
>> 
>> I know this is a lame question, but how would one write the reverse iterator.
 I tried using reverse_iterator and g++ barfs, and I can't use the same syntax
one would use in a for loop w/ a vector.
>> 
>> That's a bit hard to say without knowing what you tried... did you use
rbegin() and rend()? 
>> 
> 
> Yup - I more or less copied the example from cplusplus.com.  As an experiment,
try writing a reverse iterator in that file to the tune of:
> 
>   for (list<Building>::reverse_iterator i = buildings_.rbegin (); i !=
buildings_.rend (); ++i)
> 
> Anywhere in a function.  You should see a compiler error.
> 
> Yup, which is fixed by changing reverse_iterator to const_reverse_iterator.
> 
> The hint was in the error message:
>  no known conversion for ... 'std::_List_const_iterator<Building>' to 'const
std::_List_iterator<Building>&'

You overestimate my powers of deduction...
Thanks very much for the help!  New patch set uploaded.

Cheers,
MS

[reinhold_kainhofer.com, 2012-02-17 10:47:04]

On Fr., 17. Feb. 2012 08:47:19 CET, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Note that this whole "slow down" thing is much less relevant now that it
> was before.   My current benchmarks show a 0.2-2 second per minute
> slowdown depending on the file.

Most important: is the slow down linear in the score length, polynomial or
exponential? I particular, is there any danger of making really huge sores
impossible to run through lilypond?

Cheers,
Reinhold

[hanwenn, 2012-02-17 12:49:23]

On Fri, Feb 17, 2012 at 6:00 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>> This sounds wrong.  Where does this data come from ultimately?
>>
>
> The svg files.  I read them into lilypond and generate this informatin.

You mean the SVG fonts?  The SVG files are generated by FF in the same
step as the OTF files.  I suspect that whatever you need is easier to
generate in FF directly.

You could ask around on the fontforge mailing list.

>> Without knowing more, I suspect this data should be computed at font
>> generation time, and it should be embedded in the OTF file.
>
> It is created at font generation time (git grep font-cache.scm in
mf/GNUmakefile) but it is not part of the otf-table.

which branch has the latest version of this?

>  As I said, I'd like to do this, but I'm not comfortable enough with build to
know how its done.  Where and when are these files generated?  Are they
generated by metafont or python or

look at scripts/build/mf-to-table.py

Then there is

 scripts/build/gen-emmentaler-scripts.py

which generates a FontForge script file (.pe since it used to be
called PfaEdit) in mf/out/emmentaler-XX.pe.

The calls look like this,

 LoadTableFromFile("LILC", "feta11.otf-table");

> This doesn't seem impossible, but I'd want to make sure it's the right way to
go.  The advantage of font-cache.scm is that it defines box-hash in LilyPond at
runtime, and as hash-table-lookups are in constant time, this is lightning fast
compared to an alist, which is in linear time.

I'd store a separate hashtable of codepoint => list of boxes directly
in the OpenTypeFont struct.

> A separate issue should be opened in the tracker to use hash-tables instead of
alists for otf tables.

but it is using hashtables; check out

 Open_type_font::Open_type_font (FT_Face face)

the alist are converted to hash tables.

>I'm trying to think of this project in separable, manageable chunks.  I'd like
the first chunk to be the pushing of the skyline code you see on Rietveld so
that there is a direct effect straightaway in scores & so that we can get user
feedback before we make infrastructural changes (which may depend on what people
wind up asking for after the change).

I'd rather not introduce more fragility (cf the generated .scm files)
into the build process which is complicated enough as it is.  I think
users can wait for another week if need be.

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

[hanwenn, 2012-02-17 12:58:11]

On Fri, Feb 17, 2012 at 10:49 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> On Fri, Feb 17, 2012 at 6:00 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>>> This sounds wrong.  Where does this data come from ultimately?
>>>
>>
>> The svg files.  I read them into lilypond and generate this informatin.
>
> You mean the SVG fonts?  The SVG files are generated by FF in the same
> step as the OTF files.  I suspect that whatever you need is easier to
> generate in FF directly.
>
> You could ask around on the fontforge mailing list.

Also, nowadays, fontforge supports Python scripting. We could redo all
the craziness involving generated files and legacy .pe scripts in
plain Python.

It would be a nice and meaty project for someone to get their hands
dirty (gsoc, frogs?)

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

[mike_apollinemike.com, 2012-02-18 21:17:04]

I'm relaunching my call for optimization, as the times of the patch are back up
to prohibitive level as I've added more and more grobs into the vertical skyline
calculation (notably flags, which add 3 seconds for every minute of compile
time).  Obviously, the easy solution is to just pick and choose what grobs are
part of the calculation.  Flags, in this scenario, would get voted off the
island, as would some clefs.  However, before doing that, I'd like to have
another go at optimizing the code.

So far, from my experience, here's what doesn't speed things up:
----) Caching files

Here's what does:
----) Moving things to C++

There's still a lot of scheme code in stencil-integrals.cc (scm_foo, scm_bar,
etc.).  I don't know if this could be the reason for the slowdown.  The problem
is that it's hard to get around this, as all of the data is stored in Scheme
structures.

All ideas are appreciated!
~Mike

[janek, 2012-02-22 08:04:26]

Hi,

sorry, Mike - i got confused and forgotten that a patchset newer than what i had
is here, on Rietveld.
I see that you've fixed hairpins and added skylines to accidentals, flags and
beams - great!
Unfortunately I've spotted some problems with DynamicSigns - they are too high
(as of patchset 21).  I'll send you pdf files privately.

As for compilation times, they are better in case of bigger scores:

dynamics.ly:
master 0m0.859s
previous skylines 0m1.520s
skylines from patchset 21: 0m2.994s

Eja-Mater.ly:
master 0m7.373s
previous skylines 0m13.331s
skylines from patchset 21: 0m11.387s

tota-pulchra.ly:
master 0m8.400s
previous skylines 0m15.649s
skylines from patchset 21: 0m15.649s

HTH,
Janek

[janek, 2012-02-22 08:07:59]

On Wed, Feb 22, 2012 at 9:04 AM,  <janek.lilypond@gmail.com> wrote:
> tota-pulchra.ly:
> master 0m8.400s
> previous skylines 0m15.649s
> skylines from patchset 21: 0m15.649s

sorry, this should read 0m12.030s

[mike_apollinemike.com, 2012-02-22 08:18:25]

Hey all,

I've uploaded a first-pass attempt at implementing the suggestion that Joe
talked about (using buildings directly in the skylines).
This saves a lot of time in the calculation of skyline distance (time spent in
Axis_group_interface::skyline_spacing can go from 2 to 0.2 seconds for scores
with lots of beams and hairpins).

However, the global time takes a huge hike with this patchset because skylines
are constantly being rebuilt with padding added on.  I'm going to re-look at all
of the code and see where it is safe to just include padding when the skyline is
built.  This'll bring the compilation times back down - I'll likely have it done
in the next few days (by Monday-ish).

In the meantime, I'm noticing that using buildings to approximate lines only
saves time for line segments of a certain length.  Otherwise, it actually slows
things down.  I'll try to build this distinction into stencil-integral.cc.

Cheers,
MS

[joeneeman, 2012-02-22 09:34:43]

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393
lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis
horizon_axis, Direction sky)
This isn't quite what I had in mind (for one thing, it means that the caller has
to be aware of buildings, calculating their slope, etc.)

what about
Skyline::Skyline (vector<pair<Point, Point> > const& segments, Axis, Direction)?

it works similarly to Skyline::Skyline(vector<Box>...) except that the resulting
skyline shows the outline of the given set of line segments.

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647
lily/skyline.cc:647: out.merge (to_merge);
merge is linear, so this loop is quadratic.

[mike_apollinemike.com, 2012-02-22 15:59:16]

Hey all,

New patch set up w/ more reasonable compilation times.
If you compile \relative c' { d e f g } with -ddebug-skylines, you'll see that
the skylines are asymmetric to the left and right of objects.  I'm trying to
figure out why - any intuition would be appreciated!

Cheers,
MS

[janek, 2012-02-22 16:37:56]

On Wed, Feb 22, 2012 at 4:59 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> New patch set up w/ more reasonable compilation times.

Processing `../dynamics.ly'
master 0m0.830s
skylines 0m1.024s

Processing `../eja-mater/Eja-Mater.ly'
master 0m7.160s
skylines 0m10.878s

Processing `../tota-pulchra/tota-pulchra.ly'
master 0m8.274s
skylines 0m12.140s

As for the output itself: the problem with dynamicSigns still exists.
I've also spotted some strange behavior in beams and hairpins'
skylines, see attachment (it's from the beginning of Tota Pulchra).

HTH,
Janek

[mike_apollinemike.com, 2012-02-22 16:47:48]

Sorry, please ignore the previous patch set.  There's a bad typo that breaks
beams.  I'll post a new one when I'm sure it works 100%.

Cheers,
MS

[dak, 2012-02-22 17:36:24]

"mike@apollinemike.com" <mike@apollinemike.com> writes:

> Sorry, please ignore the previous patch set.

The way to indicate that is to set the status of the issue to
"Patch-needs_work".  This is what tells Patchy to ignore a patch set.  I
am doing this manually for now, but please try to remember doing so in
future.  It is not just information for the automatic checks.

> There's a bad typo that breaks beams.  I'll post a new one when I'm
> sure it works 100%.

-- 
David Kastrup

[mike_apollinemike.com, 2012-02-23 09:35:53]

Hey all,

The newest patch set implements several of Joe's suggestions but takes a time
hit because I am using the fancy skylines for Scripts.  This makes
script-accidental-collision.ly look better (as well as a few other regtests) but
it slows things down even more.

I think that the coding is sound, though, and it is as fast as possible given
what I know how to do.  I could maybe eliminate one or two loops here or there
(i.e. transforming buildings to points and back to buildings for the skyline
constructor that accepts skyline pairs) but this seems like chump change. Each
successive slow down comes from the skylining of a new grob, which is linear
(there's tons of scripts, so tons of subtleties in the skylines).  Of course,
any speed-up suggestions are welcome.

I know that people have suggested being pickier about what grobs have tight
skylines, but I want to include a rather large range, as with each new grob the
regtests look better and better.

Janek - all of the problems you pointed out before should be fixed in this
newest patch set - thank you very much for spotting them.

Cheers,
MS

[joeneeman, 2012-02-23 11:11:27]

On Thu, Feb 23, 2012 at 1:35 AM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

> Hey all,
>
> The newest patch set implements several of Joe's suggestions but takes a
> time hit because I am using the fancy skylines for Scripts.  This makes
> script-accidental-collision.ly look better (as well as a few other
> regtests) but it slows things down even more.
>
> I think that the coding is sound, though, and it is as fast as possible
> given what I know how to do.  I could maybe eliminate one or two loops here
> or there (i.e. transforming buildings to points and back to buildings for
> the skyline constructor that accepts skyline pairs) but this seems like
> chump change. Each successive slow down comes from the skylining of a new
> grob, which is linear (there's tons of scripts, so tons of subtleties in
> the skylines).  Of course, any speed-up suggestions are welcome.
>

I don't see any obvious things to speed up... can you update your git
branch? Then I'll profile it and take a closer look.
Cheers,
Joe

[joeneeman, 2012-02-23 11:11:47]

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362
lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f,
-infinity_f, iv[LEFT] - 2 * horizon_padding));
push_back is constant time for STL lists. No need to push_front and then
reverse.

[MikeSol, 2012-02-23 11:13:42]

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393
lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis
horizon_axis, Direction sky)
On 2012/02/22 09:34:43, joeneeman wrote:
> This isn't quite what I had in mind (for one thing, it means that the caller
has
> to be aware of buildings, calculating their slope, etc.)
> 
> what about
> Skyline::Skyline (vector<pair<Point, Point> > const& segments, Axis,
Direction)?
> 
> it works similarly to Skyline::Skyline(vector<Box>...) except that the
resulting
> skyline shows the outline of the given set of line segments.

Done.

http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647
lily/skyline.cc:647: out.merge (to_merge);
On 2012/02/22 09:34:43, joeneeman wrote:
> merge is linear, so this loop is quadratic.

It should now be n*log(n).

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362
lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f,
-infinity_f, iv[LEFT] - 2 * horizon_padding));
On 2012/02/23 11:11:47, joeneeman wrote:
> push_back is constant time for STL lists. No need to push_front and then
> reverse.

I'm not not in favor of this, but why is there a reverse in the other
non_overlapping_skyline function?  I tried to copy it as closely as possible.

[dak, 2012-02-23 12:29:06]

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#new...
lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
simple_vertical_skylines_from_possibly_transparent_stencil, 1);
Why do we need this?  I think that a _transparent_ stencil is not supposed to
create a different skyline.  That's the point of setting transparency rather
than just #f-ing the stencil.

[MikeSol, 2012-02-23 12:37:04]

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#new...
lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
simple_vertical_skylines_from_possibly_transparent_stencil, 1);
On 2012/02/23 12:29:06, dak wrote:
> Why do we need this?  I think that a _transparent_ stencil is not supposed to
> create a different skyline.  That's the point of setting transparency rather
> than just #f-ing the stencil.

The problem is TabStaff and TabVoice.  In both contexts, many grobs are set to
transparent as a default.  These are grobs that are never supposed to factor
into a vertical skyline.  I'd be interested in seeing what happens if these were
set to ##f instead of transparent, but that'd be a new project.

[dak, 2012-02-23 13:35:29]

On 2012/02/23 12:37:04, MikeSol wrote:
> http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#new...
> lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
> simple_vertical_skylines_from_possibly_transparent_stencil, 1);
> On 2012/02/23 12:29:06, dak wrote:
> > Why do we need this?  I think that a _transparent_ stencil is not supposed
to
> > create a different skyline.  That's the point of setting transparency rather
> > than just #f-ing the stencil.
> 
> The problem is TabStaff and TabVoice.  In both contexts, many grobs are set to
> transparent as a default.  These are grobs that are never supposed to factor
> into a vertical skyline.  I'd be interested in seeing what happens if these
were
> set to ##f instead of transparent, but that'd be a new project.

a) the behavior is expected and consistent
b) http://code.google.com/p/lilypond/issues/detail?id=2085 is fixed and verified

Setting that stuff to "transparent" is merely an ugly workaround used for
historical reasons.  We should not create inconsistent and complex semantics to
half-heartedly support it.

[MikeSol, 2012-02-23 14:09:12]

On 2012/02/23 13:35:29, dak wrote:
> On 2012/02/23 12:37:04, MikeSol wrote:
> > http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
> > File lily/stencil-integral.cc (right):
> > 
> >
>
http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#new...
> > lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
> > simple_vertical_skylines_from_possibly_transparent_stencil, 1);
> > On 2012/02/23 12:29:06, dak wrote:
> > > Why do we need this?  I think that a _transparent_ stencil is not supposed
> to
> > > create a different skyline.  That's the point of setting transparency
rather
> > > than just #f-ing the stencil.
> > 
> > The problem is TabStaff and TabVoice.  In both contexts, many grobs are set
to
> > transparent as a default.  These are grobs that are never supposed to factor
> > into a vertical skyline.  I'd be interested in seeing what happens if these
> were
> > set to ##f instead of transparent, but that'd be a new project.
> 
> a) the behavior is expected and consistent
> b) http://code.google.com/p/lilypond/issues/detail?id=2085 is fixed and
verified
> 
> Setting that stuff to "transparent" is merely an ugly workaround used for
> historical reasons.  We should not create inconsistent and complex semantics
to
> half-heartedly support it.

So, just to be clear, you're saying that the TabVoice/TabStaff
everything-is-transparent-when-it-should-be-##f issue is blocking this patch? 
If so, I'll open up an issue for that and mark this as blocked.

[dak, 2012-02-23 14:36:50]

On 2012/02/23 14:09:12, MikeSol wrote:
> On 2012/02/23 13:35:29, dak wrote:
> > On 2012/02/23 12:37:04, MikeSol wrote:
> > > http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc
> > > File lily/stencil-integral.cc (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/5626052/diff/40001/lily/stencil-integral.cc#new...
> > > lily/stencil-integral.cc:932: MAKE_SCHEME_CALLBACK (Grob,
> > > simple_vertical_skylines_from_possibly_transparent_stencil, 1);
> > > On 2012/02/23 12:29:06, dak wrote:
> > > > Why do we need this?  I think that a _transparent_ stencil is not
supposed
> > to
> > > > create a different skyline.  That's the point of setting transparency
> rather
> > > > than just #f-ing the stencil.
> > > 
> > > The problem is TabStaff and TabVoice.  In both contexts, many grobs are
set
> to
> > > transparent as a default.  These are grobs that are never supposed to
factor
> > > into a vertical skyline.  I'd be interested in seeing what happens if
these
> > were
> > > set to ##f instead of transparent, but that'd be a new project.
> > 
> > a) the behavior is expected and consistent
> > b) http://code.google.com/p/lilypond/issues/detail?id=2085 is fixed and
> verified
> > 
> > Setting that stuff to "transparent" is merely an ugly workaround used for
> > historical reasons.  We should not create inconsistent and complex semantics
> to
> > half-heartedly support it.
> 
> So, just to be clear, you're saying that the TabVoice/TabStaff
> everything-is-transparent-when-it-should-be-##f issue is blocking this patch? 
> If so, I'll open up an issue for that and mark this as blocked.

No, it isn't blocking this patch.  Ugly before -> ugly afterwards is not a
regression.  In particular not Ugly before because of historically motivated
user-code level workarounds with expected effects -> ugly afterwards because of
historically motivated user-code level workarounds with expected effects.

One can't even call them "sideeffects".  You can open an issue for fixing all
the "transparent" examples in our code base and possibly detecting more cases
where #f does not work as expected.

But it is not blocking this patch.  It is orthogonal to this patch, and you
should keep it that way instead of complicating your patch by introducing weird
stuff for fixing an unrelated problem in the user-code level code base.

[Neil Puttock, 2012-02-23 17:22:06]

Hi Mike,

I've tested the latest patch on a few real-world scores and there appears to be
a bit of weirdness going on with TupletNumbers when a DynamicText's nearby:

\relative c' {
  \times 2/3 { c8^\p c c }
}

Cheers,
Neil

[joeneeman, 2012-02-23 20:47:58]

On Thu, Feb 23, 2012 at 3:13 AM, <mtsolo@gmail.com> wrote:

>
>
http://codereview.appspot.com/**5626052/diff/30003/lily/**skyline.cc<http://c...
> File lily/skyline.cc (right):
>
> http://codereview.appspot.com/**5626052/diff/30003/lily/**
>
skyline.cc#newcode393<http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode393>
> lily/skyline.cc:393: Skyline::Skyline (Building b, Real start, Axis
> horizon_axis, Direction sky)
> On 2012/02/22 09:34:43, joeneeman wrote:
>
>> This isn't quite what I had in mind (for one thing, it means that the
>>
> caller has
>
>> to be aware of buildings, calculating their slope, etc.)
>>
>
>  what about
>> Skyline::Skyline (vector<pair<Point, Point> > const& segments, Axis,
>>
> Direction)?
>
>  it works similarly to Skyline::Skyline(vector<Box>..**.) except that the
>>
> resulting
>
>> skyline shows the outline of the given set of line segments.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5626052/diff/30003/lily/**
>
skyline.cc#newcode647<http://codereview.appspot.com/5626052/diff/30003/lily/skyline.cc#newcode647>
> lily/skyline.cc:647: out.merge (to_merge);
> On 2012/02/22 09:34:43, joeneeman wrote:
>
>> merge is linear, so this loop is quadratic.
>>
>
> It should now be n*log(n).
>
>
>
http://codereview.appspot.com/**5626052/diff/34001/lily/**skyline.cc<http://c...
> File lily/skyline.cc (right):
>
> http://codereview.appspot.com/**5626052/diff/34001/lily/**
>
skyline.cc#newcode362<http://codereview.appspot.com/5626052/diff/34001/lily/skyline.cc#newcode362>
> lily/skyline.cc:362: result.push_front (Building (last_end, -infinity_f,
> -infinity_f, iv[LEFT] - 2 * horizon_padding));
> On 2012/02/23 11:11:47, joeneeman wrote:
>
>> push_back is constant time for STL lists. No need to push_front and
>>
> then
>
>> reverse.
>>
>
> I'm not not in favor of this, but why is there a reverse in the other
> non_overlapping_skyline function?


Good question...

[janek, 2012-02-23 23:07:47]

Hey Mike,

On Thu, Feb 23, 2012 at 10:35 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Janek - all of the problems you pointed out before should be fixed in this
newest patch set - thank you very much for spotting them.

Actually, there's one problem left: dynamic texts aren't skylined,
their outlines are still rectangles.  See attachment.

thanks,
Janek

[mike_apollinemike.com, 2012-02-23 23:50:16]

On Feb 24, 2012, at 12:07 AM, Janek Warchoł wrote:

> Hey Mike,
> 
> On Thu, Feb 23, 2012 at 10:35 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> Janek - all of the problems you pointed out before should be fixed in this
newest patch set - thank you very much for spotting them.
> 
> Actually, there's one problem left: dynamic texts aren't skylined,
> their outlines are still rectangles.  See attachment.
> 
> thanks,
> Janek

Hey Janek,

The issue is that dynamics are created via Pango and thus I can only use
rectangles for the time being.  A separate issue in the tracker can be opened
for redoing dynamics so that they used a series of named-glyph stencils.

Cheers,
MS

[janek, 2012-02-26 12:09:08]

On Fri, Feb 24, 2012 at 12:50 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Feb 24, 2012, at 12:07 AM, Janek Warchoł wrote:
>> dynamic texts aren't skylined, their outlines are still rectangles.
>
> The issue is that dynamics are created via Pango and thus I can only use
rectangles
> for the time being.  A separate issue in the tracker can be opened for redoing
dynamics
> so that they used a series of named-glyph stencils.

Hmm.  But what about custom dynamics created using
make-dynamic-script?
(http://www.lilypond.org/doc/v2.15/Documentation/notation/expressive-marks-att...)
I mean, isn't Pango necessary to create them?  If so, it seems to me
that what you suggest will result in having two types of dynamics:
simple stencil dynamics and advanced Pango dynamics.  This doesn't
sound good to me.
I think the question is: should we rather try to "break inside Pango"
to be able to create precise outlines for everything that is processed
by Pango?  The advantage of this approach would be that lyrics and
markups would be skylined even better.

cheers,
Janek

[mike_apollinemike.com, 2012-02-26 15:51:20]

On Feb 26, 2012, at 1:08 PM, Janek Warchoł wrote:

> On Fri, Feb 24, 2012 at 12:50 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> On Feb 24, 2012, at 12:07 AM, Janek Warchoł wrote:
>>> dynamic texts aren't skylined, their outlines are still rectangles.
>> 
>> The issue is that dynamics are created via Pango and thus I can only use
rectangles
>> for the time being.  A separate issue in the tracker can be opened for
redoing dynamics
>> so that they used a series of named-glyph stencils.
> 
> should we rather try to "break inside Pango"
> to be able to create precise outlines for everything that is processed
> by Pango?  The advantage of this approach would be that lyrics and
> markups would be skylined even better.
> 

I'd like to do this...I'm just not sure if it's possible.  Joe (or someone who
knows fonts) - is there a way to get font outlines of pango fonts?

Cheers,
MS

[dak, 2012-02-26 15:54:46]

"mike@apollinemike.com" <mike@apollinemike.com> writes:

> On Feb 26, 2012, at 1:08 PM, Janek Warchoł wrote:
>
>> On Fri, Feb 24, 2012 at 12:50 AM, mike@apollinemike.com
>> <mike@apollinemike.com> wrote:
>>> On Feb 24, 2012, at 12:07 AM, Janek Warchoł wrote:
>>>> dynamic texts aren't skylined, their outlines are still rectangles.
>>> 
>>> The issue is that dynamics are created via Pango and thus I can
>>> only use rectangles
>>> for the time being.  A separate issue in the tracker can be opened
>>> for redoing dynamics
>>> so that they used a series of named-glyph stencils.
>> 
>> should we rather try to "break inside Pango"
>> to be able to create precise outlines for everything that is processed
>> by Pango?  The advantage of this approach would be that lyrics and
>> markups would be skylined even better.
>> 
>
> I'd like to do this...I'm just not sure if it's possible.  Joe (or
> someone who knows fonts) - is there a way to get font outlines of
> pango fonts?

The reason we are using Pango in the first place is because it is good
at composing Unicode (with kerning, ligatures, accents, writing
direction etc etc).  So we don't want the outlines of its fonts but
rather the outlines of the composed result.

Not that I would have a clue about the answer to either your question or
the changed question.

-- 
David Kastrup

[wl_gnu.org, 2012-02-26 18:37:35]

>>> should we rather try to "break inside Pango" to be able to create
>>> precise outlines for everything that is processed by Pango?  The
>>> advantage of this approach would be that lyrics and markups would
>>> be skylined even better.
> 
> The reason we are using Pango in the first place is because it is
> good at composing Unicode (with kerning, ligatures, accents, writing
> direction etc etc).  So we don't want the outlines of its fonts but
> rather the outlines of the composed result.

AFAIK, Pango returns a list of glyph indices and coordinates to
position glyphs.  It's straightforward to feed those indices directly
into FreeType's `FT_Load_Glyph' function which is able to return an
`FT_Outline' structure holding the glyph's outline.


    Werner

[mike_apollinemike.com, 2012-02-26 19:25:53]

On Feb 26, 2012, at 7:37 PM, Werner LEMBERG wrote:
> 
> AFAIK, Pango returns a list of glyph indices and coordinates to
> position glyphs.  It's straightforward to feed those indices directly
> into FreeType's `FT_Load_Glyph' function which is able to return an
> `FT_Outline' structure holding the glyph's outline.
> 

This is great Werner - thank you!  I just checked out FT_Outline.  It's really
straightforward, which is great.
Joe - this likely means that the build system work you were thinking about doing
w/ glyph outlines is no longer necessary and that I can scrap all of the .scm
stuff I wrote and replace it with calls to this.  I'll have time Wednesday
morning to check this out.

Cheers,
MS

[joeneeman, 2012-02-26 21:28:25]

On Sun, Feb 26, 2012 at 11:25 AM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

> On Feb 26, 2012, at 7:37 PM, Werner LEMBERG wrote:
>
>
> AFAIK, Pango returns a list of glyph indices and coordinates to
> position glyphs.  It's straightforward to feed those indices directly
> into FreeType's `FT_Load_Glyph' function which is able to return an
> `FT_Outline' structure holding the glyph's outline.
>
>
> This is great Werner - thank you!  I just checked out FT_Outline.  It's
> really straightforward, which is great.
> Joe - this likely means that the build system work you were thinking about
> doing w/ glyph outlines is no longer necessary and that I can scrap all of
> the .scm stuff I wrote and replace it with calls to this.  I'll have time
> Wednesday morning to check this out.
>

Sounds good!

[janek, 2012-02-26 22:16:34]

On Sun, Feb 26, 2012 at 8:25 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Feb 26, 2012, at 7:37 PM, Werner LEMBERG wrote:
>> AFAIK, Pango returns a list of glyph indices and coordinates to
>> position glyphs.  It's straightforward to feed those indices directly
>> into FreeType's `FT_Load_Glyph' function which is able to return an
>> `FT_Outline' structure holding the glyph's outline.
>
> This is great Werner - thank you!  I just checked out FT_Outline.  It's
> really straightforward, which is great.

Wonderful!

> Joe - this likely means that the build system work you were thinking about
> doing w/ glyph outlines is no longer necessary and that I can scrap all of
> the .scm stuff I wrote and replace it with calls to this.  I'll have time
> Wednesday morning to check this out.

I can't wait!  That's incredible!

cheers,
Janek

[mike_apollinemike.com, 2012-02-28 12:32:22]

Hey all,

I have a new patch up that simplifies the old one a great deal and uses freetype
to get outlines.

The only problem with it (that I know of) exists for some non-standard fonts. 
If you run input/regression/utf-8.ly through this, you'll see LilyPond get very
angry about not being able to find the glyph in the font.  This is because I am
stashing a font at creation time in pango-font.cc (check out the constructor -
you'll see a new member font_ being set) instead of extracting the font from the
glyphs.  In certain cases, this means that the FT_Face created in the
pango_item_string_stencil of my patch is different from that created in current
master.  I have no clue why this is the case - does anyone have any intuition
about this?

Cheers,
MS

[mike_apollinemike.com, 2012-02-28 21:33:06]

Hey all,

I've put up a new version that doesn't issue warnings.
The only problem I'm running into now is that the outlines aren't always 100%
exact.  This causes really close kerning and sometimes even overlap (check out
figured-bass-implicit.ly).  An easy (albeit kludgy) solution would be to add a
bit of padding around all glyphs that come from a pango font.  Does this seem
like a reasonable way of going about it?

Cheers,
MS

[janek, 2012-02-28 22:16:44]

Hey Mike,

there's a funny situation: seems that DynamicText behaves like
Hairpins behaved at one moment - DynamicText skylines work with some
objects, and don't work with other.  see attachments.

On Tue, Feb 28, 2012 at 10:32 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> The only problem I'm running into now is that the outlines aren't always 100%
exact.

Yes, for example letters t and l (see attachment)

> This causes really close kerning and sometimes even overlap (check out
figured-bass-implicit.ly).
>  An easy (albeit kludgy) solution would be to add a bit of padding
> around all glyphs that come from a pango font.  Does this seem
> like a reasonable way of going about it?

No; it won't fix problems with letter t and trill, and the outline of
letters l, h etc. will still not be symmetrical.

HTH,
Janek

[joeneeman, 2012-02-29 01:05:54]

On Tue, Feb 28, 2012 at 1:32 PM, mike@apollinemike.com <
mike@apollinemike.com> wrote:

> Hey all,
>
> I've put up a new version that doesn't issue warnings.
>

Any change of getting an updated git branch, too?

[mike_apollinemike.com, 2012-02-29 10:17:55]

On Feb 29, 2012, at 2:05 AM, Joe Neeman wrote:

> On Tue, Feb 28, 2012 at 1:32 PM, mike@apollinemike.com <mike@apollinemike.com>
wrote:
> Hey all,
> 
> I've put up a new version that doesn't issue warnings.
> 
> Any change of getting an updated git branch, too?

Done.

A lot of the too-close-spacing issues are now cleared up.

Cheers,
MS

[janek, 2012-03-01 00:01:25]

Hey Mike,

I did a quick check of newest patchset (30) and the results look
great!  There are some flaws, but i'd say they're not your patch's
fault.  Rather, it seems that the new super-precise skylines unveil
imperfections in vertical spacing code.
The bad news is that for scores containing a lot of lyrics (like my
SATB pieces) compilation times are now 3-4 times longer than wiith
master.  For instrumental scores the situation look better, it's 1.5-2
times longer.  I can live with these compilation times, but others
probably won't like it...

cheers,
Janek

[mike_apollinemike.com, 2012-03-01 07:14:50]

On Mar 1, 2012, at 1:01 AM, Janek Warchoł wrote:

> Hey Mike,
> 
> I did a quick check of newest patchset (30) and the results look
> great!  There are some flaws, but i'd say they're not your patch's
> fault.  Rather, it seems that the new super-precise skylines unveil
> imperfections in vertical spacing code.

I'm interested to see what they are - send them over if you can!

> The bad news is that for scores containing a lot of lyrics (like my
> SATB pieces) compilation times are now 3-4 times longer than wiith
> master.  For instrumental scores the situation look better, it's 1.5-2
> times longer.  I can live with these compilation times, but others
> probably won't like it...
> 

The precision takes a big toll.  I'm not sure the best places to speed up the
code.  Perhaps caching the FT_Face for Pango_font objects?  I'd need help from
others to help me spot places to speed this up.

Cheers,
MS

[janek, 2012-03-01 07:53:01]

On Thu, Mar 1, 2012 at 8:14 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Mar 1, 2012, at 1:01 AM, Janek Warchoł wrote:
>> I did a quick check of newest patchset (30) and the results look
>> great!  There are some flaws, but i'd say they're not your patch's
>> fault.  Rather, it seems that the new super-precise skylines unveil
>> imperfections in vertical spacing code.
>
> I'm interested to see what they are - send them over if you can!

Sure.  Files are big, so i'll send them in private mail.

>> The bad news is that for scores containing a lot of lyrics (like my
>> SATB pieces) compilation times are now 3-4 times longer than wiith
>> master.  For instrumental scores the situation look better, it's 1.5-2
>> times longer.  I can live with these compilation times, but others
>> probably won't like it...
>
> The precision takes a big toll.  I'm not sure the best places to speed up the
code.  Perhaps caching the FT_Face for Pango_font objects?  I'd need help from
others to help me spot places to speed this up.

From what i see, the skylines are now more precise than they need to
be - every glyph has a skyline of 10 or so boxes, even if it's a
single letter! (see attached)
I think the proper solution would be to:
a) set minimal "step" size to 0.2 staffspace (or more in case of bigger objects)
b) change outlines from "stairs" to glued lines (what Joe suggested).
This would allow for even less "fragments" for each skyline.

maybe caching will help, too.

cheers,
Janek

[wl_gnu.org, 2012-03-01 08:09:17]

> From what i see, the skylines are now more precise than they need to
> be - every glyph has a skyline of 10 or so boxes, even if it's a
> single letter! (see attached)

I second this.

> I think the proper solution would be to:
> a) set minimal "step" size to 0.2 staffspace (or more in case of
> bigger objects)
> b) change outlines from "stairs" to glued lines (what Joe suggested).
> This would allow for even less "fragments" for each skyline.

I still think that the `bounding outline' should be defined in the
Metafont code for Emmentaler glyphs.  However, this should be rather
easy to add later on in case we try this route.

Caching glyph outlines certainly helps, BTW.


    Werner

[hanwenn, 2012-03-01 10:52:00]

2012/3/1 Janek Warchoł <janek.lilypond@gmail.com>:

> From what i see, the skylines are now more precise than they need to
> be - every glyph has a skyline of 10 or so boxes, even if it's a
> single letter! (see attached)
> I think the proper solution would be to:
> a) set minimal "step" size to 0.2 staffspace (or more in case of bigger
objects)
> b) change outlines from "stairs" to glued lines (what Joe suggested).
> This would allow for even less "fragments" for each skyline.

It's neat that you are generating such precise skylines, but can you
show places where this makes an appreciable difference for texts?

You could look into some heuristic that limits the number of boxes
depending on their shapes, so it creates a single box for most glyphs.

For example, you could take the box enclosing the glyph and compare
its area with that of the union of the boxes, and revert to one box if
the difference is less than X percent.

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

[dak, 2012-03-01 10:57:19]

Han-Wen Nienhuys <hanwenn@gmail.com> writes:

> 2012/3/1 Janek Warchoł <janek.lilypond@gmail.com>:
>
>> From what i see, the skylines are now more precise than they need to
>> be - every glyph has a skyline of 10 or so boxes, even if it's a
>> single letter! (see attached)
>> I think the proper solution would be to:
>> a) set minimal "step" size to 0.2 staffspace (or more in case of
>> bigger objects)
>> b) change outlines from "stairs" to glued lines (what Joe suggested).
>> This would allow for even less "fragments" for each skyline.
>
> It's neat that you are generating such precise skylines, but can you
> show places where this makes an appreciable difference for texts?

Well, it is again an issue of "incest tabu" where the details of the
combining skylines make best sense for combining _heterogenous_
elements: for arranging text with text, you don't want to have things
get too closely or even interleaved.  But having a single high letter
"interlock" with a note stem can improve the overall arrangement.

-- 
David Kastrup

[hanwenn, 2012-03-01 10:59:02]

On Thu, Mar 1, 2012 at 7:57 AM, David Kastrup <dak@gnu.org> wrote:

>>> From what i see, the skylines are now more precise than they need to
>>> be - every glyph has a skyline of 10 or so boxes, even if it's a
>>> single letter! (see attached)
>>> I think the proper solution would be to:
>>> a) set minimal "step" size to 0.2 staffspace (or more in case of
>>> bigger objects)
>>> b) change outlines from "stairs" to glued lines (what Joe suggested).
>>> This would allow for even less "fragments" for each skyline.
>>
>> It's neat that you are generating such precise skylines, but can you
>> show places where this makes an appreciable difference for texts?
>
> Well, it is again an issue of "incest tabu" where the details of the
> combining skylines make best sense for combining _heterogenous_
> elements: for arranging text with text, you don't want to have things
> get too closely or even interleaved.  But having a single high letter
> "interlock" with a note stem can improve the overall arrangement.

Exactly my point: you can fix the single letter case with a single
bbox per glyph.  Why do you need more accuracy than single box per
glyph?

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

[mike_apollinemike.com, 2012-03-01 11:00:16]

On Mar 1, 2012, at 11:52 AM, Han-Wen Nienhuys wrote:

> 2012/3/1 Janek Warchoł <janek.lilypond@gmail.com>:
> 
>> From what i see, the skylines are now more precise than they need to
>> be - every glyph has a skyline of 10 or so boxes, even if it's a
>> single letter! (see attached)
>> I think the proper solution would be to:
>> a) set minimal "step" size to 0.2 staffspace (or more in case of bigger
objects)
>> b) change outlines from "stairs" to glued lines (what Joe suggested).
>> This would allow for even less "fragments" for each skyline.
> 
> It's neat that you are generating such precise skylines, but can you
> show places where this makes an appreciable difference for texts?
> 

Janek had sent out a couple e-mails before (I can't find them).  It makes a
significant difference for DynamicText, and I know he has a few examples of
lyrics as well where it makes a difference.  David's also right about the single
high letter and note stem (this was one of Janek's examples).

> You could look into some heuristic that limits the number of boxes
> depending on their shapes, so it creates a single box for most glyphs.
> 

I agree - there's a lot of room for heuristicking.

> For example, you could take the box enclosing the glyph and compare
> its area with that of the union of the boxes, and revert to one box if
> the difference is less than X percent.
> 

Remembering that 81.2% of all statistics are fabricated on the spot, I'd say
that 70% of speed-up work could be done in skyline.cc.  I think
stencil-integral.cc is as fast as its gonna get.  It may help to cache a FT_Face
in the Pango_font class as I'm not sure how long it takes to look it up.  I'm
not convinced after testing that caching font outlines significantly speeds
things up - there used to be some code for this, but I've removed it.  It can be
put back in later if it turns out that after profiling the calls to the
functions in freetype.cc sap a lot of time.

What I really need help on is profiling.  People have so far offered great
suggestions on how to do it, but I'm not too good at reading the summaries from
profilers to know exactly where the resources are being consumed.  If someone
sent me a really detailed chart with what resources were consumed on what,
that'd help immensely.  The only thing that was clear to me was that LilyPond is
spending a lot of time in skyline-related functions, but I'm not sure which
ones.  The skyline class in general looks like it needs a look-over from someone
who knows something about CS and optimization.

Cheers,
MS

[mike_apollinemike.com, 2012-03-01 11:00:36]

On Mar 1, 2012, at 11:59 AM, Han-Wen Nienhuys wrote:

>> 
>> Well, it is again an issue of "incest tabu" where the details of the
>> combining skylines make best sense for combining _heterogenous_
>> elements: for arranging text with text, you don't want to have things
>> get too closely or even interleaved.  But having a single high letter
>> "interlock" with a note stem can improve the overall arrangement.
> 
> Exactly my point: you can fix the single letter case with a single
> bbox per glyph.  Why do you need more accuracy than single box per
> glyph?
> 

DynamicText is done as a glyph-string.

Cheers,
MS

[t.daniels_treda.co.uk, 2012-03-01 11:17:31]

Janek Warchoł wrote Thursday, March 01, 2012 12:01 AM

> The bad news is that for scores containing a lot of lyrics (like my
> SATB pieces) compilation times are now 3-4 times longer than wiith
> master.  For instrumental scores the situation look better, it's 1.5-2
> times longer.  I can live with these compilation times, but others
> probably won't like it...

I wouldn't like it for one.  Could either the precise skylines for glyphs
be optionally activated, or applied only to dynamic text, not lyrics?

Trevor

[mike_apollinemike.com, 2012-03-01 11:28:01]

On Mar 1, 2012, at 12:17 PM, Trevor Daniels wrote:

> 
> Janek Warchoł wrote Thursday, March 01, 2012 12:01 AM
> 
>> The bad news is that for scores containing a lot of lyrics (like my
>> SATB pieces) compilation times are now 3-4 times longer than wiith
>> master.  For instrumental scores the situation look better, it's 1.5-2
>> times longer.  I can live with these compilation times, but others
>> probably won't like it...
> 
> I wouldn't like it for one.  Could either the precise skylines for glyphs
> be optionally activated, or applied only to dynamic text, not lyrics?
> 

The good news is that a simple property activation causes this to kick in, so
before pushing the patch, we can decide what does and doesn't get this feature
as default behavior.  Furthermore, we can create a file precision-spacing.ly
that sets tons of grobs to the more time-costly alternative for those who want
to put their finished score in a final pass through the vertical-skyline
functions.

Now, though, it's important to develop the with everything at maximum complexity
to see what type of toll it takes and get as many bugs out as possible.  I still
think there's a lot of room for speed up - as I said in my previous e-mail, I
just need help with some profiling.

Cheers,
MS

[janek, 2012-03-01 20:56:22]

On Thu, Mar 1, 2012 at 12:00 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Mar 1, 2012, at 11:52 AM, Han-Wen Nienhuys wrote:
>> It's neat that you are generating such precise skylines, but can you
>> show places where this makes an appreciable difference for texts?
>>
>
> Janek had sent out a couple e-mails before (I can't find them).
> It makes a significant difference for DynamicText, and I know
> he has a few examples of lyrics as well where it makes a difference.
> David's also right about the single high letter and note stem
> (this was one of Janek's examples).

I attach sources of my test files.  I will send pdfs compiled with
both master and Mike's patch, with some of the differences
highlighted, privately (4 snippets and 2 real-life scores, 4 MB).
I hope you'll like it :)

thanks,
Janek

[janek, 2012-03-06 14:18:03]

Mike & all,

i did a quick compile with patchset 36 and unfortunately didn't notice
significant speedups from previous version.

Compilation time differences vary widely depending on music (from 5% to 100%
longer)


PROCESSING  ../dynamics.ly  WITH MASTER: --------------

real	0m0.762s
user	0m0.656s
sys	0m0.076s
 
PROCESSING  ../dynamics.ly  WITH SKYLINES: ------------

real	0m0.799s
user	0m0.692s
sys	0m0.080s
 
 
PROCESSING  ../eja-mater/Eja-Mater.ly  WITH MASTER: --------------

real	0m7.324s
user	0m6.488s
sys	0m0.528s
 
PROCESSING  ../eja-mater/Eja-Mater.ly  WITH SKYLINES: ------------

real	0m14.040s
user	0m12.185s
sys	0m1.132s
 
 
PROCESSING  ../Epifania/Eja_Mater_klarnet.ly  WITH MASTER: --------------

real	0m2.462s
user	0m2.120s
sys	0m0.192s
 
PROCESSING  ../Epifania/Eja_Mater_klarnet.ly  WITH SKYLINES: ------------

real	0m3.314s
user	0m2.888s
sys	0m0.256s
 
 
PROCESSING  ../Epifania/Gabriel's_Oboe/choir.ly  WITH MASTER: --------------

real	0m2.539s
user	0m2.200s
sys	0m0.200s
 
PROCESSING  ../Epifania/Gabriel's_Oboe/choir.ly  WITH SKYLINES: ------------

real	0m4.081s
user	0m3.460s
sys	0m0.344s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/choir.ly  WITH MASTER: --------------

real	0m5.468s
user	0m4.848s
sys	0m0.348s
 
PROCESSING  ../Epifania/He_trusted_in_God/choir.ly  WITH SKYLINES: ------------

real	0m11.347s
user	0m9.621s
sys	0m0.988s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/continuo.ly  WITH MASTER:
--------------

real	0m2.201s
user	0m1.920s
sys	0m0.184s
 
PROCESSING  ../Epifania/He_trusted_in_God/continuo.ly  WITH SKYLINES:
------------

real	0m2.440s
user	0m2.168s
sys	0m0.176s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/full_score.ly  WITH MASTER:
--------------

real	0m8.185s
user	0m7.492s
sys	0m0.412s
 
PROCESSING  ../Epifania/He_trusted_in_God/full_score.ly  WITH SKYLINES:
------------

real	0m14.932s
user	0m12.901s
sys	0m1.248s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/viola.ly  WITH MASTER: --------------

real	0m1.779s
user	0m1.552s
sys	0m0.164s
 
PROCESSING  ../Epifania/He_trusted_in_God/viola.ly  WITH SKYLINES: ------------

real	0m1.970s
user	0m1.748s
sys	0m0.136s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/violin_I.ly  WITH MASTER:
--------------

real	0m1.582s
user	0m1.372s
sys	0m0.152s
 
PROCESSING  ../Epifania/He_trusted_in_God/violin_I.ly  WITH SKYLINES:
------------

real	0m1.796s
user	0m1.536s
sys	0m0.184s
 
 
PROCESSING  ../Epifania/He_trusted_in_God/violin_II.ly  WITH MASTER:
--------------

real	0m1.699s
user	0m1.520s
sys	0m0.124s
 
PROCESSING  ../Epifania/He_trusted_in_God/violin_II.ly  WITH SKYLINES:
------------

real	0m1.954s
user	0m1.708s
sys	0m0.172s
 
 
PROCESSING  ../Epifania/Jesus_bleibet_meine_Freude/choir.ly  WITH MASTER:
--------------

real	0m2.416s
user	0m2.152s
sys	0m0.140s
 
PROCESSING  ../Epifania/Jesus_bleibet_meine_Freude/choir.ly  WITH SKYLINES:
------------

real	0m4.625s
user	0m3.616s
sys	0m0.468s
 
 
PROCESSING  ../Epifania/Jesus_bleibet_meine_Freude/full_score.ly  WITH MASTER:
--------------

real	0m0.684s
user	0m0.580s
sys	0m0.056s
 
PROCESSING  ../Epifania/Jesus_bleibet_meine_Freude/full_score.ly  WITH SKYLINES:
------------

real	0m0.680s
user	0m0.552s
sys	0m0.068s
 
 
PROCESSING 
../Epifania/Nad_Betlejem_w_ciemną_noc/Nad_Betlejem_w._orkiestrowa_3_systemy.ly 
WITH MASTER: --------------

real	0m2.039s
user	0m1.700s
sys	0m0.152s
 
PROCESSING 
../Epifania/Nad_Betlejem_w_ciemną_noc/Nad_Betlejem_w._orkiestrowa_3_systemy.ly 
WITH SKYLINES: ------------

real	0m3.326s
user	0m2.820s
sys	0m0.260s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/choir.ly  WITH MASTER: --------------

real	0m4.176s
user	0m3.688s
sys	0m0.296s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/choir.ly  WITH SKYLINES: ------------

real	0m8.384s
user	0m7.144s
sys	0m0.780s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/continuo.ly  WITH MASTER:
--------------

real	0m0.861s
user	0m0.752s
sys	0m0.068s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/continuo.ly  WITH SKYLINES:
------------

real	0m0.873s
user	0m0.740s
sys	0m0.076s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/full_score.ly  WITH MASTER:
--------------

real	0m4.027s
user	0m3.652s
sys	0m0.248s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/full_score.ly  WITH SKYLINES:
------------

real	0m8.267s
user	0m7.156s
sys	0m0.668s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/viola.ly  WITH MASTER: --------------

real	0m0.878s
user	0m0.768s
sys	0m0.072s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/viola.ly  WITH SKYLINES: ------------

real	0m0.869s
user	0m0.772s
sys	0m0.060s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/violin_I.ly  WITH MASTER:
--------------

real	0m0.880s
user	0m0.756s
sys	0m0.084s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/violin_I.ly  WITH SKYLINES:
------------

real	0m0.938s
user	0m0.760s
sys	0m0.132s
 
 
PROCESSING  ../Epifania/O_Matko_miłościwa/violin_II.ly  WITH MASTER:
--------------

real	0m0.857s
user	0m0.772s
sys	0m0.048s
 
PROCESSING  ../Epifania/O_Matko_miłościwa/violin_II.ly  WITH SKYLINES:
------------

real	0m0.876s
user	0m0.760s
sys	0m0.072s
 
 
PROCESSING  ../hairpins.ly  WITH MASTER: --------------

real	0m0.802s
user	0m0.688s
sys	0m0.088s
 
PROCESSING  ../hairpins.ly  WITH SKYLINES: ------------

real	0m0.857s
user	0m0.768s
sys	0m0.060s
 
 
PROCESSING  ../lyrics.ly  WITH MASTER: --------------

real	0m0.838s
user	0m0.716s
sys	0m0.088s
 
PROCESSING  ../lyrics.ly  WITH SKYLINES: ------------

real	0m1.021s
user	0m0.872s
sys	0m0.108s
 
 
PROCESSING  ../psalm94-reubke/psalm94.ly  WITH MASTER: --------------

real	0m47.185s
user	0m43.411s
sys	0m2.104s
 
PROCESSING  ../psalm94-reubke/psalm94.ly  WITH SKYLINES: ------------

real	1m21.064s
user	0m59.220s
sys	0m4.160s
 
 
PROCESSING  ../slanted-trillspan.ly  WITH MASTER: --------------
Unbound variable: ly:grob::vertical-skylines-from-stencil

real	0m2.856s
user	0m0.748s
sys	0m0.176s
 
PROCESSING  ../slanted-trillspan.ly  WITH SKYLINES: ------------

real	0m1.589s
user	0m0.864s
sys	0m0.132s
 
 
PROCESSING  ../tota-pulchra/tota-pulchra.ly  WITH MASTER: --------------

real	0m8.127s
user	0m7.368s
sys	0m0.428s
 
PROCESSING  ../tota-pulchra/tota-pulchra.ly  WITH SKYLINES: ------------

real	0m14.581s
user	0m12.701s
sys	0m1.208s

[joeneeman, 2012-03-06 22:48:39]

2012/3/6 <janek.lilypond@gmail.com>

> Mike & all,
>
> i did a quick compile with patchset 36 and unfortunately didn't notice
> significant speedups from previous version.
>

Could you try the dev/jneem branch in git? It has some optimizations. If
that doesn't help, could you please send me some of the worst files?

Cheers,
Joe

[janek, 2012-03-07 07:56:46]

On Tue, Mar 6, 2012 at 11:48 PM, Joe Neeman <joeneeman@gmail.com> wrote:
> 2012/3/6 <janek.lilypond@gmail.com>
>
>> Mike & all,
>>
>> i did a quick compile with patchset 36 and unfortunately didn't notice
>> significant speedups from previous version.
>
>
> Could you try the dev/jneem branch in git? It has some optimizations. If
> that doesn't help, could you please send me some of the worst files?

They look better indeed time-wise.
As for the output, i've only did a quick look and noticed this: why
you deleted horizontal padding?  This results in mf at the bottom
right of 1st page of Tota pulchra to jump into above lyrics.
Actually, small/zero horizontal padding might give better results in
some places, but that needs thorough investigation.

Compilation times:

PROCESSING  ../dynamics.ly  WITH MASTER: --------------

real	0m0.752s
user	0m0.632s
sys	0m0.096s

PROCESSING  ../dynamics.ly  WITH SKYLINES: ------------

real	0m1.002s
user	0m0.656s
sys	0m0.092s


PROCESSING  ../eja-mater/Eja-Mater.ly  WITH MASTER: --------------

real	0m7.195s
user	0m6.320s
sys	0m0.456s

PROCESSING  ../eja-mater/Eja-Mater.ly  WITH SKYLINES: ------------

real	0m10.792s
user	0m9.145s
sys	0m0.816s


PROCESSING  ../hairpins.ly  WITH MASTER: --------------

real	0m1.100s
user	0m0.772s
sys	0m0.072s

PROCESSING  ../hairpins.ly  WITH SKYLINES: ------------

real	0m0.866s
user	0m0.772s
sys	0m0.064s


PROCESSING  ../lyrics.ly  WITH MASTER: --------------

real	0m0.906s
user	0m0.752s
sys	0m0.084s

PROCESSING  ../lyrics.ly  WITH SKYLINES: ------------

real	0m0.955s
user	0m0.824s
sys	0m0.100s


PROCESSING  ../psalm94-reubke/psalm94.ly  WITH MASTER: --------------

real	0m46.639s
user	0m42.651s
sys	0m2.232s

PROCESSING  ../psalm94-reubke/psalm94.ly  WITH SKYLINES: ------------
^C
janek@lilydev2 ~$ ./skytest

PROCESSING  ../dynamics.ly  WITH MASTER: --------------

real	0m0.760s
user	0m0.652s
sys	0m0.076s

PROCESSING  ../dynamics.ly  WITH SKYLINES: ------------

real	0m0.807s
user	0m0.692s
sys	0m0.080s


PROCESSING  ../eja-mater/Eja-Mater.ly  WITH MASTER: --------------

real	0m7.295s
user	0m6.408s
sys	0m0.528s

PROCESSING  ../eja-mater/Eja-Mater.ly  WITH SKYLINES: ------------

real	0m10.482s
user	0m9.161s
sys	0m0.800s


PROCESSING  ../hairpins.ly  WITH MASTER: --------------

real	0m0.830s
user	0m0.736s
sys	0m0.064s

PROCESSING  ../hairpins.ly  WITH SKYLINES: ------------

real	0m0.811s
user	0m0.696s
sys	0m0.088s


PROCESSING  ../lyrics.ly  WITH MASTER: --------------

real	0m0.852s
user	0m0.704s
sys	0m0.116s

PROCESSING  ../lyrics.ly  WITH SKYLINES: ------------

real	0m0.968s
user	0m0.844s
sys	0m0.084s


PROCESSING  ../psalm94-reubke/psalm94.ly  WITH MASTER: --------------

real	0m45.944s
user	0m42.799s
sys	0m2.144s

PROCESSING  ../psalm94-reubke/psalm94.ly  WITH SKYLINES: ------------

real	1m9.338s
user	0m48.959s
sys	0m3.320s


PROCESSING  ../slanted-trillspan.ly  WITH MASTER: --------------
Unbound variable: ly:grob::vertical-skylines-from-stencil

real	0m2.852s
user	0m0.724s
sys	0m0.176s

PROCESSING  ../slanted-trillspan.ly  WITH SKYLINES: ------------

real	0m1.361s
user	0m0.800s
sys	0m0.128s


PROCESSING  ../tota-pulchra/tota-pulchra.ly  WITH MASTER: --------------

real	0m8.149s
user	0m7.416s
sys	0m0.360s

PROCESSING  ../tota-pulchra/tota-pulchra.ly  WITH SKYLINES: ------------

real	0m10.730s
user	0m9.625s
sys	0m0.672s

cheers,
Janek

[joeneeman, 2012-03-07 10:43:31]

2012/3/6 Janek Warchoł <janek.lilypond@gmail.com>

> On Tue, Mar 6, 2012 at 11:48 PM, Joe Neeman <joeneeman@gmail.com> wrote:
> > 2012/3/6 <janek.lilypond@gmail.com>
> >
> >> Mike & all,
> >>
> >> i did a quick compile with patchset 36 and unfortunately didn't notice
> >> significant speedups from previous version.
> >
> >
> > Could you try the dev/jneem branch in git? It has some optimizations. If
> > that doesn't help, could you please send me some of the worst files?
>
> They look better indeed time-wise.
> As for the output, i've only did a quick look and noticed this: why
> you deleted horizontal padding?  This results in mf at the bottom
> right of 1st page of Tota pulchra to jump into above lyrics.
>

The way skyline padding was being done made the code very messy. Also,
padding was being added in multiple places, so some things were getting
extra padding. However, the capability for doing padding is still there
(for example, with the outside-staff-horizon-padding property). I agree,
though, that the defaults will need to be revisited.


> Actually, small/zero horizontal padding might give better results in
> some places, but that needs thorough investigation.
>

Something that, FWIW, never happened with the previous defaults. A bunch of
the lines I deleted had comments like
// TODO: figure out what horizon_padding should be

Cheers,
Joe

[janek, 2012-03-07 21:32:47]

On Wed, Mar 7, 2012 at 11:43 AM, Joe Neeman <joeneeman@gmail.com> wrote:
>
> 2012/3/6 Janek Warchoł <janek.lilypond@gmail.com>
>> They look better indeed time-wise.
>> As for the output, i've only did a quick look and noticed this: why
>> you deleted horizontal padding?  This results in mf at the bottom
>> right of 1st page of Tota pulchra to jump into above lyrics.
>
>
> The way skyline padding was being done made the code very messy. Also,
> padding was being added in multiple places, so some things were getting
> extra padding. However, the capability for doing padding is still there (for
> example, with the outside-staff-horizon-padding property). I agree, though,
> that the defaults will need to be revisited.

ok.  i can't say anything about the code, sorry - i really don't have
the time to read it.  If you say that capability for horizontal
padding is still there, i'm fine with this.

>> Actually, small/zero horizontal padding might give better results in
>> some places, but that needs thorough investigation.
>
> Something that, FWIW, never happened with the previous defaults.

do you mean that there was no padding previously?  Sure, some of the
padding is Mike's addition, but in the scores compiled with master i
see padding around stems for example: their outlines are 3 times wider
than stems themselves.  That's not present in your version, see
attached.

HTH,
Janek

[joeneeman, 2012-03-07 21:50:21]

2012/3/7 Janek Warchoł <janek.lilypond@gmail.com>

> On Wed, Mar 7, 2012 at 11:43 AM, Joe Neeman <joeneeman@gmail.com> wrote:
> >
> > 2012/3/6 Janek Warchoł <janek.lilypond@gmail.com>
> >> Actually, small/zero horizontal padding might give better results in
> >> some places, but that needs thorough investigation.
> >
> > Something that, FWIW, never happened with the previous defaults.
>
> do you mean that there was no padding previously?


No, I mean that the previous default values were never investigated. When
the code was written, values were hard-coded with comments like "TODO:
what's the proper value?" Years later, those comments were still there...

Cheers,
Joe

[janek, 2012-03-07 22:09:16]

On Wed, Mar 7, 2012 at 10:50 PM, Joe Neeman <joeneeman@gmail.com> wrote:
> 2012/3/7 Janek Warchoł <janek.lilypond@gmail.com>
>>
>> On Wed, Mar 7, 2012 at 11:43 AM, Joe Neeman <joeneeman@gmail.com> wrote:
>> >
>> > 2012/3/6 Janek Warchoł <janek.lilypond@gmail.com>
>> >> Actually, small/zero horizontal padding might give better results in
>> >> some places, but that needs thorough investigation.
>> >
>> > Something that, FWIW, never happened with the previous defaults.
>>
>> do you mean that there was no padding previously?
>
> No, I mean that the previous default values were never investigated. When
> the code was written, values were hard-coded with comments like "TODO:
> what's the proper value?" Years later, those comments were still there...

ah, ok, i understand now.
thanks,
Janek

[mike_apollinemike.com, 2012-03-08 09:04:58]

Hey all,

An update on this patch.
After having correctly skylined dynamics, I'm realizing that serifed fonts and
italicized fonts are posing a problem for snug vertical alignment.  Joe and I
have been back and forth about a solution and he has graciously accepted to
tag-team the problem with me, but it'll likely require several rounds of back
and forth  between he and I to get it up to snuff.  So, you won't be seeing a
new patch for a bit, but when you do, it'll lead to better results.

Cheers,
MS

[MikeSol, 2012-07-01 14:58:08]

It's back...

The only thing that doesn't the work the way I'd expect it is Skyline::padded. 
It seems to not be adding the padding correctly (Joe - any ideas)?  This causes
tighter than desired spacing in alignment-order.ly.  Other than that it is all
reviewable!  For those who were keeping score before, files like size16.ly,
size20.ly etc. were a major nuisance and are now kept at bay.

Cheers,
MS

[Keith, 2012-07-01 23:03:56]

I like the direction in which this goes, making things closer, because it is
easier for users to add padding when needed than to persuade LilyPond to space
things more closely.

I did a quick test on some music.  <http://k-ohara.oco.net/Lilypond/> Ledger
lines might not be getting the space they need.  In rare cases, all the systems
were overlaid on the top of the page.  Several objects might need an increase in
their default padding.  

On 2012/07/01 14:58:08, MikeSol wrote:
> The only thing that doesn't the work the way I'd expect it is Skyline::padded.


Maybe the bit with last_end, noted below, will fix it; but what was wrong with
the horizon_padding in the old code ?

[Keith, 2012-07-01 23:04:16]

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode693
lily/skyline.cc:693: Skyline::padded (Real horizon_padding) const
The horizon_padding built in to Skyline::Skyline had nice beveled corners; does
this result in nice beveled corners ?

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode699
lily/skyline.cc:699: if (last_end > -infinity_f)
I don't see any assignments to 'last_end' so this seems to be always false.

[MikeSol, 2012-07-02 04:58:21]

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode693
lily/skyline.cc:693: Skyline::padded (Real horizon_padding) const
On 2012/07/01 23:04:17, Keith wrote:
> The horizon_padding built in to Skyline::Skyline had nice beveled corners;
does
> this result in nice beveled corners ?

It did - I want to get Joe's feedback first before moving back large chunks of
the old code, as he was the one who created a lot of this new Skyline stuff and
I want to make sure I'm understanding it correctly.

http://codereview.appspot.com/5626052/diff/78001/lily/skyline.cc#newcode699
lily/skyline.cc:699: if (last_end > -infinity_f)
On 2012/07/01 23:04:17, Keith wrote:
> I don't see any assignments to 'last_end' so this seems to be always false.

True - that's one of the problems.  The other is that height is always an empty
interval.  I've rewritten it as:


Skyline
Skyline::padded (Real horizon_padding) const
{
  vector<Box> pad_boxes;
  Real last_end = -infinity_f;
  for (list<Building>::const_iterator i = buildings_.begin (); i !=
buildings_.end (); ++i)
    {
      if (last_end > -infinity_f)
        {
          // Add the box that pads the left side of the current building.
          Real start = last_end - horizon_padding;
          Real height = i->height (last_end);
          if (height > -infinity_f)
            pad_boxes.push_back (Box (Interval (start, last_end),
                                      Interval (min (height - 0.01, height), max
(height - 0.01, height))));
        }

      if (i->end_ < infinity_f)
        {
          // Add the box that pads the right side of the current building.
          Real start = i->end_;
          Real end = start + horizon_padding;
          Real height = i->height (start);
          if (height > -infinity_f)
            pad_boxes.push_back (Box (Interval (start, end),
                                      Interval (min (height - 0.01, height), max
(height - 0.01, height))));
        }
    }

  // We created pad_boxes assuming that sky_ is UP.  To get padded to use
  // the UP side of the boxes, we need to create it pointing UP even if
  // it doesn't really.
  Skyline padded (pad_boxes, X_AXIS, UP);
  padded.sky_ = sky_;

  padded.merge (*this);
  return padded;
}

and still no result as expected...

[janek, 2012-07-03 08:19:17]

Hi all,

my favorite patch returned!  Yay!
I did some testing and the results are here:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705594/patchset37-results....
(sorry, too big for an attachment - 200 kB)
In short:
- staffswitch lines are broken
- barnumbers collide with ties
- some ties change shape
- some things move too close to each other, as if padding was nixed
- some skylines are really weird
- sometimes objects interlock too much.

hope this helps!
Janek

[mike_apollinemike.com, 2012-07-03 17:51:49]

On 2 juil. 2012, at 01:03, k-ohara5a5a@oco.net wrote:

> I like the direction in which this goes, making things closer, because
> it is easier for users to add padding when needed than to persuade
> LilyPond to space things more closely.
> 
> I did a quick test on some music.  <http://k-ohara.oco.net/Lilypond/>
> Ledger lines might not be getting the space they need.  In rare cases,
> all the systems were overlaid on the top of the page.  Several objects
> might need an increase in their default padding.

Hey Keith,

Could you tell me the page and the document where the system overlay happens?

Cheers,
MS

[Keith, 2012-07-04 01:18:05]

On Tue, 03 Jul 2012 10:51:05 -0700, mike@apollinemike.com
<mike@apollinemike.com> wrote:

> Could you tell me the page and the document where the system overlay happens?
>

Second page of the bassoon part.
http://k-ohara.oco.net/Lilypond/TightSkylines/woods-Bassoon1.pdf

The source is one directory up, but it's a big source.  You might just look at
the 2.15.40 output in the parallel directory /2.15.40/ to see if you recognize
the situation that is fooling your patch.

[janek, 2012-07-04 07:34:41]

On Mon, Jul 2, 2012 at 1:03 AM,  <k-ohara5a5a@oco.net> wrote:
> I did a quick test on some music.  <http://k-ohara.oco.net/Lilypond/>
> Ledger lines might not be getting the space they need.  In rare cases,
> all the systems were overlaid on the top of the page.  Several objects
> might need an increase in their default padding.

At some moment my scores experienced similar effect.  I think it was
around patchset 35 (36 was fine).

hth,
Janek

[mike_apollinemike.com, 2012-07-04 10:54:21]

On 3 juil. 2012, at 10:18, Janek Warchoł wrote:

> Hi all,
> 
> my favorite patch returned!  Yay!
> I did some testing and the results are here:
>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705594/patchset37-results....

Thanks Janek!  Very valuable stuff.

> (sorry, too big for an attachment - 200 kB)
> In short:
> - staffswitch lines are broken

This is because VoiceFollower is not set as (cross-staff . #t) in
define-grobs.scm.  I'm not sure why...this seems like a good idea irrespective
of how the skyline work pans out.

> - barnumbers collide with ties

This came from a rare problem that arose from the tie threading through the bar
number.  Fixed.

> - some ties change shape

This will take me some time to figure out...I'm gonna have to look at all the
parameters going into tie scoring and see which ones are changing.  I'll likely
push something to staging the weekend or next week in the style of
Tie_formatting_problem::spew () that gives tons of specs and then run the output
through python to pick up on changes.  Of course, if any tie gurus have
intuition about this, lemme know!

> - some things move too close to each other, as if padding was nixed

Should be fixed.

> - some skylines are really weird

This is likely a directioning problem - all upwards pointing skylines seem to
come out just fine.  The culprit would most likely be in stencil-integral.cc:
the notion of "UP" must be hardcoded somewhere that it shouldn't be.

> - sometimes objects interlock too much.

This was the case in the lyric example you sent.  I fixed it by adding light
skyline-horizontal-padding to lyrics.

> 
> hope this helps!
> Janek

I won't post a new patchset until all of the problems are fixed, but know that
most of them are.  I very much appreciate the research!

Cheers,
MS

[janek, 2012-07-04 13:17:37]

On Wed, Jul 4, 2012 at 11:31 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> Thanks Janek!  Very valuable stuff.

:)

>> (sorry, too big for an attachment - 200 kB)
>> In short:
>> - staffswitch lines are broken
>
> This is because VoiceFollower is not set as (cross-staff . #t) in
define-grobs.scm.  I'm not sure why...this seems like a good idea irrespective
of how the skyline work pans out.

I find it even more surprising that Glissandos work perfectly.  Aren't
they related to VoiceFollowers?

>> - some ties change shape
>
> This will take me some time to figure out...I'm gonna have to look at all the
parameters going into tie scoring and see which ones are changing.  I'll likely
push something to staging the weekend or next week in the style of
Tie_formatting_problem::spew () that gives tons of specs and then run the output
through python to pick up on changes.  Of course, if any tie gurus have
intuition about this, lemme know!

No idea when i'll have time to finish Tie Report, but as tie shapes
are screwed in general, i wouldn't care much about this change.  New
shape isn't worse than previous one, i'd say.

>> - some skylines are really weird
>
> This is likely a directioning problem - all upwards pointing skylines seem to
come out just fine.  The culprit would most likely be in stencil-integral.cc:
the notion of "UP" must be hardcoded somewhere that it shouldn't be.
> If you have a moment to test out weird-skylines.ly on different versions of
the patchset, I'd appreciate it!

Tenuto skyline seems to be wrong since the beginning - i've tested
patchsets 36, 34, 32 and 30, it looked similar everywhere.
Sharp and forte skylines seemed ok in patchset 36, see here:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patc...
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patc...

>> - sometimes objects interlock too much.
>
> This was the case in the lyric example you sent.  I fixed it by adding light
skyline-horizontal-padding to lyrics.

Hmm.  I have another idea, which would be more difficult but also
extremely awesome, and it would be very handy for area spacing (see
http://lists.gnu.org/archive/html/lilypond-devel/2012-03/msg00507.html):
make the skyline shape "magnetic".  See illustration here:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/magnetic_skylines_d...

What do you think?

cheers,
Janek

[mike_apollinemike.com, 2012-07-04 13:36:15]

On 4 juil. 2012, at 15:17, Janek Warchoł wrote:

> On Wed, Jul 4, 2012 at 11:31 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> Thanks Janek!  Very valuable stuff.
> 
> :)
> 
>>> (sorry, too big for an attachment - 200 kB)
>>> In short:
>>> - staffswitch lines are broken
>> 
>> This is because VoiceFollower is not set as (cross-staff . #t) in
define-grobs.scm.  I'm not sure why...this seems like a good idea irrespective
of how the skyline work pans out.
> 
> I find it even more surprising that Glissandos work perfectly.  Aren't
> they related to VoiceFollowers?

They're set to cross-staff when they are cross-staff, but VoiceFollowers are by
definition always cross staff.

> 
>>> - some ties change shape
>> 
>> This will take me some time to figure out...I'm gonna have to look at all the
parameters going into tie scoring and see which ones are changing.  I'll likely
push something to staging the weekend or next week in the style of
Tie_formatting_problem::spew () that gives tons of specs and then run the output
through python to pick up on changes.  Of course, if any tie gurus have
intuition about this, lemme know!
> 
> No idea when i'll have time to finish Tie Report, but as tie shapes
> are screwed in general, i wouldn't care much about this change.  New
> shape isn't worse than previous one, i'd say.

I think that there's supposed to be a bit of space in there, no?  What does
Gould say?
What's important is to figure out why this change is happening.  As ties only
ever use Skylines made from boxes (and not from stencil integrals), there must
be some weird subtlety about spacing somewhere (or perhaps the new Skyline code)
that is causing this.

> 
>>> - some skylines are really weird
>> 
>> This is likely a directioning problem - all upwards pointing skylines seem to
come out just fine.  The culprit would most likely be in stencil-integral.cc:
the notion of "UP" must be hardcoded somewhere that it shouldn't be.
>> If you have a moment to test out weird-skylines.ly on different versions of
the patchset, I'd appreciate it!
> 
> Tenuto skyline seems to be wrong since the beginning - i've tested
> patchsets 36, 34, 32 and 30, it looked similar everywhere.
> Sharp and forte skylines seemed ok in patchset 36, see here:
>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patc...
>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patc...

The tenuto is important, then.  There must be something about it (maybe its
symmetry) that confuses the the code.  The sharp and forte are also funky...it
seems that certain regions may have too short estimations.  I'll investigate.

> 
>>> - sometimes objects interlock too much.
>> 
>> This was the case in the lyric example you sent.  I fixed it by adding light
skyline-horizontal-padding to lyrics.
> 
> Hmm.  I have another idea, which would be more difficult but also
> extremely awesome, and it would be very handy for area spacing (see
> http://lists.gnu.org/archive/html/lilypond-devel/2012-03/msg00507.html):
> make the skyline shape "magnetic".  See illustration here:
>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/magnetic_skylines_d...
> 
> What do you think?

It wouldn't be too bad from a coding perspective. The function deholify already
does this for single skylines: try the skyline of a large font-size string "f  
_   f" or something like that and the holes should be plugged with a sloped
building.

When there are multiple grobs involved, what becomes tricky is knowing which
ones should allow for snug-fitting and which ones not.  I think that, as time
goes on, we'll realize where the snug spacing is too close and will modify it w/
padding values.

Cheers,
MS

[mail_philholmes.net, 2012-07-04 13:52:14]

----- Original Message ----- 
From: <mike@apollinemike.com>
To: "Janek Warchoł" <janek.lilypond@gmail.com>

> > In short:
> > - staffswitch lines are broken
>
> This is because VoiceFollower is not set as (cross-staff . #t) in 
> define-grobs.scm.  I'm not sure why...this seems like a good idea 
> irrespective of how the skyline work pans out.


I've not been following the thread fully, but my understanding is that these 
should only be displayed when they're positively switched on - e.g. with 
\showStaffSwitch.

--
Phil Holmes 

[janek, 2012-07-04 18:58:30]

On Wed, Jul 4, 2012 at 3:36 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
>
>> No idea when i'll have time to finish Tie Report, but as tie shapes
>> are screwed in general, i wouldn't care much about this change.  New
>> shape isn't worse than previous one, i'd say.
>
> I think that there's supposed to be a bit of space in there, no?  What does
Gould say?

Definitely, current output is wrong.  But as i said, i'm not sure if
it's worth bothering as ties are screwed in general.

> What's important is to figure out why this change is happening.  As ties only
ever use Skylines made from boxes (and not from stencil integrals), there must
be some weird subtlety about spacing somewhere (or perhaps the new Skyline code)
that is causing this.

Indeed, whatever the output is it shouldn't change because of new skylines.

>> Hmm.  I have another idea, which would be more difficult but also
>> extremely awesome, and it would be very handy for area spacing (see
>> http://lists.gnu.org/archive/html/lilypond-devel/2012-03/msg00507.html):
>> make the skyline shape "magnetic".  See illustration here:
>>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/magnetic_skylines_d...
>>
>> What do you think?
>
> It wouldn't be too bad from a coding perspective. The function deholify
already does this for single skylines: try the skyline of a large font-size
string "f   _   f" or something like that and the holes should be plugged with a
sloped building.
>
> When there are multiple grobs involved, what becomes tricky is knowing which
ones should allow for snug-fitting and which ones not.  I think that, as time
goes on, we'll realize where the snug spacing is too close and will modify it w/
padding values.

Do you mean that padding should influence skylines' "magnetism"?
Something like this:
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705597/magnetic_skylines_-...
(i.e. there would be only one setting, which would affect both the
padding and magnetism of the skylines)?
I think it might be a good idea.

cheers,
Janek

[mike_apollinemike.com, 2012-07-05 15:23:25]

On 4 juil. 2012, at 21:58, Janek Warchoł wrote:

> On Wed, Jul 4, 2012 at 3:36 PM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> 
>>> No idea when i'll have time to finish Tie Report, but as tie shapes
>>> are screwed in general, i wouldn't care much about this change.  New
>>> shape isn't worse than previous one, i'd say.
>> 
>> I think that there's supposed to be a bit of space in there, no?  What does
Gould say?
> 
> Definitely, current output is wrong.  But as i said, i'm not sure if
> it's worth bothering as ties are screwed in general.
> 
>> What's important is to figure out why this change is happening.  As ties only
ever use Skylines made from boxes (and not from stencil integrals), there must
be some weird subtlety about spacing somewhere (or perhaps the new Skyline code)
that is causing this.
> 
> Indeed, whatever the output is it shouldn't change because of new skylines.
> 
>>> Hmm.  I have another idea, which would be more difficult but also
>>> extremely awesome, and it would be very handy for area spacing (see
>>> http://lists.gnu.org/archive/html/lilypond-devel/2012-03/msg00507.html):
>>> make the skyline shape "magnetic".  See illustration here:
>>>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/magnetic_skylines_d...
>>> 
>>> What do you think?
>> 
>> It wouldn't be too bad from a coding perspective. The function deholify
already does this for single skylines: try the skyline of a large font-size
string "f   _   f" or something like that and the holes should be plugged with a
sloped building.
>> 
>> When there are multiple grobs involved, what becomes tricky is knowing which
ones should allow for snug-fitting and which ones not.  I think that, as time
goes on, we'll realize where the snug spacing is too close and will modify it w/
padding values.
> 
> Do you mean that padding should influence skylines' "magnetism"?
> Something like this:
>
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705597/magnetic_skylines_-...
> (i.e. there would be only one setting, which would affect both the
> padding and magnetism of the skylines)?
> I think it might be a good idea.
> 
> cheers,
> Janek

Hey all,

New patch set up that fixes all of the problems Janek pointed out and the major
problem Keith pointed out.  If any of the minor ones still exist in any of your
scores, lemme know.  Minimal examples help!

Cheers,
MS

[janek, 2012-07-05 16:05:57]

On Thu, Jul 5, 2012 at 5:23 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> New patch set up that fixes all of the problems Janek pointed out

I confirm, but the fun doesn't end yet :P

\relative f' { c4( d e f) g1 }
\addlyrics { blaaaa lah }

\relative f'' { c4( b a g) f1 }
\addlyrics { blaaaa lah }

\relative f'' { c8( a ) }
\addlyrics { blaa, }

\relative f'' { c8([ a )] }
\addlyrics { blaa, }

->
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705598/weird-melismas_skyl...
LOL

hth,
Janek

(and script-stack-order.ly is still wrong)

[dak, 2012-07-09 08:42:16]

I actually only wanted to get rid of the whitespace errors plagueing every test
of this patch, but one programming error jumped out so badly at me that I had to
flag it as well.

http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:726: 
Whitespace in empty line.

http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:767: 
Whitespace in empty line.

http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:798: 
Whitespace in empty line.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode281
lily/skyline.cc:281: bool dirty = false;
Trailing whitespace at end of line, but actually the whole line seems like it
should be deleted, or otherwise the loop will always execute exactly once since
the variable `dirty' that is being set in the loop is not the same as the
variable `dirty' that is checked at its end.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode583
lily/skyline.cc:583: 
Whitespace in empty line.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode710
lily/skyline.cc:710: end = i->start_;
Trailing whitespace.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode711
lily/skyline.cc:711: pad_buildings.push_back (Building (start, height, height,
end));
Trailing whitespace.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode728
lily/skyline.cc:728: pad_buildings.push_back (Building (start, height, height -
horizon_padding, end));
Trailing whitespace.

http://codereview.appspot.com/5626052/diff/79059/lily/skyline.cc#newcode834
lily/skyline.cc:834: return NOT_ENOUGH_INFO;
Trailing whitespace.

http://codereview.appspot.com/5626052/diff/79059/lily/system.cc
File lily/system.cc (right):

http://codereview.appspot.com/5626052/diff/79059/lily/system.cc#newcode426
lily/system.cc:426: return grobs_scm;
Trailing whitespace.

[joeneeman, 2012-07-14 14:01:08]

Just looked at skyline-related stuff for now...

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode292
lily/skyline.cc:292: Real p1 = left->end_ * left->slope_ + left->y_intercept_;
use left->height (left_end_) instead of calculating it by hand

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode293
lily/skyline.cc:293: Real p2 = i->start_ * i->slope_ + i->y_intercept_;
i->height (i->start_)

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode295
lily/skyline.cc:295: center->y_intercept_ = p2 - (center->end_ *
center->slope_);
I think that *center = Building (center->start_, p1, p2, center->end_) is
clearer

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302
lily/skyline.cc:302: while (dirty);
I don't understand this do while (dirty) part. It seems to me that unless you
have two empty segments in a row, you only need one pass. On the other hand, if
you have two empty segments in a row then this function will fail anyway because
you will end up setting center->slope_ and center->y_intercept_ both to
-infinity, which is bound to start creating NaN sooner or later.

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh
File lily/include/skyline.hh (right):

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newc...
lily/include/skyline.hh:58: NOT_ENOUGH_INFO
I wish I could convince you to think of a skyline as a region instead of just
the boundary of that region. Once you think of it that way, it becomes clear
that this information can be easily obtained from the Skyline_pair, using the
existing distance function.

Suppose s and t are Skyline_pairs.
max(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) = -infinity
means the objects don't overlap horizontally at all, so it's meaningless to talk
about which one is higher (I think this is what you're calling NOT_ENOUGH_INFO).

min(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) > 0
means that the objects intersect.

s[UP].distance(t[DOWN]) <= 0 and s[DOWN].distance(t[UP]) > 0
means that s is below t

t[UP].distance(s[DOWN]) <= 0 and t[DOWN].distance(s[UP]) > 0
means that t is below s

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc#newcode796
lily/skyline.cc:796: */
Since padding doesn't seem to be involved in this function, you should delete
this comment.

[MikeSol, 2012-07-14 18:33:10]

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode292
lily/skyline.cc:292: Real p1 = left->end_ * left->slope_ + left->y_intercept_;
On 2012/07/14 14:01:08, joeneeman wrote:
> use left->height (left_end_) instead of calculating it by hand

Done.

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode293
lily/skyline.cc:293: Real p2 = i->start_ * i->slope_ + i->y_intercept_;
On 2012/07/14 14:01:08, joeneeman wrote:
> i->height (i->start_)

Done.

http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302
lily/skyline.cc:302: while (dirty);
On 2012/07/14 14:01:08, joeneeman wrote:
> I don't understand this do while (dirty) part. It seems to me that unless you
> have two empty segments in a row, you only need one pass. On the other hand,
if
> you have two empty segments in a row then this function will fail anyway
because
> you will end up setting center->slope_ and center->y_intercept_ both to
> -infinity, which is bound to start creating NaN sooner or later.

I'll rewrite this so that it combines all interior skylines with an infinite y
intercept and then only does one pass.

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh
File lily/include/skyline.hh (right):

http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newc...
lily/include/skyline.hh:58: NOT_ENOUGH_INFO
On 2012/07/14 14:01:08, joeneeman wrote:
> I wish I could convince you to think of a skyline as a region instead of just
> the boundary of that region. Once you think of it that way, it becomes clear
> that this information can be easily obtained from the Skyline_pair, using the
> existing distance function.
> 
> Suppose s and t are Skyline_pairs.
> max(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) = -infinity
> means the objects don't overlap horizontally at all, so it's meaningless to
talk
> about which one is higher (I think this is what you're calling
NOT_ENOUGH_INFO).
> 
> min(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) > 0
> means that the objects intersect.
> 
> s[UP].distance(t[DOWN]) <= 0 and s[DOWN].distance(t[UP]) > 0
> means that s is below t
> 
> t[UP].distance(s[DOWN]) <= 0 and t[DOWN].distance(s[UP]) > 0
> means that t is below s

This logic uses two calls to distance, which is expensive, whereas using the
e-num gets this info from one call to distance.  I can use the method you
propose, but don't you think that'd cause a performance hit?

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/90001/lily/skyline.cc#newcode796
lily/skyline.cc:796: */
On 2012/07/14 14:01:08, joeneeman wrote:
> Since padding doesn't seem to be involved in this function, you should delete
> this comment.

Done.

[joeneeman, 2012-07-15 15:30:03]

On Sat, Jul 14, 2012 at 8:33 PM, <mtsolo@gmail.com> wrote:

>
> http://codereview.appspot.com/**5626052/diff/85004/lily/**
>
skyline.cc#newcode302<http://codereview.appspot.com/5626052/diff/85004/lily/skyline.cc#newcode302>
> lily/skyline.cc:302: while (dirty);
> On 2012/07/14 14:01:08, joeneeman wrote:
>
>> I don't understand this do while (dirty) part. It seems to me that
>>
> unless you
>
>> have two empty segments in a row, you only need one pass. On the other
>>
> hand, if
>
>> you have two empty segments in a row then this function will fail
>>
> anyway because
>
>> you will end up setting center->slope_ and center->y_intercept_ both
>>
> to
>
>> -infinity, which is bound to start creating NaN sooner or later.
>>
>
> I'll rewrite this so that it combines all interior skylines with an
> infinite y intercept and then only does one pass.


This sounds like a good idea. If you put the skyline combination part in a
separate function, we could just use it after merges, etc, so that the
property of not having adjacent empty buildings is always maintained.


> http://codereview.appspot.com/**5626052/diff/90001/lily/**
>
include/skyline.hh<http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh>
> File lily/include/skyline.hh (right):
>
> http://codereview.appspot.com/**5626052/diff/90001/lily/**
>
include/skyline.hh#newcode58<http://codereview.appspot.com/5626052/diff/90001/lily/include/skyline.hh#newcode58>
> lily/include/skyline.hh:58: NOT_ENOUGH_INFO
> On 2012/07/14 14:01:08, joeneeman wrote:
>
>> I wish I could convince you to think of a skyline as a region instead
>>
> of just
>
>> the boundary of that region. Once you think of it that way, it becomes
>>
> clear
>
>> that this information can be easily obtained from the Skyline_pair,
>>
> using the
>
>> existing distance function.
>>
>
>  Suppose s and t are Skyline_pairs.
>> max(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) = -infinity
>> means the objects don't overlap horizontally at all, so it's
>>
> meaningless to talk
>
>> about which one is higher (I think this is what you're calling
>>
> NOT_ENOUGH_INFO).
>
>  min(s[UP].distance(t[DOWN]), s[DOWN].distance(t[UP])) > 0
>> means that the objects intersect.
>>
>
>  s[UP].distance(t[DOWN]) <= 0 and s[DOWN].distance(t[UP]) > 0
>> means that s is below t
>>
>
>  t[UP].distance(s[DOWN]) <= 0 and t[DOWN].distance(s[UP]) > 0
>> means that t is below s
>>
>
> This logic uses two calls to distance, which is expensive, whereas using
> the e-num gets this info from one call to distance.  I can use the
> method you propose, but don't you think that'd cause a performance hit?


Won't you need two skyline comparisons anyway? If T's up skyline is always
above S's down skyline, then S and T may intersect but you won't know until
you've compared T's down skyline with S's up skyline. Anyway, I'm always in
favor of writing simple code first and then optimizing once things have
been measured.

Cheers,
Joe

[Keith, 2012-08-14 04:21:32]

Worked nicely on some piano music and a Dvorak symphony.

The code is bewildering.  So far I've mostly read the comments.

The bit with the activation-factor is pretty ugly. 
What was the size of the problem ?  Did the script that should have fit lack
space of padding, 0.5*padding, or epsilon ?

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-scrip...
File input/regression/text-script-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-scrip...
input/regression/text-script-vertical-skylines.ly:5: kerning.
It is not obvious we want this for TextScripts, 
  {b'^"Élever" b'^"brusquement"}
so don't enshrine the requirement in a regtest.

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-brac...
File input/regression/volta-bracket-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-brac...
input/regression/volta-bracket-vertical-skylines.ly:4: texidoc = "Volta brackets
are vertically kerned to objects below them.
Good, but "fit" not "kerned"
To me, 'to kern' includes making many aesthetic judgments to come up with
individual adjustments for each fitting pair.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:629: Three rounds:
I don't understand the comment, but what I needed to figure out was :
Given pre-padded vertical skylines 'pair' for an outside-staff Grob, this
figures out the vertical position of the Grob.  It raises the skylines in 'pair'
and shifts (along the skyline) 'horizontal_skylines' by the same amount.

Using horizontal_skylines (with the buildings pointing horizontally) to
determine vertical position, is a new concept worth mentioning.
 
I'm still figuring out 'exempt', '*_forest', etc.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:658: check for horizontal edges jinnying up
I learned the local dialect 200 miles north, so I don't know what this means. Is
this the case where the edges lie side a by each, or kitty corner ?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:728: We need to take the padding away now that it
has been shifted.  Otherwise
I can't find where the padding was ever added.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:751: edge of the just-placed grob).  Otherwise, we
skip it until the next pass.
This comment describes the old way of solving the problem you saw with
'midi-dynamics.pdf' but you removed the old code.  Maybe you want to use the old
method, but make code and comment agree one way or another.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:778: Note we only use 75% of padding and apply the
full compliment only if
It looks like you add 50% padding here, then pass to
avoid_outside_staff_collision() which removes 37.5% of padding away.  Probably
not what you meant to do.

The verbs 'use' and 'apply' are vague enough that the comment didn't help me see
what you meant to do.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:843: Three rounds:
Does this part of the comment apply to the code below ?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:890: feature present in one regrest :(
You tease us.  Which regression test ?

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc#newcode1368
lily/beam.cc:1368: shift += 0.01;
* beamdir ?

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc
File lily/skyline-pair.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc#newcod...
lily/skyline-pair.cc:103: // other[UP] then we will not intersect.
Worth mentioning that the shift direction d is LEFT RIGHT for vertical skylines,
all axes reversed for horizontal.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode157
lily/skyline.cc:157: Real ret = (y - y_intercept_ - slope_ * x) / slope_;
(y - height(x))

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode287
lily/skyline.cc:287: // Since a skyline should always be normalized, we can
I think you mean "this function requires the skyline to be normalized, so that
..."

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode922
lily/skyline.cc:922: // direction which will result in THIS and OTHER not
overlapping.
.. in the given direction /along/ the skyline, locally called the horizon.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties....
scm/define-grob-properties.scm:663: (outside-staff-padding-activation-factor
,number? "The percentage
This is extremely confusing.  It sounds like you allow grobs to come within a
factor of the user-requested padding of protrusions from the staff, but if the
protrusion comes out too far you jump away to full padding.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:913: (vertical-skylines .
,ly:grob::vertical-skylines-from-stencil)
For Flags, I would think horizontal-skylines-from-stencil would be useful ...
but maybe it is only used for outside-staff placement ?

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:1590: ; allows double flat of F to be nestled over dots of
C
Can't you adjust for that on something more specific to the symptom, like
DotColumn or Accidental ?

[MikeSol, 2012-08-16 04:15:38]

Thanks for the review!

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-scrip...
File input/regression/text-script-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/text-scrip...
input/regression/text-script-vertical-skylines.ly:5: kerning.
On 2012/08/14 04:21:33, Keith wrote:
> It is not obvious we want this for TextScripts, 
>   {b'^"Élever" b'^"brusquement"}
> so don't enshrine the requirement in a regtest.

Done.

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-brac...
File input/regression/volta-bracket-vertical-skylines.ly (right):

http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-brac...
input/regression/volta-bracket-vertical-skylines.ly:4: texidoc = "Volta brackets
are vertically kerned to objects below them.
On 2012/08/14 04:21:33, Keith wrote:
> Good, but "fit" not "kerned"
> To me, 'to kern' includes making many aesthetic judgments to come up with
> individual adjustments for each fitting pair.  

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:629: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
> I don't understand the comment, but what I needed to figure out was :
> Given pre-padded vertical skylines 'pair' for an outside-staff Grob, this
> figures out the vertical position of the Grob.  It raises the skylines in
'pair'
> and shifts (along the skyline) 'horizontal_skylines' by the same amount.
> 
> Using horizontal_skylines (with the buildings pointing horizontally) to
> determine vertical position, is a new concept worth mentioning.
>  
> I'm still figuring out 'exempt', '*_forest', etc.

Better comment added.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:658: check for horizontal edges jinnying up
On 2012/08/14 04:21:33, Keith wrote:
> I learned the local dialect 200 miles north, so I don't know what this means.
Is
> this the case where the edges lie side a by each, or kitty corner ?

Elaine Gould uses the phrase "jinnying up" in behind bars, and given that we are
using that as one of our base texts, I'm ok using the phrase here.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:728: We need to take the padding away now that it
has been shifted.  Otherwise
On 2012/08/14 04:21:33, Keith wrote:
> I can't find where the padding was ever added.

The call to grow in add_grobs_of_one_priority.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:751: edge of the just-placed grob).  Otherwise, we
skip it until the next pass.
On 2012/08/14 04:21:33, Keith wrote:
> This comment describes the old way of solving the problem you saw with
> 'midi-dynamics.pdf' but you removed the old code.  Maybe you want to use the
old
> method, but make code and comment agree one way or another.

Excellent observation - I'd completely forgotten this. Perhaps this takes care
of the midi-dynamics.ly problem altogether...will investigate.

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:778: Note we only use 75% of padding and apply the
full compliment only if
On 2012/08/14 04:21:33, Keith wrote:
> It looks like you add 50% padding here, then pass to
> avoid_outside_staff_collision() which removes 37.5% of padding away.  Probably
> not what you meant to do.
> 
> The verbs 'use' and 'apply' are vague enough that the comment didn't help me
see
> what you meant to do.

I think that both remove 50%, no?

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:843: Three rounds:
On 2012/08/14 04:21:33, Keith wrote:
> Does this part of the comment apply to the code below ?

The comment's blech...I'll redo it in a later patchset (remind me if I don't).

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:890: feature present in one regrest :(
On 2012/08/14 04:21:33, Keith wrote:
> You tease us.  Which regression test ?

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/beam.cc#newcode1368
lily/beam.cc:1368: shift += 0.01;
On 2012/08/14 04:21:33, Keith wrote:
> * beamdir ?

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc
File lily/skyline-pair.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline-pair.cc#newcod...
lily/skyline-pair.cc:103: // other[UP] then we will not intersect.
On 2012/08/14 04:21:33, Keith wrote:
> Worth mentioning that the shift direction d is LEFT RIGHT for vertical
skylines,
> all axes reversed for horizontal.

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode157
lily/skyline.cc:157: Real ret = (y - y_intercept_ - slope_ * x) / slope_;
On 2012/08/14 04:21:33, Keith wrote:
> (y - height(x))

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode287
lily/skyline.cc:287: // Since a skyline should always be normalized, we can
On 2012/08/14 04:21:33, Keith wrote:
> I think you mean "this function requires the skyline to be normalized, so that
> ..." 

Done.

http://codereview.appspot.com/5626052/diff/105001/lily/skyline.cc#newcode922
lily/skyline.cc:922: // direction which will result in THIS and OTHER not
overlapping.
On 2012/08/14 04:21:33, Keith wrote:
> .. in the given direction /along/ the skyline, locally called the horizon.

Done.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grob-properties....
scm/define-grob-properties.scm:663: (outside-staff-padding-activation-factor
,number? "The percentage
On 2012/08/14 04:21:33, Keith wrote:
> This is extremely confusing.  It sounds like you allow grobs to come within a
> factor of the user-requested padding of protrusions from the staff, but if the
> protrusion comes out too far you jump away to full padding.

That's how it's currently implemented.  It is the only way I've found to get the
old behavior in stuff like midi-pedal.ly, but I'll try reimplementing the
comment above add_grobs_of_one_priority.

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:913: (vertical-skylines .
,ly:grob::vertical-skylines-from-stencil)
On 2012/08/14 04:21:33, Keith wrote:
> For Flags, I would think horizontal-skylines-from-stencil would be useful ...
> but maybe it is only used for outside-staff placement ?

I'll look into this after pushing all of this stuff - remind me if I forget!

http://codereview.appspot.com/5626052/diff/105001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:1590: ; allows double flat of F to be nestled over dots of
C
On 2012/08/14 04:21:33, Keith wrote:
> Can't you adjust for that on something more specific to the symptom, like
> DotColumn or Accidental ?

No, as padding is added on only at the PaperColumn level. It may be possible to
make this more refined, though...I'll look into it in a separate commit.  As
usual, remind me if I forget.

[MikeSol, 2012-08-16 06:21:40]

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/105001/lily/axis-group-interface.c...
lily/axis-group-interface.cc:778: Note we only use 75% of padding and apply the
full compliment only if
On 2012/08/14 04:21:33, Keith wrote:
> It looks like you add 50% padding here, then pass to
> avoid_outside_staff_collision() which removes 37.5% of padding away.  Probably
> not what you meant to do.
> 
> The verbs 'use' and 'apply' are vague enough that the comment didn't help me
see
> what you meant to do.

Sorry, I see the problem now (I missed the 0.75).
I brought back the logic of the comment above & it looks like it obviates the
need for this whole 50%-of-padding thing.  Running regtests now.

[Keith, 2012-08-17 08:12:56]

patch
real	17m55.095s
user	17m10.848s
sys	0m11.381s
git master
real	16m16.228s
user	15m30.543s
sys	0m10.624s

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:636: overlap whereas the horizontal skylines do.
Really?  In the example you draw below, the lower vertical skyline of (obj B)
would intersect the upper vertical skyline of (obj A) because the skyline fills
in the concavity in (obj B)

Protect ASCII art comments with a first-column *.  Similar comments in the
note-collision code were destroyed when someone ran the emacs auto-indenter over
the files.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:706: do
// ... while dirty

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:746: egg doll style, there will be no
"intersection" because they never
Skyline:distance() would report this case, with a negative distance.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:780: while (dirty);
I am beginning to understand the new code. Would you consider again using
distance() instead of the repeated calls to intersects() in the while(dirty)
loop ?  It might look simpler now that you've been away from the code for a
while.

The Skyline::distance()s have all the information you want.  If you are trying
to place, by moving UP, an object with skylines 'pair' among a set of skylines
'forest[j]' then the distance you need to move has several constraints

floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
ceilings[j] = forest[j][DOWN]~distance~pair[UP]

You know the solution will be to move an amount equal to one of the floors[j],
and you want to find the smallest that fits, so make
  trials = sort (floors)
and find the smallest among trials that satisfies
   trials[n] > 0
&&  for all j (trials[n] > floors[j]
               || trials[n] < ceilings[j] )

Somebody might complain that this is quadratic in the number of objects to
place, but the problem placing N objects while looking for holes left in earlier
placements is a quadratic task.  But now the inner loop is simple comparisons,
rather than re-checking of skyline intersections and distances with the
while(dirty) method.

In any case, the number N seems to be sufficiently limited in practice (thanks
maybe to 'exempt') based on reported score compilation times.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
If I remove all additions to the 'riders' array,
'tuplet-number-outside-staff-priority.ly' stays the same.

I tried, but failed, to create a case where keeping track of 'riders' helps.
  \relative c'' {
    \override TupletBracket #'outside-staff-priority = #1
    \times 2/3 { a8\trill a\trill a\trill } }
What case did you use ?

http://codereview.appspot.com/5626052/diff/106004/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/skyline.cc#newcode793
lily/skyline.cc:793: For systems, padding is not added at creation time. 
Padding is
Stray comment?

[joeneeman, 2012-08-17 11:19:26]

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:651: -------------------------------/
I've experimented with disabling the horizontal skylines, and it didn't actually
make any difference to the regtests. Could you add something that exercises
this?

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:658: are avoided
Have you measured the costs of not having exempt?

[mike7, 2012-08-17 14:21:21]

On 17 août 2012, at 07:19, joeneeman@gmail.com wrote:

> 
>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:651: -------------------------------/
> I've experimented with disabling the horizontal skylines, and it didn't
> actually make any difference to the regtests. Could you add something
> that exercises this?
> 

I could - it'll take me a bit of time to cook it up.  Essentially the horizontal
stuff exists to differentiate between the two images in the attached file. The
top one shouldn't shifted whereas the bottom should and the only way to know is
via the horizontal skylines.

>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:658: are avoided
> Have you measured the costs of not having exempt?

Not yet - I'm sure there is a minimal speedup for certain scores, but we can
certainly delete it down the line if people think it's more trouble than it's
worth.

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

[mike7, 2012-08-17 14:28:50]

On 17 août 2012, at 10:21, "mike@mikesolomon.org" <mike@mikesolomon.org> wrote:

> On 17 août 2012, at 07:19, joeneeman@gmail.com wrote:
> 
>> 
>>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
>> File lily/axis-group-interface.cc (right):
>> 
>>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
>> lily/axis-group-interface.cc:651: -------------------------------/
>> I've experimented with disabling the horizontal skylines, and it didn't
>> actually make any difference to the regtests. Could you add something
>> that exercises this?
>> 
> 
> I could - it'll take me a bit of time to cook it up.  Essentially the
horizontal stuff exists to differentiate between the two images in the attached
file. The top one shouldn't shifted whereas the bottom should and the only way
to know is via the horizontal skylines.
> 
>>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
>> lily/axis-group-interface.cc:658: are avoided
>> Have you measured the costs of not having exempt?
> 
> Not yet - I'm sure there is a minimal speedup for certain scores, but we can
certainly delete it down the line if people think it's more trouble than it's
worth.
> 
>> 
>> http://codereview.appspot.com/5626052/
>> 
>> _______________________________________________
>> lilypond-devel mailing list
>> lilypond-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/lilypond-devel
> 
> <horizontalcollision.png>
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

I cooked up a quick test and it's not giving me the results I want - the
algorithm shouldn't shift the second image above the first but it does.  I'll
look into it...

\version "2.15.30"

foo = \markup \translate #'(0 . 6)
              \path #0.1 #'((moveto 0 0)
                            (lineto 0 6)
                            (lineto 8 6)
                            (lineto 8 5)
                            (lineto 4 5)
                            (lineto 4 1)
                            (lineto 8 1)
                            (lineto 8 0)
                            (lineto 0 0))

bar = \markup \translate #'(6 . 0)
              \path #0.1 #'((moveto 0 0)
                            (lineto 0 10)
                            (lineto -8 10)
                            (lineto -8 9)
                            (lineto -1 9)
                            (lineto -1 4)
                            (lineto -3 4)
                            (lineto -3 3)
                            (lineto -1 3)
                            (lineto -1 0)
                            (lineto 0 0))

\relative c'' {
  a^\foo
  a^\bar
}

[MikeSol, 2012-08-17 17:16:25]

I've forgotten why I added the horizontal skyline stuff so I've taken it out in
a new version that I'll post after regtests.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:658: are avoided
On 2012/08/17 11:19:26, joeneeman wrote:
> Have you measured the costs of not having exempt?

I've decided to scrap exempt in a new version - will upload soon.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:780: while (dirty);
On 2012/08/17 08:12:56, Keith wrote:
> I am beginning to understand the new code. Would you consider again using
> distance() instead of the repeated calls to intersects() in the while(dirty)
> loop ?  It might look simpler now that you've been away from the code for a
> while.
> 
> The Skyline::distance()s have all the information you want.  If you are trying
> to place, by moving UP, an object with skylines 'pair' among a set of skylines
> 'forest[j]' then the distance you need to move has several constraints
> 
> floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
> ceilings[j] = forest[j][DOWN]~distance~pair[UP]
> 
> You know the solution will be to move an amount equal to one of the floors[j],
> and you want to find the smallest that fits, so make
>   trials = sort (floors)
> and find the smallest among trials that satisfies
>    trials[n] > 0
> &&  for all j (trials[n] > floors[j]
>                || trials[n] < ceilings[j] )
> 
> Somebody might complain that this is quadratic in the number of objects to
> place, but the problem placing N objects while looking for holes left in
earlier
> placements is a quadratic task.  But now the inner loop is simple comparisons,
> rather than re-checking of skyline intersections and distances with the
> while(dirty) method.
> 
> In any case, the number N seems to be sufficiently limited in practice (thanks
> maybe to 'exempt') based on reported score compilation times.

Hey Keith,

I am not adverse to using distance if possible - my trouble case is the
following.

AAAAAAAAA

    BBBBBBBBBBBBB

Image that object A above is placed first and LilyPond is now considering
whether it should leave B where it is or place it above A.  The distance
function would dictate that it needs to be shifted above A because its DOWN
skyline is below A's UP.  However, it never intersects, so there's no need to
shift it.  This is why I'm looking for intersections. Maybe I'm thinking of it
incorrectly, though?

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/17 08:12:56, Keith wrote:
> If I remove all additions to the 'riders' array,
> 'tuplet-number-outside-staff-priority.ly' stays the same.
> 
> I tried, but failed, to create a case where keeping track of 'riders' helps.
>   \relative c'' {
>     \override TupletBracket #'outside-staff-priority = #1
>     \times 2/3 { a8\trill a\trill a\trill } }
> What case did you use ?

\version "2.17.0"
#(ly:set-option 'debug-skylines #t)
 \relative c'' {
   \override TupletBracket #'outside-staff-priority = #1
   \times 2/3 { a4\trill a\trill a\trill } }

If you comment out the rider business, the vertical-skylines will not be correct
for axis-group-interface. Comment it back in and they will.

[Keith, 2012-08-17 19:25:29]

On Fri, 17 Aug 2012 10:16:25 -0700, <mtsolo@gmail.com> wrote:

>
>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:780: while (dirty);
> On 2012/08/17 08:12:56, Keith wrote:
>> I am beginning to understand the new code. Would you consider again using
>> distance() instead of the repeated calls to intersects() in the while(dirty)
>> loop ?  It might look simpler now that you've been away from the code
>> for awhile.

>> floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
>> ceilings[j] = forest[j][DOWN]~distance~pair[UP]

>> You know the solution will be to move an amount equal to one of the
floors[j],
>> and you want to find the smallest that fits, so make
>>    trials = sort (floors)
>> and find the smallest among trials that satisfies
>>     trials[n] > 0
>> &&  for all j (trials[n] > floors[j]
>>                 || trials[n] < ceilings[j] )

> I am not adverse to using distance if possible - my trouble case is the
> following.
>
> AAAAAAAAA
>
>      BBBBBBBBBBBBB
>
> Image that object A above is placed first and LilyPond is now
> considering whether it should leave B where it is or place it above A.
> The distance function would dictate that it needs to be shifted above A
> because its DOWN skyline is below A's UP.

We need not let the distance function between A[UP] and B[DOWN] dictate, because
having positive distance between A[DOWN] and B[UP] is another solution.

Object AA, a member of the forest, sets two constraints
        floor = padding  -  AA[UP] ~distance~ BB[DOWN]
        ceiling = AA[DOWN] ~distance~ BB[UP]  - padding
and we are checking that a trial location satisfies
        (trial >= floor) OR (trial <= ceiling)

I suppose you show BB having been placed just above the staff, and that it still
lies below the ceiling set by AA, so this trial position satisfies the first
test in the OR above, so we win.

All these distances can be computed once, before raising the skylines at all,
because they tell you right away what trial values of raising BB will clear AA. 
That would be much simpler for people like me to understand than computing the
four intersects() conditions, after having moved the skylines by zero or more
bump(s).


On the question of where to store padding, simple padding was previously added
by the code using distance().  That seems best because it is a simple addition
and different applications might have different padding to the same objects. 
Then you don't have to worry about adding padding multiple times to a skyline,
deciding which of two skylines to pad, etc.

The other thing is 'horizon-padding' which fattens the buildings in the skyline
themselves, and has in the past been implemented by reshaping the Skyline
object.  The user-variables skyline-(vertical|horizontal)-padding apply so far
to symmetric situations, and the code simply horizon-pads both skylines so you
get the effect of twice the number you enter.

[joeneeman, 2012-08-17 20:57:51]

On Sat, Aug 18, 2012 at 5:34 AM, Keith OHara <k-ohara5a5a@oco.net> wrote:

> On Fri, 17 Aug 2012 10:16:25 -0700, <mtsolo@gmail.com> wrote:
>
>
>> http://codereview.appspot.com/**5626052/diff/106004/lily/axis-**
>>
group-interface.cc#newcode780<http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode780>
>> lily/axis-group-interface.cc:**780: while (dirty);
>> On 2012/08/17 08:12:56, Keith wrote:
>>
>>> I am beginning to understand the new code. Would you consider again using
>>> distance() instead of the repeated calls to intersects() in the
>>> while(dirty)
>>> loop ?  It might look simpler now that you've been away from the code
>>> for awhile.
>>>
>>
>  floors[j] = padding - forest[j][UP]~distance~pair[**DOWN]
>>> ceilings[j] = forest[j][DOWN]~distance~pair[**UP]
>>>
>>
>  You know the solution will be to move an amount equal to one of the
>>> floors[j],
>>> and you want to find the smallest that fits, so make
>>>    trials = sort (floors)
>>> and find the smallest among trials that satisfies
>>>     trials[n] > 0
>>> &&  for all j (trials[n] > floors[j]
>>>                 || trials[n] < ceilings[j] )
>>>
>>
>  I am not adverse to using distance if possible - my trouble case is the
>> following.
>>
>> AAAAAAAAA
>>
>>      BBBBBBBBBBBBB
>>
>> Image that object A above is placed first and LilyPond is now
>> considering whether it should leave B where it is or place it above A.
>> The distance function would dictate that it needs to be shifted above A
>> because its DOWN skyline is below A's UP.
>>
>
> We need not let the distance function between A[UP] and B[DOWN] dictate,
> because having positive distance between A[DOWN] and B[UP] is another
> solution.
>

If you check out the dev/jneem-skylines (which is a simplified but not
(yet) feature-compatible version of Mike's branch), you'll see that I'm
using this method under the name 'bool Skyline_pair::intersects.'

Cheers,
Joe

[mike7, 2012-08-17 22:50:48]

On 17 août 2012, at 16:57, Joe Neeman <joeneeman@gmail.com> wrote:

> 
> On Sat, Aug 18, 2012 at 5:34 AM, Keith OHara <k-ohara5a5a@oco.net> wrote:
> On Fri, 17 Aug 2012 10:16:25 -0700, <mtsolo@gmail.com> wrote:
> 
> 
>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
> lily/axis-group-interface.cc:780: while (dirty);
> On 2012/08/17 08:12:56, Keith wrote:
> I am beginning to understand the new code. Would you consider again using
> distance() instead of the repeated calls to intersects() in the while(dirty)
> loop ?  It might look simpler now that you've been away from the code
> for awhile.
> 
> floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
> ceilings[j] = forest[j][DOWN]~distance~pair[UP]
> 
> You know the solution will be to move an amount equal to one of the
floors[j],
> and you want to find the smallest that fits, so make
>    trials = sort (floors)
> and find the smallest among trials that satisfies
>     trials[n] > 0
> &&  for all j (trials[n] > floors[j]
>                 || trials[n] < ceilings[j] )
> 
> I am not adverse to using distance if possible - my trouble case is the
> following.
> 
> AAAAAAAAA
> 
>      BBBBBBBBBBBBB
> 
> Image that object A above is placed first and LilyPond is now
> considering whether it should leave B where it is or place it above A.
> The distance function would dictate that it needs to be shifted above A
> because its DOWN skyline is below A's UP.
> 
> We need not let the distance function between A[UP] and B[DOWN] dictate,
because having positive distance between A[DOWN] and B[UP] is another solution.
> 
> If you check out the dev/jneem-skylines (which is a simplified but not (yet)
feature-compatible version of Mike's branch), you'll see that I'm using this
method under the name 'bool Skyline_pair::intersects.'
> 
> Cheers,
> Joe
> 

This is great, Joe.  If you can get it to a point where it is feature compatible
such that it eliminates the calls to both intersection and distance, lemme know.
 I'll have time to work on it together in the AM for the next few days.  I am
starting to understand how this solution works but I don't think I could write
it from scratch w/o doing more thinking so if you have something almost
functional I'd rather go from that.

Cheers,
MS

[Keith, 2012-08-18 02:42:37]

This is barely slower than master on an orchestral score and parts
  patch                 master
  real    16m16.228s    real      15m54.212s
  user    15m30.543s    user      15m5.920s
  sys     0m10.624s     sys       0m10.649s

but piano music (Chopin op45) takes 25% longer

  real    0m16.245s     real      0m12.862s
  user    0m15.573s     user      0m12.232s
  sys     0m0.254s      sys       0m0.217s

patch                 master
real    16m16.228s    real      15m54.212s
user    15m30.543s    user      15m5.920s
sys     0m10.624s     sys       0m10.649s

real    0m16.245s     real      0m12.862s
user    0m15.573s     user      0m12.232s
sys     0m0.254s      sys       0m0.217s

We should clean up the code, most importantly because having operations that do
not progress toward the goal is quite confusing, and time-consuming, for any
code maintainer.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/17 17:16:25, MikeSol wrote:
> On 2012/08/17 08:12:56, Keith wrote:
> If you comment out the rider business, the vertical-skylines will not be
correct
> for axis-group-interface. 

That is very subtle, very minor, changes only the debug output, not what would
normally be printed from that example, and is different from the comment
indicates.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:697: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance ((*pair)[-d]));
When d = dir, the direction we intend to move, this tells us how far we need to
move, but when d = -dir, this tells us by how far we have encroached into the
next obstacle, which we will eventually need to hop over.  

What you really want to put on the bumps list is how far we need to move to hope
over this next obstacle.  But we only tested the relevant skylines for
'intersection' ... if only we had stached them distance().

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:701: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance (to_flip));
This finds the distance to move such that the top skyline of the new object just
encloses, Russian egg doll style, one of the already-placed skylines.  We never
want that to be the final position, so don't put it on the bumps list.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:732: pair->raise (min_bump);
  printf("bumping %.3f\n", min_bump);
shows that for a simple case
  \relative f {f'^"mouse" c'' f,,^"EEEEK" d''}
we bump around quite a lot:
"mouse"
bumping 0.567
bumping 1.894
"EEEK"
bumping 0.443
bumping 0.048
bumping 0.095
bumping 0.191
bumping 0.382
bumping 0.459
bumping 1.344
bumping 0.161
bumping 0.322
bumping 0.578

[MikeSol, 2012-08-18 10:12:00]

Thanks for the performance tests!  I have no problem changing the function
avoid_outside_staff_collisions to be faster - it's just that I haven't wrapped
my head around how distance alone can do it.

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
lily/axis-group-interface.cc:945: feature present in
tuplet-number-outside-staff-priority.
On 2012/08/18 02:42:37, Keith wrote:
> On 2012/08/17 17:16:25, MikeSol wrote:
> > On 2012/08/17 08:12:56, Keith wrote:
> > If you comment out the rider business, the vertical-skylines will not be
> correct
> > for axis-group-interface. 
> 
> That is very subtle, very minor, changes only the debug output, not what would
> normally be printed from that example, and is different from the comment
> indicates.

\relative c'' {
   \override TupletBracket #'outside-staff-priority = #1
   \override TupletNumber #'font-size = #5
   \times 2/3 { a4\trill a\trill^"foo" a\trill }
}

I added this as a regtest.  If you comment out the rider business, you'll see
that foo is printed on top of the tuplet number.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:697: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance ((*pair)[-d]));
On 2012/08/18 02:42:37, Keith wrote:
> When d = dir, the direction we intend to move, this tells us how far we need
to
> move, but when d = -dir, this tells us by how far we have encroached into the
> next obstacle, which we will eventually need to hop over.  
> 
> What you really want to put on the bumps list is how far we need to move to
hope
> over this next obstacle.  But we only tested the relevant skylines for
> 'intersection' ... if only we had stached them distance().

I'm not sure what you mean when you say "we only tested the relevant skylines
for intersection ... if only we had stached them distance()" - one because I
don't know what stached means in this context (from urban dictionary: The art of
placing a ficticious mustache on an individual or oneself, drawing laughter and
humor to this occurance.) and two because every intersection call is followed by
a distance call, so there is bumping going on in this case.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:701: bumps.push_back (dir *
vertical_skyline_forest[dir][j][d].distance (to_flip));
On 2012/08/18 02:42:37, Keith wrote:
> This finds the distance to move such that the top skyline of the new object
just
> encloses, Russian egg doll style, one of the already-placed skylines.  We
never
> want that to be the final position, so don't put it on the bumps list.

That is a possible scenario, but Russian egg doll style enclosing is tested for
below, so even if this does happen it'd be rectified on the next pass.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:732: pair->raise (min_bump);
On 2012/08/18 02:42:37, Keith wrote:
>   printf("bumping %.3f\n", min_bump);
> shows that for a simple case
>   \relative f {f'^"mouse" c'' f,,^"EEEEK" d''}
> we bump around quite a lot:
> "mouse"
> bumping 0.567
> bumping 1.894
> "EEEK"
> bumping 0.443
> bumping 0.048
> bumping 0.095
> bumping 0.191
> bumping 0.382
> bumping 0.459
> bumping 1.344
> bumping 0.161
> bumping 0.322
> bumping 0.578

Agreed.  I'd love for this to go faster.
I am for getting this into current master ASAP so that people can work on
speeding it up - I'm not at all adverse to the idea of using distance the way
you're talking about, but I don't completely understand how it'd work yet and it
seems like something that could be done in a separate commit.  What'd be nice
about having this in master is that it'd be easier to spot in the regtests if
efficiency changes to the algorithm have a layout impact.  Of course, one could
run regtests with this patch as a baseline, but it is a pain.

[joeneeman, 2012-08-21 07:22:58]

On 2012/08/17 19:25:29, Keith wrote:
> On Fri, 17 Aug 2012 10:16:25 -0700, <mailto:mtsolo@gmail.com> wrote:
> 
> >
> >
>
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.c...
> > lily/axis-group-interface.cc:780: while (dirty);
> > On 2012/08/17 08:12:56, Keith wrote:
> >> I am beginning to understand the new code. Would you consider again using
> >> distance() instead of the repeated calls to intersects() in the
while(dirty)
> >> loop ?  It might look simpler now that you've been away from the code
> >> for awhile.
> 
> >> floors[j] = padding - forest[j][UP]~distance~pair[DOWN]
> >> ceilings[j] = forest[j][DOWN]~distance~pair[UP]
> 
> >> You know the solution will be to move an amount equal to one of the
> floors[j],
> >> and you want to find the smallest that fits, so make
> >>    trials = sort (floors)
> >> and find the smallest among trials that satisfies
> >>     trials[n] > 0
> >> &&  for all j (trials[n] > floors[j]
> >>                 || trials[n] < ceilings[j] )
> 
> > I am not adverse to using distance if possible - my trouble case is the
> > following.
> >
> > AAAAAAAAA
> >
> >      BBBBBBBBBBBBB
> >
> > Image that object A above is placed first and LilyPond is now
> > considering whether it should leave B where it is or place it above A.
> > The distance function would dictate that it needs to be shifted above A
> > because its DOWN skyline is below A's UP.
> 
> We need not let the distance function between A[UP] and B[DOWN] dictate,
because
> having positive distance between A[DOWN] and B[UP] is another solution.
> 
> Object AA, a member of the forest, sets two constraints
>         floor = padding  -  AA[UP] ~distance~ BB[DOWN]
>         ceiling = AA[DOWN] ~distance~ BB[UP]  - padding
> and we are checking that a trial location satisfies
>         (trial >= floor) OR (trial <= ceiling)
> 
> I suppose you show BB having been placed just above the staff, and that it
still
> lies below the ceiling set by AA, so this trial position satisfies the first
> test in the OR above, so we win.
> 
> All these distances can be computed once, before raising the skylines at all,
> because they tell you right away what trial values of raising BB will clear
AA. 
> That would be much simpler for people like me to understand than computing the
> four intersects() conditions, after having moved the skylines by zero or more
> bump(s).
> 

I've implemented this method in dev/jneem-skylines and it looks pretty neat.
Next, padding...

[Keith, 2012-08-21 07:32:15]

On 2012/08/18 10:12:00, MikeSol wrote:

> Agreed.  I'd love for this to go faster.

The goal is not speed, but comprehensibility of code.

The code is much shorter with direct use of distance() so there is less for the
human reader to hold in his head.  

There was, however, no measurable difference in speed on my piano-music test
case.

[Keith, 2012-08-21 07:34:06]

On 2012/08/21 07:32:15, Keith wrote:

> There was, however, no measurable difference in speed on my > piano-music test
case.

at least, no speed difference from my attempt at simplification
<http://codereview.appspot.com/6462087/>
I haven't tried Joe's.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:804: bool use_separate_constructor_skyline =
(horizon_padding != 0) & (padding != 0);
Should this AND be an OR ?
I am guessing you want a separate copy to alter with padding, of either type,
the way you put padding in the skylines themselves.

[Keith, 2012-08-21 07:42:38]

On 2012/08/18 10:12:00, MikeSol wrote:

> \relative c'' {
>    \override TupletBracket #'outside-staff-priority = #1
>    \override TupletNumber #'font-size = #5
>    \times 2/3 { a4\trill a\trill^"foo" a\trill }
> }
> 
> I added this as a regtest.  If you comment out the rider business, 
> you'll see that foo is printed on top of the tuplet number.

You gave TupletBracket a priority, but not TupletNumber.  Wouldn't it make more
sense to have
  \override TupletNumber #'outside-staff-priority = #1
as well ?

If I strip out the special-case code <http://codereview.appspot.com/6462087>
then all the Grobs honor the priorities I set for them.

[mike7, 2012-08-21 09:43:51]

On 21 août 2012, at 03:42, k-ohara5a5a@oco.net wrote:

> On 2012/08/18 10:12:00, MikeSol wrote:
> 
>> \relative c'' {
>>    \override TupletBracket #'outside-staff-priority = #1
>>    \override TupletNumber #'font-size = #5
>>    \times 2/3 { a4\trill a\trill^"foo" a\trill }
>> }
> 
>> I added this as a regtest.  If you comment out the rider business,
>> you'll see that foo is printed on top of the tuplet number.
> 
> You gave TupletBracket a priority, but not TupletNumber.  Wouldn't it
> make more sense to have
>  \override TupletNumber #'outside-staff-priority = #1
> as well ?
> 
> If I strip out the special-case code
> <http://codereview.appspot.com/6462087> then all the Grobs honor the
> priorities I set for them.
> 
> http://codereview.appspot.com/5626052/

It would make more sense, but I'm trying to cover all scenarios.  In the regtest
above, I think that "foo" should never collide w/ the tuplet number because of
outside-staff-priority being set for either TupletBracket or TupletNumber.

Cheers,
MS

[joeneeman, 2012-08-22 03:43:03]

On 2012/08/21 07:42:38, Keith wrote:
> On 2012/08/18 10:12:00, MikeSol wrote:
> 
> > \relative c'' {
> >    \override TupletBracket #'outside-staff-priority = #1
> >    \override TupletNumber #'font-size = #5
> >    \times 2/3 { a4\trill a\trill^"foo" a\trill }
> > }
> > 
> > I added this as a regtest.  If you comment out the rider business, 
> > you'll see that foo is printed on top of the tuplet number.
> 
> You gave TupletBracket a priority, but not TupletNumber.  Wouldn't it make
more
> sense to have
>   \override TupletNumber #'outside-staff-priority = #1
> as well ?
> 
> If I strip out the special-case code <http://codereview.appspot.com/6462087>
> then all the Grobs honor the priorities I set for them.


Have you tried setting TupletNumber's priority to be smaller than TupletBracket?
It results TupletNumber moving twice as far as it should (making it collide with
"foo"), because first it translates itself and then it gets moved again because
TupletBracket moves, and its position is measured relative to TupletBracket.

It would also be possible for some grob to insert itself between the
TupletBracket and the TupletNumber, which isn't particularly nice either. So I
think some form of "rider" functionality is necessary. I do it a little
differently in my branch, which even allows the TupletNumber to affect the
position of the TupletBracket.

Cheers,
Joe

[Keith, 2012-08-22 04:20:20]

Works nice.  
But so far the density of problems in the code as been pretty high.  These
problems seem to be in corner cases for which the code is (unnecessarily?)
complex.  It is not clear why the _use_ of skylines has to change so much at the
same time as the _shapes_ of skylines change.

Maybe Joe or someone will pick out the good parts. Otherwise, if we push it to
master we can probably fix any further problems as they are discovered.  My
guess is there will be fewer problems from this than there were from
'pure-from-neighbor', or the MIDI changes just before 2.14.

On 2012/08/21 09:43:51, mike7 wrote:
> On 21 août 2012, at 03:42, mailto:k-ohara5a5a@oco.net wrote:
> 
> > On 2012/08/18 10:12:00, MikeSol wrote:
> > 
> >> \relative c'' {
> >>   \override TupletBracket #'outside-staff-priority = #1
> >>   \override TupletNumber #'font-size = #5
> >>   \times 2/3 { a4\trill a\trill^"foo" a\trill }
> >> }

> > Wouldn't it make more sense to have
> >  \override TupletNumber #'outside-staff-priority = #1
> > as well ?

> It would make more sense, but I'm trying to cover all scenarios. 
>  In the regtest above, I think that "foo" should never collide
>  w/ the tuplet number because of outside-staff-priority being
>  set for either TupletBracket or TupletNumber.

You are entitled to your opinion, but if you leave out the "foo" the
TupletNumber collides with the trill.  I think I should be able to do something
about that.  If I do the sensible thing and give the TupletNumber the same
outside-staff-priority as the bracket, your new code says "can't have that" and
continues to collide them.

Maybe we can compromise and give a priority-less child the priority of his
parent ... and of course drop all the book-keeping about 'riders'.

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/5626052/diff/104124/lily/axis-group-interface.c...
lily/axis-group-interface.cc:980: vertical_skyline_forest[d].push_back
(doctored_skylines);
This selects only the outer skyline of the on-staff objects, so if you get
Russian-egg-doll packing you won't notice.  

The tucking of dynamics inside slurs in 'slur-dynamics.ly' looks good, but with
a slight change we see it is really a bug :
  \relative c' {  b( d\( f\)\p b,)  g( d'\(\< d\)\! g,) }

We can get dynamics to tuck under slurs safely, if we
  \override Slur #'outside-staff-priority = #1

[Keith, 2012-08-28 04:08:11]

Still works well, still the same speed, and now the code makes much more sense.

The 25% extra time required to set a short score goes down if I 
    \override Accidental #'vertical-skylines = #'()
    \override TextScript #'vertical-skylines = #'()
    \override TextScript #'horizontal-skylines = #'()
    \override Script #'vertical-skylines = #'()
    ...
seemingly in proportion to the number of objects for which I give up detailed
skylines.

After the simplification, many functions in skyline- and skyline-pair.cc are
unused, and thus untested :
left(), right(), intersects(), smallest_shift(), is_singleton(), invert().

[mike7, 2012-08-28 08:00:49]

On 28 août 2012, at 06:08, k-ohara5a5a@oco.net wrote:

> Still works well, still the same speed, and now the code makes much more
> sense.
> 
> The 25% extra time required to set a short score goes down if I
>    \override Accidental #'vertical-skylines = #'()
>    \override TextScript #'vertical-skylines = #'()
>    \override TextScript #'horizontal-skylines = #'()
>    \override Script #'vertical-skylines = #'()
>    ...
> seemingly in proportion to the number of objects for which I give up
> detailed skylines.
> 

It may be worth it from a UI perspective to come up with a
\cruderVerticalSkylineApproximationsForFasterCompilation command (or something
of that ilk).  I am bad at UI stuff, so I'll let someone else think of a good
thing to call this and good vertical-skylines to override (you should set them
to ly:grob::simple-vertical-skylines-from-stencil instead of '()).  The list you
have above looks like a good start - I'd add LyricText to it as well.  Janek
would be able to come up w/ an exhaustive list.

> After the simplification, many functions in skyline- and skyline-pair.cc
> are unused, and thus untested :
> left(), right(), intersects(), smallest_shift(), is_singleton(),
> invert().
> 

Got rid of the cruft in staging.  Most utility functions are a dime a dozen.
Using them intelligently is what's hard. So people can just rewrite these things
if they need them.

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

Cheers,
MS

[Keith, 2012-09-16 18:52:55]

http://codereview.appspot.com/5626052/diff/114002/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/5626052/diff/114002/lily/skyline.cc#newcode111
lily/skyline.cc:111: if (start_height != end_height)
Does anyone know the reason for this change?

For some unusual input (issue 2836)
 { \override NoteHead #'stencil = ##f b2 b  } 
both _heights can be NaNs, due to some other change in this patch.   

This re-arrangement of logic looks like it /might/ have been intended to set
slope_=0 for NaNs on input, but in fact we get slope_=NaN and are trapped at the
assertion below.  (My reading of the standard says NaN==NaN is false, NaN!=NaN
is true, so gcc seems to comply.)