Fixes issue 39 by raising stems

Fixes issue 39 by raising stems

Facultative

Potential fix for issue 39

[Carl, 2011-01-09 05:33:26]

LGTM.

Carl

[Keith, 2011-01-09 07:03:45]

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:276: // magic numbers...
already been said, referring to this block.  I wanted assurance from the comment
that these numbers are intentionally non-monotonic, and reminder that they are
in half-spaces (right?)

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:278: Real rv = raise_vals[durlog - 3];
Do we know 2 < durlog < 7 here? Forever?

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:280: Real xtra = (distant_half_collide ||
close_half_collide) ? 0.0 : 1.3;
I take you to mean,  if (full_collide) rv += 1.3;

http://codereview.appspot.com/3934041/diff/2001/lily/stem-engraver.cc
File lily/stem-engraver.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/stem-engraver.cc#newcode68
lily/stem-engraver.cc:68: stem_->set_property ("note-collision-bump",
scm_from_double (0.0));
Tell us why you use a new property, instead of adjusting 'length.   I would need
a better name to understand this after I forget about this bug; for the way it
acts now, something like extra-raise-tip would help the future me.

http://codereview.appspot.com/3934041/diff/2001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/stem.cc#newcode321
lily/stem.cc:321: return scm_from_double (stem_end + xtra);
Today I remmber that xtra<>0 only for upstems, but someday we will wonder you
/raise/ the flag by note-collision-bump,
even if the stem points down?

