Issue 2462: don't change ideal spacing when adding a rod

When objects like lyrics are added to the PaperColumns, LilyPond inserts
rods between these columns to ensure that the objects won't overlap.
However, the ideal distance should remain unchanged.  For example, in

\layout {
  ragged-right = ##f
}
\relative f' { \repeat unfold 8 a4 }
\addlyrics { \repeat unfold 4 la \repeat unfold 4 straight }

notes in 2nd measure have long lyrics attached to them - min_distance_
of the springs between these columns should be adjusted, but distance_
(the ideal distance) should not: it should be the same as for notes
with short lyrics.  When the line is stretched so much that the minimum
distances are not involved, the springs between notes in both measures
should behave (almost) identical.

[Keith, 2014-07-03 04:42:48]

This does make sense, but if you change the basic rules of a Spring (allowing
the natural length to be less than the minimum-distance constraint, so it stays
at the minimum length until the line is considerably stretched) you should try
to make the change consistently everywhere ?

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc
File lily/spring.cc (left):

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc#oldcode192
lily/spring.cc:192: distance_ = max (distance_, min_distance_);
Only caller is Simple_spacer::add_rod() and it certainly seems simpler if adding
a min_distance constraint (adding a 'rod') leaves the ideal spacing (the
'spring') unchanged.

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc
File lily/spring.cc (right):

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc#newcode87
lily/spring.cc:87: distance_ = max (min_distance_, distance_ * r);
maybe  *= r;

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc#newcode120
lily/spring.cc:120: avg_distance = max (min_distance + 0.3, avg_distance);
Logically, this would be removed as well, except that it also adds 0.3.  When
you tried removing this line, you said there were unwanted side-effects. What
cases needed the extra 0.3 of space?  It would be much better to include it in
padding parameters, than to have this deeply-hidden offset.

[janek, 2014-07-03 06:56:20]

Updated description.

On 2014/07/03 04:42:48, Keith wrote:
> This does make sense, but if you change the basic rules of a Spring (allowing
> the natural length to be less than the minimum-distance constraint, so it
stays
> at the minimum length until the line is considerably stretched) you should try
> to make the change consistently everywhere ?

Yes, that's what i'm aiming for.  However, as you remember, some of these
changes produce unwanted side-effects - for example, we would have to find
another way of implementing that 0.3.  Removing this single line fixes some
of the problems (without any bad side effects, as far as i know), so i thought
that we could start with this, and then work on making the rest of the code
work correctly.

I'm working on this with my cousin Franek, and he has a patch that aims to be
a bigger cleanup of the springs - unfortunately, there is some bug in it (extra
space is inserted after time signature, for example in beam-rest-extreme.ly).
Maybe you could help us with debuggin that?  The patch is in frax/springs branch
of my github fork:

https://github.com/janek-warchol/lilypond/tree/frax/springs

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc
File lily/spring.cc (right):

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc#newcode87
lily/spring.cc:87: distance_ = max (min_distance_, distance_ * r);
On 2014/07/03 04:42:48, Keith wrote:
> maybe  *= r;

yeah, probably.

https://codereview.appspot.com/108280044/diff/1/lily/spring.cc#newcode120
lily/spring.cc:120: avg_distance = max (min_distance + 0.3, avg_distance);
On 2014/07/03 04:42:48, Keith wrote:
> Logically, this would be removed as well, except that it also adds 0.3.  When
> you tried removing this line, you said there were unwanted side-effects. What
> cases needed the extra 0.3 of space? 

0.3 is needed to "slow down" approaching min_distance_, for example in
beam-rest-extreme.ly - because of the accidentals and suspended notes, the
min_distance_ is bigger than distance_, which means that even for "natural"
(ragged-right) spacing the springs would be set to the min distance (making the
accidentals touch previous notes).  We want to add some space, so that the
accidentals will touch previous notes only when there is a significant
compressing force.

Of course, the way in which this is done is really not very good.

> It would be much better to include it in
> padding parameters, than to have this deeply-hidden offset.

Absolutely, that's what we're planning to do.

[Keith, 2014-07-04 07:07:42]

