Improve beam count handling with subdivided beams

Improve beam count handling with subdivided beams

Subdivision beam count was improved in issue 4355, commit 8fa2d858,
but partially reverted in commit 0382ed88.

- Before 4355 there was always one beam at the subdivision
- After 4355 the beam count reflected the metric value of
  the subdivision's location
- After commit 8fa2d858 the beam count matched baseMoment

The behaviour after 4355 is correct as it gives an immediate
feedback on the metric situation, and is suggested by Gould.
The later revert was to avoid a side-effect, namely the inconsistency
with shortened beams: If the remaining length of the beam is less
than the length of the current metric group the result was
confusing.

This patch reverts the effect of the latest commit, reinstating
the behaviour of issue 4355. However, it improves handling to
remove the unwanted side-effect:
- when the remaining length of the beam is shorter than the
  next group (e.g. subdivision at an 1/8 point, but only 3/32
  left) the beam count matches the length of the remainder:
  1/16 <= remainder < 1/8 => 2 beams
  1/32 <= remainder < 1/16 => 3 beams
  etc.
- but if only one stem is left after the subdivision this is not
  applied.  The subdivision has the regular number matching the
  metric value, and the trailing single stem has beamlets according
  to its actual length

[Carl, 2015-12-22 00:48:54]

THe code looks good to me.  Just some minor details to look at.

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/new/su...
File Documentation/snippets/new/subdividing-beams.ly (right):

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/new/su...
Documentation/snippets/new/subdividing-beams.ly:7: \version "2.19.34"
I think you should remove the header above here, because you are in
Documentation/snippets/new

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/subdiv...
File Documentation/snippets/subdividing-beams.ly (right):

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/subdiv...
Documentation/snippets/subdividing-beams.ly:5: %
I think that in Documentation/snippets/new you should not have the DO NOT EDIT
header -- just the snippet.  THen you won't have the doubled header you have
here.

https://codereview.appspot.com/276560043/diff/1/lily/include/beaming-pattern.hh
File lily/include/beaming-pattern.hh (right):

https://codereview.appspot.com/276560043/diff/1/lily/include/beaming-pattern....
lily/include/beaming-pattern.hh:76: Moment remaining_length (int idx) const;
Why did you choose to make remaining-length public?  It seems to me that the in
your usage it can all be private -- that is, I don't see any use case where some
other beaming pattern needs to get the remaining length in this beaming pattern.

At least in your code, I don't see any public uses.

But I don't feel strongly about this, and it does seem to have some similarities
with the other public functions.

[git, 2015-12-22 00:59:02]

Respond to review comments

[git, 2015-12-22 00:59:40]

Thanks for the comments, changes uploaded.

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/new/su...
File Documentation/snippets/new/subdividing-beams.ly (right):

https://codereview.appspot.com/276560043/diff/1/Documentation/snippets/new/su...
Documentation/snippets/new/subdividing-beams.ly:7: \version "2.19.34"
On 2015/12/22 00:48:53, Carl wrote:
> I think you should remove the header above here, because you are in
> Documentation/snippets/new

Done.

https://codereview.appspot.com/276560043/diff/1/lily/include/beaming-pattern.hh
File lily/include/beaming-pattern.hh (right):

https://codereview.appspot.com/276560043/diff/1/lily/include/beaming-pattern....
lily/include/beaming-pattern.hh:76: Moment remaining_length (int idx) const;
On 2015/12/22 00:48:53, Carl wrote:
> Why did you choose to make remaining-length public?  It seems to me that the
in
> your usage it can all be private -- that is, I don't see any use case where
some
> other beaming pattern needs to get the remaining length in this beaming
pattern.
> 
> At least in your code, I don't see any public uses.
> 
> But I don't feel strongly about this, and it does seem to have some
similarities
> with the other public functions.

I don't feel strong about this either. It was just the idea that it *might* be
useful from outside.

