Fix beamlet pointing wrong way when tuplet 16th beamed with 8th (issue 2113)

Fix beamlet pointing wrong way when tuplet 16th beamed with 8th (issue 2113)

[Keith, 2011-12-17 08:24:10]

Maybe just restore the old tests (from before the issue 11 fix) with the comment
 //Try to avoid sticking-out flags

http://codereview.appspot.com/5495076/diff/1/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5495076/diff/1/lily/beaming-pattern.cc#newcode104
lily/beaming-pattern.cc:104: return (infos_[i].rhythmic_importance_ < infos_[i +
1].rhythmic_importance_)
If this fixes the reported bug then,
1) the triplet 16th has the same rhytmic importance as the 8th note, which seems
odd
2) it won't fix the case where the timing is the other way
{ \times 2/3 { b16 b b } b8 b4  b8 \times 2/3 { b16 b b } b4 }

[Carl, 2011-12-17 12:54:34]

On 2011/12/17 08:24:10, Keith wrote:
> Maybe just restore the old tests (from before the issue 11 fix) with the
comment
>  //Try to avoid sticking-out flags

No, that old condition caused other problems in ignoring rhythmic importance and
had beamlets pointing the wrong way.

> If this fixes the reported bug then,
> 1) the triplet 16th has the same rhytmic importance as the 8th note, which
seems
> odd

You're right.  It is odd.  It didn't seem odd to me at the time, but I wasn't
thinking straight.  Getting the right rhythmic importance will fix things.  And
I know why it's currently wrong; now I just have to figure out how to get it
right.

> 2) it won't fix the case where the timing is the other way
> { \times 2/3 { b16 b b } b8 b4  b8 \times 2/3 { b16 b b } b4 }

Thanks for pointing this out.  I *thought* about this case, but didn't test it. 
And in my mind, when the wrong rhythmic importance was right, it would have
worked....

[Carl, 2011-12-17 15:54:27]

New patch set uploaded.

[Keith, 2011-12-17 18:55:18]

This looks good.
I found a way to break it, but only with perverse input.

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc#newco...
lily/beaming-pattern.cc:174: int tuplet_count = (int) factor.den ();
The name 'tuplet_count' is used differently later in this file.  This usage, 3
for triplets, seems to fit the name better.

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc#newco...
lily/beaming-pattern.cc:210: int tuplet_count = factor.num ();
The other use of the name: how many notes would fill the time if they were not
in a tuplet

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc#newco...
lily/beaming-pattern.cc:273: Moment ratio = ((tuplet_count == 1) ? dt :
tuplet_dt) / options.base_moment_ / tuplet_moment;
I would think you want tuplet_count = 3 here for
  { \times 1/3 { b16 b b } b8 b8 \times 1/3 { b16 b b } }
but you get the other definition with tuplet_count = 1 and the beamlets get
pointed wrongly.

[Keith, 2011-12-17 19:11:07]

My perverse example should have dotted-eighths, to make at least some sense
{ \times 1/3 { b16 b b } b8. b8. \times 1/3 { b16 b b } }

[Carl, 2011-12-18 04:25:37]

New patch set uploaded.

Thanks for the review, Keith, and for trying to break the code.  I appreciate
the thorough testing.

I've tried to put it through a torture test of my own, and as long as I keep
things rational, it appears to work.  I can "break" it by starting a tuplet at a
weird time, but I'm not sure it's broken, given the weird timing in the measures
that break it.

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/5495076/diff/1004/lily/beaming-pattern.cc#newco...
lily/beaming-pattern.cc:174: int tuplet_count = (int) factor.den ();
Yes, tuplet_count should have been the denominator.  Somehow I made a mistake
earlier using it as the numerator, then copied the mistake here.

I've renamed the variable to make it clearer.

[Keith, 2011-12-19 06:43:08]

Looks good to me, of course.