[Keith, 2011-01-09 09:52:42]

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:177: if (touch)
why not apply the stem lengthening here?  This boolean says we can try to line
the stems up, and that is the case when big flags can interfere.  
(I do not think you want to address the case of stem_to_stem, below. It includes
back-to-back chords that that are either acceptable without lengthening, or
would require a different approach altogether : << g'64 \\ <g' a'>2 >> )

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:269: else if (distant_half_collide || close_half_collide
|| full_collide) {
placing your new code here, you affect situations with shift>0, that is up-stem
to the right. << g'64 \\ <g' a'>2 >>
Why?

[MikeSol, 2011-01-09 13:28:25]

New patch uploaded to the issue tracker to address Keith's comments.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:177: if (touch)
On 2011/01/09 09:52:42, Keith wrote:
> why not apply the stem lengthening here?  This boolean says we can try to line
> the stems up, and that is the case when big flags can interfere.  
> (I do not think you want to address the case of stem_to_stem, below. It
includes
> back-to-back chords that that are either acceptable without lengthening, or
> would require a different approach altogether : << g'64 \\ <g' a'>2 >> )

Done.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:269: else if (distant_half_collide || close_half_collide
|| full_collide) {
On 2011/01/09 09:52:42, Keith wrote:
> placing your new code here, you affect situations with shift>0, that is
up-stem
> to the right. << g'64 \\ <g' a'>2 >>
> Why?

Moved above.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:276: // magic numbers...
On 2011/01/09 07:03:45, Keith wrote:
> already been said, referring to this block.  I wanted assurance from the
comment
> that these numbers are intentionally non-monotonic, and reminder that they are
> in half-spaces (right?)

Done.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:278: Real rv = raise_vals[durlog - 3];
On 2011/01/09 07:03:45, Keith wrote:
> Do we know 2 < durlog < 7 here? Forever?

Done.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:280: Real xtra = (distant_half_collide ||
close_half_collide) ? 0.0 : 1.3;
On 2011/01/09 07:03:45, Keith wrote:
> I take you to mean,  if (full_collide) rv += 1.3;

Done.

http://codereview.appspot.com/3934041/diff/2001/lily/stem-engraver.cc
File lily/stem-engraver.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/stem-engraver.cc#newcode68
lily/stem-engraver.cc:68: stem_->set_property ("note-collision-bump",
scm_from_double (0.0));
On 2011/01/09 07:03:45, Keith wrote:
> Tell us why you use a new property, instead of adjusting 'length.   I would
need
> a better name to understand this after I forget about this bug; for the way it
> acts now, something like extra-raise-tip would help the future me.

Done.

[Carl, 2011-01-11 17:20:27]

Looks very good to me.

I'd like to see the property name changed to extra-stem-length.

Thanks,

Carl

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc#newcode322
lily/stem.cc:322: Real extra_raise_tip = scm_to_double (me->get_property
("extra-raise-tip"));
I'd prefer extra-stem-length.  I realize that when the flags are down we already
have an adjustment.  But I think that extra-stem-length is a better description
than extra-raise-tip.

[Keith, 2011-01-12 00:31:36]

I seem to be playing bad-cop to Carl's good cop.  Maybe I'm being picky because
the original issue didn't bother me very much, so I don't like side effects to
the fix.  I'll say what I don't like, test at my own suggestions at my slow
pace, and let Carl overrule if appropriate.

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:177: if (touch)
On 2011/01/09 09:52:42, Keith wrote:
> why not apply the stem lengthening here? 

Oops, then we lengthen in more common cases, <<g32\\g32>> and
merge-differently-headed, which looked better the old way.

I think the ideal target set is (touch && !merge_possible) but I will be slow to
test (have an evening meeting).

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc#newcode322
lily/stem.cc:322: Real extra_raise_tip = scm_to_double (me->get_property
("extra-raise-tip"));
On 2011/01/11 17:20:27, Carl wrote:
> But I think that extra-stem-length is a better description

It is a better description of what we want, yes, but then it should act that
way.

The note-collisions determine a minimum-stem-length that probably should apply
12 lines up, near the length adjustment for the stem's own noteheads.  
length := min (me->"length", minimum-stem-length)

Then stems that are lengthened anyway to reach the staff centerline <<c32\\c2>>
should be left alone, as well as those lengthened due to with chords in the
flagged stem << <g f' g'>64\\g2 >>.

[c_sorensen, 2011-01-12 02:51:26]

On 1/11/11 5:31 PM, "k-ohara5a5a@oco.net" <k-ohara5a5a@oco.net> wrote:

> I seem to be playing bad-cop to Carl's good cop.  Maybe I'm being picky
> because the original issue didn't bother me very much, so I don't like
> side effects to the fix.  I'll say what I don't like, test at my own
> suggestions at my slow pace, and let Carl overrule if appropriate.

No, you're being the thorough cop to Carl's much less-thorough cop.

If it passes regtests, I consider it fine.

If it breaks other things that aren't in regtests, you catch it.  I think
your work is invaluable.

When fixing bugs, we likely ought to have the rule of "first do no harm".
If we make common automatic engraving worse in order to fix uncommon
problems, we're going backwards.

Please keep up your careful checking!

> 
> 
> http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
> File lily/note-collision.cc (right):
> 
> http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcode
> 177
> lily/note-collision.cc:177: if (touch)
> On 2011/01/09 09:52:42, Keith wrote:
>> why not apply the stem lengthening here?
> 
> Oops, then we lengthen in more common cases, <<g32\\g32>> and
> merge-differently-headed, which looked better the old way.
> 
> I think the ideal target set is (touch && !merge_possible) but I will be
> slow to test (have an evening meeting).
> 
> http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc
> File lily/stem.cc (right):
> 
> http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc#newcode322
> lily/stem.cc:322: Real extra_raise_tip = scm_to_double (me->get_property
> ("extra-raise-tip"));
> On 2011/01/11 17:20:27, Carl wrote:
>> But I think that extra-stem-length is a better description
> 
> It is a better description of what we want, yes, but then it should act
> that way.

I have not followed the code carefully enough to understand the difference.
What is the difference between "extra-raise-tip" and "extra-stem-length" in
terms of how it acts?

> 
> The note-collisions determine a minimum-stem-length that probably should
> apply 12 lines up, near the length adjustment for the stem's own
> noteheads.
> length := min (me->"length", minimum-stem-length)
> 
> Then stems that are lengthened anyway to reach the staff centerline
> <<c32\\c2>> should be left alone, as well as those lengthened due to
> with chords in the flagged stem << <g f' g'>64\\g2 >>.

This seems reasonable to me.

Thanks,

Carl

[Keith, 2011-01-12 07:20:22]

On 2011/01/12 02:51:26, c_sorensen_byu.edu wrote:
> What is the difference between "extra-raise-tip" and "extra-stem-length" in
> terms of how it acts?
> 
I was associating variable-names with the names of functions whose return values
they affect.  We are here adjusting the output of Stem::calc_stem_end_position()
so extra-raise-tip seemed honest.

When you said you preferred the name extra-stem-length my mind went to
Stem::calc_length().  The return from calc_length() already has the extra length
required if there are tremolo bars, then gets the correction for chords on this
stem, then lengthened if required to reach the centerline.  I thought to myself,
Carl is probably thinking Mike's correction applies earlier in the flow,
constructed test cases to see the difference that would make, and concluded that
I don't like the effects of applying the issue39 fix this late in the data flow.

[MikeSol, 2011-01-12 15:34:22]

Fixed (I think).  Will run regtests in a bit.  Meanwhile, it makes this cleanly:


\relative c' {
\time 2/4
<< { \autoBeamOff c16 s4.. c'16 s4.. c,16 s4.. <c' e g c>16 s4.. c4 } \\ { b,2
b'2 c,2 c'2 c4 } >>
}

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc
File lily/note-collision.cc (right):

http://codereview.appspot.com/3934041/diff/2001/lily/note-collision.cc#newcod...
lily/note-collision.cc:177: if (touch)
On 2011/01/12 00:31:36, Keith wrote:
> On 2011/01/09 09:52:42, Keith wrote:
> > why not apply the stem lengthening here? 
> 
> Oops, then we lengthen in more common cases, <<g32\\g32>> and
> merge-differently-headed, which looked better the old way.
> 
> I think the ideal target set is (touch && !merge_possible) but I will be slow
to
> test (have an evening meeting).

Done.

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc#newcode322
lily/stem.cc:322: Real extra_raise_tip = scm_to_double (me->get_property
("extra-raise-tip"));
On 2011/01/12 00:31:36, Keith wrote:
> On 2011/01/11 17:20:27, Carl wrote:
> > But I think that extra-stem-length is a better description
> 
> It is a better description of what we want, yes, but then it should act that
> way.
> 
> The note-collisions determine a minimum-stem-length that probably should apply
> 12 lines up, near the length adjustment for the stem's own noteheads.  
> length := min (me->"length", minimum-stem-length)
> 
> Then stems that are lengthened anyway to reach the staff centerline
<<c32\\c2>>
> should be left alone, as well as those lengthened due to with chords in the
> flagged stem << <g f' g'>64\\g2 >>. 

Done.

[Keith, 2011-01-13 06:03:14]

On 2011/01/12 15:34:22, MikeSol wrote:
> Fixed (I think).  

I like how this patch behaves!
No problems at all this time in the music I threw at it  (but I haven't yet seen
the original issue 39 in music either).

Playing around with the various situations, only 2 problems.

1) the flag in << e'32. \\ e'2>> raises enough to let the dot try to slip in,
and the dot merges with the flag.  Adding 0.2 to raise_tip gives enough
clearance.  This case isn't really within issue39 because the flagged note goes
on the right, but it falls within the test I suggested:
> > I think the ideal target set is (touch && !merge_possible) but I will be
slow

2) the flag raises for no reason on a few chords where stem-shorten affects the
flag; compare
<< <a' c''>16 \\ a'2 >>  << <c'' e''>16 \\ c''2 >>

http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc#newcode398
lily/stem.cc:398: if (initial_length + extra_stem_length + hp[DOWN] > stem_end)
stem-shorten fooled this filter by reducing your pre-computation of stem_end,
but not reducing initial_length.  I see the contortions you went through to
apply your correction within stem_length().  If you think it fits more naturally
in calc_stem_end_position, that is probably wiser.  I think I see a good fit but
this time I'll test before I suggest.

[mikesol_ufl.edu, 2011-01-13 11:23:28]

Thanks Keith,

I think that the case you're talking about (<< e'32. \\ e'2>>) is a problem with
my code, which triggers stem raising for flags that fall on the right even
though these flags do note cause intersection problems.  Is there a good way to
weed this out?

On Jan 13, 2011, at 1:03 AM, k-ohara5a5a@oco.net wrote:

> On 2011/01/12 15:34:22, MikeSol wrote:
>> Fixed (I think).
> 
> I like how this patch behaves!
> No problems at all this time in the music I threw at it  (but I haven't
> yet seen the original issue 39 in music either).
> 
> Playing around with the various situations, only 2 problems.
> 
> 1) the flag in << e'32. \\ e'2>> raises enough to let the dot try to
> slip in, and the dot merges with the flag.  Adding 0.2 to raise_tip
> gives enough clearance.  This case isn't really within issue39 because
> the flagged note goes on the right, but it falls within the test I
> suggested:
>> > I think the ideal target set is (touch && !merge_possible) but I
> will be slow
> 
> 2) the flag raises for no reason on a few chords where stem-shorten
> affects the flag; compare
> << <a' c''>16 \\ a'2 >>  << <c'' e''>16 \\ c''2 >>
> 
See below.
> 
> http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc
> File lily/stem.cc (right):
> 
> http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc#newcode398
> lily/stem.cc:398: if (initial_length + extra_stem_length + hp[DOWN] >
> stem_end)
> stem-shorten fooled this filter by reducing your pre-computation of
> stem_end, but not reducing initial_length.  I see the contortions you
> went through to apply your correction within stem_length().  If you
> think it fits more naturally in calc_stem_end_position, that is probably
> wiser.  I think I see a good fit but this time I'll test before I
> suggest.
> 
I am working on an article for the next two days, so if you have time to cook up
a patch that better deals with stem-shorten, I'd be much obliged!

