Code review - Issue 5626052: Gets vertical skylines from grob stencilshttps://codereview.appspot.com/2012-09-16T18:52:55+00:00rietveld
Message from unknown
2012-02-04T21:18:02+00:00MikeSolurn:md5:2633c3f566aee5bf23bd98e516aede52
Message from mtsolo@gmail.com
2012-02-04T21:28:16+00:00MikeSolurn:md5:a560d5ddc8bb1ac1dd4b4d8184d69b40
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
Message from unknown
2012-02-05T10:36:04+00:00MikeSolurn:md5:53b720ca98f276d3485afd236734114b
Message from unknown
2012-02-05T10:46:43+00:00MikeSolurn:md5:fccefccd7a2947be1abf6c83323ff95b
Message from unknown
2012-02-05T19:33:43+00:00MikeSolurn:md5:4277ee1a06722c7e850b860ce27f6eff
Message from unknown
2012-02-05T19:49:28+00:00MikeSolurn:md5:f902cef44ef2390db5522b6e39892291
Message from unknown
2012-02-05T22:38:56+00:00MikeSolurn:md5:a0beed278d6c3db598ce271764343c34
Message from unknown
2012-02-06T07:51:27+00:00MikeSolurn:md5:54a27438c3bcb509b2164ba111fdfcb1
Message from unknown
2012-02-06T10:38:57+00:00MikeSolurn:md5:620a1a45b63fd6609105c84d77e83672
Message from unknown
2012-02-06T11:31:19+00:00MikeSolurn:md5:cbd6ae775c54be0126a79bcdc62df332
Message from unknown
2012-02-06T12:11:41+00:00MikeSolurn:md5:eeff58dc6ca32a627eccc65689927557
Message from unknown
2012-02-06T16:41:41+00:00MikeSolurn:md5:4e4759f306d0c7191492dcb03da3a1d0
Message from mtsolo@gmail.com
2012-02-06T16:42:47+00:00MikeSolurn:md5:efcf89233c72f8aab89d6c10ed80877b
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
Message from mtsolo@gmail.com
2012-02-06T16:59:35+00:00MikeSolurn:md5:1624557df0d505e8a315ac3f9a651b3b
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
Message from mail@philholmes.net
2012-02-06T17:06:16+00:00mail_philholmes.neturn:md5:7be69754915760931869cb52257997b2
----- 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
Message from unknown
2012-02-06T17:27:45+00:00MikeSolurn:md5:cfd6df676ee76b214d0c635c7e3ea128
Message from n.puttock@gmail.com
2012-02-06T17:36:06+00:00Neil Puttockurn:md5:a6261eb652b8841748748a50e01fee19
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
Message from mike@apollinemike.com
2012-02-06T17:50:59+00:00mike_apollinemike.comurn:md5:420b0680cc04ed429f172842863a6b56
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
Message from wl@gnu.org
2012-02-06T18:45:56+00:00wl_gnu.orgurn:md5:76e11b0dda3eef9c0d485f12cfa2913d
>> 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
Message from graham@percival-music.ca
2012-02-06T18:52:38+00:00Graham Percivalurn:md5:779d7c477a92b4ac45445e0fa08c5043
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
Message from joeneeman@gmail.com
2012-02-06T19:00:55+00:00joeneemanurn:md5:5c7b45d07ddc27e928d419e08712a4d5
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/mailman/listinfo/lilypond-devel>
>
Message from mike@apollinemike.com
2012-02-06T19:25:34+00:00mike_apollinemike.comurn:md5:62200e3b171eac8856624d00697abb56
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
Message from janek.lilypond@gmail.com
2012-02-06T22:23:08+00:00janekurn:md5:07646ec401b7f1c6b73780d3b881d53d
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
Message from unknown
2012-02-14T09:24:02+00:00MikeSolurn:md5:48e5c4da96433edc2765425a250703b9
Message from mtsolo@gmail.com
2012-02-14T09:26:55+00:00MikeSolurn:md5:0448d70e9f7f46fd0c862945da46fa9b
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.
Message from janek.lilypond@gmail.com
2012-02-14T16:11:44+00:00janekurn:md5:f523f76cd9122b197dc813ba9a331854
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
Message from mike@apollinemike.com
2012-02-14T18:17:33+00:00mike_apollinemike.comurn:md5:73f0d1ae6a52a1a0b8aea630efd80e2f
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
Message from janek.lilypond@gmail.com
2012-02-14T18:46:41+00:00janekurn:md5:9e6a04ec44bc1e1e8fd053e60336f0b8
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
Message from mike@apollinemike.com
2012-02-15T01:06:06+00:00mike_apollinemike.comurn:md5:803c985337297161e250bf93f335ccc8
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
Message from janek.lilypond@gmail.com
2012-02-15T06:00:51+00:00janekurn:md5:09a6d3b6ea6afb584f8af17ec7e6d475
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
Message from mike@apollinemike.com
2012-02-15T07:07:56+00:00mike_apollinemike.comurn:md5:8d5a6379f7149f08a43bc8b368d05c57
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
Message from janek.lilypond@gmail.com
2012-02-15T07:25:23+00:00janekurn:md5:739731e185d4f95829d5346df3b2c6b7
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.
Message from mike@apollinemike.com
2012-02-15T07:37:15+00:00mike_apollinemike.comurn:md5:1df0749ae14cf8c4bcc9e50303134a4e
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
Message from janek.lilypond@gmail.com
2012-02-15T07:49:13+00:00janekurn:md5:c7b2da4c40e2d976d44e5ea6681a0fdc
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
Message from mike@apollinemike.com
2012-02-15T07:59:14+00:00mike_apollinemike.comurn:md5:cd40d336dcb05a147c1ca597c7cee435
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
Message from janek.lilypond@gmail.com
2012-02-15T08:11:07+00:00janekurn:md5:091f7c84c31c12eab3cc56d26277dbc3
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
Message from unknown
2012-02-15T08:13:23+00:00MikeSolurn:md5:eb7935fa4b1af7b780434cbfee45f2e8
Message from mike@apollinemike.com
2012-02-15T08:17:26+00:00mike_apollinemike.comurn:md5:c1e2f4f26fe1623c126f1188e5cdf2bf
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
Message from janek.lilypond@gmail.com
2012-02-15T09:29:46+00:00janekurn:md5:6b4699f52fa6964bbe217fdde85e7862
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
Message from mike@apollinemike.com
2012-02-15T10:13:55+00:00mike_apollinemike.comurn:md5:00d0f42e3163f04ddabbbbca78d1944e
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
Message from graham@percival-music.ca
2012-02-15T10:26:17+00:00Graham Percivalurn:md5:b843629fc4d2a649fb17a8704520bb5f
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?
Message from mike@apollinemike.com
2012-02-15T10:31:56+00:00mike_apollinemike.comurn:md5:8827d1726a5175235d0e8ccf2ca5ff3a
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.
Message from reinhold@kainhofer.com
2012-02-15T14:25:57+00:00reinhold_kainhofer.comurn:md5:62719b0e7ad9347fde0e3370fafa2e7d
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
Message from joeneeman@gmail.com
2012-02-16T08:09:10+00:00joeneemanurn:md5:fa2b1e19e710577e8dcbc303f68e2576
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#newcode659
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#newcode543
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.
Message from mtsolo@gmail.com
2012-02-16T10:08:39+00:00MikeSolurn:md5:029afae394e02cab56effb61c90ba176
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#newcode659
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#newcode543
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.
Message from joeneeman@gmail.com
2012-02-16T10:40:19+00:00joeneemanurn:md5:12e316fa8936199dba6b7ea5bc05e307
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#newcode659
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.
Message from mike@apollinemike.com
2012-02-16T11:14:44+00:00mike_apollinemike.comurn:md5:473344d70ea0afc6ac0c15345a6988eb
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
Message from hanwenn@gmail.com
2012-02-16T12:16:16+00:00hanwennurn:md5:393d6fda6e463351f158af36e9cee6a8
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
Message from hanwenn@gmail.com
2012-02-16T12:21:45+00:00hanwennurn:md5:93f1c8d6c488fc5012e875b0d27a786b
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
Message from janek.lilypond@gmail.com
2012-02-16T12:42:17+00:00janekurn:md5:afb93dd72d703a63726c80613b26d146
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
Message from joeneeman@gmail.com
2012-02-16T20:31:09+00:00joeneemanurn:md5:b76228a376a2274478e1c8382331808a
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
Message from unknown
2012-02-16T23:13:30+00:00MikeSolurn:md5:7383cb6e3e583e8d48172ced80f94b05
Message from mike@apollinemike.com
2012-02-17T07:42:54+00:00mike_apollinemike.comurn:md5:36d0b83061a4d004b2c373368db0d596
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
Message from mike@apollinemike.com
2012-02-17T07:47:23+00:00mike_apollinemike.comurn:md5:ae8f950541c4e89b886eec13f501a146
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
Message from mike@apollinemike.com
2012-02-17T08:00:53+00:00mike_apollinemike.comurn:md5:326ea7c9c3ea13a257778d0c73b32e23
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
Message from joeneeman@gmail.com
2012-02-17T08:14:33+00:00joeneemanurn:md5:d62fbc3242d875a0080c0eb409fea29b
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...
Message from joeneeman@gmail.com
2012-02-17T08:22:49+00:00joeneemanurn:md5:2a1658f6f98423ed924be2fc229eadb6
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>&'
Message from unknown
2012-02-17T08:37:39+00:00MikeSolurn:md5:14f88fcbfbe87095e82685f7e097d199
Message from mike@apollinemike.com
2012-02-17T08:39:01+00:00mike_apollinemike.comurn:md5:5e4caee4ca3ecd6fd441b6e15d196b95
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
Message from reinhold@kainhofer.com
2012-02-17T10:47:04+00:00reinhold_kainhofer.comurn:md5:916621538a4bd2c541627daa43f5b4c6
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
Message from hanwenn@gmail.com
2012-02-17T12:49:23+00:00hanwennurn:md5:57db9b047f3ccc6f9644df3a6774e742
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
Message from hanwenn@gmail.com
2012-02-17T12:58:11+00:00hanwennurn:md5:6e65bf4702c4da72e7ef72ad00360f4f
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
Message from unknown
2012-02-17T15:28:42+00:00MikeSolurn:md5:2634e0778d396deb323c0a961b23ac88
Message from unknown
2012-02-17T15:44:31+00:00MikeSolurn:md5:1b66aa18384ca6af70261a99ddc69b78
Message from unknown
2012-02-18T20:12:18+00:00MikeSolurn:md5:68a3c2f3ad65342d77f248923e0873b5
Message from mike@apollinemike.com
2012-02-18T21:17:04+00:00mike_apollinemike.comurn:md5:f39e7bba051abe0f3fe12afbe589a581
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
Message from unknown
2012-02-19T09:37:19+00:00MikeSolurn:md5:74a31ebe5fa35368c069583d09dfbadc
Message from unknown
2012-02-19T19:38:41+00:00MikeSolurn:md5:dfeed6b2f16eb94c449ee9e6feee88b2
Message from janek.lilypond@gmail.com
2012-02-22T08:04:26+00:00janekurn:md5:c4c87c9874820db44a787b36344037e1
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
Message from janek.lilypond@gmail.com
2012-02-22T08:07:59+00:00janekurn:md5:508e9d7e8fc4260a1e39806ea34c5f3e
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
Message from unknown
2012-02-22T08:09:24+00:00MikeSolurn:md5:8347a3e5d62675bbca5dabe3e9068382
Message from mike@apollinemike.com
2012-02-22T08:18:25+00:00mike_apollinemike.comurn:md5:928786bcdf8c9e3729ed25a5fb19441d
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
Message from joeneeman@gmail.com
2012-02-22T09:34:43+00:00joeneemanurn:md5:1c80330e969c763c0124641acc193d26
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.
Message from unknown
2012-02-22T15:57:01+00:00MikeSolurn:md5:d2bc42ee412ad5a1a86a9dbef12de333
Message from mike@apollinemike.com
2012-02-22T15:59:16+00:00mike_apollinemike.comurn:md5:93a602ecafd5da65abde6ce6b651a38d
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
Message from janek.lilypond@gmail.com
2012-02-22T16:37:56+00:00janekurn:md5:29b4bc2bb88c11a6e0eb51c037f36558
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
Message from mike@apollinemike.com
2012-02-22T16:47:48+00:00mike_apollinemike.comurn:md5:d59b04f6c107e535675ebf981a9b3985
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
Message from dak@gnu.org
2012-02-22T17:36:24+00:00dakurn:md5:dae38c3cf34cac58f956392cbb3c3d88
"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
Message from unknown
2012-02-23T09:30:25+00:00MikeSolurn:md5:a598124eb65526eaab6e596e5ff3870a
Message from mike@apollinemike.com
2012-02-23T09:35:53+00:00mike_apollinemike.comurn:md5:e6e72af5952bcac116cc21a009991f00
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
Message from joeneeman@gmail.com
2012-02-23T11:11:27+00:00joeneemanurn:md5:c5b7e7ea58ed0e65bca2458c09f33446
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
Message from joeneeman@gmail.com
2012-02-23T11:11:47+00:00joeneemanurn:md5:e5d3283ce28f1e9476eea18ad3e5f52c
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.
Message from mtsolo@gmail.com
2012-02-23T11:13:42+00:00MikeSolurn:md5:17361ac4d1196eee750c259afe77a6c5
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.
Message from unknown
2012-02-23T12:22:14+00:00MikeSolurn:md5:b68abb329f0d6318563388213a641b1e
Message from dak@gnu.org
2012-02-23T12:29:06+00:00dakurn:md5:f8cc51ab0ce732c0006715e85432c9a9
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#newcode932
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.
Message from mtsolo@gmail.com
2012-02-23T12:37:04+00:00MikeSolurn:md5:50518f7f2cdd28731c9090f4a4728e28
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#newcode932
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.
Message from dak@gnu.org
2012-02-23T13:35:29+00:00dakurn:md5:89d79e3d1a37db42820d53a3e68f4af2
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#newcode932
> 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.
Message from mtsolo@gmail.com
2012-02-23T14:09:12+00:00MikeSolurn:md5:ed2271b176aae55ffa0a8c69677ac615
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#newcode932
> > 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.
Message from dak@gnu.org
2012-02-23T14:36:50+00:00dakurn:md5:0df259288ad4e5fcd632105b370c8c3f
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#newcode932
> > > 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.
Message from n.puttock@gmail.com
2012-02-23T17:22:06+00:00Neil Puttockurn:md5:2b28e3f77cf7db4c8bed645b015a9899
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
Message from joeneeman@gmail.com
2012-02-23T20:47:58+00:00joeneemanurn:md5:926b114faa584d74869238fa258a8a73
On Thu, Feb 23, 2012 at 3:13 AM, <mtsolo@gmail.com> wrote:
>
> http://codereview.appspot.com/**5626052/diff/30003/lily/**skyline.cc<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<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://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<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...
Message from janek.lilypond@gmail.com
2012-02-23T23:07:47+00:00janekurn:md5:68b7726e340080fdbfe71980ab60703f
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
Message from unknown
2012-02-23T23:47:15+00:00MikeSolurn:md5:1e6fa04f8c7e054f6e54551549d33e25
Message from mike@apollinemike.com
2012-02-23T23:50:16+00:00mike_apollinemike.comurn:md5:b51d148fc7abec1b98c648788d2a12d3
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
Message from janek.lilypond@gmail.com
2012-02-26T12:09:08+00:00janekurn:md5:e69f7534737b552dac9be95d049d37c8
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-attached-to-notes#new-dynamic-marks)
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
Message from mike@apollinemike.com
2012-02-26T15:51:20+00:00mike_apollinemike.comurn:md5:0e23f1a627c52a1be2cd4ef9e522cb52
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
Message from dak@gnu.org
2012-02-26T15:54:46+00:00dakurn:md5:bfa58ecfa68a291fda3ab9441bc66151
"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
Message from wl@gnu.org
2012-02-26T18:37:35+00:00wl_gnu.orgurn:md5:f3f2885b63d4002d208c87eb59a03596
>>> 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
Message from mike@apollinemike.com
2012-02-26T19:25:53+00:00mike_apollinemike.comurn:md5:222d911c84d39bef02fc80d1b895b6bf
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
Message from joeneeman@gmail.com
2012-02-26T21:28:25+00:00joeneemanurn:md5:09e6c5b285d5319cc2a2b3a728a45aec
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!
Message from janek.lilypond@gmail.com
2012-02-26T22:16:34+00:00janekurn:md5:cea5f457380f677561346ca8d48dc8cd
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
Message from unknown
2012-02-28T12:26:58+00:00MikeSolurn:md5:dec948172efce473c75dcf91e8164655
Message from mike@apollinemike.com
2012-02-28T12:32:22+00:00mike_apollinemike.comurn:md5:882400f84809202457e3bae734c4b3cc
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
Message from unknown
2012-02-28T21:29:31+00:00MikeSolurn:md5:0972aa34bb5857ea18c13837a76f351a
Message from mike@apollinemike.com
2012-02-28T21:33:06+00:00mike_apollinemike.comurn:md5:53381e55f6de30df6d92afa3f893c81b
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
Message from janek.lilypond@gmail.com
2012-02-28T22:16:44+00:00janekurn:md5:47d4158d7c1b7c78485009fab61aa8be
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
Message from joeneeman@gmail.com
2012-02-29T01:05:54+00:00joeneemanurn:md5:ac5da36062b49227085ef740fd2a09a8
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?
Message from unknown
2012-02-29T08:57:33+00:00MikeSolurn:md5:966d44e7bfcd71687e29db5c6d490e68
Message from unknown
2012-02-29T10:15:14+00:00MikeSolurn:md5:9b9310c6d999fdf54817532207f11b27
Message from mike@apollinemike.com
2012-02-29T10:17:55+00:00mike_apollinemike.comurn:md5:b46ca3e059ff7c283aeb7d86737df797
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
Message from janek.lilypond@gmail.com
2012-03-01T00:01:25+00:00janekurn:md5:18ec64beaa41ca34ba0788451bc4e15b
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
Message from mike@apollinemike.com
2012-03-01T07:14:50+00:00mike_apollinemike.comurn:md5:3540e0e3f5b083de6def9cb9bb12fce6
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
Message from janek.lilypond@gmail.com
2012-03-01T07:53:01+00:00janekurn:md5:3081a8464c108839ec89e11a50cb6a6f
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
Message from wl@gnu.org
2012-03-01T08:09:17+00:00wl_gnu.orgurn:md5:c8d19b6be1af7445b0969dd19b5b8090
> 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
Message from unknown
2012-03-01T08:50:59+00:00MikeSolurn:md5:ab83743c7165821aa9d77eaad0b683c8
Message from hanwenn@gmail.com
2012-03-01T10:52:00+00:00hanwennurn:md5:d40acbb48ef0651287fa454637e8bfd8
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
Message from dak@gnu.org
2012-03-01T10:57:19+00:00dakurn:md5:ed1e1f667250dc6ef7141af5a6a7209b
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
Message from hanwenn@gmail.com
2012-03-01T10:59:02+00:00hanwennurn:md5:b68344a4384699540791ba8572775713
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
Message from mike@apollinemike.com
2012-03-01T11:00:16+00:00mike_apollinemike.comurn:md5:a183fc9fc9d1d6c8f975ef5fa0912282
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
Message from mike@apollinemike.com
2012-03-01T11:00:36+00:00mike_apollinemike.comurn:md5:b7d852132f3d53ab3018cb096f932556
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
Message from t.daniels@treda.co.uk
2012-03-01T11:17:31+00:00t.daniels_treda.co.ukurn:md5:78f71aec45710c5a47e7f54093bbae9f
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
Message from mike@apollinemike.com
2012-03-01T11:28:01+00:00mike_apollinemike.comurn:md5:28ac938f88686d004a715b9e8e104bd8
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
Message from unknown
2012-03-01T14:45:19+00:00MikeSolurn:md5:fa7e95d9b146643c0271e204e233b314
Message from janek.lilypond@gmail.com
2012-03-01T20:56:22+00:00janekurn:md5:f17270640b4633da3ac432ec71537a37
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
Message from unknown
2012-03-02T17:37:02+00:00MikeSolurn:md5:ba2469b249b02d521ec2f039633b3aaa
Message from unknown
2012-03-02T17:53:02+00:00MikeSolurn:md5:a22b7d31c267cf7ad264eba295bff170
Message from unknown
2012-03-03T07:29:33+00:00MikeSolurn:md5:c6445cb5872ca9d9c31aba35b37a3012
Message from unknown
2012-03-03T21:45:20+00:00MikeSolurn:md5:c8cf224fa933bfd5f9f4c9e9a0fddaff
Message from janek.lilypond@gmail.com
2012-03-06T14:18:03+00:00janekurn:md5:c28b6df4d638e85a677bf60e4e4faf22
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
Message from joeneeman@gmail.com
2012-03-06T22:48:39+00:00joeneemanurn:md5:f686f2fa889371f425551b21b4fbe1ab
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
Message from janek.lilypond@gmail.com
2012-03-07T07:56:46+00:00janekurn:md5:7dd280f2dc8a0177055a74b82d3c9db6
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
Message from joeneeman@gmail.com
2012-03-07T10:43:31+00:00joeneemanurn:md5:87361320a39026ddf086f54cb850ea74
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
Message from janek.lilypond@gmail.com
2012-03-07T21:32:47+00:00janekurn:md5:418ddacc69b9e1cfe14167c5c14f9e88
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
Message from joeneeman@gmail.com
2012-03-07T21:50:21+00:00joeneemanurn:md5:2c924f628158c31b57ab609c2d95d65b
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
Message from janek.lilypond@gmail.com
2012-03-07T22:09:16+00:00janekurn:md5:30323b8d11b19d09d641ae4d0065928e
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
Message from mike@apollinemike.com
2012-03-08T09:04:58+00:00mike_apollinemike.comurn:md5:34b48de9c694e353a9a62760d11fa64b
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
Message from unknown
2012-07-01T14:55:19+00:00MikeSolurn:md5:ead382f0717644243a1b6b34bb2378e9
Message from mtsolo@gmail.com
2012-07-01T14:58:08+00:00MikeSolurn:md5:49e1cf6e80d10b663e34b26ff0f42fd6
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
Message from k-ohara5a5a@oco.net
2012-07-01T23:03:56+00:00Keithurn:md5:1d79e243a86a7ff3d2a069e1debcc70a
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 ?
Message from k-ohara5a5a@oco.net
2012-07-01T23:04:16+00:00Keithurn:md5:39cebfc30229a3a92561adb1e3c30d92
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.
Message from mtsolo@gmail.com
2012-07-02T04:58:21+00:00MikeSolurn:md5:913a54c0b711475c476a44affcbd91c2
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...
Message from janek.lilypond@gmail.com
2012-07-03T08:19:17+00:00janekurn:md5:1cd7f32983b5c0295b3f2ff035cf9d59
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.tar.gz
(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
Message from mike@apollinemike.com
2012-07-03T17:51:49+00:00mike_apollinemike.comurn:md5:5cbce49e2c73513a937235cf5abedcec
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
Message from k-ohara5a5a@oco.net
2012-07-04T01:18:05+00:00Keithurn:md5:c6554f3e66c10620370c35671f73b857
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.
Message from janek.lilypond@gmail.com
2012-07-04T07:34:41+00:00janekurn:md5:27b3271590e4a921e48043e2cb04c824
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
Message from mike@apollinemike.com
2012-07-04T10:54:21+00:00mike_apollinemike.comurn:md5:1d75040bccddf1466a044e3a6d56ea04
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.tar.gz
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
Message from janek.lilypond@gmail.com
2012-07-04T13:17:37+00:00janekurn:md5:f09b7fd52d57888d4a543a1c811d6dde
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_patchset-36.pdf
http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patchset-37.pdf
>> - 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_demo.pdf
What do you think?
cheers,
Janek
Message from mike@apollinemike.com
2012-07-04T13:36:15+00:00mike_apollinemike.comurn:md5:7c9ad4e47eb2483c5d4f0de11d16e22c
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_patchset-36.pdf
> http://lilypond-stuff.1065243.n5.nabble.com/file/n5705595/weird-skylines_patchset-37.pdf
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_demo.pdf
>
> 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
Message from mail@philholmes.net
2012-07-04T13:52:14+00:00mail_philholmes.neturn:md5:4cc54aff81be71dbe419c3c08310224f
----- 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
Message from janek.lilypond@gmail.com
2012-07-04T18:58:30+00:00janekurn:md5:e294df25683e2d09352bc1113246efc9
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_demo.pdf
>>
>> 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_-_padding.pdf
(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
Message from unknown
2012-07-05T15:21:48+00:00MikeSolurn:md5:1b0b5af2316a6ba8197d4fd6f4f13cb5
Message from mike@apollinemike.com
2012-07-05T15:23:25+00:00mike_apollinemike.comurn:md5:c9b79cdb2f0ee0829919fe090a1ea22e
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_demo.pdf
>>>
>>> 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_-_padding.pdf
> (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
Message from janek.lilypond@gmail.com
2012-07-05T16:05:57+00:00janekurn:md5:15f87b85ec0b43b4322ea7866d542297
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_skylines.pdf
LOL
hth,
Janek
(and script-stack-order.ly is still wrong)
Message from unknown
2012-07-09T06:19:45+00:00MikeSolurn:md5:3889dc766e959d02b8851f999bcff7e5
Message from dak@gnu.org
2012-07-09T08:42:16+00:00dakurn:md5:cc550a2140d6ae2251b96212465e6368
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#newcode726
lily/axis-group-interface.cc:726:
Whitespace in empty line.
http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc#newcode767
lily/axis-group-interface.cc:767:
Whitespace in empty line.
http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc#newcode798
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.
Message from unknown
2012-07-09T09:56:03+00:00MikeSolurn:md5:67d83f59800bac351ec2cb6d8ea87157
Message from unknown
2012-07-14T09:47:48+00:00MikeSolurn:md5:f81f77b85ea43f4b8cc7220a3a490d4b
Message from joeneeman@gmail.com
2012-07-14T14:01:08+00:00joeneemanurn:md5:758970987a3595c67481190de08d2b2b
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#newcode58
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.
Message from mtsolo@gmail.com
2012-07-14T18:33:10+00:00MikeSolurn:md5:d4903b930b31d846bca38cc944e8387b
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#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?
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.
Message from joeneeman@gmail.com
2012-07-15T15:30:03+00:00joeneemanurn:md5:1e0e4b0a96818bca9371b0e3d3cc28fa
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
Message from unknown
2012-08-12T15:38:21+00:00MikeSolurn:md5:d5ef380d514535216a7b24bf574d81c0
Message from unknown
2012-08-13T07:26:13+00:00MikeSolurn:md5:fc42439405fc43589729e6954c3bbed4
Message from k-ohara5a5a@oco.net
2012-08-14T04:21:32+00:00Keithurn:md5:1699f0369974d86a23468c089fc0b832
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-script-vertical-skylines.ly
File input/regression/text-script-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly#newcode5
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-bracket-vertical-skylines.ly
File input/regression/volta-bracket-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly#newcode4
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.cc#newcode629
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.cc#newcode658
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.cc#newcode728
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.cc#newcode751
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.cc#newcode778
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.cc#newcode843
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.cc#newcode890
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#newcode103
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#newcode663
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#newcode913
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#newcode1590
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 ?
Message from unknown
2012-08-16T04:06:02+00:00MikeSolurn:md5:d3ec4730eb88726c97a8765ddd53be02
Message from mtsolo@gmail.com
2012-08-16T04:15:38+00:00MikeSolurn:md5:a212436e0dd8133d22f62ff75cace23b
Thanks for the review!
http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly
File input/regression/text-script-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/text-script-vertical-skylines.ly#newcode5
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-bracket-vertical-skylines.ly
File input/regression/volta-bracket-vertical-skylines.ly (right):
http://codereview.appspot.com/5626052/diff/105001/input/regression/volta-bracket-vertical-skylines.ly#newcode4
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.cc#newcode629
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.cc#newcode658
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.cc#newcode728
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.cc#newcode751
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.cc#newcode778
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.cc#newcode843
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.cc#newcode890
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#newcode103
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#newcode663
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#newcode913
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#newcode1590
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.
Message from mtsolo@gmail.com
2012-08-16T06:21:40+00:00MikeSolurn:md5:6728f4d9941366086efb04e6b6fa92c1
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.cc#newcode778
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.
Message from unknown
2012-08-17T00:12:50+00:00MikeSolurn:md5:047aee7a770e11f886b3814f3b6a30ba
Message from k-ohara5a5a@oco.net
2012-08-17T08:12:56+00:00Keithurn:md5:33454fbeabd38b2c4c7f544ed2dc716b
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.cc#newcode636
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.cc#newcode706
lily/axis-group-interface.cc:706: do
// ... while dirty
http://codereview.appspot.com/5626052/diff/106004/lily/axis-group-interface.cc#newcode746
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.cc#newcode780
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.cc#newcode945
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?
Message from joeneeman@gmail.com
2012-08-17T11:19:26+00:00joeneemanurn:md5:c8df07680715f261f312bad681ed33d8
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.cc#newcode651
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.cc#newcode658
lily/axis-group-interface.cc:658: are avoided
Have you measured the costs of not having exempt?
Message from mike@mikesolomon.org
2012-08-17T14:21:21+00:00mike7urn:md5:ebb732db2194e39d2ad03fd92ff6e043
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.cc#newcode651
> 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.cc#newcode658
> 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
Message from mike@mikesolomon.org
2012-08-17T14:28:50+00:00mike7urn:md5:709bf081501f2116b1a9ca0d8263e74d
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.cc#newcode651
>> 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.cc#newcode658
>> 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
}
Message from mtsolo@gmail.com
2012-08-17T17:16:25+00:00MikeSolurn:md5:d2ce70c0b8b2d6e27ef69557573c6a6c
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.cc#newcode658
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.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 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.cc#newcode945
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.
Message from unknown
2012-08-17T19:24:23+00:00MikeSolurn:md5:b60fd1e54267be82667a16af81ff16f3
Message from k-ohara5a5a@oco.net
2012-08-17T19:25:29+00:00Keithurn:md5:2ab86069180ea95cccd7cd0bc300c7dc
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
> 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.
Message from joeneeman@gmail.com
2012-08-17T20:57:51+00:00joeneemanurn:md5:13511d8ce734fd2498b3f3c2678818d7
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
Message from mike@mikesolomon.org
2012-08-17T22:50:48+00:00mike7urn:md5:02ed1733ddc435055c3b942a9ee226d2
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.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
>
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
Message from k-ohara5a5a@oco.net
2012-08-18T02:42:37+00:00Keithurn:md5:28b716ec8759f86e6d7e3e1fdb5171f7
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.cc#newcode945
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.cc#newcode697
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.cc#newcode701
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.cc#newcode732
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
Message from mtsolo@gmail.com
2012-08-18T10:12:00+00:00MikeSolurn:md5:317ae18b42e61935169970adcc37c264
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.cc#newcode945
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.cc#newcode697
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.cc#newcode701
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.cc#newcode732
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.
Message from joeneeman@gmail.com
2012-08-21T07:22:58+00:00joeneemanurn:md5:c3706c5d57ed2da5f0e9a2a62438f85c
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.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.
>
> 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...
Message from k-ohara5a5a@oco.net
2012-08-21T07:32:15+00:00Keithurn:md5:bda7a7d5f42bac9c091f9bf727a34487
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.
Message from k-ohara5a5a@oco.net
2012-08-21T07:34:06+00:00Keithurn:md5:4b7e4d669900d98ec34d7b8eb7efa8f5
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.cc#newcode804
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.
Message from k-ohara5a5a@oco.net
2012-08-21T07:42:38+00:00Keithurn:md5:fc2cf26f51ae54b08fdbc3823062de94
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.
Message from mike@mikesolomon.org
2012-08-21T09:43:51+00:00mike7urn:md5:7e3c41a706986b61d1372e1e02eb8b63
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
Message from joeneeman@gmail.com
2012-08-22T03:43:03+00:00joeneemanurn:md5:d43071739d2fc3482aa863b81c04aa8d
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
Message from k-ohara5a5a@oco.net
2012-08-22T04:20:20+00:00Keithurn:md5:93cfe47f5751d2e8a2c24f64da1d138c
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.cc#newcode980
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
Message from unknown
2012-08-25T15:46:38+00:00MikeSolurn:md5:065b3d1fc2ba08580803619e04bf4cea
Message from unknown
2012-08-25T17:51:44+00:00MikeSolurn:md5:2a5a3a9467d9aa2749580dd5f8e7da05
Message from k-ohara5a5a@oco.net
2012-08-28T04:08:11+00:00Keithurn:md5:9c82f16e570f3b4d3200a05a066ce7d1
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().
Message from mike@mikesolomon.org
2012-08-28T08:00:49+00:00mike7urn:md5:c8b93eb8991920b505de65d782738846
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
Message from k-ohara5a5a@oco.net
2012-09-16T18:52:55+00:00Keithurn:md5:80f7be4a83e6f3c422122afee902432c
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.)