Includes empty skylines in minimum translation calculations

Includes empty skylines in minimum translation calculations

[MikeSol, 2013-02-10 10:43:41]

Fixes StanzaNumber

[Keith, 2013-02-10 19:45:46]

Doesn't this have the same problem as my attempted patch for issue 1669 ?

[mike7, 2013-02-10 19:58:42]

On 10 févr. 2013, at 20:45, k-ohara5a5a@oco.net wrote:

> Doesn't this have the same problem as my attempted patch for issue 1669
> ?
> 
> https://codereview.appspot.com/7304068/
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

Hadn't seen this.
Issue 3160 is then a duplicate...worth merging into this issue, no?

Cheers,
SM

[MikeSol, 2013-02-12 06:50:30]

Avoids over-estimating pure heights of systems with removable vertical axis
groups.

[MikeSol, 2013-02-12 07:01:17]

Restores previous translates for empty elements

[Keith, 2013-02-12 19:15:47]

https://codereview.appspot.com/7304068/diff/5/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/7304068/diff/5/lily/align-interface.cc#newcode65
lily/align-interface.cc:65: an empty extent, delete it from the list instead. If
the extent is
Could you update the comments and include the other changes from
https://codereview.appspot.com/4515158/

There is some copying that is no longer needed if we leave all the
VerticalAxisGroups in the array.

https://codereview.appspot.com/7304068/diff/5/lily/align-interface.cc#newcode116
lily/align-interface.cc:116: && !(*skip_elt)[i])
I though we needed this up on line 99, so that we we skip line 129, which would
include the skyline of a staff with only rests, if that staff would be removed
as empty.

and then we also avoid the second check of pure-height that would have to search
through the line for keep-alive events (for every tentative line-breaking
possibility)

[MikeSol, 2013-02-20 12:10:18]

Responses to Keith's suggestions.

[MikeSol, 2013-02-21 09:48:13]

Rebase against current master

[Keith, 2013-02-24 08:18:39]

There's some book-keeping error that I can't find.  The example at issue 3160
comes out wrong.

https://codereview.appspot.com/7304068/diff/16001/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/7304068/diff/16001/lily/align-interface.cc#new...
lily/align-interface.cc:65: an empty extent, delete it from the list instead. If
the extent is
There is no longer any special treatment to empty extents, in this function.

https://codereview.appspot.com/7304068/diff/16001/lily/align-interface.cc#new...
lily/align-interface.cc:293: // gets the same translation as the last non-empty
element before it.
Rather than remove the empty Lines, then put them back in again with the same
'translate' as the previous, it might be more clear to make one pass through the
array to fill in the missing translates[].

[MikeSol, 2013-02-24 10:14:06]

Issues programming error for dead axis groups used in unpure alignment

[Keith, 2013-02-27 06:06:43]

Also needs some decruftification.

https://codereview.appspot.com/7304068/diff/22001/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/7304068/diff/22001/lily/align-interface.cc#new...
lily/align-interface.cc:299: vector<Grob *> non_empty_elems;
The disassembly and reassembly of the array is ridiculous, but with the comment
at least people will not be confused, merely laugh.

https://codereview.appspot.com/7304068/diff/22001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/7304068/diff/22001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:2031: (horizon-padding . 100)
You don't need this anymore, and it will terribly confuse some poor songwriter
somewhere, if left in.

[MikeSol, 2013-02-27 10:53:22]

Decruftify stanza number

[mike7, 2013-02-27 11:14:21]

On 27 févr. 2013, at 07:06, k-ohara5a5a@oco.net wrote:

> Also needs some decruftification.
> 
> 
> https://codereview.appspot.com/7304068/diff/22001/lily/align-interface.cc
> File lily/align-interface.cc (right):
> 
>
https://codereview.appspot.com/7304068/diff/22001/lily/align-interface.cc#new...
> lily/align-interface.cc:299: vector<Grob *> non_empty_elems;
> The disassembly and reassembly of the array is ridiculous, but with the
> comment at least people will not be confused, merely laugh.
> 
> https://codereview.appspot.com/7304068/diff/22001/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
https://codereview.appspot.com/7304068/diff/22001/scm/define-grobs.scm#newcod...
> scm/define-grobs.scm:2031: (horizon-padding . 100)
> You don't need this anymore, and it will terribly confuse some poor
> songwriter somewhere, if left in.

Removed.

Thanks for the review!

Cheers,
MS

[dak, 2013-03-05 10:56:40]

https://codereview.appspot.com/7304068/diff/27001/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/7304068/diff/27001/lily/align-interface.cc#new...
lily/align-interface.cc:74: vector<Skyline_pair> *const ret,
Usage of const here is "dear compiler, I promise that ret will not point to
anything else in this function".  Which is usually not very interesting.  You
probably mean that the pointer is not going to be used for changing the passed
vector.  That, however, would rather be
vector<Skyline_pair> const *ret

https://codereview.appspot.com/7304068/diff/27001/lily/align-interface.cc#new...
lily/align-interface.cc:75: vector<bool> *const skip_elt)
Same here.  Admittedly, this was wrong before, and you changed the const
association of "elements" probably without much thinking (it was wrong
previously, too), but at any rate this should be fixed.  No need to restart the
countdown for that, but it should get retested to make sure that the changed
"const" qualifier actually is accepted by the compiler.