Issue 4274: Fix a cyclic dependency in Rest_collision

https://code.google.com/p/lilypond/issues/detail?id=4274

Rest_collision::calc_positioning_done now uses the pure height of rests because the height of certain rests depends on their position on the staff, which Rest_collision is responsible for adjusting.

There are differences in regression test output that I can not yet explain.  The differences do not appear when I run lilypond by itself, so I'm expecting to have to dig up the exact command the regtests run.  I'm posting this review anyway in hopes that a reviewer will recognize the problem.

[Keith, 2015-01-27 05:21:47]

Are you sure there is a cycle?

> Rest_collision::calc_positioning_done now uses the pure
> height of rests because the height of certain rests depends
> on their position on the staff, which Rest_collision is
> responsible for adjusting.

Rest_collision does use the height of the rests to determine the height of the
rests, but I do not see where calling the Rest::height() function can call
Rest_collision() cyclically.

This calculation seems to be of the type
  h = 3;
  if (moon is full) 
   h = h + 2;

Much of LilyPond layout code is done through function calls, so there is often
risk of cycles like
  int h(void) {
    int ret = 3;
    if (moon is full)
      ret = h() + 2;
    return ret;
  }
but I don't see how that can happen here.

Is there some LilyPond input that causes infinite recursion or gives the error
message from the trap for cyclic dependencies ?

https://codereview.appspot.com/193590043/diff/1/lily/rest-collision.cc
File lily/rest-collision.cc (right):

https://codereview.appspot.com/193590043/diff/1/lily/rest-collision.cc#newcod...
lily/rest-collision.cc:195: Real diff = d * ((last_y - y) / staff_space);
The idea here seemed to be that any outer-voice rests are offset based on
inner-voice rests, which were just placed by the translate() call above.  
pure_height() seems to freshly look up the stencils so it would ignore any
previous positioning.  The result is used in the translate() below which offsets
the rests relative to their most recent position, so the old code using the most
recent positions seems correct.

[Dan Eble, 2015-01-27 13:09:58]

On 2015/01/27 05:21:47, Keith wrote:
> Are you sure there is a cycle?

> Rest_collision does use the height of the rests to determine the height of the
> rests, but I do not see where calling the Rest::height() function can call
> Rest_collision() cyclically.

> Is there some LilyPond input that causes infinite recursion or gives the error
> message from the trap for cyclic dependencies ?

In master there is no message because of the way Rest::height works.  It gets a
stencil for the rest without actually calling get_property("stencil"), which
means there is no record that the stencil is already being calculated at the
time Rest_collision asks for the extent of the rest, which asks for the stencil,
which is being calculated.

I discovered this when I tried to use get_property("stencil") from Rest::height
while trying to fix issue 4245.

[Keith, 2015-01-28 03:51:00]

On Tue, 27 Jan 2015 05:09:58 -0800, <nine.fierce.ballads@gmail.com> wrote:

> On 2015/01/27 05:21:47, Keith wrote:
>
> I discovered this when I tried to use get_property("stencil") from
> Rest::height while trying to fix issue 4245.
>

It looks like some rests (whole and half rests) have two glyphs, with and
without the ledger line, so the *final* stencil should not be generated until
after rest_collision is finished, and maybe after beams are set and rests moved
to avoid them, when we finally know if the rest needs a ledger.

So when you ask for the stencil in order to figure note-spacing, the routine to
generate the stencil asks for most all layout to be completed, so it knows in
the general case whether it should fetch the glyph with a ledger.

There is some code trying to handle that problem in
   Rest::generic_extent_callback (Grob *me, Axis a)
(but it seems strange that if we want the a=Y_AXIS extent, it includes a
possible ledger -- we would seem to need to know the height of a rest in order
to know how far we need to move it to clear a note.)
I do not like that it re-fetches the glyph from font forge every time
generic_extent_callback() is called; hopefully the extent is cached somewhere.

It seems that we want to look up a tentative rest glyph (either with or without
ledger) to get the extent, figure out note spacing and line-breaking, and
rest-collisions, and beaming, all with that extent, and then finalize the
stencil at some point after we are finished moving rests vertically, but before
printing.

If you can follow the sequence of calls between Scheme and C, maybe you can find
a simple way to get this tentative extent for issue 4245.  Use of the
"pure-height" in rest-collision.cc would assume, for purposes of placing a given
rest, that every other rest is on staff-center.

[Dan Eble, 2015-01-29 01:40:33]

Yes, the ledgered rest stuff is a big tangle.
See https://codereview.appspot.com/200860043/ for the best I've been able to do
without changing Rest_collision.