On Wed, 02 Jul 2014 23:56:20 -0700, <janek.lilypond@gmail.com> wrote:

>> you should try
>> to make the change consistently everywhere ?
>
> Yes, that's what i'm aiming for.  However, as you remember, some of
> these changes produce unwanted side-effects

I remember you said there were side effects, but you did not until now
say what they were.

You say you want ideal spacing to be at least as large as the required
'rod' that prevents collisions, if the collisions involve accidentals,

> 0.3 is needed to "slow down" approaching min_distance_, for example in
> beam-rest-extreme.ly - because of the accidentals and suspended notes,
> the min_distance_ is bigger than distance_, which means that even for
> "natural" (ragged-right) spacing the springs would be set to the min
> distance (making the accidentals touch previous notes).  We want to add
> some space [to the ideal distance_ -KOH], so that the accidentals willtouch
previous notes only when there is a significant compressing force.
>

but not if the note-columns are wide due to lyrics.

> Description:
> When objects like lyrics are added to the PaperColumns, LilyPond inserts
> rods between these columns to ensure that the objects won't overlap.
> However, the ideal distance should remain unchanged.

It is surprising that you distinguish those cases by lengthening the ideal
spacing in  merge_springs() but not lengthening the ideal spacing in
set_blocking_force().  I would have expected both functions to be used in
each of your examples, the one with accidentals and that with lyrics.

[janek, 2014-07-04 21:27:04]

Hi Keith,

2014-07-04 9:07 GMT+02:00 Keith OHara <k-ohara5a5a@oco.net>:
> On Wed, 02 Jul 2014 23:56:20 -0700, <janek.lilypond@gmail.com> wrote:
>
>>> you should try
>>> to make the change consistently everywhere ?
>>
>> Yes, that's what i'm aiming for.  However, as you remember, some of
>> these changes produce unwanted side-effects
>
> I remember you said there were side effects, but you did not until now
> say what they were.

I apologize - i mistakenly thought you saw the regtest results back
then.  Now i realize that you couldn't have seen them.

> You say you want ideal spacing to be at least as large as the required
> 'rod' that prevents collisions, if the collisions involve accidentals,
>
>> 0.3 is needed to "slow down" approaching min_distance_, for example in
>> beam-rest-extreme.ly - because of the accidentals and suspended notes,
>> the min_distance_ is bigger than distance_, which means that even for
>> "natural" (ragged-right) spacing the springs would be set to the min
>> distance (making the accidentals touch previous notes).  We want to add
>> some space [to the ideal distance_ -KOH], so that the accidentals
>> willtouch previous notes only when there is a significant compressing force.
>
> but not if the note-columns are wide due to lyrics.
>
>> Description:
>> When objects like lyrics are added to the PaperColumns, LilyPond inserts
>> rods between these columns to ensure that the objects won't overlap.
>> However, the ideal distance should remain unchanged.
>
> It is surprising that you distinguish those cases by lengthening the ideal
> spacing in  merge_springs() but not lengthening the ideal spacing in
> set_blocking_force().

That's actually not what i want to do; I apologize for being unclear.
I *want* the columns to behave the same way in case of both
accidentals and lyrics (and everything else).  The desired behaviour
is that changing min_distance shouldn't affect ideal distance.

