Re: PATCH: Issue 638 Autobeaming

[Carl Sorensen]

On 12/17/09 1:25 AM, "David Kastrup" <address@hidden> wrote:

> Carl Sorensen <address@hidden> writes:
> 
>> On 12/16/09 10:23 PM, "Frédéric Bron" <address@hidden> wrote:
>> 
>>>> At last, thanks to help above and beyond the call of duty by Neil, I
>>>> have finally got the autobeam engraver fixed so it beams 4 4 right
>>>> when there are 16th notes in the 2nd or 4th beat of the measure.
>>> 
>>> Very nice job. That's now a good reason for me to upgrade to 2.13.X.
>>> Does this apply only to 4/4 or is it more general? Is it
>>> custumizable?  and how does it interact with normal commands to set
>>> auto-beaming rules?
>> 
>> There is nothing specific about 4/4 in this patch.  It will apply any
>> time that there is a bigger group for longer duration notes (e.g. 1/8
>> notes) and a smaller group for shorter duration notes (e.g. 1/16
>> notes).
>> 
>> I've not looked carefully at all of the places in standard beaming
>> that it might apply.  I fixed the fundamental problem, so it should
>> work everywhere.
> 
> Ok, stupid question time.  I've glanced over the code, and the way it
> looks to me is that when the previous shortest duration of some note
> group changes, it resets data structures and puts up new accounting,
> continuing with the new accounting.

Not quite.  When a new shortest duration occurs, it checks to see if the
presence of the new shortest duration would cause a previous "continue
the beam at this note" decision to shift to an "end the beam at this note"
decision.

If that decision changes, then and only then is the previous beam (i.e. the
one with the new "end the beam at this note" decision) finished
and engraved, and a new beam (starting immediately after the end of the
previous beam is started.
  
> 
> Deep breath.

Me, too.

> 
> So it would appear that no terminal/irreversible decision based on the
> minimum duration has been done yet at this point of time.
> 
> If that is the case, why not postpone all of the minimum-duration
> dependent accounting to the time where it is actually _needed_?

As far as I can see, it *is* postponed until it is actually needed.  The
problem is that the span of minimum-duration effects is potentially
dependent on both past and future notes.  Most of the beaming cases are
correctly handled at the time the note is received.  In a few cases,
however, the new shortest duration causes a need for a previous break.  So a
recheck is needed, and if necessary, the necessary changes are made.

> 
> There does not seem to be much sense in making some temporary
> calculation based on possibly wrong assumptions when one can safely do
> it at a later point of time anyway.
> 
> So it would appear to me that either we can do the calculation later
> anyway, or that there might be circumstances where the recalculation
> based on the new minimum-duration may not be able to revert some
> decision based on the old assumption, which might be a bug.
> 
> It is quite possible that I am missing some detail here (actually, I am
> missing _all_ details right now because I did not bother to look at this
> level yet, but there might be some _relevant_ detail among them).
>

If you want to understand the details enough to find the potential bug, I'd
be delighted.

If you want to completely rewrite the autobeaming algorithm in order to
avoid the potential bug, I'd be delighted.

If you can show me a case where this potential bug is exhibited, I'd be even
more delighted.
 
> This is just what hits me as a gut feeling about this patch right now.

My gut feeling about this patch is that it is correct.  The limitation in
the previous code has been identified (and not just by me, but by others who
had looked at the bug previously).  The code has been tested on every
condition I could find to see if the beaming was correct.  It has been
demonstrated to be so.  The regression test suite has completed
successfully.  Everything that used to work still works.  Every case that
didn't work (that has been brought to our attention) now works, without any
special code that checks for special cases.

If you have specific comments about a better algorithm based on your
understanding of the way things *really* work, not just gut feel, then I'd
be happy to consider them.

If you find that I've done dangerous things with my C++ code, I'd be
*really* happy to know about it.

If the code I've written is unclear, or convoluted, or could be reorganized
to be easier to read and maintain, then I'd appreciate suggestions about
making it better.  I *want* to write good code, not bad code.

But a review that says "I don't really understand the details, but I don't
trust this" is not particularly helpful to me.  You see, I've done my best
to understand the details, and I *do* trust this. General comments that the
algorithm isn't right aren't likely to change my mind.  Specific
identification of problems is much more likely to cause me to make changes.

Thanks,

Carl