|
|
Created:
9 years, 10 months ago by janek Modified:
9 years, 9 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionWhen 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.
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 10
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.)
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
|