Revised autobeam settings patch -- cleaned up debug comments

Revised autobeam settings patch -- cleaned up debug comments
in code and eliminated the irrelevant changes in
Documentation/snippets just due to running makelsr.py


Redo autobeam settings to make resetting easier
Autobeaming now depends on context properties that can be
\set by the user.   When the time signature is changed, default
autobeam settings for the time signature are read and the context
properties are changed to set the autobeaming properties.

This change eliminates \overrideBeamSettings and \revertBeamSettings.

New functions have been defined to set time signature default
properties:
\overrideTimeSignatureSettings
and
\revertTimeSignatureSettings
in order to give autobeam settings persistence through time signature
changes.

A Scheme function make-setting has been defined to make it
easier to create a time signature setting.

[Carl, 2010-06-15 22:27:48]

I've posted a revised patch under a new issue, because I was working with my git
branches to get the patch to only include the changed files.

I've eliminated the leftover debug code, fixed up the mistakes in ly/bagpipe.ly,
and adjusted the description of timeSignatureSettings.

I'm sorry about the confusion on the earlier patch.  I believe this one is
better for review.

Thanks,

Carl

[t.daniels_treda.co.uk, 2010-06-16 09:18:20]

Carl.D.Sorensen wrote Tuesday, June 15, 2010 11:27 PM

> Description:
> Revised autobeam settings patch -- cleaned up debug comments
> in code and eliminated the irrelevant changes in
> Documentation/snippets just due to running makelsr.py
>
> Please review this at http://codereview.appspot.com/1667044/show

I've run a few examples through this new code and so
far it all works extremely well.  

One or two of the default beam settings might be 
improved (while you're changing the beaming, that is - I 
think the behaviour below is probably as in the current 
releases, not introduced in this patch).  The most 
important is illustrated by

\relative c' {
  \time 3/4
  % In 3/4 time never beam an odd number of 8th notes or two
  % 8th notes in different beats
  f8 f f f f f
  f16 f f f f f f f f f f f
  f32 f f f f f f f f f f f
  f f f f f f f f f f f f
  f4 r8 f f f     % incorrect!
  f8 f~f f f f    % incorrect!
  d'4. c8 b8. a16  % incorrect!
}

I think here it would be better to break quaver beams
every beat (unless you have a trick up your sleeve
to recognise these incorrect cases).

Also in 4/2 I think it would be better to remove the
32nds beam rule so they are grouped as 16ths are.

It might also be good to add triplet beaming to a few
more common time signatures, as is done for 4/4.

The documentation looks much improved.  I would only
suggest saying how to disable autobeaming so the 
beaming follows beatLength somewhere early, and maybe
including an example showing how to use \set beamSettings
to create settings for more than one beam type (getting
the Scheme brackets right is tricky for everyone who is
unfamiliar with Scheme!)

Congratulations on a fine piece of work!  It's a credit
to your persistence in persevering with this issue!

Trevor

[c_sorensen, 2010-06-16 12:30:52]

On 6/16/10 3:18 AM, "Trevor Daniels" <t.daniels@treda.co.uk> wrote:

> 
> 
> Carl.D.Sorensen wrote Tuesday, June 15, 2010 11:27 PM
> 
>> Description:
>> Revised autobeam settings patch -- cleaned up debug comments
>> in code and eliminated the irrelevant changes in
>> Documentation/snippets just due to running makelsr.py
>> 
>> Please review this at http://codereview.appspot.com/1667044/show
> 
> I've run a few examples through this new code and so
> far it all works extremely well.
> 
> One or two of the default beam settings might be
> improved (while you're changing the beaming, that is - I
> think the behaviour below is probably as in the current
> releases, not introduced in this patch).  The most
> important is illustrated by
> 
> \relative c' {
>   \time 3/4
>   % In 3/4 time never beam an odd number of 8th notes or two
>   % 8th notes in different beats
>   f8 f f f f f
>   f16 f f f f f f f f f f f
>   f32 f f f f f f f f f f f
>   f f f f f f f f f f f f
>   f4 r8 f f f     % incorrect!
>   f8 f~f f f f    % incorrect!
>   d'4. c8 b8. a16  % incorrect!
> }
> 
> I think here it would be better to break quaver beams
> every beat (unless you have a trick up your sleeve
> to recognise these incorrect cases).

