Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group.

http://codereview.appspot.com/6498077

Patchy the autobot says: passes tests.

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group.

http://codereview.appspot.com/6498077

This newest incarnation of the patch compiles Keith's test-case cleanly:

http://www.mutopiaproject.org/cgibin/piece-info.cgi?id=1776

It also does a better job with the regtest I added.

I have no clue why there is a regtest diff with a tab voice example (it's the only one) and as I can't actually see anything that's changed it's hard for me to make heads or tails of it. I'm not bothered by it if no one else is.

If people dig this approach, the same thing can be done for all cross-staff spanners (beams, tuplet-brackets, etc.).

Patchy the autobot says: passes tests. attached reg test in next comment

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group.

http://codereview.appspot.com/6498077

Mike this patch fails make test. I cannot work out which test it is on - the way patchy results are not organized is something I am still getting familiar with, however it looks like it is something in the MusicXML reg tests.

sorry that is best I can do at the moment (if you were waiting on the results).

James

Thanks James - it actually made it through on my system, so I'll run it again. I made some changes since the last patch, so hopefully the segfault fairy will smile upon my changes and push it through make check.

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

The approximation is almost always an overshoot for the time being. A more robust strategy would be to create SlurStubs for every VerticalAxisGroup on which the cross-staff slur encompassed a note column. Then, the skyline added would be the union of all these skylines where the top skyline acted as a top bound and the low skyline acted as a lower bound, so if intermediary skylines went over these bounds, those parts of the skyline would be thrown out. This can be done in a separate commit: it'd require making a Skyline::crop function that crops one skyline by another and would require creating multiple SlurStubs in the engraver as well as figuring out the right amount to shift them for each vertical axis group.

http://codereview.appspot.com/6498077

Mike,

Still cannot get this to pass. I got a different kind of error this time so I manually did all the checks.

Start on clean master mkdir build ./autogen.sh --noconfigure cd build ../configure --disable-optimising make make test-baseline

all ok.

Then I applied (or tried) to apply your patch and got a ton of 'Hunks Failed' messages.

Could you look at your tree and see if it is 'clean'?

James

Sorry Mike, I think I made a mistake doing this manually, let me run the test again.

James

OK Mike.

--snip-- GNU LilyPond 2.17.2 /home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/define-grobs.scm:28:3: In expression (quasiquote (# # # ...)): /home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/define-grobs.scm:28:3: Unbound variable: ly:slur::extremal-stub-vertical-skylines --snip--

Does that help?

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Patchy the autobot says: passes tests. Reg tests attached below

Keith and David seem to have reservations about thispatch, on Rietveld.

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Build results are available at

http://grenouille.lilynet.net/patches-tests/2801/test-results

13:56:14 (UTC) Begin LilyPond compile, previous commit at 38b47996485336eb01d9698f3f52e36f6bb9b0af 13:56:28 From git.savannah.gnu.org:/srv/git/lilypond 38b4799..1ce92ec master -> master 38b4799..1ce92ec staging -> staging f37e505..049bdd9 translation -> translation 13:56:36 Merged master, now at: 1ce92ec5c984dfc05c3431a9f4185b453b457656 13:56:38 Success: sudo -u lilybuild ./autogen.sh --noconfigure 13:57:18 Success: sudo -u lilybuild /home/lilybuild/master/configure --disable-optimising 13:57:28 Success: sudo -u lilybuild nice make clean 14:12:46 Success: sudo -u lilybuild nice make -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1 14:34:29 Success: sudo -u lilybuild nice make test-baseline -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1

15:53:41 Issue 2801: Patch: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. 15:53:41 Issue 2801: Testing patch issue6498077_11003_diff 15:53:42 Success: sudo -u lilybuild git apply --index /home/jmandereau/lily-test-patches/issue6498077_11003.diff 15:53:44 Success: sudo -u lilybuild ./autogen.sh --noconfigure 15:54:10 Success: sudo -u lilybuild /home/lilybuild/master/configure --disable-optimising 15:54:20 Success: sudo -u lilybuild nice make clean 16:09:47 Success: sudo -u lilybuild nice make -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1 16:32:29 Success: sudo -u lilybuild nice make check -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1

Pass make tests I won't bother with attaching screenshots

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Build results are available at

http://grenouille.lilynet.net/patches-tests/2801/test-results

06:56:26 (UTC) Begin LilyPond compile, previous commit at 1ce92ec5c984dfc05c3431a9f4185b453b457656 06:56:41 From git.savannah.gnu.org:/srv/git/lilypond 1ce92ec..5fed61c staging -> staging 06:56:41 Success: No new commits in master 06:59:28 Using test baseline from previous build.

06:59:28 Issue 2801: Patch: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. 06:59:28 Issue 2801: Testing patch issue6498077_4046_diff 06:59:29 Success: sudo -u lilybuild git apply --index /home/jmandereau/lily-test-patches/issue6498077_4046.diff 06:59:31 Success: sudo -u lilybuild ./autogen.sh --noconfigure 07:00:08 Success: sudo -u lilybuild /home/lilybuild/master/configure --disable-optimising 07:00:19 Success: sudo -u lilybuild nice make clean 07:15:50 Success: sudo -u lilybuild nice make -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1 07:39:12 Success: sudo -u lilybuild nice make check -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1

Programming errors reported in reg tests

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Build results are available at

http://grenouille.lilynet.net/patches-tests/2801/test-results

18:56:12 (UTC) Begin LilyPond compile, previous commit at 2519edb6a369867db150df724a01cac9fce69703 18:56:20 From git.savannah.gnu.org:/srv/git/lilypond 2519edb..1092c0a master -> master 2519edb..1092c0a staging -> staging 18:56:30 Merged master, now at: 1092c0a96b39227cee9e51fb80d9518ad740cf1d 18:56:33 Success: sudo -u lilybuild ./autogen.sh --noconfigure 18:57:12 Success: sudo -u lilybuild /home/lilybuild/master/configure --disable-optimising 18:57:22 Success: sudo -u lilybuild nice make clean 19:13:11 Success: sudo -u lilybuild nice make -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1 19:35:08 Success: sudo -u lilybuild nice make test-baseline -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1

19:35:14 Issue 2801: Patch: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. 19:35:14 Issue 2801: Testing patch issue6498077_29_diff 19:35:14 Success: sudo -u lilybuild git apply --index /home/jmandereau/lily-test-patches/issue6498077_29.diff 19:35:16 Success: sudo -u lilybuild ./autogen.sh --noconfigure 19:35:42 Success: sudo -u lilybuild /home/lilybuild/master/configure --disable-optimising 19:35:48 Success: sudo -u lilybuild nice make clean 19:51:22 Success: sudo -u lilybuild nice make -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1 20:15:07 Success: sudo -u lilybuild nice make check -j2 CPU_COUNT=2 ANTI_ALIAS_FACTOR=1

I'm not getting any of the programming errors when I run the regtest and I'm on a build with optimizing disabled (I think). Could someone do me a favor and run the regtests on this to see if these programming errors are coming up?

I recently pushed some work for LedgerLineSpanners, but the programming errors didn't arise there either and that patch has nothing to do with this one.

Curious...

http://codereview.appspot.com/6498077

Patchy the autobot says: passes tests. Reg Tests here: https://www.yousendit.com/download/TEhVT2pOR0YyWGMwTWRVag (3mb)

Counted down to 20120910, please push, and perhaqps create a tracker item for the promised doc patch.

Pushed as 89b7e75f6888d2020f7fe8b258e818c0c5e7a223

Mike, I just looked at the patch, and you have a totally separate set of Slur properties for SlurStub. Any override of Slur properties will not end up in the SlurStub properties. You fix a few cases, like \slurUp and \slurDown, but that is totally unrobust and a recipe for disaster since the normal case now will be that the behavior of slurs and slur stubs will diverge as soon as you override one.

Also tweaks on the slur will not end up on the slur stub. One remedy might be to take a look at the definition of grob::calc-property-by-copy and create something like grob::calc-property-by-grob-copy that will not use event-cause but rather the immediate grob cause for its initialization and use it like grob::calc-property-by-copy is used in scm/define-grobs.scm. I am not sure that there will not be an even better way than element-by-element cloning, but that seems like the fastest remedy for me right now and allows individual tweaks and overrides in the grob definitions themselves and by the user.

The code, as it currently stands, is a disaster because SlurStubs and Slurs will diverge as soon as you try any manipulation of Slur properties.

I don't understand how it does not trigger your self-review reflexes when you have to change the definitions of things like \slurUp. There are frequent discussions on the mailing list about things like tweaking the control points and many other aspects of Slurs, and all those will break if you initialize SlurStubs independently from their causing Slurs.

Please fix this as soon as possible.

Will look @ this tomorrow or the day after.

Given the artifacts in the latest tests of issue 2815, I lean strongly towards reverting this patch for now. http://code.google.com/p/lilypond/issues/detail?id=2815#c13>

Before reverting it, what I'd like to see is some sort of call-by-call map of the related code seeing what values are being sent in as inputs and what is being returned as outputs. I agree that this is problematic and should be squashed, but it makes it easier to work on if it is already in master. Also, it could be the case that there is actually interaction between 2815 and this patch, in which case that's important to know. I would only revert it if somehow the code makes other parts of LilyPond regress or if the cause seemed so esoteric (some unknown phenomenon related to g++, for example) that it was too much of a question mark to have in current master.

"Easier to work on if it is already in master" is nonsense. Master is a moving target, debugging things will have to take place in a fixed copy.

2815 is a pure front-end only patch. It may change some order of garbage collection but that's it. If the regtests exactly in the area you have been working on flip around, it is not related in any useful manner. It may well be that you are working with uninitialized memory or are not properly protecting variables you are working with.

If this code did not make LilyPond regress, you would not need to tamper with all standard predefined slur overrides in our code base in the first place. Obviously you can't catch any overrides in user code in that manner.

I don't see the point in destabilizing master with known faulty code. I don't understand why you think debugging can't happen in a private branch.

Because this sort of thing never showed up when I was working on a private branch, even when I was using it as a baseline against other changes.

I'll revert it and cherry pick it to a private branch to see if I can make it show up. If not, I'll need people's help to reproduce the bug.

Through the history of this patch, our and James' and Grenouille's test runs reported different tests involving cross-staff slurs with big "distances" but no visible changes. Probably that should have been a clue.

I missed that - I'm looking @

http://grenouille.lilynet.net/patches-tests/2801/test-results/

right now. Where is that info? That'd help with debugging.

"No visible changes" would be par for the course for single-system output with changes in inter-system stubs.

In there is single system output the SlurStubs should be suicided long before any of these callbacks return values.

David - could you confirm what single-system regtests you're talking about? I'll do some tests to see if stubs aren't killed off, but they should be suicided as soon as the cross-staff callback is called (which is before the vertical-skyline callback is called).

Running tests now with a change - it looks like dead stubs were making it into the skyline, which was probably causing the garbage collection problems. I added a test to weed them out.

Regarding comment #40: can be mostly disregarded as I misremembered. span-bar-articulation.ly would have been the only single-system test flagged in my own issue. It may still be interesting since that is also the only one with significant coloring (suggesting an offset) in the regtest image rather than just dimensional differences.

But could also be due to the larger scale.

Regarding comment #42: if anything makes it into the skyline, there should be references to it, so it should be safe from garbage collection as long as those references exist and are properly marked during the marking phase of the garbage collector. Of course, assuming that all your data structures keeping references properly mark them (via derived_mark and relatives). So garbage collection itself should not pose a problem as long as the code is clean.

If you want a recompilation with significantly different memory layout, reconfiguring with

-fkeep-inline-functions

is a pretty solid candidate for making quite a different executable (also one more useful under gdb). Should be a good candidate for triggering compilation-dependent flukes when compared to an executable without that option.

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Posted a new patch that implements one of David's suggestions about copying grob properties.

I've also been able to reproduce the regtest problem from David's patch. Could someone please confirm that other patch sets cause the same problem? If not, what about David's patch could trigger the memory problem? As memcheck does not catch any invalid reads or conditional jumps outside of the normal ones from Guile, I am having a very hard time tracking down the cause of this.

My question from before still stands - is there any tool that'll take gprof data (or valgrind or whatever) from multiple runs and do a tree that shows where the runs converge and diverge in terms of functions called, argument values and return values?

Patchy the autobot says: passes tests. Reg test in next comment

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

Patchy the autobot says: passes tests. Reg test in next comment

Mike is working on another patch set. This one would still cause issue 2837, and intermittently causes about 5 staff-spaces extra staff-spacing. The last time I ran make check on this patch, everything visible in 'beam-cross-staff-slope.ly' moved down by 4.9456964833876 staff-spaces -- making me suspect an invisible slur pushed against the top margin. The cropped preview images show no change because everything moved by the same amount.

I agree that this is likely what's happening, but I can't figure out why.

The problem is that it is impossible to recreate outside of the regtests (I tried -fkeep-inline-functions, applying different patches, etc. but to no avail).

My guess is that somewhere in the slur-scoring code, using pure heights and pure offsets is leading to inconsistent results. They have never been used this far downstream before, so I'm guessing that they may be freed up earlier. But at least the inconsistency is consistent - I'm getting 4.9456964833876 whenever it does go off track.

Are you compiling with -disable-optimising like the regtests (in which case keep-inline-functions is implied anyway and will not make a difference. Sorry, did not think of that)?

Yup - I even made a test patch where I deleted all regtests save the problematic ones to speed up the process and the diff came out squeaky clean. So the presence of the other regtests does something to memory use that triggers this problem.

git checkout release/2.17.2 lily/parser.yy will also change the memory allocation/release patterns significantly. lilypond -ddebug-gc might cause some changes. Sorry, this is all poking around in the dark.

I think I've found the nature of the problem. I did a run of the regtests with a bunch of messages printed to std:err and it looks like every time there is a divergence, the order in which the grobs are read changes. This comes from sorting grobs by pointers and then running ::uniq. In theory this shouldn't matter, but in practice...

1) LilyPond requests pure height from grob X. 2) Grob X's pure height is somehow dependent on grob Y. 3) Depending on how a vector has been pointer-sorted... 3a) Grob Y's actual offset has not yet been calculated because of the way a given vector is sorted, in which is pure offset is returned. 3b) Grob Y's actual offset has been calculated because it was triggered first and stashed in dim_cache_. Grob::pure_relative_y_coordinate will always return this as opposed to the pure value.

