Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[dak]

On 2012/09/04 08:09:21, mike7 wrote:
> On 4 sept. 2012, at 09:45, mailto:address@hidden wrote:

> > Works for me.  16% slower than master.
> > (I'll try make clean and make.)
> > It makes no change for the Chopin; can you give an example where it
> > helps?

> In the Chopin, ragged-bottom is false so the difference can't really
be seen.
> The piece isn't a good test case for how the patch changes engraving
but it is
> an excellent test case for efficiency.

> >
> > I still do not understand how creating separate SlurStubs helps.

> Check out the regtest.  Also checkout les-nereides.ly (which is what
the patch
> was created to fix (and does fix (why don't we have les-nereides.ly in
the
> regtest comparison?))).

> > At the
> > time when we build system skylines, what information is in the stubs
> > that is not also in the slurs themselves?

> It is not that there is more information in the stubs - it's just
different.
> They use an approximation of the vertical-skylines instead of the
actual ones at
> a point in the code where skylines are needed but the actual ones
cannot be used
> (to wit: Axis_group_interface::skyline_spacing).

> If we calculated the actual control points for cross staff slurs at
the time
> VerticalAxisGroup skylines were calculated, it would trigger a
vertical
> alignment.  As an experiment, try removing the lines that weed out
cross-staff
> grobs in Axis_group_interface::skyline_spacing and run all of the beam
regtests
> - you'll see what I mean.

> > Can you explain the need for
> > stubs ?
> > -- without using the word 'pure' because I do not know what you mean
> > when you say that word -- you could mean 'with the boolean of that
name
> > set to true' in any one of dozens of functions.
> >

> I need to use the word pure, but I'll use it judiciously :-)

> SlurStubs run the control point calculations using not the actual
heights and
> coordinates (which would trigger a vertical alignment) but rather pure
heights
> and coordinates.  Checkout the changes to slur-scoring.cc: almost all
of them
> are changing extent to maybe_pure_extent and stuff of that ilk.  In
the case of
> stubs, this generates approximations of control points and therefore
an
> approximation of the vertical skylines.  We feed this approximation
into the
> vertical skyline calculations.

> >
> >
http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc
> > File lily/axis-group-interface.cc (right):
> >
> >

http://codereview.appspot.com/6498077/diff/4046/lily/axis-group-interface.cc#newcode806
> > lily/axis-group-interface.cc:806:
> > Axis_group_interface::add_interior_skylines
> > move this back up to line 616 for the sake of history
> >

> ok

> >
http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc
> > File lily/slur-engraver.cc (right):
> >
> >

http://codereview.appspot.com/6498077/diff/4046/lily/slur-engraver.cc#newcode347
> > lily/slur-engraver.cc:347: vector<Grob *>::const_iterator it =
> > // Keep just one stub per Staff-like construct
> > // Remove stubs if we have seen one already in the same vag,
> >
> > ??  Do slurs that stay within a single staff produce stubs, and do
we
> > remove them here?

> Ya.  There is no way to know if a slur will stay within a single staff
until all
> its noteheads have been acknowledged.

> http://codereview.appspot.com/6498077/


So, _now_ please take every sentence and every answer in this mail,
rewrite it in the form of a comment and stick it in the file in the
places where people would be looking for it.

_This_ is the kind of information that needs to be in comments in the
file, at the places where you would be looking for it.  Putting it in
some comments in a Rietveld tracker does not make sense.

Probably we should make it a rule that a patch submitter can't post
followup comments but only followup patches.  That way the information
ends up where it needs to be.

http://codereview.appspot.com/6498077/