Achieving this behaviour when columns are wide due to lyrics is
simple: th, but i don't know how to achieve the desired behaviour
(i.e.

[janek, 2014-07-04 21:38:49]

I'm sorry, i mistakenly clicked "send" too early...

2014-07-04 23:26 GMT+02:00 Janek Warchoł <janek.lilypond@gmail.com>:
>
> That's actually not what i want to do; I apologize for being unclear.
> I *want* the columns to behave the same way in case of both
> accidentals and lyrics (and everything else).  The desired behaviour
> is that changing min_distance shouldn't affect ideal distance.
>
> Achieving this behaviour when columns are wide due to lyrics is
> simple:

Achieving this behaviour when columns are wide due to lyrics is
simple: this patch does this.  However, i didn't manage to fix the
problem for other cases: when i try removing similar lines of code
(e.g. line 120 in merge_springs), the 2462 issue becomes fixed indeed
(i.e. changing min_distance doesn't affect ideal distance), but there
are side effects (too tight spacing in some cases, for example in
beam-rest-extreme.ly) which i'm unable to fix.

After looking at the code and thinking for several hours, it seems
that this problem is too hard for me at the moment :( - i don't know
how to change the code in a way that completely fixes 2462 but doesn't
introduce unwanted side-effects.  As i wrote in a previous email, my
cousin Franek started working on this issue as well, but he got stuck
too (and he's away for at least a week right now).

So, since fixing the whole issue is beyond my capabilities at the
moment, i suggest to fix half of the issue by removing this one line.
Obviously, the downside is that the code would become inconsistent,
but maybe the partial improvement is worth it.

Is the situation clearer now?

thanks for review,
Janek

[Keith, 2014-07-05 17:46:46]

On Fri, 04 Jul 2014 14:38:19 -0700, Janek Warchoł <janek.lilypond@gmail.com>
wrote:

> 2014-07-04 23:26 GMT+02:00 Janek Warchoł <janek.lilypond@gmail.com>:
>>
>> That's actually not what i want to do; I apologize for being unclear.
>> I *want* the columns to behave the same way in case of both
>> accidentals and lyrics (and everything else).  The desired behaviour
>> is that changing min_distance shouldn't affect ideal distance.

Sensible, for the data object Springs.  The special behavior when min_distance
is larger or within 0.3 of ideal is confusing.  The fact that the special
behavior appears only after using merge_springs() makes things more confusing.

Notice that frax/springs keeps that special behavior, in Spring::length().  The
natural, force=0, length of a spring is *always* at least min_distance+0.3

>> Achieving this [desired, not-special] behaviour when columns arewide due to
lyrics is simple:

> but there
> are side effects (too tight spacing in some cases, for example in
> beam-rest-extreme.ly) which i'm unable to fix.

Trace back and see what code uses that +0.3 to good effect, though probably by
accident.

Only spacing-spanner.cc uses Spring::merge_springs(), and when setting
'beam-rest-extreme.ly' it is used to 'merge' only one spring!

So one good effect is that accidentals are prevented from getting too close to
the previous note.  That would be better handled with padding associated with
the accidental
<http://code.google.com/p/lilypond/issues/detail?id=3869>

You could limit the changes in one patch, if you like, by moving the "ideal =
max(idea, min_distance +0.3)" to the point where spacing-spanner.cc finds
min_distance to avoid collisions.  A magic number is better than a magic number
in a mysterious place, and making the 0.3 a parameter is safer to do after we
narrow down its beneficial effect.

If merge_springs() behaves more simply, it might do a little better for
<http://code.google.com/p/lilypond/issues/detail?id=3887>
(I notice that the option 'average-spacing-wishes' is no longer checked; the
code seems to always average spacing between voices.)

[janek, 2014-07-07 10:21:42]

Hi,

sorry for delayed reply - i had a busy weekend (been on a wedding!)

2014-07-05 19:46 GMT+02:00 Keith OHara <k-ohara5a5a@oco.net>:
> On Fri, 04 Jul 2014 14:38:19 -0700, Janek Warchoł <janek.lilypond@gmail.com>
> wrote:
>
>> 2014-07-04 23:26 GMT+02:00 Janek Warchoł <janek.lilypond@gmail.com>:
>>>
>>>
>>> That's actually not what i want to do; I apologize for being unclear.
>>> I *want* the columns to behave the same way in case of both
>>> accidentals and lyrics (and everything else).  The desired behaviour
>>> is that changing min_distance shouldn't affect ideal distance.
>
>
> Sensible, for the data object Springs.  The special behavior when
> min_distance is larger or within 0.3 of ideal is confusing.  The fact that
> the special behavior appears only after using merge_springs() makes things
> more confusing.

Definitely.

> Notice that frax/springs keeps that special behavior, in Spring::length().
> The natural, force=0, length of a spring is *always* at least
> min_distance+0.3

Yes, and that's intended (see below).  However, there may be a better
way of implementing this - i'm not fully satisfied with how it's done
in frax/springs.

>>> Achieving this [desired, not-special] behaviour when columns arewide due
>>> to lyrics is simple:
>
>> but there
>> are side effects (too tight spacing in some cases, for example in
>> beam-rest-extreme.ly) which i'm unable to fix.
>
>
> Trace back and see what code uses that +0.3 to good effect, though probably
> by accident.
>
> Only spacing-spanner.cc uses Spring::merge_springs(), and when setting
> 'beam-rest-extreme.ly' it is used to 'merge' only one spring!

Interesting, i didn't notice that!

> So one good effect is that accidentals are prevented from getting too close
> to the previous note.  That would be better handled with padding associated
> with the accidental
> <http://code.google.com/p/lilypond/issues/detail?id=3869>

I'm not sure if this actually is what we want.  Padding would mean
that the accidental _never_ gets close to the previous notehead, while
we would acutally want to permit that when the compressing force is
sufficiently large.  So, i think that this has to be done at the level
of springs.

> You could limit the changes in one patch, if you like, by moving the "ideal
> = max(idea, min_distance +0.3)" to the point where spacing-spanner.cc finds
> min_distance to avoid collisions.  A magic number is better than a magic
> number in a mysterious place, and making the 0.3 a parameter is safer to do
> after we narrow down its beneficial effect.
>
> If merge_springs() behaves more simply, it might do a little better for
> <http://code.google.com/p/lilypond/issues/detail?id=3887>
> (I notice that the option 'average-spacing-wishes' is no longer checked; the
> code seems to always average spacing between voices.)

ok - this looks like a good path to follow.  Unfortunately, i don't
know when i'll have enough time to chew on this stuff - it seems that
i'll have a busy week (or two).  Please don't feel ignored if I post a
new alignment-related patch instead of working on this issue - i still
understand alignment code much better than springs, so i can put
togethter an alignment patch in time that would be insufficient to do
anything reasonable with springs.

Anyway, thanks a lot for your comments - i understand the springs
better now.  Sooner or later a patch will be written :)
best,
Janek

[Keith, 2014-07-07 15:17:49]

On 2014/07/07 10:21:42, janek wrote:
> ok - this looks like a good path to follow.  Unfortunately, i don't
> know when i'll have enough time to chew on this stuff - it seems that
> i'll have a busy week (or two). 

Let's countdown and push this as-is, then.

[janek, 2014-07-07 16:31:43]

2014-07-07 17:17 GMT+02:00  <k-ohara5a5a@oco.net>:
> On 2014/07/07 10:21:42, janek wrote:
>>
>> ok - this looks like a good path to follow.  Unfortunately, i don't
>> know when i'll have enough time to chew on this stuff - it seems that
>> i'll have a busy week (or two).
>
> Let's countdown and push this as-is, then.

okay!  Should we leave issue 2462 opened, or rather create a new issue
for the spring code refactoring?

thanks,
Janek

[janek, 2014-07-11 06:40:39]

Thanks for review! Pushed as

commit ab6842155a003ba7d9243507594e3e973ebbb3e4
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Mon Jun 30 22:14:16 2014 +0200

     Issue 2462 : don't change ideal spacing when adding a rod
    
    When objects like lyrics are added to the PaperColumns, LilyPond inserts
    rods between these columns to ensure that the objects won't overlap.
    However, the ideal distance should remain unchanged.  For example, in
    
    \layout {
      ragged-right = ##f
    }
    \relative f' { \repeat unfold 8 a4 }
    \addlyrics { \repeat unfold 4 la \repeat unfold 4 straight }
    
    notes in 2nd measure have long lyrics attached to them - min_distance_
    of the springs between these columns should be adjusted, but distance_
    (the ideal distance) should not: it should be the same as for notes
    with short lyrics.  When the line is stretched so much that the minimum
    distances are not involved, the springs between notes in both measures
    should behave (almost) identical.
    
    Note that there are more such problems in the springs code, for example
    in merge_springs.  The code should be rewritten, but that's not trivial.
    Since this one-liner fixes some instances of the problem without any bad
    side-effects, it makes sense to include it separately.


I have opened https://code.google.com/p/lilypond/issues/detail?id=4006 for the
remainder of the spring code problems.