So there you have it. One way to solve this may be to disallow Grob::pure_relative_y_coordinate from ever returning something out of dim_cache_. This has no bearing on the caching in item.cc, so it shouldn't result in too much of a slow-down. I'll run regtests on a patch doing this...

Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines.

http://codereview.appspot.com/6498077

The patch doesn't do slurs over line breaks yet, but otherwise handles Keith's test case (op45.ly) much better and errs on the side of caution when approximating slurs. At worst this results in the old collisions, but often it introduces at least a bit of space.

Patchy the autobot says: passes tests. Reg test here: https://www.yousendit.com/download/TEhXQk05WkJuSlRIRHRVag

but otherwise handles Keith's test case (op45.ly) much better

.. in that it does less harm than earlier patches. This patch takes only 7% longer than master and causes only one collision. I'm still hoping to find an example that this patch actually improves.

les-nereides.ly cross-staff-slur-vertical-spacing.ly

Where does it cause a collision?

The collision in the Chopin is from the fix for issue 2589 within this patch. I had set Fingering to 'avoid-slurs on the inside for that piece, so the fingering and accent on the same note in measure 85 http://k-ohara.oco.net/Lilypond/chopin_prelude_op45_slurs.pdf had inconsistent placement instructions, which LilyPond formerly resolved somehow.

