Fixes issue 1628.

Fixes issue 1628.

[MikeSol, 2011-08-15 21:17:23]

Cheers,
MS

[Keith, 2011-08-16 05:48:21]

So far, so good.
Would you include the corresponding changes to 
phrasing-slur-engraver.cc (mostly a duplication of slur-engraver.cc) in your
next patch set?

[hanwenn, 2011-08-16 11:06:28]

Your patch is doing much more than address issue 1628.  Can you do
just the change to the engraver to close issue 1628?   Any ensuing
collisions should be made into a new issue.

Are you sure exclude_extra_objects_outside_x_range() is really needed?
We already have

          Real x = info.extents_[X_AXIS].linear_combination (info.idx_);

          if (!slur_wid.contains (x))
            continue;

I don't think we can realistically mess with the offsets of
extra-encompass objects in the slur scoring (or, in general: change
dependency order during formatting): other objects may already have
used the object's position (which you are trying to invalidate) to
make other decisions.  Eg. what if a skyline algorithm has already
read the position of the grob?

If you want to have more adaptive behavior, you need to have a
super-grob which coordinates between slur and encompass objects, like
we have for note and rest-collisions.

On Mon, Aug 15, 2011 at 6:17 PM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> Cheers,
> MS
>
> Description:
> Fixes issue 1628.
>
> Please review this at http://codereview.appspot.com/4876051/
>
> Affected files:
>  M lily/include/slur-scoring.hh
>  M lily/include/slur.hh
>  M lily/new-fingering-engraver.cc
>  M lily/slur-configuration.cc
>  M lily/slur-engraver.cc
>  M lily/slur-scoring.cc
>  M lily/slur.cc
>  M scm/define-grob-properties.scm
>  M scm/define-grobs.scm
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
>



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mikesol_ufl.edu, 2011-08-16 11:12:47]

On Aug 16, 2011, at 1:06 PM, Han-Wen Nienhuys wrote:

> Your patch is doing much more than address issue 1628.  Can you do
> just the change to the engraver to close issue 1628?   Any ensuing
> collisions should be made into a new issue.
> 

OK.  The reason that I added all that extra stuff was for a
string-number-arond-slur regtest with slurs that explicitly has "outside" and
"inside" written in certain places.  These get changed with my patch.  I assumed
that this was important, so I implemented the behavior necessary for the regtest
not to change.  My patch, then, will introduce a regression from this test (or,
alternatively, I could just change the outside/inside labels to the new results,
but again, I'm not sure how important they are).

> Are you sure exclude_extra_objects_outside_x_range() is really needed?
> We already have
> 
>          Real x = info.extents_[X_AXIS].linear_combination (info.idx_);
> 
>          if (!slur_wid.contains (x))
>            continue;
> 

Didn't see this, so no, it's not needed.

> I don't think we can realistically mess with the offsets of
> extra-encompass objects in the slur scoring (or, in general: change
> dependency order during formatting): other objects may already have
> used the object's position (which you are trying to invalidate) to
> make other decisions.  Eg. what if a skyline algorithm has already
> read the position of the grob?
> 
> If you want to have more adaptive behavior, you need to have a
> super-grob which coordinates between slur and encompass objects, like
> we have for note and rest-collisions.
> 

This is not a bad idea.  As my summer of lily comes to a close in a couple
weeks, I may try to get this up and running.

Cheers,
MS

[MikeSol, 2011-08-17 10:21:53]

New patchset uploaded w/ minimal changes made (just the engravers).

[Keith, 2011-08-17 20:42:37]

LGTM

Now string numbers move around slurs as well.
That didn't work in the old patch, nor in 2.14.

The old regtest string-number-around-slur.ly avoided collisions only by
accident.  Different pitches would cause collisions with the string numbers, but
after this patch they really move #'around the slur.

http://codereview.appspot.com/4876051/diff/8001/input/regression/string-numbe...
File input/regression/string-number-around-slur.ly (right):

http://codereview.appspot.com/4876051/diff/8001/input/regression/string-numbe...
input/regression/string-number-around-slur.ly:9: \textLengthOn
Alternatively, if you remove \textLengthOn, the numbers go to their original
in-in-out positions, and the natural spacing gives a more realistic test.

[MikeSol, 2011-08-25 07:38:14]

Pushed as 29d1121d260318ed07f152c346a1a69e5dadb69d.

Keith: I ultimately decided not to change the textLengthOn in the regtest so
that the test can test if objects from the new-fingering-engraver have their
callbacks chained in the correct order.  Even if this leads to an aesthetically
inferior result, it shows the correct behavior in action.

Cheers,
MS