Cheers,
MS

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

[MikeSol, 2011-06-25 10:53:40]

On 2011/01/13 11:23:28, mikesol_ufl.edu wrote:
> Thanks Keith,
> 
> I think that the case you're talking about (<< e'32. \\ e'2>>) is a problem
with
> my code, which triggers stem raising for flags that fall on the right even
> though these flags do note cause intersection problems.  Is there a good way
to
> weed this out?
> 
> On Jan 13, 2011, at 1:03 AM, mailto:k-ohara5a5a@oco.net wrote:
> 
> > On 2011/01/12 15:34:22, MikeSol wrote:
> >> Fixed (I think).
> > 
> > I like how this patch behaves!
> > No problems at all this time in the music I threw at it  (but I haven't
> > yet seen the original issue 39 in music either).
> > 
> > Playing around with the various situations, only 2 problems.
> > 
> > 1) the flag in << e'32. \\ e'2>> raises enough to let the dot try to
> > slip in, and the dot merges with the flag.  Adding 0.2 to raise_tip
> > gives enough clearance.  This case isn't really within issue39 because
> > the flagged note goes on the right, but it falls within the test I
> > suggested:
> >> > I think the ideal target set is (touch && !merge_possible) but I
> > will be slow
> > 
> > 2) the flag raises for no reason on a few chords where stem-shorten
> > affects the flag; compare
> > << <a' c''>16 \\ a'2 >>  << <c'' e''>16 \\ c''2 >>
> > 
> See below.
> > 
> > http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc
> > File lily/stem.cc (right):
> > 
> > http://codereview.appspot.com/3934041/diff/29001/lily/stem.cc#newcode398
> > lily/stem.cc:398: if (initial_length + extra_stem_length + hp[DOWN] >
> > stem_end)
> > stem-shorten fooled this filter by reducing your pre-computation of
> > stem_end, but not reducing initial_length.  I see the contortions you
> > went through to apply your correction within stem_length().  If you
> > think it fits more naturally in calc_stem_end_position, that is probably
> > wiser.  I think I see a good fit but this time I'll test before I
> > suggest.
> > 
> I am working on an article for the next two days, so if you have time to cook
up
> a patch that better deals with stem-shorten, I'd be much obliged!
> 
> Cheers,
> MS
> 
> > http://codereview.appspot.com/3934041/
> 