Maybe the better approach for this is to add it as a private method (as you say
I don't have any immediate public use), if someone might need it later it can
still be made public.

[pkx166h, 2015-12-22 11:50:44]

https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
File Documentation/snippets/subdividing-beams.ly (right):

https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
Documentation/snippets/subdividing-beams.ly:7: %% Note: this file works from
version 2.19.34
Urs, any snippet that is in Document/Snippets/new/.., shouldn't have the header.
It just starts with the '\version' number.

If you are creating a new snippet then obviously you don't add the heading and
just submit the patch with the new file in .../smippets/new/..

If you are editing an existing snippet then edit the snippet that is in 'new'
(if there is one) and upload the patch, or if the snippet is not in new - then
it is going to from the LSR probably - but still needs to be copied from
wherever it is in Documentation/snippets/... (without header) into
snippets/new/.. so that the build stuff/makelsr can do its thing and
update/overwrite the existing file.

I guess the CG could be clearer on this (it's on my todo list) - I forget all
the nuances between LSR and ..Documentation/snippets/*

Finally if you are changing a snippet then you need to run makelsr so that any
snippet in snippets/new gets updated and copied to the correct place for doc
build to reference (assuming it appears in the main docs like NR) or for it to
be included in the snippet pdfs.

But ...

Normally we don't (or at least I don't) run makelsr and then submit the patch
for review (as it can make reviewing your changes a chore) but it will need to
be run to test the patch (I do that manually often) and it is also good practice
to submit makelsr as a separate commit when pushing any changes t hat involve
snippets. I assume to make it easier to manage if something goes wrong.

If I have got any of this wrong then perhaps someone can correct me so I can
update the LSR section of the CG.

[git, 2015-12-22 18:34:43]

https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
File Documentation/snippets/subdividing-beams.ly (right):

https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
Documentation/snippets/subdividing-beams.ly:7: %% Note: this file works from
version 2.19.34
So unfortunately I still don't know what to do with the snippet. This is the
situation:
- The snippet already exists and is used in the NR
- It was not in Documentation/snippets/new before I copied it there

So this seems to indicate that I should updated the snippet in the LSR.

But while it will probably compile with 2.18.0 it doesn't give proper result
before 2.19.34, and the modified text refers to that new functionality of my
patch.

So what should I do now?

[Trevor Daniels, 2015-12-22 19:51:18]

On 2015/12/22 18:34:43, git wrote:
>
https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
> File Documentation/snippets/subdividing-beams.ly (right):
> 
>
https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets/su...
> Documentation/snippets/subdividing-beams.ly:7: %% Note: this file works from
> version 2.19.34
> So unfortunately I still don't know what to do with the snippet. This is the
> situation:
> - The snippet already exists and is used in the NR
> - It was not in Documentation/snippets/new before I copied it there
> 
> So this seems to indicate that I should updated the snippet in the LSR.
> 
> But while it will probably compile with 2.18.0 it doesn't give proper result
> before 2.19.34, and the modified text refers to that new functionality of my
> patch.
> 
> So what should I do now?

What you did was OK; only snippets that work correctly with 2.18.2
should be edited in the LSR.  Just edit the headers as James suggested.

Trevor

[c_sorensen, 2015-12-22 19:51:56]

Copy the snippet to Documentation/snippets/new.

Remove the machine-generated header.

Make your changes.

Submit the patch.

Carl

On 12/22/15 11:34 AM, "git@ursliska.de" <git@ursliska.de> wrote:

>
>https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets
>/subdividing-beams.ly
>File Documentation/snippets/subdividing-beams.ly (right):
>
>https://codereview.appspot.com/276560043/diff/20001/Documentation/snippets
>/subdividing-beams.ly#newcode7
>Documentation/snippets/subdividing-beams.ly:7: %% Note: this file works
>from version 2.19.34
>So unfortunately I still don't know what to do with the snippet. This is
>the situation:
>- The snippet already exists and is used in the NR
>- It was not in Documentation/snippets/new before I copied it there
>
>So this seems to indicate that I should updated the snippet in the LSR.
>
>But while it will probably compile with 2.18.0 it doesn't give proper
>result before 2.19.34, and the modified text refers to that new
>functionality of my patch.
>
>So what should I do now?
>
>https://codereview.appspot.com/276560043/

[git, 2015-12-23 07:59:26]

Revert changes to snippets file

[dak, 2015-12-23 09:06:56]

On 2015/12/23 07:59:26, git wrote:
> Revert changes to snippets file

At the current point of time, you only added Documentation/snippets/new/xxx.ly
while keeping Documentation/snippets/xxx.ly unchanged.  That's the right
organisation for a commit keeping manual and automatic changes apart.  The
actual change is then performed in a separate commit named
"Run scripts/auxiliar/makelsr.py".

Now if the old snippet stops compiling altogether, the usual remedy will be to
write a convert-ly rule fixing that, and create a separate "Run
scripts/auxiliar/update-with-convert-ly.sh" commit.

At any rate, it is good to avoid uncompilable commits in master, so the best
remedy is to do this sort of thing, when the intermediate state won't compile,
in a separate branch which is then merged in a single commit into master.

The next best thing is to just fold the automated changes into the same commit. 
That probably requires less haggling with Git but is slightly less nice to the
commit history.

But this complication is just for intermediate states that won't compile: if the
intermediate states compile fine by themselves, one can submit them as separate
commits just fine.

[git, 2015-12-23 13:34:56]

Simplify code by cleaning up "old" code

[git, 2015-12-23 13:37:10]

Add commit with updated doc snippet