If we adjusted the beaming so it never *started* a beam on the final note of
a beat would that solve these problems?  There is provision in the code to
check for whether a beam can start; right now we say a beam can start
anywhere.  It occurs to me that in the cases above, we could resolve with a
beam starting rule.

> 
> Also in 4/2 I think it would be better to remove the
> 32nds beam rule so they are grouped as 16ths are.
> 

I'll be happy to do so.

> It might also be good to add triplet beaming to a few
> more common time signatures, as is done for 4/4.

With the new beaming rules, I don't think we should need to add triplet
beaming in most cases.

> 
> The documentation looks much improved.  I would only
> suggest saying how to disable autobeaming so the
> beaming follows beatLength somewhere early, and maybe
> including an example showing how to use \set beamSettings
> to create settings for more than one beam type (getting
> the Scheme brackets right is tricky for everyone who is
> unfamiliar with Scheme!)

Great suggestions, thanks!

> 
> Congratulations on a fine piece of work!  It's a credit
> to your persistence in persevering with this issue!
> 

Thanks for the kind words.

Carl

[Neil Puttock, 2010-06-16 22:21:43]

Hi Carl,

There's a file missing from this set: time-signature-settings.scm.

Cheers,
Neil

[Neil Puttock, 2010-06-16 23:31:23]

Carl, have you looked at the regtest output?

