First pass at avoiding very high slurs (fixes issue 163).

First pass at avoiding very high slurs (fixes issue 163).

[MikeSol, 2011-07-23 20:50:47]

Hey all,

This fixes issue 163.  The only downside is that it adds another entry to an
already-crowded details list.  I also use a magic number of 2 (you'll see a
comment about it) that should likely itself either be a details entry or be
taken from an existing details entry.

I agree with Han-Wen's comment in the source that there are too many properties
and that it risks to become unwieldy, so although I like this patch because it
makes a few of my scores look better, I'd like to have a discussion about
consolidating and/or better documenting the details list in conjunction with the
pushing of this patch.  Ideally, I'd like to see a regtest that clearly
demonstrates the utility (and indispensability) of each entry in the details
list.  By constructing these regtests, it'll likely make pruning the list
easier.

Cheers,
MS

[lemzwerg, 2011-07-24 05:39:52]

I don't mind if we have another obscure entry in the detail list currently.  If
your patches fixes the problem reliably, this would be a great immediate help.

IMHO, at some point in the hopefully not too distant future, the whole handling
of slurs and ties must be re-examined to make it more user-friendly and robust,
and to improve its shapes, both on the macro and micro level. Additionally, due
to the very graphical nature of the problem, it deserves a texinfo-like
documentation (with many, many images), to be generated from the corresponding
comments in the source code files.

[lemniskata.bernoulliego, 2011-07-24 08:04:16]

2011/7/24  <lemzwerg@googlemail.com>:
> I don't mind if we have another obscure entry in the detail list
> currently.  If your patches fixes the problem reliably, this would be a
> great immediate help.
>
> IMHO, at some point in the hopefully not too distant future, the whole
> handling of slurs and ties must be re-examined to make it more
> user-friendly and robust, and to improve its shapes, both on the macro
> and micro level. Additionally, due to the very graphical nature of the
> problem, it deserves a texinfo-like documentation (with many, many
> images), to be generated from the corresponding comments in the source
> code files.

+100
I'm currently preparing an extensive report on the matter of ties.  I
estimate that 30% of work is done.

cheers,
Janek

[wl_gnu.org, 2011-07-24 08:06:40]

> I'm currently preparing an extensive report on the matter of ties.
> I estimate that 30% of work is done.

This is great news!


    Werner

[pkx166h, 2011-07-24 09:49:23]

Make passes and reg tests look pretty good too

See http://code.google.com/p/lilypond/issues/detail?id=163#c12

for output

[mike_apollinemike.com, 2011-07-24 10:34:31]

On Jul 24, 2011, at 11:49 AM, pkx166h@gmail.com wrote:

> Make passes and reg tests look pretty good too
> 
> See http://code.google.com/p/lilypond/issues/detail?id=163#c12
> 
> for output
> 
> http://codereview.appspot.com/4817048/

Thanks James!

A lot of the changes are ugly (there are only a couple that are better).  The
code exists to weed out extreme cases without having an impact upon other cases.
 Ideally, the current regtests shouldn't change (or the change should be so
minimal that we have trouble seeing it).  I'll give it another go.

Cheers,
MS

[James.Lowe_datacore.com, 2011-07-24 11:17:34]

Mike
________________________________________
From: lilypond-devel-bounces+james.lowe=datacore.com@gnu.org
[lilypond-devel-bounces+james.lowe=datacore.com@gnu.org] on behalf of
mike@apollinemike.com [mike@apollinemike.com]
Sent: 24 July 2011 10:55
To: mtsolo@gmail.com; lemzwerg@googlemail.com;
lemniskata.bernoulliego@gmail.com; wl@gnu.org; pkx166h@gmail.com;
lilypond-devel@gnu.org; reply@codereview.appspotmail.com
Subject: Re: First pass at avoiding very high slurs (fixes issue 163). 
(issue4817048)

On Jul 24, 2011, at 11:49 AM, pkx166h@gmail.com wrote:

> Make passes and reg tests look pretty good too
>
> See http://code.google.com/p/lilypond/issues/detail?id=163#c12
>
> for output
>
> http://codereview.appspot.com/4817048/

Thanks James!

A lot of the changes are ugly (there are only a couple that are better).  The
code exists to weed out extreme cases without having an impact upon other cases.
 Ideally, the current regtests shouldn't change (or the change should be so
minimal that we have trouble seeing it).  I'll give it another go.

Cheers,
MS
_______________________________________________
----

No problem, could you (and others if possible) when you put a patch 'back' up -
remember to set the tracker to patch - new if it needs another set of reg test
checks? 