I have been thinking about making the option for Scripts and such things to "avoid-slur = outside" apply for normal Slurs only, letting Scripts go inside cross-staff Slurs. The need to let Fingering inside cross-staff slurs was my reason for the troublesome override in Chopin Op45.

If we stop trying to place Scripts, etc., outside cross-staff Slurs then we can keep the usual collision avoidance for those Scripts, etc.

Ah, ok. That aside, do you think this is a better solution? Setting ragged-bottom to ##t, I can see that the SlurStubs help a bit whenever they are printed. Once I figure out how to do them over line-breaks, which'll be in a separate patch, I imagine that the other collisions will go away as well.

The more I work on this, the more I realize that there is no good way to deal with cross-staff grobs and spacing. I've more or less given up on my grand linear programming project as there are to many if/then decisions to be able to approximate any process with linear programming. One solution may be swimming back upstream. For example, do the spacing with stubs, then calculate cross staff grob placement, then redo the spacing with the full skylines, and if this causes a bad result go back upstream to recalculate the spacing. We'd just need to figure out some way to watch for infinite cycles, but a multi-pass system seems swingable and more closely approximates how human engravers deal with these things.

I've more or less given up on my grand linear programming project as there are to many if/then decisions to be able to approximate any process with linear programming.

Uh, how does this make sense? One point of linear programming is to bound a decision tree from exponential size to some factor above linear.