Keith: are you still interested in tackling this problem?  I know there have
been some changes in note-collision.cc, so I'll likely scrap this patch and
start from scratch, but if you think you have a better handle on how to tackle
this issue, feel free to take a stab at it - I'm in composin' mode and don't
have much development time these days :(

Cheers,
MS

[Keith, 2011-06-25 18:58:15]

On Sat, 25 Jun 2011 03:53:40 -0700, <mtsolo@gmail.com> wrote:

> Keith: are you still interested in tackling this problem?  I know there
> have been some changes in note-collision.cc, so I'll likely scrap this
> patch and start from scratch,

I purposely put it off until 2.14 was out.
Looking at it now, I am more motivated to fix the other chord-collision stuff
first :

the dot-notehead collision you noticed (just want to polish my patch)
Issue 618 (still fails for << <g' a'>2 \\ g'4 >> )
and issue 984.

I still have a branch with your patch, just in case I get to issue 39 before
you, if you want to close the Rietveld.

[lemniskata.bernoulliego, 2011-06-25 20:24:43]

Hi,

have you considered fixing issue 39 by shortening the flag (as we're
going to have plenty of shortened flags available)? Or maybe
shortening a flag a bit ang lenghtening the stem a bit would be the
best solution?
As Carl found a way to separate flags from noteheads in font
(http://codereview.appspot.com/4662055/), the work on shortened flags
starts moving again.

cheers,
Janek

[Keith, 2011-06-25 22:12:31]

On Sat, 25 Jun 2011 13:24:28 -0700, Jan Warchoł
<lemniskata.bernoulliego@gmail.com> wrote:

> have you considered fixing issue 39 by shortening the flag (as we're
> going to have plenty of shortened flags available)? Or maybe
> shortening a flag a bit and lenghtening the stem a bit would be the
> best solution?


Yes, and that simplifies the task a bit because any code that lengthens the stem
has to cooperate with other code that changes the stem length for other reasons
(such as for reaching back to the centerline when the stemmed note is on ledger
lines).

The general challenge is recognizing the situations that benefit from shorter
flag (or longer stem) without false positives.

[MikeSol, 2011-06-29 14:20:23]

On 2011/06/25 18:58:15, Keith wrote:
> On Sat, 25 Jun 2011 03:53:40 -0700, <mailto:mtsolo@gmail.com> wrote:
> 
> > Keith: are you still interested in tackling this problem?  I know there
> > have been some changes in note-collision.cc, so I'll likely scrap this
> > patch and start from scratch,
> 
> I purposely put it off until 2.14 was out.
> Looking at it now, I am more motivated to fix the other chord-collision stuff
> first :
> 
> the dot-notehead collision you noticed (just want to polish my patch)
> Issue 618 (still fails for << <g' a'>2 \\ g'4 >> )
> and issue 984.
> 
> I still have a branch with your patch, just in case I get to issue 39 before
> you, if you want to close the Rietveld.
> 

I'll close the Rietveld, then.  You'll probably get to it before me if you're
dabbling in chord-collision - I'm doing a fair bit of fixin' in other areas of
lilyland.

Thanks!
~Mike