I'm having a bit of a hard time keeping up. I realise that a lot of 'fixes' to
patches that didn't 'quite' pass (depending on your point of view) or patches
that are just syntax/spacing changes probably don't need another reg test and
there is no need to change the tracker label.

As I say, I do try to follow all the Rietveld changes, but can miss some or am
not conversant in the code enough to know if a patch 'change' warrants a new reg
test or not - in which case I am relying on the 'Patch - new label in the
tracker. Some devs update the tracker and the reitveld issue but some
don't/forget etc. in which case I might not do a re-re-review for them.

James

[MikeSol, 2011-07-24 13:50:53]

New patchset uploaded with minor tweaks.
I still need to think about a better way to implement the change in score_edges.
 The idea is that if there is a large jump right at the beginning or end of the
stem, we should favor attachments points that are closer to the Y-position of
the second note-column and not the first.  Currently, my code is too extreme: it
pulls certain attachment points up too high (I'm thinking of slur-rest.ly).

[MikeSol, 2011-07-27 07:37:43]

On 2011/07/24 13:50:53, MikeSol wrote:
> New patchset uploaded with minor tweaks.
> I still need to think about a better way to implement the change in
score_edges.
>  The idea is that if there is a large jump right at the beginning or end of
the
> stem, we should favor attachments points that are closer to the Y-position of
> the second note-column and not the first.  Currently, my code is too extreme:
it
> pulls certain attachment points up too high (I'm thinking of slur-rest.ly).

Hey all,

I was able to get rid of the change in score_edges with another parameter.
I think this does the trick and fixes every example in issue 163 save the one in
comment 4 from Werner.  I'm not sure if this example is undesirable - on IMSLP
there are a few Brahms intermezzi (op. 118 119) that make comparable engraving
decisions.

Three regtests are added with relatively extreme situations that now behave
nicely.

Cheers,
MS

[Trevor Daniels, 2011-07-27 13:37:23]

http://codereview.appspot.com/4817048/diff/13001/lily/include/slur-score-para...
File lily/include/slur-score-parameters.hh (right):

http://codereview.appspot.com/4817048/diff/13001/lily/include/slur-score-para...
lily/include/slur-score-parameters.hh:32: Real max_distance_from_head_penalty_;
duplicated

[mike_apollinemike.com, 2011-07-27 13:41:22]

On Jul 27, 2011, at 3:37 PM, tdanielsmusic@googlemail.com wrote:

> 
>
http://codereview.appspot.com/4817048/diff/13001/lily/include/slur-score-para...
> File lily/include/slur-score-parameters.hh (right):
> 
>
http://codereview.appspot.com/4817048/diff/13001/lily/include/slur-score-para...
> lily/include/slur-score-parameters.hh:32: Real
> max_distance_from_head_penalty_;
> duplicated
> 
> http://codereview.appspot.com/4817048/

Fixed - thanks!

Cheers,
MS

[pkx166h, 2011-07-27 19:30:41]

Passes Make and reg tests.

[pkx166h, 2011-07-27 19:50:12]

On 2011/07/27 19:30:41, J_lowe wrote:
> Passes Make and reg tests.

Sorry I updated the wrong Rietveld issue. This does pass make but there are reg
test differences - see

http://code.google.com/p/lilypond/issues/detail?id=163#c17

[hanwenn, 2011-07-28 02:34:06]

as an overall comment; I would try to look at the buggy examples in more detail
to understand exactly what is going wrong.  It is easy to add an extra demerit,
but (as you can see) hard to make it work with all the other cases that we
handle.

http://codereview.appspot.com/4817048/diff/15002/lily/slur-configuration.cc
File lily/slur-configuration.cc (right):

http://codereview.appspot.com/4817048/diff/15002/lily/slur-configuration.cc#n...
lily/slur-configuration.cc:249: Real encompass_refpoint = minmax (state.dir_,
state.encompass_infos_[j].head_, state.encompass_infos_[j].stem_);
refpoint usually has another meaning within lilypond.

http://codereview.appspot.com/4817048/diff/15002/lily/slur-configuration.cc#n...
lily/slur-configuration.cc:252: if (fabs (encompass_refpoint) > highest_point
is this reasonable for non-horizontal slurs? Why don't you check for the maximum
dy instead?

http://codereview.appspot.com/4817048/diff/15002/lily/slur-configuration.cc#n...
lily/slur-configuration.cc:256: * 2 is the max distance allowable from the
highest notehead before
2 ?