In outside-staff-spacing, for example, if grobs can slide other under grobs, they do, which means that they are no longer part of the sideline. It is not a linear distinction:

if (stashable_under_other_grob) stash; else place_over

The skyline changes from having the extent of the grob to not in a non-gradual way.

This sorta thing can be articulated through mixed integer linear programming using 1 for true and 0 for false, but it would become mixed-integer quadratic because you're multiplying these 1s and 0s by other things that are themselves variables in the linear program. I know of no package that supports this sorta thing and it seems like it'd take for ages even if it were possible

Comment 65's multiple-pass suggestion might only need two passes: 1) tentatively shape cross-staff Slurs and Beams using the minimum-distances that we compute just before stretching staves to fill the page, including those tentative shapes in the System skylines only. 2) finalize the cross-staff spanner stencils after stretching staves to fill the page.

It is more complicated than that for a couple reasons:

1) If grobs with outside staff priority are placed above the cross-staff slur (like a tempo indication), they will need to be re-placed as well. 2) If the cross staff slur happens on the interior of a system (say from staves 2 to 3 in a 4-staff system), then the staves on either side will need respacing. Furthermore, if another cross-staff slur exists from staff 1 to 4, it will depend on the height of the interior one.

Neither of these are prohibitively difficult but they require a sophisticated sort of rewinding mechanism so that the solutions to the spring-and-rods problem used in Page_layout_problem::find_system_offsets can become the new heights off of which a new round of calculations are based. Objects whose Y-offsets depend on the slur (stuff with outside-staff-priority or stuff with avoid-slur set to outside or around come to mind) and objects that depend on these objects need to have their offsets and extents wiped, their original callback functions restored, and the calculations redone. Of course, if any of this becomes more complicated through user tweaks (i.e. if the color of a grob depends on the extent of a cross-staff-slur) these need to be redone as well. So a system would need to be put in place where property-dependency was completely accounted for and any property dependent on the Y-offset of a cross-staff grob could be reset.

You'd then have to run this however many times such that the values in the solution_ vector of the Page_layout_problem diverge by less than epsilon between successive runs. Of course this risks becoming cyclic, so LilyPond would need to perhaps say that if after X runs the spacing hasn't been found, use the widest spacing generated so far.

Counted down to 20120927, please push.

I'm gonna hold off on pushing this until we're certain that there's not a better way to do it with property resets.