I've copied time-signature-settings.scm from the other set (I assume it's
correct) and run `make check'; there's a significant number of changed tests
showing up.  In particular, 6/8, 9/8 and 12/8 tests are mostly unbeamed.

There's some weirdness going on in 2/4 whereby some beats are unbeamed.

Incomplete bars are unbeamed, even if they constitued complete beats.

Cheers,
Neil

[Carl, 2010-06-19 04:55:25]

On 2010/06/16 23:31:23, Neil Puttock wrote:
> Carl, have you looked at the regtest output?

Yes, but I made a mistake when I did so.  I've now got the regtest output all
working right
except for one beam in measure 5 of slur-scoring.ly.

In the baseline, notes 2/8, 3/8, and 4/8 are all beamed together.  In the new
version, 2/8 stands alone, and 3/8 and 4/8 are beamed together.

Both versions can be used, according to Ross.  The reason we have 2/8 standing
alone is to prevent 2/8, 3/8, and 4/8 from being beamed together in 3/4 time,
where that would be an error.

I propose that we leave the autobeaming as is, and manually beam measure 5 of
slur-scoring.ly if we want to have those 3 notes beamed together.  But I didn't
want to do that, because I didn't want to hide the change in auto-beam behavior.
> 
> There's some weirdness going on in 2/4 whereby some beats are unbeamed.
> 
> Incomplete bars are unbeamed, even if they constitued complete beats.
> 

Were you referring to the reg tests, or to some other files?  If it's other
files, could you please send me a copy?

Thanks,

Carl

> Cheers,
> Neil

[Carl, 2010-06-19 04:56:20]

Oh, I forgot to mention that a new version of the patch has been posted for
review.

THanks,

Carl

[Neil Puttock, 2010-06-20 20:23:32]

On 2010/06/19 04:55:25, Carl wrote:

> Both versions can be used, according to Ross.  The reason we have 2/8 standing
> alone is to prevent 2/8, 3/8, and 4/8 from being beamed together in 3/4 time,
> where that would be an error.

I don't consider that an error; it's traditional typesetting style. :)
 
> Were you referring to the reg tests, or to some other files?  If it's other
> files, could you please send me a copy?

It was from the regtests, but it looks like you've fixed it now (grace notes
breaking the previous beam).

Cheers,
Neil

[Neil Puttock, 2010-06-22 22:58:53]

Hi Carl,

Here are some preliminary comments.  I'll try to give the patch a more thorough
going over at the weekend.

The indentation for the alist in time-signature-settings.scm is a bit compressed
(particularly for beam-settings).  Here's a sample of how it looks after
reindentation:

(define-public default-time-signature-settings
  '(
    ;; in 2 2 time:
    ;;   default: end beams on 1 2 note boundaries
    ;;   end beams with 32nd notes each 1 4 beat
    ((2 . 2) .
     ((beat-length . (1 . 2))
      (measure-grouping . (1 1))
      (beam-settings .
		     ((end .
			   (((1 . 8) . (4 4))
			    ((1 . 32) . (8 8 8 8))))))))

Cheers,
Neil

http://codereview.appspot.com/1667044/diff/21001/22013
File Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly
(right):

http://codereview.appspot.com/1667044/diff/21001/22013#newcode1
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:1: %%
Note: this file works from version 2.13.22
remove

http://codereview.appspot.com/1667044/diff/21001/22013#newcode31
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:31: } %
begin verbatim
remove

http://codereview.appspot.com/1667044/diff/21001/22025
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22025#newcode425
ly/music-functions-init.ly:425: #(override-time-signature-setting
Is override-time-signature-setting likely to be used outside this music function
wrapper?  If not, then it would be simpler to remove ly:export from the function
and remove the #{ }# block.

(same for revert-time-signature-setting)

http://codereview.appspot.com/1667044/diff/21001/22026
File python/convertrules.py (right):

http://codereview.appspot.com/1667044/diff/21001/22026#newcode3013
python/convertrules.py:3013: stderr_write(NOT_SMART % _("\overrideBeamSettings
is obsolete; see \set BeamSettings and \overrideTimeSignatureSettings.\n"))
NOT_SMART needs a sentence continuation

http://codereview.appspot.com/1667044/diff/21001/22028
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22028#newcode476
scm/define-context-properties.scm:476: time signatures.  Contains an elements
for various time signatures.  The
Contains elements

http://codereview.appspot.com/1667044/diff/21001/22028#newcode477
scm/define-context-properties.scm:477: element for each time signature contains
entries for @code{measure-length},
measureLength

http://codereview.appspot.com/1667044/diff/21001/22028#newcode478
scm/define-context-properties.scm:478: @code{measure-grouping}, and
@code{beamSettings}.")
measureGrouping

http://codereview.appspot.com/1667044/diff/21001/22031
File scm/music-functions.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22031#newcode561
scm/music-functions.scm:561: (map (lambda(x) (/ (* 8 x) den))
lambda (x)

http://codereview.appspot.com/1667044/diff/21001/22032
File scm/time-signature-settings.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22032#newcode197
scm/time-signature-settings.scm:197: ((beat-length . (1 . 4))
I think (3 . 4) is more likely

e.g., Chopin's G minor Ballade

http://codereview.appspot.com/1667044/diff/21001/22033
File scripts/build/output-distance.py (right):

http://codereview.appspot.com/1667044/diff/21001/22033#newcode90
scripts/build/output-distance.py:90: system ('compare -depth 8 %(dir)s/crop1.png
%(dir)s/crop2.png %(dir)s/diff.png' % locals ())
not part of this patch set

[Carl, 2010-06-23 01:51:10]

THanks for the review, Neil.

As far as the indentation on the alist goes, did you reindent the whole list, or
just the beam-settings part?  I ask this, because the sample you gave has
different indentation for beam-settings than for the time signature.

Thanks,

Carl

http://codereview.appspot.com/1667044/diff/21001/22013
File Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly
(right):

http://codereview.appspot.com/1667044/diff/21001/22013#newcode1
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:1: %%
Note: this file works from version 2.13.22
On 2010/06/22 22:58:53, Neil Puttock wrote:
> remove

Fixed, and changed version to 2.13.26.

http://codereview.appspot.com/1667044/diff/21001/22013#newcode31
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:31: } %
begin verbatim
On 2010/06/22 22:58:53, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/1667044/diff/21001/22015
File Documentation/snippets/new/reverting-default-beam-endings.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22015#newcode1
Documentation/snippets/new/reverting-default-beam-endings.ly:1: \version
"2.13.24"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22017
File input/regression/les-nereides.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22017#newcode1
input/regression/les-nereides.ly:1: \version "2.13.4"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22023
File ly/bagpipe.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22023#newcode12
ly/bagpipe.ly:12: \version "2.13.20"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22025
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22025#newcode425
ly/music-functions-init.ly:425: #(override-time-signature-setting
On 2010/06/22 22:58:53, Neil Puttock wrote:
> Is override-time-signature-setting likely to be used outside this music
function
> wrapper?  If not, then it would be simpler to remove ly:export from the
function
> and remove the #{ }# block.
> 
> (same for revert-time-signature-setting)

Done.

http://codereview.appspot.com/1667044/diff/21001/22026
File python/convertrules.py (right):

http://codereview.appspot.com/1667044/diff/21001/22026#newcode3013
python/convertrules.py:3013: stderr_write(NOT_SMART % _("\overrideBeamSettings
is obsolete; see \set BeamSettings and \overrideTimeSignatureSettings.\n"))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> NOT_SMART needs a sentence continuation

Done.

http://codereview.appspot.com/1667044/diff/21001/22028
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22028#newcode476
scm/define-context-properties.scm:476: time signatures.  Contains an elements
for various time signatures.  The
On 2010/06/22 22:58:53, Neil Puttock wrote:
> Contains elements

Done.

http://codereview.appspot.com/1667044/diff/21001/22028#newcode477
scm/define-context-properties.scm:477: element for each time signature contains
entries for @code{measure-length},
On 2010/06/22 22:58:53, Neil Puttock wrote:
> measureLength

Done.

http://codereview.appspot.com/1667044/diff/21001/22028#newcode478
scm/define-context-properties.scm:478: @code{measure-grouping}, and
@code{beamSettings}.")
On 2010/06/22 22:58:53, Neil Puttock wrote:
> measureGrouping

Done.

http://codereview.appspot.com/1667044/diff/21001/22031
File scm/music-functions.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22031#newcode561
scm/music-functions.scm:561: (map (lambda(x) (/ (* 8 x) den))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> lambda (x)

Done.

http://codereview.appspot.com/1667044/diff/21001/22032
File scm/time-signature-settings.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22032#newcode197
scm/time-signature-settings.scm:197: ((beat-length . (1 . 4))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> I think (3 . 4) is more likely
> 
> e.g., Chopin's G minor Ballade
> 

We achieve the equivalent of 3/4 beat length by setting the beat length to 1/4
and using measureGrouping of (3 3) and a default end rule of (1 . 8) . (6 6).

This is necessary because we want to have measure grouping signs of triangles
for the first and second half of the measure, which requires a group of 3.

I think we can make this stronger by eliminating the (1 . 8) end rule and
replacing it with (1 . 4) . (3 3) .  This has the effect of making any beam
break at 3/4, unless a finer rule is given.

At present, the rules are basically translated directly from the current
beam-settings.scm.  I was planning on overhauling this list once the basic
functionality of the patch was accepted.  But perhaps I should go ahead and
revise this list now.

http://codereview.appspot.com/1667044/diff/21001/22033
File scripts/build/output-distance.py (right):

http://codereview.appspot.com/1667044/diff/21001/22033#newcode90
scripts/build/output-distance.py:90: system ('compare -depth 8 %(dir)s/crop1.png
%(dir)s/crop2.png %(dir)s/diff.png' % locals ())
On 2010/06/22 22:58:53, Neil Puttock wrote:
> not part of this patch set

It's not part of my patch set in git.  This change shows up due to my using a
version with the patch applied as my reference.  I'll try to get the right
reference for the next set.

[Neil Puttock, 2010-06-27 22:39:57]

On 2010/06/23 01:51:10, Carl wrote:

> As far as the indentation on the alist goes, did you reindent the whole list,
or
> just the beam-settings part?  I ask this, because the sample you gave has
> different indentation for beam-settings than for the time signature.

It's indented from the start of the alist in emacs.

> At present, the rules are basically translated directly from the current
> beam-settings.scm.  I was planning on overhauling this list once the basic
> functionality of the patch was accepted.  But perhaps I should go ahead and
> revise this list now.

I'm thinking more of the way the beaming is broken when there's a rest. 
Currently, 6/4 will beam

r8 c c c c c

with one beam spanning the notes, whereas your patch only beams the last four
notes unless I change the beat length.

Cheers,
Neil